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

[Andrew Jeffery]

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 at 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
-------------- 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/20170314/850af970/attachment.sig>