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