Re: i2c: i801: Don't silently correct invalid transfer size

[Jean Delvare <jdelvare@suse.de>]

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>

-- 
Jean Delvare
SUSE L3 Support