[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 at kaod.org>

Thanks,

C.