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

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

[Eddie James]

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@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)
+{
+	unsigned int i;
+	u16 checksum = 0;
+	u16 checksum_resp = get_unaligned_be16(&resp->data[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);
+
 done:
 	mutex_unlock(&occ->occ_lock);
 
-- 
1.8.3.1

Re: [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@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
> 

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

[Stewart Smith]

Eddie James <eajames@linux.vnet.ibm.com> writes:
> 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@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)

Minor nitpick: It looks like 'checksum' is the standard rather than 'check_sum':

[stewart@birb linux]$ ack check_sum|wc -l
133
[stewart@birb linux]$ ack checksum|wc -l
7284

Would a more accurate name be occ_resp_verify_checksum() ? By the name
'occ_check_sum' alone, I'd assume that it was returning the calculated
checksum rather than verifying it.

-- 
Stewart Smith
OPAL Architect, IBM.

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

[Eddie James]

On 04/30/2018 06:44 PM, Andrew Jeffery wrote:
>
> 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@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?

So that I don't have to convert it from BE again.

>
>> +{
>> +	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?

  Good ideas.

>
>> +
>> +	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.

Well it still works; if the data length is incorrect but less than the 
max, then the checksum won't match.

Thanks,
Eddie

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

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

[Andrew Jeffery]

On Wed, 2 May 2018, at 00:10, Eddie James wrote:
> 
> 
> On 04/30/2018 06:44 PM, Andrew Jeffery wrote:
> >
> > 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@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?
> 
> So that I don't have to convert it from BE again.
> 
> >
> >> +{
> >> +	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?
> 
>   Good ideas.
> 
> >
> >> +
> >> +	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.
> 
> Well it still works; if the data length is incorrect but less than the 
> max, then the checksum won't match.

Maybe: you're defining where your checksum lives in terms of the length, which is covered by the checksum but only if you've found the correct one, which you might not have if the length is corrupt. But it's pretty unlikely that it would pass, and a checksum is probabilistic anyway.

> 
> Thanks,
> Eddie
> 
> >
> > Cheers,
> >
> > Andrew
> >
> >>   done:
> >>   	mutex_unlock(&occ->occ_lock);
> >>   
> >> -- 
> >> 1.8.3.1
> >>
>