Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

[Octavian Purdila <octavian.purdila@intel.com>]

On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@kernel.org> wrote:

<snip>

>> +     /*
>> +      * Buffer to hold the packet for read or write transfers. One
>> +      * is enough since we can't have multiple transfers in
>> +      * parallel on the i2c adapter.
>> +      */
>> +     union {
>> +             struct {
>> +                     u8 port;
>> +                     u8 addr;
>> +                     u8 mem_addr_len;
>> +                     __le32 mem_addr;
>> +                     __le16 buf_len;
>> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> +             } __packed tx;
>> +             struct {
>> +                     __le16 buf_len;
>> +                     u8 buf[DLN2_I2C_MAX_XFER_SIZE];
>> +             } __packed rx;
>> +     } buf;
>
> While this works in this case due to the extra copy you do in
> dln2_transfer, allocating buffers that would (generally) be used for DMA
> transfers as part of a larger structure is a recipe for trouble.
>
> It's probably better to allocate separately, if only to prevent people
> from thinking there might be a bug here.
>

Just to make sure I understand this, what could the issues be? The
buffers not being aligned or not allocated in continuous physical
memory?

<snip>

>> +
>> +     rx_buf_len = le16_to_cpu(dln2->buf.rx.buf_len);
>> +     if (rx_len < rx_buf_len + sizeof(dln2->buf.rx.buf_len))
>> +             return -EPROTO;
>> +
>> +     if (data_len > rx_buf_len)
>> +             data_len = rx_buf_len;
>
> You're still not checking that the received data does not overflow the
> supplied buffer as I already commented on v3.
>
>> +
>> +     memcpy(data, dln2->buf.rx.buf, data_len);
>> +
>> +     return data_len;
>> +}

Hmm, perhaps I am missing something, but we never transfer more then
data_len, where data_len is the size of the buffer supplied by the
user.

<snip>

>> +
>> +     platform_set_drvdata(pdev, dln2);
>> +
>> +     ret = device_create_file(dev, &dev_attr_freq);
>> +     if (ret < 0) {
>> +             dev_err(dev, "failed to add freq attribute\n");
>> +             return ret;
>> +     }
>
> There are a couple of problems here. First, you should not make this an
> attribute of the platform device, which is created before any driver is
> bound (might not ever happen).
>
> Instead add the attribute to the i2c adapter below. However, you need to
> do this using device attribute groups to avoid racing with userspace (as
> you are when using device_create_file after the device itself has been
> created).
>
> You should probably also make your attribute name less generic by adding
> a "dln2_"-prefix.

Thanks for the detailed review and explanations, as always :)