| Message ID | 20140713171717.25497712@endymion.delvare |
|---|---|
| State | Accepted |
| Headers | show |
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@suse.de> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Wolfram Sang <wsa@the-dreams.de> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@suse.de> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > --- > > 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.
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@suse.de> >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: Wolfram Sang <wsa@the-dreams.de> >>> --- >>> 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@roeck-us.net> Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@suse.de> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Wolfram Sang <wsa@the-dreams.de> > --- > 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
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@suse.de> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Wolfram Sang <wsa@the-dreams.de> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> With this clarified, Wolfram, what is is that you do not understand?
Dunno anymore, probably the confusion confused me :)
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@suse.de> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Wolfram Sang <wsa@the-dreams.de> Applied to for-next, thanks!
--- 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++) {
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@suse.de> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Wolfram Sang <wsa@the-dreams.de> --- drivers/i2c/i2c-stub.c | 2 ++ 1 file changed, 2 insertions(+)