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

[Dmitry Torokhov]

Hi Andi,

On Tue, Jul 28, 2026 at 10:30:33PM +0200, Andi Shyti wrote:
> 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.

Oops, I missed the fact that that document is for libi2c, not the
kernel. I'll drop this chunk.

> 
> 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.

Yeah, I can do this.

Thanks.

-- 
Dmitry