[PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Stephen Douthit]

Hello all,

We ran into an issue where the ipmi_ssif and i2c-ismt drivers don't
agree on the format for data returned by i2c_smbus_read_block_data()

Looking at the traffic on the wire with a beagle analyzer:
-----
Packet Details   (Values in hex; [S] = Start condition;
                  [P] = Stop condition; * = No Ack)
[S] <10:R> 12 1C 01 00 00 80 02 1C 02 8F BE 12 00 25 12 41 01 00 00* [P]
-----

Looking at the matching kernel trace:
-----
kssif0010-759   [001] ....  1435.891090: smbus_read: i2c-0 a=010 f=0000 c=3 BLOCK_DATA
kssif0010-759   [001] ....  1436.202906: smbus_reply: i2c-0 a=010 f=0000 c=3 BLOCK_DATA l=20 [13-12-1c-01-00-00-80-02-1c-02-8f-be-12-00-25-12-41-01-00-00]
kssif0010-759   [001] ....  1436.202908: smbus_result: i2c-0 a=010 f=0000 c=3 BLOCK_DATA rd res=0
-----

So basically the byte count already precedes the data in the dma_buffer,
then the driver sticks desc->rxbytes in front of this resulting in the
trace above.

The first patch tackles this.

The second patch in the series adds a sanity check on the byte count
supplied by the slave device.  This might be a nice to have, but is
probably less critical.

-Steve

[PATCH 1/2] i2c: ismt: Don't duplicate the receive length for block reads

[Stephen Douthit]

According to Table 15-14 of the C2000 EDS (Intel doc #510524) the
rx data pointed to by the descriptor dptr contains the byte count.

desc->rxbytes reports all bytes read on the wire, including the
"byte count" byte.  So if a device sends 4 bytes in response to a
block read, on the wire and in the DMA buffer we see:

count data1 data2 data3 data4
 0x04  0xde  0xad  0xbe  0xef

That's what we want to return in data->block to the next level.

Instead we were actually prefixing that with desc->rxbytes:

bad
count count data1 data2 data3 data4
 0x05  0x04  0xde  0xad  0xbe  0xef

This was discovered while developing a BMC solution relying on the
ipmi_ssif.c driver which was trying to interpret the bogus length
field as part of the IPMI response.

Signed-off-by: Stephen Douthit <stephend@adiengineering.com>
Tested-by: Dan Priamo <danp@adiengineering.com>
---
 drivers/i2c/busses/i2c-ismt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index e98e44e..9af2337 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -341,8 +341,8 @@ static int ismt_process_desc(const struct ismt_desc *desc,
 			break;
 		case I2C_SMBUS_BLOCK_DATA:
 		case I2C_SMBUS_I2C_BLOCK_DATA:
-			memcpy(&data->block[1], dma_buffer, desc->rxbytes);
-			data->block[0] = desc->rxbytes;
+			memcpy(data->block, dma_buffer, desc->rxbytes);
+			data->block[0] = desc->rxbytes - 1;
 			break;
 		}
 		return 0;
-- 
2.7.5

[PATCH 2/2] i2c: ismt: Return EMSGSIZE for block reads with bogus length

[Stephen Douthit]

Compare the number of bytes actually seen on the wire to the byte
count field returned by the slave device.

Previously we just overwrote the byte count returned by the slave
with the real byte count and let the caller figure out if the
message was sane.

Signed-off-by: Stephen Douthit <stephend@adiengineering.com>
Tested-by: Dan Priamo <danp@adiengineering.com>
---
 drivers/i2c/busses/i2c-ismt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index 9af2337..22ffcb7 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -341,8 +341,10 @@ static int ismt_process_desc(const struct ismt_desc *desc,
 			break;
 		case I2C_SMBUS_BLOCK_DATA:
 		case I2C_SMBUS_I2C_BLOCK_DATA:
+			if (desc->rxbytes != dma_buffer[0] + 1)
+				return -EMSGSIZE;
+
 			memcpy(data->block, dma_buffer, desc->rxbytes);
-			data->block[0] = desc->rxbytes - 1;
 			break;
 		}
 		return 0;
-- 
2.7.5

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On Mon, Aug 07, 2017 at 05:10:58PM -0400, Stephen Douthit wrote:
> Hello all,
> 
> We ran into an issue where the ipmi_ssif and i2c-ismt drivers don't
> agree on the format for data returned by i2c_smbus_read_block_data()
> 
> Looking at the traffic on the wire with a beagle analyzer:
> -----
> Packet Details   (Values in hex; [S] = Start condition;
>                   [P] = Stop condition; * = No Ack)
> [S] <10:R> 12 1C 01 00 00 80 02 1C 02 8F BE 12 00 25 12 41 01 00 00* [P]
> -----
> 
> Looking at the matching kernel trace:
> -----
> kssif0010-759   [001] ....  1435.891090: smbus_read: i2c-0 a=010 f=0000 c=3 BLOCK_DATA
> kssif0010-759   [001] ....  1436.202906: smbus_reply: i2c-0 a=010 f=0000 c=3 BLOCK_DATA l=20 [13-12-1c-01-00-00-80-02-1c-02-8f-be-12-00-25-12-41-01-00-00]
> kssif0010-759   [001] ....  1436.202908: smbus_result: i2c-0 a=010 f=0000 c=3 BLOCK_DATA rd res=0
> -----
> 
> So basically the byte count already precedes the data in the dma_buffer,
> then the driver sticks desc->rxbytes in front of this resulting in the
> trace above.
> 
> The first patch tackles this.
> 
> The second patch in the series adds a sanity check on the byte count
> supplied by the slave device.  This might be a nice to have, but is
> probably less critical.

Both patches look good to me. Seth, Neil, do you agree?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Stephen Douthit]

On 8/14/2017 3:31 PM, Wolfram Sang wrote:
> On Mon, Aug 07, 2017 at 05:10:58PM -0400, Stephen Douthit wrote:
>> Hello all,
>>
>> We ran into an issue where the ipmi_ssif and i2c-ismt drivers don't
>> agree on the format for data returned by i2c_smbus_read_block_data()
>>
>> Looking at the traffic on the wire with a beagle analyzer:
>> -----
>> Packet Details   (Values in hex; [S] = Start condition;
>>                   [P] = Stop condition; * = No Ack)
>> [S] <10:R> 12 1C 01 00 00 80 02 1C 02 8F BE 12 00 25 12 41 01 00 00* [P]
>> -----
>>
>> Looking at the matching kernel trace:
>> -----
>> kssif0010-759   [001] ....  1435.891090: smbus_read: i2c-0 a=010 f=0000 c=3 BLOCK_DATA
>> kssif0010-759   [001] ....  1436.202906: smbus_reply: i2c-0 a=010 f=0000 c=3 BLOCK_DATA l=20 [13-12-1c-01-00-00-80-02-1c-02-8f-be-12-00-25-12-41-01-00-00]
>> kssif0010-759   [001] ....  1436.202908: smbus_result: i2c-0 a=010 f=0000 c=3 BLOCK_DATA rd res=0
>> -----
>>
>> So basically the byte count already precedes the data in the dma_buffer,
>> then the driver sticks desc->rxbytes in front of this resulting in the
>> trace above.
>>
>> The first patch tackles this.
>>
>> The second patch in the series adds a sanity check on the byte count
>> supplied by the slave device.  This might be a nice to have, but is
>> probably less critical.
> 
> Both patches look good to me. Seth, Neil, do you agree?
> 

Ping.

Not sure what the usual review time is, let me know if this is premature.

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]


> >> So basically the byte count already precedes the data in the dma_buffer,
> >> then the driver sticks desc->rxbytes in front of this resulting in the
> >> trace above.
> >>
> >> The first patch tackles this.
> >>
> >> The second patch in the series adds a sanity check on the byte count
> >> supplied by the slave device.  This might be a nice to have, but is
> >> probably less critical.
> > 
> > Both patches look good to me. Seth, Neil, do you agree?
> > 
> 
> Ping.
> 
> Not sure what the usual review time is, let me know if this is premature.

I applied both patches to for-next (v4.14) now to get a broader
audience. for-current (v4.13) might have been also applicable, but I
don't want to apply the patches there without the driver maintainer
acks. I hope this works for you.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Neil Horman]

On Tue, Aug 29, 2017 at 12:22:25PM +0200, Wolfram Sang wrote:
> 
> > >> So basically the byte count already precedes the data in the dma_buffer,
> > >> then the driver sticks desc->rxbytes in front of this resulting in the
> > >> trace above.
> > >>
> > >> The first patch tackles this.
> > >>
> > >> The second patch in the series adds a sanity check on the byte count
> > >> supplied by the slave device.  This might be a nice to have, but is
> > >> probably less critical.
> > > 
> > > Both patches look good to me. Seth, Neil, do you agree?
> > > 
> > 
> > Ping.
> > 
> > Not sure what the usual review time is, let me know if this is premature.
> 
> I applied both patches to for-next (v4.14) now to get a broader
> audience. for-current (v4.13) might have been also applicable, but I
> don't want to apply the patches there without the driver maintainer
> acks. I hope this works for you.
> 

Sorry, I've been on vacation, yes, the patches look good to me

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]


> Sorry, I've been on vacation, yes, the patches look good to me

All fine, I was just being cautious.

> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Dan Priamo]

Hi,

We are using Linux stable kernel version 4.4.y and would like to see these changes included in that version.
So once these patches are merged in, can they be tagged for other Linux stable kernel releases to pick up these changes? 

Thank you!
Dan

-----Original Message-----
From: Wolfram Sang [mailto:wsa@the-dreams.de] 
Sent: Tuesday, August 29, 2017 7:50 AM
To: Neil Horman <nhorman@tuxdriver.com>
Cc: Steve Douthit <stephend@adiengineering.com>; seth.heasley@intel.com; Dan Priamo <danp@adiengineering.com>; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads


> Sorry, I've been on vacation, yes, the patches look good to me

All fine, I was just being cautious.

> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Thanks!

Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

> We are using Linux stable kernel version 4.4.y and would like to see
> these changes included in that version. So once these patches are
> merged in, can they be tagged for other Linux stable kernel releases
> to pick up these changes?

Okay, since I haven't pushed out yet, I can re-apply the bugfix to
for-current and add the stable tag to it.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

[Dan Priamo]

Thank you!

-----Original Message-----
From: Wolfram Sang [mailto:wsa@the-dreams.de] 
Sent: Tuesday, August 29, 2017 4:09 PM
To: Dan Priamo <danp@adiengineering.com>
Cc: Neil Horman <nhorman@tuxdriver.com>; Steve Douthit <stephend@adiengineering.com>; seth.heasley@intel.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/2] i2c: ismt: Fix length handling for SMBus block reads

> We are using Linux stable kernel version 4.4.y and would like to see 
> these changes included in that version. So once these patches are 
> merged in, can they be tagged for other Linux stable kernel releases 
> to pick up these changes?

Okay, since I haven't pushed out yet, I can re-apply the bugfix to for-current and add the stable tag to it.