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

[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@jms.id.au>
Signed-off-by: Andrew Jeffery <andrew@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

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Fri, 2017-03-10 at 22:51 +1030, Andrew Jeffery wrote:
> Protect against suspect hardware causing the bus to read off more data
> than defined in the SMBus specification.

Sorry, I forgot to set the subject prefix. The patch applies cleanly to
dev-4.7 and dev-4.10, and should probably be applied to both.

The dev-4.7 tree with this patch applied was tested on Zaius.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Cédric Le Goater]

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@jms.id.au>
> Signed-off-by: Andrew Jeffery <andrew@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;
>  }
>  
>  /**
> 

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

[Cédric Le Goater]

On 03/10/2017 02:38 PM, Cédric Le Goater wrote:
> 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 :

It failed after 1720 unbind/bind ... So I applied the patch but with it, the 
device as a weird id :  

[  165.830000] ucd9000 11-0064: Device ID �
[  165.830000] ucd9000 11-0064: Unsupported device

C.
 
> 	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@jms.id.au>
>> Signed-off-by: Andrew Jeffery <andrew@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;
>>  }
>>  
>>  /**
>>
> 

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

[Xo Wang]

On Fri, Mar 10, 2017 at 6:20 AM, Cédric Le Goater <clg@kaod.org> wrote:
> On 03/10/2017 02:38 PM, Cédric Le Goater wrote:
>> 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.
>>>

Excellent. We tracked down failures to the same error, that the
occasional 0xff from the UCD is interpreted as SMBus block length.
However, the symptom we observed for the overrun is a kernel panic:

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)

I'm not sure that the UCD probe failures are totally related to the
bug that this patch fixes.

cheers
xo

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2994 bytes --]

On Fri, 2017-03-10 at 15:20 +0100, Cédric Le Goater wrote:
> On 03/10/2017 02:38 PM, Cédric Le Goater wrote:
> > 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 :
> 
> It failed after 1720 unbind/bind ... So I applied the patch but with it, the 
> device as a weird id :  
> 
> [  165.830000] ucd9000 11-0064: Device ID �
> [  165.830000] ucd9000 11-0064: Unsupported device

A couple of notes were missing from the commit message - the warning of
 stack corruption and stack trace that Xo mentioned, and that the
driver will produce this message when we see the failure that the patch
is working around. If the kernel didn't panic/reboot after seeing this
then the workaround was successful.

The bus driver can't determine whether the buffer is invalid or not at
the point of assembling the response, and so can't error out. Instead,
we pass the buffer back out to the ucd9000, which errors out during
probe as the response is not what it was expecting.

I can send again with an updated commit message if there's no other
feedback.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Cédric Le Goater]

On 03/10/2017 10:59 PM, Andrew Jeffery wrote:
> On Fri, 2017-03-10 at 15:20 +0100, Cédric Le Goater wrote:
>> On 03/10/2017 02:38 PM, Cédric Le Goater wrote:
>>> 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 :
>>
>> It failed after 1720 unbind/bind ... So I applied the patch but with it, the 
>> device as a weird id :  
>>
>> [  165.830000] ucd9000 11-0064: Device ID �
>> [  165.830000] ucd9000 11-0064: Unsupported device
> 
> A couple of notes were missing from the commit message - the warning of
>  stack corruption and stack trace that Xo mentioned, and that the
> driver will produce this message when we see the failure that the patch
> is working around. If the kernel didn't panic/reboot after seeing this
> then the workaround was successful.
> 
> The bus driver can't determine whether the buffer is invalid or not at
> the point of assembling the response, and so can't error out. Instead,
> we pass the buffer back out to the ucd9000, which errors out during
> probe as the response is not what it was expecting.
> 
> I can send again with an updated commit message if there's no other
> feedback.

No. It looks good to me.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

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

[Cédric Le Goater]

On 03/10/2017 10:17 PM, Xo Wang wrote:
> On Fri, Mar 10, 2017 at 6:20 AM, Cédric Le Goater <clg@kaod.org> wrote:
>> On 03/10/2017 02:38 PM, Cédric Le Goater wrote:
>>> 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.
>>>>
> 
> Excellent. We tracked down failures to the same error, that the
> occasional 0xff from the UCD is interpreted as SMBus block length.
> However, the symptom we observed for the overrun is a kernel panic:
> 
> 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)
> 
> I'm not sure that the UCD probe failures are totally related to the
> bug that this patch fixes.

No. I think the device is sometimes quite slow to respond and also 
the bus recovery path seems a bit fragile.

C. 

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

[Joel Stanley]

On Fri, Mar 10, 2017 at 10:51 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> --- 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);

kzalloc?


> @@ -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++)

Lets assume msgbuf1[0] + 1 > sizeof(*data) means junk in the buffer
and return an error instead of a buffer of junk.

If you want to print a debug message there it might be useful for
future debugging.

>                                 data->block[i] = msgbuf1[i];
>                         break;
>                 }
> -       return 0;
> +out:
> +       kfree(msgbuf0);
> +       kfree(msgbuf1);

Do we cleanup in all paths? Is there any value in using devm_ apis?

Cheers,

Joel

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3731 bytes --]

In addition to your comments there's also a bug, ret needs to be
initialised to zero. I had the change locally and had tested it, but
had forgotten to amend the commit.

On Tue, 2017-03-14 at 15:59 +1030, Joel Stanley wrote:
> > On Fri, Mar 10, 2017 at 10:51 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > --- 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);
> 
> kzalloc?

Arguably not required as in the valid response case we read off the
correct number of bytes, and in the invalid response case we're going
to return an error.

> 
> 
> > @@ -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++)
> 
> Lets assume msgbuf1[0] + 1 > sizeof(*data) means junk in the buffer
> and return an error instead of a buffer of junk.
> 
> If you want to print a debug message there it might be useful for
> future debugging.

I'll do both.

> 
> >                                 data->block[i] = msgbuf1[i];
> >                         break;
> >                 }
> > -       return 0;
> > +out:
> > +       kfree(msgbuf0);
> > +       kfree(msgbuf1);
> 
> Do we cleanup in all paths? 

I searched 'return' and replaced it with goto in each instance. So as
long as that's good enough, yes.

> Is there any value in using devm_ apis?

No value in devm_ as we don't want the memory to hang around until
unbind.

Unless we do use devm_ and stash the pointers somewhere in the adapter?
That would amortise the allocation cost, but it's more changes to the
core.

Andrew

> 
> Cheers,
> 
> Joel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]