[PATCH linux dev-4.13] fsi: occ: Add check for OCC response checksum

[Andrew Jeffery]

On Tue, 1 May 2018, at 06:35, Eddie James wrote:
> The OCC specification indicates that it is an error scenario if the
> response checksum doesn't match the sum of the bytes of the response.
> The driver needs to perform this calculation and check, and return an
> error if it's a mismatch.
> 
> Signed-off-by: Eddie James <eajames at linux.vnet.ibm.com>
> ---
>  drivers/fsi/fsi-occ.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index 45ae13c..16f18df 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -427,6 +427,27 @@ static int occ_release(struct inode *inode, struct 
> file *file)
>  	.release = occ_release,
>  };
>  
> +static int occ_check_sum(struct occ_response *resp, u16 data_length)

Why pass in data_length when it's been extracted from resp?

> +{
> +	unsigned int i;
> +	u16 checksum = 0;
> +	u16 checksum_resp = get_unaligned_be16(&resp->data[data_length]);

This looks like the definition of struct occ_response is broken:

struct occ_response {
	u8 seq_no;
	u8 cmd_type;
	u8 return_status;
	__be16 data_length;
	u8 data[OCC_RESP_DATA_BYTES];
	__be16 checksum;
} __packed;

The checksum member is misleading, as we're not always transferring 4089 bytes of data, but it is necessary to "avoid" an out-of-bounds memory access if we do. I think it's worth redefining data as `u8 data[OCC_RESP_DATA_BYTES + 2]` and dropping the checksum member.

Also can you add a comment explaining why you're doing an "out of bounds" access through the data member by indexing with data_length?

> +
> +	checksum += resp->seq_no;
> +	checksum += resp->cmd_type;
> +	checksum += resp->return_status;
> +	checksum += (data_length >> 8) + (data_length & 0xFF);
> +
> +	for (i = 0; i < data_length; ++i) {
> +		checksum += resp->data[i];
> +	}
> +
> +	if (checksum != checksum_resp)
> +		return -EBADMSG;
> +
> +	return 0;
> +}
> +
>  static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
>  			     ssize_t len)
>  {
> @@ -692,6 +713,8 @@ static void occ_worker(struct work_struct *work)
>  
>  	xfr->resp_data_length = resp_data_length + 7;
>  
> +	rc = occ_check_sum(resp, resp_data_length);
> +

It's a pity there aren't two checksums: one for the fixed length data (covering the variable data length value) and one for the variable length data. We're pretty much reduced to inferring that the data length is fine if it doesn't exceed OCC_RESP_DATA_BYTES, but that doesn't mean that it isn't corrupt.

Cheers,

Andrew

>  done:
>  	mutex_unlock(&occ->occ_lock);
>  
> -- 
> 1.8.3.1
>