[PATCH] i2c-core: Expand buffers used in i2c_smbus_xfer_emulated()

[Andrew Jeffery]

Protect against suspect hardware causing the bus to read off more data
than defined in the SMBus specification.

Some investigation that suggests that the DEVICE_ID command (0xfd) to
a UCD9000 occasionally returns all 0xff, causing the bus driver to
overrun the 34-byte data buffer provided by the I2C core with a 255-byte
broken response. This patch should prevent trashing the stack and also
cause the UCD9000's probe to fail.

The I2C core doesn't help itself in this case as it does not provide any
means for the bus driver to sanity check the caller-provided
response-buffer length against the device-provided payload length. At
the point where we perform the I2C transfers for the SMBus protocol
we've lost the information about what type of transaction is being
performed (i.e. that we're performing an SMBus block transfer), so we
cannot in general make assumptions about the buffer size based on the
protocol (i2c, SMBus, PMBus).  Further, struct i2c_msg is defined in the
uapi headers so no further members (e.g. a length field) can be added
without breaking userspace.

Instead, let's expand the buffers such that they will fit any payload
length reported by the bus.

The patch has been tested on a Zaius system, consecutively unbinding and
binding the UCD9000 driver over 1000 times. Whilst errors were reported
during probe in some cases ("ucd9000: probe of 0-0064 failed with error
-74"), the BMC remained stable and did not reboot. Triggering the bug
without the patch typically at most requires in the order of 100
unbind/bind attempts, often significantly less.

Finally, the host was successfully powered up immediately after the 1000
unbind/bind attempts.

Suggested-by: Joel Stanley <joel at jms.id.au>
Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
---
 drivers/i2c/i2c-core.c | 61 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index af11b658984d..072fd82920a2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2894,16 +2894,26 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 				   char read_write, u8 command, int size,
 				   union i2c_smbus_data *data)
 {
-	/* So we need to generate a series of msgs. In the case of writing, we
-	  need to use only one message; when reading, we need two. We initialize
-	  most things with sane defaults, to keep the code below somewhat
-	  simpler. */
-	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
-	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
+	/*
+	 * So we need to generate a series of msgs. In the case of writing, we
+	 * need to use only one message; when reading, we need two. We initialize
+	 * most things with sane defaults, to keep the code below somewhat
+	 * simpler.
+	 *
+	 * Also, allocate 256 byte buffers as protection against suspect
+	 * hardware like the UCD90xx, where on occasion the returned block
+	 * length is 0xff[1].
+	 *
+	 * [1] https://github.com/openbmc/openbmc/issues/998
+	 */
+	unsigned char *msgbuf0 = kmalloc(0x100, GFP_KERNEL);
+	unsigned char *msgbuf1 = kmalloc(0x100, GFP_KERNEL);
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
-	int i;
 	u8 partial_pec = 0;
 	int status;
+	s32 ret;
+	int i;
+
 	struct i2c_msg msg[2] = {
 		{
 			.addr = addr,
@@ -2918,6 +2928,9 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 		},
 	};
 
+	if (!(msgbuf0 && msgbuf1))
+		return -ENOMEM;
+
 	msgbuf0[0] = command;
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -2970,7 +2983,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 				dev_err(&adapter->dev,
 					"Invalid block write size %d\n",
 					data->block[0]);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto out;
 			}
 			for (i = 1; i < msg[0].len; i++)
 				msgbuf0[i] = data->block[i-1];
@@ -2983,7 +2997,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			dev_err(&adapter->dev,
 				"Invalid block write size %d\n",
 				data->block[0]);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		msg[0].len = data->block[0] + 2;
 		for (i = 1; i < msg[0].len; i++)
@@ -3001,7 +3016,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 				dev_err(&adapter->dev,
 					"Invalid block write size %d\n",
 					data->block[0]);
-				return -EINVAL;
+				ret = -EINVAL;
+				goto out;
 			}
 			for (i = 1; i <= data->block[0]; i++)
 				msgbuf0[i] = data->block[i];
@@ -3009,7 +3025,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 		break;
 	default:
 		dev_err(&adapter->dev, "Unsupported transaction %d\n", size);
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto out;
 	}
 
 	i = ((flags & I2C_CLIENT_PEC) && size != I2C_SMBUS_QUICK
@@ -3028,14 +3045,18 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	}
 
 	status = i2c_transfer(adapter, msg, num);
-	if (status < 0)
-		return status;
+	if (status < 0) {
+		ret = status;
+		goto out;
+	}
 
 	/* Check PEC if last message is a read */
 	if (i && (msg[num-1].flags & I2C_M_RD)) {
 		status = i2c_smbus_check_pec(partial_pec, &msg[num-1]);
-		if (status < 0)
-			return status;
+		if (status < 0) {
+			ret = status;
+			goto out;
+		}
 	}
 
 	if (read_write == I2C_SMBUS_READ)
@@ -3056,11 +3077,17 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 			break;
 		case I2C_SMBUS_BLOCK_DATA:
 		case I2C_SMBUS_BLOCK_PROC_CALL:
-			for (i = 0; i < msgbuf1[0] + 1; i++)
+			for (i = 0;
+			     i < min((size_t)(msgbuf1[0] + 1), sizeof(*data));
+			     i++)
 				data->block[i] = msgbuf1[i];
 			break;
 		}
-	return 0;
+out:
+	kfree(msgbuf0);
+	kfree(msgbuf1);
+
+	return ret;
 }
 
 /**
-- 
2.9.3