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
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 <[email protected]>
Tested-by: Dan Priamo <[email protected]>
---
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
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 <[email protected]>
Tested-by: Dan Priamo <[email protected]>
---
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
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?
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.
> Sorry, I've been on vacation, yes, the patches look good to me
All fine, I was just being cautious.
> Acked-by: Neil Horman <[email protected]>
Thanks!
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 <[email protected]>
> >> 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.
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:[email protected]]
Sent: Tuesday, August 29, 2017 7:50 AM
To: Neil Horman <[email protected]>
Cc: Steve Douthit <[email protected]>; [email protected]; Dan Priamo <[email protected]>; [email protected]; [email protected]
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 <[email protected]>
Thanks!
> 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.
Thank you!
-----Original Message-----
From: Wolfram Sang [mailto:[email protected]]
Sent: Tuesday, August 29, 2017 4:09 PM
To: Dan Priamo <[email protected]>
Cc: Neil Horman <[email protected]>; Steve Douthit <[email protected]>; [email protected]; [email protected]; [email protected]
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.