> From: David Cohen [mailto:david.a.cohen@xxxxxxxxxxxxxxx] > Sent: Tuesday, November 12, 2013 3:44 PM > > On 11/12/2013 03:09 PM, Paul Zimmerman wrote: > > >> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Alan Stern > >> Sent: Tuesday, November 12, 2013 7:51 AM > >> > >> On Mon, 11 Nov 2013, David Cohen wrote: > >> > >>> In order to avoid DWC3 to stall, we need to update req->length (this is > >>> the most important fix). kmalloc() is updated too to prevent USB > >>> controller to overflow buffer boundaries. > >> > >> Here I disagree. > >> > >> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it. > >> Gadget drivers should not have to worry. Most especially, gadget > >> drivers should not lie about a request length. > > > > The whole point of this quirk is to lie to the DWC3 driver. The quirk is > > only enabled for the DWC3 driver. > > > >> If the UDC driver decides to round up req->length before sending it to > >> the hardware, that's okay. > > > > Not really. If the DWC3 driver unconditionally rounds up req->length, > > then in the case where the buffer has not been expanded to a multiple of > > maxpacket, by this quirk or otherwise, there is the potential to write > > beyond the end of the allocation. > > > > If we do what you suggest, then all the gadgets will have to be audited > > to make sure an OUT buffer with a non-aligned length is never passed to > > the DWC3 driver. > > I was really convinced about not updating rep->length was a better idea > until I read this argument of yours: with this approach buffer overflow > can silently happen. > > > > > I still think that's the best solution anyway. Just make that the rule, > > and then there is no need for this quirk at all. And there is no need to > > round up req->length in the DWC3 driver either. > > I'd have nothing against that, but I'm not sure how it would affect > other environments, given embedded environments are pretty sensitive to > memory consumption (and performance). There would be no performance impact. The memory impact would be less than 1024 bytes per OUT ep (worst case), and most gadget devices only have a couple of OUT eps. I don't think it's a problem. > > > >> But req->length should be set to len, not data_len. > > > > According to gadget.h: > > > > @buf: Buffer used for data > > @length: Length of that data > > > > So why shouldn't length be the length of the allocated data buffer? > > Remember, this is for the OUT direction only, so the host has control > > over how much data is actually sent. You could even argue that an OUT > > data buffer should always be a multiple of the max packet size, given > > how USB works. > > How about something like this, to let USB controllers to know about > whole allocated memory and avoid hidden overflows. Does that make sense > to you? > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 25a5007f92e3..973b57b709ab 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -31,13 +31,16 @@ struct usb_ep; > * struct usb_request - describes one i/o request > * @buf: Buffer used for data. Always provide this; some controllers > * only use PIO, or don't use DMA for some endpoints. > + * @length: Length of that data > + * @buf_pad: Some USB controllers may need to pad buffer size due to > alignment > + * constraints. This keeps track of how much memory was allocated > to @buf > + * in addition to @length. > * @dma: DMA address corresponding to 'buf'. If you don't set this > * field, and the usb controller needs one, it is responsible > * for mapping and unmapping the buffer. > * @sg: a scatterlist for SG-capable controllers. > * @num_sgs: number of SG entries > * @num_mapped_sgs: number of SG entries mapped to DMA (internal) > - * @length: Length of that data > * @stream_id: The stream id, when USB3.0 bulk streams are being used > * @no_interrupt: If true, hints that no completion irq is needed. > * Helpful sometimes with deep request queues that are handled > @@ -91,6 +94,7 @@ struct usb_ep; > struct usb_request { > void *buf; > unsigned length; > + unsigned buf_pad; > dma_addr_t dma; > > struct scatterlist *sg; Yes, I think that would work. But you only need 11 bits or so for buf_pad, so make it 'unsigned buf_pad:11" and move it down with the rest of the bitfields. And you could also report -EOVERFLOW in the DWC3 driver, as Alan suggested, if the received data is more than 'length'. -- Paul -- 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
