On Fri, Jun 21, 2013 at 10:23 PM, Konstantin Filatov
<kfilatov@xxxxxxxxxxxxx> wrote:
> On 06/21/2013 05:47 PM, Ming Lei wrote:
>>
>> The bug can be easily reproduced with Gadget Zero in full_speed mode
>> connected to a host with UHCI controller by the standard test from
>> tools/usb/ with command line
>> testusb -a -t 7 -c 2000 -s 4096 -v 41
>> I am wondering if it is a valid test, since the built sg list has one
>> middle sg
>> which size is not maxp aligned.
>>
>> Did you run the test on ehci controller? and is the transfer OK on ehci
>> controller?
>
> Yes, for ehci the test succeded.
Did you compare the transfer data? And what is the actual transfer length?
>
> This test exists inside linux kernel source tree for long time. It is
> important to check usb infrastructure integrity.
>
> The test utility 'testusb' uses ioctl call to a driver 'usbtest', and
> actually the driver run a test (in the kernel space) on the host. The
> hardware (Gadget Zero device) verifies the test transfer.
>
>>> The test crashes with SIGBUS.
>>
>> IMO, the test may cause buffer crash.
In the current implementation, no mater what 'this_sg_len' is, the transfer
size of td is always set as maxp of the endpoint if it isn't the last packet, so
the middle sg which size isn't maxp aligned will be overflowed for IN transfer,
then buffer crash is caused.
> No buffer crash. The driver signals about the test failed in such way.
>
>
>> --- a/drivers/usb/host/uhci-q.c
>> +++ b/drivers/usb/host/uhci-q.c
>> @@ -977,6 +977,9 @@ static int uhci_submit_common(struct uhci_hcd *uhci,
>> struct urb *urb,
>> for (;;) { /* Allow zero length packets */
>> int pktsze = maxsze;
>>
>> + if (this_sg_len < pktsze) /* The short packet */
>> + pktsze = this_sg_len;
>> This will build a short packet transfer descriptor which is not the last
>> one, and it isn't correct.
>
> Why do you think so? Could you please refer to the specification of UHCI.
See 5.3.2 Pipes of USB 1.1 spec:
An IRP may require multiple data payloads to move the client data over the bus.
The data payloads for such a multiple data payload IRP are expected to be of
the maximum packet size until the last data payload that contains the remainder
of the overall IRP.
And same description can be found in USB 2.0 spec.
>
>> The attached patch may avoid the memory crash, but I am wondering if
>> we need to consider the situation, or a warning should be needed.
>>
>> diff --git a/drivers/usb/host/uhci-q.c b/drivers/usb/host/uhci-q.c
>> index 041c6dd..5c98044 100644
>> --- a/drivers/usb/host/uhci-q.c
>> +++ b/drivers/usb/host/uhci-q.c
>> @@ -969,6 +969,13 @@ static int uhci_submit_common(struct uhci_hcd
>> *uhci, struct urb *urb,
>> pktsze = len;
>> if (!(urb->transfer_flags & URB_SHORT_NOT_OK))
>> status &= ~TD_CTRL_SPD;
>> + } else {
>> + /* skip the middle short transfer */
>> + if (this_sg_len < maxsze) {
>> + len -= this_sg_len;
>> + this_sg_len = 0;
>> + goto skip;
>> + }
>> }
>>
>> if (plink) {
>> @@ -989,6 +996,7 @@ static int uhci_submit_common(struct uhci_hcd
>> *uhci, struct urb *urb,
>> data += pktsze;
>> this_sg_len -= pktsze;
>> len -= maxsze;
>> +skip:
>> if (this_sg_len <= 0) {
>> if (--i <= 0 || len <= 0)
>> break;
>>
>>
>
> Your patch skips a chunk of data to transfer. This is a corruption of data.
How can a corruption of data be caused?
> It's unacceptable. We can return an error code if we treat the request as
> malformed, but we can't transfer data selectively.
>
Thanks,
--
Ming Lei
--
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
