Patchwork usb-host: fix handling of zero-length packets

login
register
mail settings
Submitter Johannes Stezenbach
Date April 2, 2012, 11:50 a.m.
Message ID <20120402115034.GB22763@sig21.net>
Download mbox | patch
Permalink /patch/150197/
State New
Headers show

Comments

Johannes Stezenbach - April 2, 2012, 11:50 a.m.
Zero-length packets are valid but currently cause
qemu to crash:

Program terminated with signal 11, Segmentation fault.
#0  0xf7589f26 in usb_host_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/host-linux.c:886
886         prem = p->iov.iov[v].iov_len;
(gdb) bt
#0  0xf7589f26 in usb_host_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/host-linux.c:886
#1  0xf757ecf8 in usb_device_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/bus.c:135
#2  0xf75805cc in usb_process_one (p=0xf91606b0) at qemu/hw/usb/core.c:365
#3  0xf7580c9f in usb_handle_packet (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/core.c:385
#4  0xf74f9a0b in ehci_execute (q=0xf9160640) at qemu/hw/usb/hcd-ehci.c:1393
...
(gdb) p p->iov
$3 = {iov = 0x0, niov = 0, nalloc = 0, size = 0}
(gdb) f 4
#4  0xf74f9a0b in ehci_execute (q=0xf9160640) at /home/js/src/other/qemu/hw/usb/hcd-ehci.c:1393
1393        ret = usb_handle_packet(dev, &q->packet);
(gdb) p q->sgl
$4 = {sg = 0xf9160410, nsg = 0, nalloc = 5, size = 0}

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
Note that I didn't do sufficient testing.  I have a proprietary
firmware update tool which does one large bulk transfer with
a size which is a multiple of 512.  With qemu and kvm from
Debian sid (qemu-1.0.1+dfsg-1, kvm-1.0+dfsg-9) the zero-length
packet at the end causes timeout.  qemu from git master
or kraxel/usb.45 crashes.  With this patch my firmware
update worked.

 hw/usb/host-linux.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Johannes Stezenbach - April 17, 2012, 10:06 a.m.
Hi Gerd,

On Mon, Apr 02, 2012 at 01:50:35PM +0200, Johannes Stezenbach wrote:
> Zero-length packets are valid but currently cause
> qemu to crash:

did you have a chance to look at this?


Thanks,
Johannes

> Program terminated with signal 11, Segmentation fault.
> #0  0xf7589f26 in usb_host_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/host-linux.c:886
> 886         prem = p->iov.iov[v].iov_len;
> (gdb) bt
> #0  0xf7589f26 in usb_host_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/host-linux.c:886
> #1  0xf757ecf8 in usb_device_handle_data (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/bus.c:135
> #2  0xf75805cc in usb_process_one (p=0xf91606b0) at qemu/hw/usb/core.c:365
> #3  0xf7580c9f in usb_handle_packet (dev=0xf8fd4820, p=0xf91606b0) at qemu/hw/usb/core.c:385
> #4  0xf74f9a0b in ehci_execute (q=0xf9160640) at qemu/hw/usb/hcd-ehci.c:1393
> ...
> (gdb) p p->iov
> $3 = {iov = 0x0, niov = 0, nalloc = 0, size = 0}
> (gdb) f 4
> #4  0xf74f9a0b in ehci_execute (q=0xf9160640) at /home/js/src/other/qemu/hw/usb/hcd-ehci.c:1393
> 1393        ret = usb_handle_packet(dev, &q->packet);
> (gdb) p q->sgl
> $4 = {sg = 0xf9160410, nsg = 0, nalloc = 5, size = 0}
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> ---
> Note that I didn't do sufficient testing.  I have a proprietary
> firmware update tool which does one large bulk transfer with
> a size which is a multiple of 512.  With qemu and kvm from
> Debian sid (qemu-1.0.1+dfsg-1, kvm-1.0+dfsg-9) the zero-length
> packet at the end causes timeout.  qemu from git master
> or kraxel/usb.45 crashes.  With this patch my firmware
> update worked.
> 
>  hw/usb/host-linux.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
> index a382f0a..35106cd 100644
> --- a/hw/usb/host-linux.c
> +++ b/hw/usb/host-linux.c
> @@ -883,16 +883,15 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
>      }
>  
>      v = 0;
> -    prem = p->iov.iov[v].iov_len;
> -    pbuf = p->iov.iov[v].iov_base;
> +    prem = 0;
> +    pbuf = 0;
>      rem = p->iov.size;
> -    while (rem) {
> -        if (prem == 0) {
> -            v++;
> +    for (;;) {
> +        if (prem == 0 && rem > 0) {
>              assert(v < p->iov.niov);
>              prem = p->iov.iov[v].iov_len;
>              pbuf = p->iov.iov[v].iov_base;
> -            assert(prem <= rem);
> +            v++;
>          }
>          aurb = async_alloc(s);
>          aurb->packet = p;
> @@ -937,6 +936,8 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
>                  return USB_RET_STALL;
>              }
>          }
> +        if (rem == 0)
> +            break;
>      }
>  
>      return USB_RET_ASYNC;
> 
>

Patch

diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index a382f0a..35106cd 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -883,16 +883,15 @@  static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
     }
 
     v = 0;
-    prem = p->iov.iov[v].iov_len;
-    pbuf = p->iov.iov[v].iov_base;
+    prem = 0;
+    pbuf = 0;
     rem = p->iov.size;
-    while (rem) {
-        if (prem == 0) {
-            v++;
+    for (;;) {
+        if (prem == 0 && rem > 0) {
             assert(v < p->iov.niov);
             prem = p->iov.iov[v].iov_len;
             pbuf = p->iov.iov[v].iov_base;
-            assert(prem <= rem);
+            v++;
         }
         aurb = async_alloc(s);
         aurb->packet = p;
@@ -937,6 +936,8 @@  static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
                 return USB_RET_STALL;
             }
         }
+        if (rem == 0)
+            break;
     }
 
     return USB_RET_ASYNC;