Re: [RESEND PATCH] i2c: smbus: make i2c_smbus_read_block_data() safer

[Andi Shyti <andi.shyti@kernel.org>]

Hi Dmitry,

thanks for your patch. The core, indeed, does not know the size
of the caller's buffer, so this can be, indeed, handled in the
generic I2C code, instead of delegating it to individual host
drivers.

...

> diff --git a/Documentation/i2c/dev-interface.rst b/Documentation/i2c/dev-interface.rst
> index c277a8e1202b..4df5330aca00 100644
> --- a/Documentation/i2c/dev-interface.rst
> +++ b/Documentation/i2c/dev-interface.rst
> @@ -161,9 +161,10 @@ for details) through the following functions::
>    __s32 i2c_smbus_process_call(int file, __u8 command, __u16 value);
>    __s32 i2c_smbus_block_process_call(int file, __u8 command, __u8 length,
>                                       __u8 *values);
> -  __s32 i2c_smbus_read_block_data(int file, __u8 command, __u8 *values);
> +  __s32 i2c_smbus_read_block_data(int file, __u8 command, __u8 length,
> +                                  __u8 *values);
>    __s32 i2c_smbus_write_block_data(int file, __u8 command, __u8 length,
> -                                   __u8 *values);
> +                                   const __u8 *values);

The kernel and userspace APIs should remain aligned. If the
kernel helper gains a buffer-length argument, it makes sense to
consider the same improvement for libi2c.

This requires a coordinated i2c-tools change. Updating this
prototype here alone would document a userspace function which
does not yet exist.

Wolfram, do you have any opinion here?

>  
>  All these transactions return -1 on failure; you can read errno to see
>  what happened. The 'write' transactions return 0 on success; the

...

> +	if (length > I2C_SMBUS_BLOCK_MAX)
> +		return -EINVAL;
> +
>  	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
>  				I2C_SMBUS_READ, command,
>  				I2C_SMBUS_BLOCK_DATA, &data);
>  	if (status)
>  		return status;
>  
> -	memcpy(values, &data.block[1], data.block[0]);
> -	return data.block[0];
> +	ret_len = min(length, data.block[0]);

I don't like silently truncating the response.

If the device returns more data than the supplied buffer can
hold, the caller should be notified. Otherwise, a truncated
response is indistinguishable from a valid shorter response.

I think this should return an error, for example -EMSGSIZE, when
data.block[0] is greater than length.

Thanks,
Andi

> +	memcpy(values, &data.block[1], ret_len);
> +	return ret_len;
>  }