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

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

[Dmitry Torokhov]

i2c_smbus_read_block_data() is dangerous to use because it may deliver
up to I2C_SMBUS_BLOCK_MAX (32) bytes, which may be surprising to the
caller. Callers tend to allocate buffers of sizes big enough to hold
data from a well-behaving device and do not expect that
i2c_smbus_read_block_data() may attempt to write more data than
expected.

To make i2c_smbus_read_block_data() safer to use, change it so that
it accepts size of the supplied buffer as another argument and ensure
that it will not copy more data than the size of the buffer.

To allow users to gradually transition to the new API employ some
macro trickery allowing calling i2c_smbus_read_block_data() with either
3 or 4 arguments. When called with 3 arguments it is assumed that
the buffer size is I2C_SMBUS_BLOCK_MAX bytes. Once everyone is
transitioned to the 4 argument form the macros should be removed.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

This was sent out in October of '24 but unfortunately I did not get a
reply from Wolfram...

 Documentation/i2c/dev-interface.rst |  5 +++--
 drivers/i2c/i2c-core-smbus.c        | 21 +++++++++++++--------
 include/linux/i2c.h                 | 19 +++++++++++++++++--
 3 files changed, 33 insertions(+), 12 deletions(-)

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);
 
 All these transactions return -1 on failure; you can read errno to see
 what happened. The 'write' transactions return 0 on success; the
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index fa63bee0b345..1be0a47c6b30 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -208,11 +208,11 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 
 /**
- * i2c_smbus_read_block_data - SMBus "block read" protocol
+ * __i2c_smbus_read_block_data - SMBus "block read" protocol
  * @client: Handle to slave device
  * @command: Byte interpreted by slave
- * @values: Byte array into which data will be read; big enough to hold
- *	the data returned by the slave.  SMBus allows at most 32 bytes.
+ * @length: size of the @values array. SMBus allows at most 32 bytes
+ * @values: Byte array into which data will be read
  *
  * This executes the SMBus "block read" protocol, returning negative errno
  * else the number of data bytes in the slave's response.
@@ -222,22 +222,27 @@ EXPORT_SYMBOL(i2c_smbus_write_word_data);
  * support this; its emulation through I2C messaging relies on a specific
  * mechanism (I2C_M_RECV_LEN) which may not be implemented.
  */
-s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
-			      u8 *values)
+s32 __i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
+				u8 length, u8 *values)
 {
 	union i2c_smbus_data data;
+	int ret_len;
 	int status;
 
+	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]);
+	memcpy(values, &data.block[1], ret_len);
+	return ret_len;
 }
-EXPORT_SYMBOL(i2c_smbus_read_block_data);
+EXPORT_SYMBOL(__i2c_smbus_read_block_data);
 
 /**
  * i2c_smbus_write_block_data - SMBus "block write" protocol
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 14ab4d3055af..3e685476fa15 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -174,8 +174,23 @@ i2c_smbus_write_word_swapped(const struct i2c_client *client,
 }
 
 /* Returns the number of read bytes */
-s32 i2c_smbus_read_block_data(const struct i2c_client *client,
-			      u8 command, u8 *values);
+s32 __i2c_smbus_read_block_data(const struct i2c_client *client,
+				u8 command, u8 length, u8 *values);
+/*
+ * This monstrosity allows to call i2c_smbus_read_block_data() with either
+ * 3 or 4 arguments and will be removed once all users have been switched
+ * to the 4 argument version.
+ */
+#define __i2c_smbus_read_block_data_3arg(client, cmd, values)			\
+	__i2c_smbus_read_block_data(client, cmd, I2C_SMBUS_BLOCK_MAX, values)
+#define __i2c_smbus_read_block_data_4arg(client, cmd, length, values)		\
+	__i2c_smbus_read_block_data(client, cmd, length, values)
+#define __i2c_smbus_read_block_data_impl(_1, _2, _3, _4, impl, ...) impl
+#define i2c_smbus_read_block_data(client, cmd, varargs...)			\
+	__i2c_smbus_read_block_data_impl(client, cmd, varargs,			\
+					 __i2c_smbus_read_block_data_4arg,	\
+					 __i2c_smbus_read_block_data_3arg)	\
+					 (client, cmd, varargs)
 s32 i2c_smbus_write_block_data(const struct i2c_client *client,
 			       u8 command, u8 length, const u8 *values);
 /* Returns the number of read bytes */
-- 
2.55.0.795.g602f6c329a-goog


-- 
Dmitry

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

[Andi Shyti]

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;
>  }

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