Re: [RFC PATCH 2/2] usb: dwc3: Add chained TRB support for ep0

[Felipe Balbi]

Hi,

On Mon, Jun 01, 2015 at 03:23:48PM +0530, Kishon Vijay Abraham I wrote:
> >>+		transferred = transfer_size - length;
> >>+		buf = (u8 *)buf + transferred;
> >>+		ur->actual += transferred;
> >
> >this is dangerous. The extra size is because you *must* align OUT to
> >wMaxPacketSize, so you cannot allow more than the original req->length
> >to be copied into buf. That bounce buffer, is really supposed to be a
> 
> Here we are not handling bounce buffer. The bounce buffer is used only for
> the 2nd TRB which actually programs to receive data that is less than bounce
> buffer size. The 1st TRB will always be max packet aligned and the data is
> directly copied to the request buffer. (However note that if the request
> length is less than bounce buffer size, we'll use 1 TRB only)
> 
> To summarize..
> We are splitting req->length into 2 TRB's if the req->length is not aligned
> to wMaxPacketSize _and_ req->length is greater than bounce buffer size. By
> this way we can make the 2nd TRB to receive data lesser than or equal to
> bounce buffer size and the rest of it can be received using the 1st TRB.
> 
> Consider the following case.
> ur->length = 612;
> maxp = 512;
> 
> This case can't be handled by the existing bounce buffer mechanism since the
> size of bounce buffer is only 512. So we program 2 TRB's.
> First TRB
> trb->size = 512; /* We don't need bounce buffer for this TRB since it is max
> packet aligned. The data is directly loaded to the request buffer. */
> 
> Second TRB
> trb->size = 512 /* For the remaining 100 bytes we use bounce buffer and it
> uses the same existing bounce buffer mechanism. But instead of copying the
> data to the start of the request buffer, it has to be appended to the data
> that is received due to first TRB. */

this is all fine, but you need to make sure that you only copy the
remaining 100 bytes. Never, ever, ever copy anything past that.

> >throw-away buffer and should never have data in it. You should really
> >add a big fat WARN() if transferred > req->length.
> >
> >The thing is that if host sends more data than we were expecting, this
> >could be someone trying to use our driver as an exploit vector, trying
> >to send more data than it should. We must be robust against that.
> 
> This is handled in the existing bounce buffer mechanism and I use the same
> bounce buffer mechanism for the 2nd TRB.

ok.

-- 
balbi

Attachment: signature.asc
Description: Digital signature