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

Andrew Jeffery andrew at aj.id.au
Wed May 2 01:22:34 AEST 2018



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


More information about the openbmc mailing list