[PATCH linux dev-4.{7,10} v2] i2c-core: Expand buffers used in i2c_smbus_xfer_emulated()
Andrew Jeffery
andrew at aj.id.au
Wed Mar 15 11:27:45 AEDT 2017
On Tue, 2017-03-14 at 19:50 +1030, 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 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 at jms.id.au>
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > Reviewed-by: Cédric Le Goater <clg at kaod.org>
> ---
> drivers/i2c/i2c-core.c | 74 ++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 56 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index af11b658984d..a46c4a39aee8 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,26 @@ 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);
> + return -EIO;
The irony. Please fix when you apply (if there are no other issues).
> + }
> +
> > + for (i = 0; i < recvd; i++)
> > data->block[i] = msgbuf1[i];
> +
> > break;
> > }
> > - return 0;
> > + }
> +out:
> > + kfree(msgbuf0);
> > + kfree(msgbuf1);
> +
> > + return ret;
> }
>
> /**
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170315/3540068b/attachment.sig>
More information about the openbmc
mailing list