| Message ID | a5e9b8b5-82af-96a1-b983-d98c2fa140f9@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: i801: Don't silently correct invalid transfer size | expand |
Hi Heiner, On Sun, 07 Nov 2021 22:57:00 +0100, Heiner Kallweit wrote: > If an invalid block size is provided, reject it instead of silently > changing it to a supported value. Especially critical I see the case of > a write transfer with block length 0. In this case we have no guarantee > that the byte we would write is valid. When silently reducing a read to > 32 bytes then we don't return an error and the caller may falsely > assume that we returned the full requested data. > > If this change should break any (broken) caller, then I think we should > fix the caller. Fully agreed. > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 2c48691d2..638198b4b 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -761,6 +761,11 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data * > int result = 0; > unsigned char hostc; > > + if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) > + data->block[0] = I2C_SMBUS_BLOCK_MAX; > + else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) > + return -EPROTO; > + > if (command == I2C_SMBUS_I2C_BLOCK_DATA) { > if (read_write == I2C_SMBUS_WRITE) { > /* set I2C_EN bit in configuration register */ > @@ -774,16 +779,6 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data * > } > } > > - if (read_write == I2C_SMBUS_WRITE > - || command == I2C_SMBUS_I2C_BLOCK_DATA) { > - if (data->block[0] < 1) > - data->block[0] = 1; > - if (data->block[0] > I2C_SMBUS_BLOCK_MAX) > - data->block[0] = I2C_SMBUS_BLOCK_MAX; > - } else { > - data->block[0] = 32; /* max for SMBus block reads */ > - } > - > /* Experience has shown that the block buffer can only be used for > SMBus (not I2C) block transactions, even though the datasheet > doesn't mention this limitation. */ Reviewed-by: Jean Delvare <jdelvare@suse.de>
On Sun, Nov 07, 2021 at 10:57:00PM +0100, Heiner Kallweit wrote: > If an invalid block size is provided, reject it instead of silently > changing it to a supported value. Especially critical I see the case of > a write transfer with block length 0. In this case we have no guarantee > that the byte we would write is valid. When silently reducing a read to > 32 bytes then we don't return an error and the caller may falsely > assume that we returned the full requested data. > > If this change should break any (broken) caller, then I think we should > fix the caller. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 2c48691d2..638198b4b 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -761,6 +761,11 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data * int result = 0; unsigned char hostc; + if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) + data->block[0] = I2C_SMBUS_BLOCK_MAX; + else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) + return -EPROTO; + if (command == I2C_SMBUS_I2C_BLOCK_DATA) { if (read_write == I2C_SMBUS_WRITE) { /* set I2C_EN bit in configuration register */ @@ -774,16 +779,6 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data * } } - if (read_write == I2C_SMBUS_WRITE - || command == I2C_SMBUS_I2C_BLOCK_DATA) { - if (data->block[0] < 1) - data->block[0] = 1; - if (data->block[0] > I2C_SMBUS_BLOCK_MAX) - data->block[0] = I2C_SMBUS_BLOCK_MAX; - } else { - data->block[0] = 32; /* max for SMBus block reads */ - } - /* Experience has shown that the block buffer can only be used for SMBus (not I2C) block transactions, even though the datasheet doesn't mention this limitation. */
If an invalid block size is provided, reject it instead of silently changing it to a supported value. Especially critical I see the case of a write transfer with block length 0. In this case we have no guarantee that the byte we would write is valid. When silently reducing a read to 32 bytes then we don't return an error and the caller may falsely assume that we returned the full requested data. If this change should break any (broken) caller, then I think we should fix the caller. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)