On Wed, Sep 17, 2014 at 12:44 PM, Johan Hovold <johan@xxxxxxxxxx> 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 :)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
