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

[Andrew Jeffery]

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
-------------- 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/20170311/7cf7d5bc/attachment.sig>