[PATCH] i2c-core: Expand buffers used in i2c_smbus_xfer_emulated()
Cédric Le Goater
clg at kaod.org
Sat Mar 11 00:38:24 AEDT 2017
On 03/10/2017 01:21 PM, Andrew Jeffery wrote:
> 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.
I could not reproduce the issue on a witherspoon after a 1000 unbind/bind
but there were some errors still :
ucd9000 11-0064: Device ID UCD90160|2.3.4.0000|110603
ucd9000 11-0064: Failed to read number of active pages
ucd9000: probe of 11-0064 failed with error -74
ucd9000 11-0064: Device ID UCD90160|2.3.4.0000|110603
...
ucd9000 11-0064: Device ID UCD90160|2.3.4.0000|110603
ucd9000 11-0064: Device ID UCD90160|2.3.4.0000|110603
ucd9000 11-0064: Device ID UCD90160|2.3.4.0000|110603
i2c_aspeed i2c-11: Invalid state (msg (null), pending 0)?
i2c_aspeed i2c-11: Invalid state (msg (null), pending 0)?
i2c_aspeed i2c-11: Invalid state (msg (null), pending 0)?
i2c i2c-11: i2c_ast: timeout waiting for bus free
i2c_aspeed i2c-11: Invalid state (msg (null), pending 0)?
i2c_aspeed i2c-11: Invalid state (msg (null), pending 0)?
i2c i2c-11: i2c_ast: timeout waiting for bus free
The last ones suggest that there is an issue with bus recovery.
Do All the systems have a UCD90160 ?
Thanks,
C.
> 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;
> }
>
> /**
>
More information about the openbmc
mailing list