[PATCH] i2c: smbus: drop redundant block size checks

[PATCH] i2c: smbus: drop redundant block size checks

[Jean Delvare]

Commit 3051cd060fa4 ("i2c: smbus: reject oversized block transfers in
the common path") added a block size check in __i2c_smbus_xfer(),
before the tracepoints. Such checks were already present but later in
the call chain, in i2c_smbus_xfer_emulated(). Thus we now have
redundant checks and the checks in i2c_smbus_xfer_emulated() can no
longer fail. This is effectively dead code so we can drop it.

One difference between the original checks and the new one is that
the original checks logged an error message when an oversized block
transfer was attempted. Restore this behavior.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
Another difference between the checks is that the new check rejects
zero-sized block transfers. While they are uncommon and were not
valid under SMBus 2.0, they are now explicitly allowed by SMBus 3.0,
so disallowing them could cause problems in the future.

While I can see how oversized block transfers could cause an
out-of-buffer memory access in the tracepoint code, I fail to see
why zero-sized block transfers would be problematic. Is there any
actual code path that was found to break with block size 0?

 drivers/i2c/i2c-core-smbus.c |   29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

--- linux-7.3.orig/drivers/i2c/i2c-core-smbus.c
+++ linux-7.3/drivers/i2c/i2c-core-smbus.c
@@ -318,7 +318,8 @@ static void i2c_smbus_try_get_dmabuf(str
 
 /*
  * Simulate a SMBus command using the I2C protocol.
- * No checking of parameters is done!
+ * No checking of parameters is done, block length has been checked
+ * in __i2c_smbus_xfer() already.
  */
 static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 				   unsigned short flags,
@@ -402,13 +403,6 @@ static s32 i2c_smbus_xfer_emulated(struc
 			i2c_smbus_try_get_dmabuf(&msg[1], 0);
 		} else {
 			msg[0].len = data->block[0] + 2;
-			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
-				dev_err(&adapter->dev,
-					"Invalid block write size %d\n",
-					data->block[0]);
-				return -EINVAL;
-			}
-
 			i2c_smbus_try_get_dmabuf(&msg[0], command);
 			memcpy(msg[0].buf + 1, data->block, msg[0].len - 1);
 		}
@@ -416,12 +410,6 @@ static s32 i2c_smbus_xfer_emulated(struc
 	case I2C_SMBUS_BLOCK_PROC_CALL:
 		nmsgs = 2; /* Another special case */
 		read_write = I2C_SMBUS_READ;
-		if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
-			dev_err(&adapter->dev,
-				"Invalid block write size %d\n",
-				data->block[0]);
-			return -EINVAL;
-		}
 
 		msg[0].len = data->block[0] + 2;
 		i2c_smbus_try_get_dmabuf(&msg[0], command);
@@ -433,13 +421,6 @@ static s32 i2c_smbus_xfer_emulated(struc
 		i2c_smbus_try_get_dmabuf(&msg[1], 0);
 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
-		if (data->block[0] > I2C_SMBUS_BLOCK_MAX) {
-			dev_err(&adapter->dev, "Invalid block %s size %d\n",
-				str_read_write(read_write == I2C_SMBUS_READ),
-				data->block[0]);
-			return -EINVAL;
-		}
-
 		if (read_write == I2C_SMBUS_READ) {
 			msg[1].len = data->block[0];
 			i2c_smbus_try_get_dmabuf(&msg[1], 0);
@@ -576,8 +557,12 @@ s32 __i2c_smbus_xfer(struct i2c_adapter
 	     (protocol == I2C_SMBUS_BLOCK_DATA &&
 	      read_write == I2C_SMBUS_WRITE)) &&
 	    (data->block[0] == 0 ||
-	     data->block[0] > I2C_SMBUS_BLOCK_MAX))
+	     data->block[0] > I2C_SMBUS_BLOCK_MAX)) {
+		dev_err(&adapter->dev, "Invalid block %s size %hhu\n",
+			str_read_write(read_write == I2C_SMBUS_READ),
+			data->block[0]);
 		return -EINVAL;
+	}
 
 	/* If enabled, the following two tracepoints are conditional on
 	 * read_write and protocol.


-- 
Jean Delvare
SUSE L3 Support