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

[Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>]

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