[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