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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Sep 2014 13:07:51 +0300
From:	Octavian Purdila <octavian.purdila@...el.com>
To:	Johan Hovold <johan@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>, wsa@...-dreams.de,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Daniel Baluta <daniel.baluta@...el.com>,
	Laurentiu Palcu <laurentiu.palcu@...el.com>,
	linux-usb@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v4 2/3] i2c: add support for Diolan DLN-2 USB-I2C adapter

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