i2c-core: Expand buffers used in i2c_smbus_xfer_emulated()

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 UCD9xxx power sequencer 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. Usually the result was the following:

         Kernel panic - not syncing: stack-protector: Kernel stack is corrupted
         in: 802e5e38

         CPU: 0 PID: 1559 Comm: zaius_vcs.sh Not tainted
         4.7.10-f26558191540830589fe03932d05577957670b8d #2
         Hardware name: ASpeed SoC
         [<801072f8>] (unwind_backtrace) from [<80105558>] (show_stack+0x10/0x14)
         [<80105558>] (show_stack) from [<8016d06c>] (panic+0xb8/0x248)
         [<8016d06c>] (panic) from [<80110df4>] (__stack_chk_fail+0x10/0x14)
         [<80110df4>] (__stack_chk_fail) from [<802e5e38>]
         (i2c_smbus_xfer+0x508/0x54c)
         [<802e5e38>] (i2c_smbus_xfer) from [<ffffffff>] (0xffffffff)

This patch prevents the broken response from trashing the surrounding
memory and returns an error code up the call-chain.

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 move the buffers off the stack and expand them 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 system 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@jms.id.au>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>

1 files changed, 57 insertions, 18 deletions

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index af11b658984d..674a1e514c94 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;
+	s32 ret = 0;
 	int status;
+	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,17 +3045,23 @@ 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)
+	if (read_write == I2C_SMBUS_READ) {
+		size_t recvd;
+
 		switch (size) {
 		case I2C_SMBUS_BYTE:
 			data->byte = msgbuf0[0];
@@ -3056,11 +3079,27 @@ 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++)
+			recvd = msgbuf1[0] + 1;
+
+			if (recvd > sizeof(*data)) {
+				dev_warn(&adapter->dev,
+						"SMBus block operation received invalid payload length of %zu\n",
+						recvd);
+				ret = -EIO;
+				goto out;
+			}
+
+			for (i = 0; i < recvd; i++)
 				data->block[i] = msgbuf1[i];
+
 			break;
 		}
-	return 0;
+	}
+out:
+	kfree(msgbuf0);
+	kfree(msgbuf1);
+
+	return ret;
 }
 
 /**