[PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Jean Delvare]

I2C block transfers can have a size up to 32 bytes. If starting close
to the end of the address space, there may not be enough room to write
that many bytes (on I2C block writes) or not enough bytes to be read
(on I2C block reads.) In that case, we must shorten the transfer so
that it does not exceed the address space.

Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/i2c-stub.c |    2 ++
 1 file changed, 2 insertions(+)

--- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
+++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
@@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
 		 * We ignore banks here, because banked chips don't use I2C
 		 * block transfers
 		 */
+		if (data->block[0] > 256 - command)	/* Avoid overrun */
+			data->block[0] = 256 - command;
 		len = data->block[0];
 		if (read_write == I2C_SMBUS_WRITE) {
 			for (i = 0; i < len; i++) {


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Guenter Roeck]

On 07/13/2014 08:17 AM, Jean Delvare wrote:
> I2C block transfers can have a size up to 32 bytes. If starting close
> to the end of the address space, there may not be enough room to write
> that many bytes (on I2C block writes) or not enough bytes to be read
> (on I2C block reads.) In that case, we must shorten the transfer so
> that it does not exceed the address space.
>
> Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> ---
>   drivers/i2c/i2c-stub.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
> +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
> @@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
>   		 * We ignore banks here, because banked chips don't use I2C
>   		 * block transfers
>   		 */
> +		if (data->block[0] > 256 - command)	/* Avoid overrun */
> +			data->block[0] = 256 - command;

Hi Jean,

is it safe to overwrite data->block[0], or should it be something
like the following ?

		if (data->block[0] > 256 - command)	/* Avoid overrun */
			len = 256 - command;
		else
			len = data->block[0];

Also, wonder what happens in the real world if anyone does that.
Would the write stop at offset 255, or would it wrap and write from 0 ?

Thanks,
Guenter

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Jean Delvare]

Hi Guenter,

On Sun, 13 Jul 2014 08:44:24 -0700, Guenter Roeck wrote:
> On 07/13/2014 08:17 AM, Jean Delvare wrote:
> > I2C block transfers can have a size up to 32 bytes. If starting close
> > to the end of the address space, there may not be enough room to write
> > that many bytes (on I2C block writes) or not enough bytes to be read
> > (on I2C block reads.) In that case, we must shorten the transfer so
> > that it does not exceed the address space.
> >
> > Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> > Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> > ---
> >   drivers/i2c/i2c-stub.c |    2 ++
> >   1 file changed, 2 insertions(+)
> >
> > --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
> > +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
> > @@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
> >   		 * We ignore banks here, because banked chips don't use I2C
> >   		 * block transfers
> >   		 */
> > +		if (data->block[0] > 256 - command)	/* Avoid overrun */
> > +			data->block[0] = 256 - command;
> 
> is it safe to overwrite data->block[0], or should it be something
> like the following ?
> 
> 		if (data->block[0] > 256 - command)	/* Avoid overrun */
> 			len = 256 - command;
> 		else
> 			len = data->block[0];

It's not only safe, it is desired. Otherwise the caller doesn't know
this is a short read, and it may take the end of the block buffer for
actual data. Check the code in
i2c-core.c:i2c_smbus_read_i2c_block_data(), you'll see it uses and even
returns block[0]. Same for writes, that's the only way to notify the
caller of short writes.

> Also, wonder what happens in the real world if anyone does that.
> Would the write stop at offset 255, or would it wrap and write from 0 ?

Depends on the chip, I've seen both implementations.

It doesn't really matter what i2c-stub does, as device drivers should
never do that. I just did not want to risk data leak or corruption in
case it ever happens.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Guenter Roeck]

On 07/13/2014 09:24 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sun, 13 Jul 2014 08:44:24 -0700, Guenter Roeck wrote:
>> On 07/13/2014 08:17 AM, Jean Delvare wrote:
>>> I2C block transfers can have a size up to 32 bytes. If starting close
>>> to the end of the address space, there may not be enough room to write
>>> that many bytes (on I2C block writes) or not enough bytes to be read
>>> (on I2C block reads.) In that case, we must shorten the transfer so
>>> that it does not exceed the address space.
>>>
>>> Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
>>> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
>>> ---
>>>    drivers/i2c/i2c-stub.c |    2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
>>> +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
>>> @@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
>>>    		 * We ignore banks here, because banked chips don't use I2C
>>>    		 * block transfers
>>>    		 */
>>> +		if (data->block[0] > 256 - command)	/* Avoid overrun */
>>> +			data->block[0] = 256 - command;
>>
>> is it safe to overwrite data->block[0], or should it be something
>> like the following ?
>>
>> 		if (data->block[0] > 256 - command)	/* Avoid overrun */
>> 			len = 256 - command;
>> 		else
>> 			len = data->block[0];
>
> It's not only safe, it is desired. Otherwise the caller doesn't know
> this is a short read, and it may take the end of the block buffer for
> actual data. Check the code in
> i2c-core.c:i2c_smbus_read_i2c_block_data(), you'll see it uses and even
> returns block[0]. Same for writes, that's the only way to notify the
> caller of short writes.
>
>> Also, wonder what happens in the real world if anyone does that.
>> Would the write stop at offset 255, or would it wrap and write from 0 ?
>
> Depends on the chip, I've seen both implementations.
>
> It doesn't really matter what i2c-stub does, as device drivers should
> never do that. I just did not want to risk data leak or corruption in
> case it ever happens.
>

Ok, makes sense.

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

Thanks,
Guenter

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Wolfram Sang]

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

On Sun, Jul 13, 2014 at 05:17:17PM +0200, Jean Delvare wrote:
> I2C block transfers can have a size up to 32 bytes. If starting close

Shouldn't that be "256 bytes"? 32 is SMBUS transfer size? Otherwise I
don't understand the patch.

> to the end of the address space, there may not be enough room to write
> that many bytes (on I2C block writes) or not enough bytes to be read
> (on I2C block reads.) In that case, we must shorten the transfer so
> that it does not exceed the address space.
> 
> Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> ---
>  drivers/i2c/i2c-stub.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
> +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
> @@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
>  		 * We ignore banks here, because banked chips don't use I2C
>  		 * block transfers
>  		 */
> +		if (data->block[0] > 256 - command)	/* Avoid overrun */
> +			data->block[0] = 256 - command;
>  		len = data->block[0];
>  		if (read_write == I2C_SMBUS_WRITE) {
>  			for (i = 0; i < len; i++) {
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Guenter Roeck]

On Thu, Jul 17, 2014 at 07:27:20PM +0200, Wolfram Sang wrote:
> On Sun, Jul 13, 2014 at 05:17:17PM +0200, Jean Delvare wrote:
> > I2C block transfers can have a size up to 32 bytes. If starting close
> 
> Shouldn't that be "256 bytes"? 32 is SMBUS transfer size? Otherwise I
> don't understand the patch.
> 
If I understand correctly, this is still an SMBus command, which is limited
to 32 bytes. Maybe the description should read "SMBus I2C block transfers ...".

Guenter


> > to the end of the address space, there may not be enough room to write
> > that many bytes (on I2C block writes) or not enough bytes to be read
> > (on I2C block reads.) In that case, we must shorten the transfer so
> > that it does not exceed the address space.
> > 
> > Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> > Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> > ---
> >  drivers/i2c/i2c-stub.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > --- linux-3.16-rc4.orig/drivers/i2c/i2c-stub.c	2014-07-12 11:56:30.933096483 +0200
> > +++ linux-3.16-rc4/drivers/i2c/i2c-stub.c	2014-07-13 17:01:02.891235856 +0200
> > @@ -220,6 +220,8 @@ static s32 stub_xfer(struct i2c_adapter
> >  		 * We ignore banks here, because banked chips don't use I2C
> >  		 * block transfers
> >  		 */
> > +		if (data->block[0] > 256 - command)	/* Avoid overrun */
> > +			data->block[0] = 256 - command;
> >  		len = data->block[0];
> >  		if (read_write == I2C_SMBUS_WRITE) {
> >  			for (i = 0; i < len; i++) {
> > 
> > 
> > -- 
> > Jean Delvare
> > SUSE L3 Support

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Jean Delvare]

On Thu, 17 Jul 2014 10:57:49 -0700, Guenter Roeck wrote:
> On Thu, Jul 17, 2014 at 07:27:20PM +0200, Wolfram Sang wrote:
> > On Sun, Jul 13, 2014 at 05:17:17PM +0200, Jean Delvare wrote:
> > > I2C block transfers can have a size up to 32 bytes. If starting close
> > 
> > Shouldn't that be "256 bytes"? 32 is SMBUS transfer size? Otherwise I
> > don't understand the patch.
>
> If I understand correctly, this is still an SMBus command, which is limited
> to 32 bytes. Maybe the description should read "SMBus I2C block transfers ...".

That's correct, "I2C block transfers" despite the name really are
SMBus-style transfers and the SMBus buffer size limit of 32 bytes thus
applies.

With this clarified, Wolfram, what is is that you do not understand?
The start of the block transfer can be anywhere in 0-255, and the size
can be in 1-32, so without a boundary check, one can attempt to read or
write up to offset 255 + 32 - 1 = 286, which is way beyond the
the end of the chip->words array. My patch prevents that from happening.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Wolfram Sang]

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

> With this clarified, Wolfram, what is is that you do not understand?

Dunno anymore, probably the confusion confused me :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-stub: Avoid an array overrun on I2C block transfers

[Wolfram Sang]

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

On Sun, Jul 13, 2014 at 05:17:17PM +0200, Jean Delvare wrote:
> I2C block transfers can have a size up to 32 bytes. If starting close
> to the end of the address space, there may not be enough room to write
> that many bytes (on I2C block writes) or not enough bytes to be read
> (on I2C block reads.) In that case, we must shorten the transfer so
> that it does not exceed the address space.
> 
> Signed-off-by: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]