[Qemu-devel] [PULL 0/6] usb: misc bugfixes

[Qemu-devel] [PULL 0/6] usb: misc bugfixes

[Gerd Hoffmann]

  Hi,

Here comes the usb patch queue, with a collection
of small usb bugfixes in various places.

please pull,
  Gerd

The following changes since commit 8eb779e4223a18db9838a49ece1bc72cfdfb7761:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2016-02-22 16:55:41 +0000)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-usb-20160223-1

for you to fetch changes up to fa1298c2d623522eda7b4f1f721fcb935abb7360:

  ohci: allocate timer only once. (2016-02-23 11:13:18 +0100)

----------------------------------------------------------------
usb: misc bugfixes.

----------------------------------------------------------------
Gerd Hoffmann (1):
      ohci: allocate timer only once.

Gonglei (1):
      usb: add pid check at the first of uhci_handle_td()

Peter Maydell (1):
      tusb6010: move from hw/timer to hw/usb

Prasad J Pandit (3):
      usb: check USB configuration descriptor object
      usb: check RNDIS message length
      usb: check RNDIS buffer offsets & length

 hw/timer/Makefile.objs       |  1 -
 hw/usb/Makefile.objs         |  2 ++
 hw/usb/core.c                | 18 +++++++++---------
 hw/usb/dev-network.c         | 12 ++++++++----
 hw/usb/hcd-ohci.c            | 34 ++++++++--------------------------
 hw/usb/hcd-uhci.c            | 22 ++++++++++++++++------
 hw/{timer => usb}/tusb6010.c |  0
 7 files changed, 43 insertions(+), 46 deletions(-)
 rename hw/{timer => usb}/tusb6010.c (100%)

[Qemu-devel] [PULL 1/6] usb: check USB configuration descriptor object

[Gerd Hoffmann]

From: Prasad J Pandit <pjp@fedoraproject.org>

When processing remote NDIS control message packets, the USB Net
device emulator checks to see if the USB configuration descriptor
object is of RNDIS type(2). But it does not check if it is null,
which leads to a null dereference error. Add check to avoid it.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Message-id: 1455188480-14688-1-git-send-email-ppandit@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-network.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 985a629..5dc4538 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -654,7 +654,8 @@ typedef struct USBNetState {
 
 static int is_rndis(USBNetState *s)
 {
-    return s->dev.config->bConfigurationValue == DEV_RNDIS_CONFIG_VALUE;
+    return s->dev.config ?
+            s->dev.config->bConfigurationValue == DEV_RNDIS_CONFIG_VALUE : 0;
 }
 
 static int ndis_query(USBNetState *s, uint32_t oid,
-- 
1.8.3.1

[Qemu-devel] [PULL 2/6] tusb6010: move from hw/timer to hw/usb

[Gerd Hoffmann]

From: Peter Maydell <peter.maydell@linaro.org>

The TUSB6010 is a USB controller (as the name suggests). Move it from
hw/timer (where it was accidentally filed in 2013 when we moved
everything out of hw/) to hw/usb.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 1455883404-10976-1-git-send-email-peter.maydell@linaro.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/timer/Makefile.objs       | 1 -
 hw/usb/Makefile.objs         | 2 ++
 hw/{timer => usb}/tusb6010.c | 0
 3 files changed, 2 insertions(+), 1 deletion(-)
 rename hw/{timer => usb}/tusb6010.c (100%)

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 133bd0d..5cfea6e 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -25,7 +25,6 @@ obj-$(CONFIG_OMAP) += omap_gptimer.o
 obj-$(CONFIG_OMAP) += omap_synctimer.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
 obj-$(CONFIG_SH4) += sh_timer.o
-obj-$(CONFIG_TUSB6010) += tusb6010.o
 obj-$(CONFIG_DIGIC) += digic-timer.o
 
 obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 8f00fbd..2717027 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -10,6 +10,8 @@ common-obj-$(CONFIG_USB_EHCI_SYSBUS) += hcd-ehci-sysbus.o
 common-obj-$(CONFIG_USB_XHCI) += hcd-xhci.o
 common-obj-$(CONFIG_USB_MUSB) += hcd-musb.o
 
+obj-$(CONFIG_TUSB6010) += tusb6010.o
+
 # emulated usb devices
 common-obj-$(CONFIG_USB) += dev-hub.o
 common-obj-$(CONFIG_USB) += dev-hid.o
diff --git a/hw/timer/tusb6010.c b/hw/usb/tusb6010.c
similarity index 100%
rename from hw/timer/tusb6010.c
rename to hw/usb/tusb6010.c
-- 
1.8.3.1

[Qemu-devel] [PULL 3/6] usb: check RNDIS message length

[Gerd Hoffmann]

From: Prasad J Pandit <pjp@fedoraproject.org>

When processing remote NDIS control message packets, the USB Net
device emulator uses a fixed length(4096) data buffer. The incoming
packet length could exceed this limit. Add a check to avoid it.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Message-id: 1455648821-17340-2-git-send-email-ppandit@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index bea5e1e..45fa00c 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -129,9 +129,16 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
     }
 
     usb_packet_copy(p, s->setup_buf, p->iov.size);
+    s->setup_index = 0;
     p->actual_length = 0;
     s->setup_len   = (s->setup_buf[7] << 8) | s->setup_buf[6];
-    s->setup_index = 0;
+    if (s->setup_len > sizeof(s->data_buf)) {
+        fprintf(stderr,
+                "usb_generic_handle_packet: ctrl buffer too small (%d > %zu)\n",
+                s->setup_len, sizeof(s->data_buf));
+        p->status = USB_RET_STALL;
+        return;
+    }
 
     request = (s->setup_buf[0] << 8) | s->setup_buf[1];
     value   = (s->setup_buf[3] << 8) | s->setup_buf[2];
@@ -152,13 +159,6 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
         }
         s->setup_state = SETUP_STATE_DATA;
     } else {
-        if (s->setup_len > sizeof(s->data_buf)) {
-            fprintf(stderr,
-                "usb_generic_handle_packet: ctrl buffer too small (%d > %zu)\n",
-                s->setup_len, sizeof(s->data_buf));
-            p->status = USB_RET_STALL;
-            return;
-        }
         if (s->setup_len == 0)
             s->setup_state = SETUP_STATE_ACK;
         else
@@ -177,7 +177,7 @@ static void do_token_in(USBDevice *s, USBPacket *p)
     request = (s->setup_buf[0] << 8) | s->setup_buf[1];
     value   = (s->setup_buf[3] << 8) | s->setup_buf[2];
     index   = (s->setup_buf[5] << 8) | s->setup_buf[4];
- 
+
     switch(s->setup_state) {
     case SETUP_STATE_ACK:
         if (!(s->setup_buf[0] & USB_DIR_IN)) {
-- 
1.8.3.1

[Qemu-devel] [PULL 4/6] usb: check RNDIS buffer offsets & length

[Gerd Hoffmann]

From: Prasad J Pandit <pjp@fedoraproject.org>

When processing remote NDIS control message packets,
the USB Net device emulator uses a fixed length(4096) data buffer.
The incoming informationBufferOffset & Length combination could
overflow and cross that range. Check control message buffer
offsets and length to avoid it.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Message-id: 1455648821-17340-3-git-send-email-ppandit@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-network.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5dc4538..c6abd38 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -916,8 +916,9 @@ static int rndis_query_response(USBNetState *s,
 
     bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
     buflen = le32_to_cpu(buf->InformationBufferLength);
-    if (bufoffs + buflen > length)
+    if (buflen > length || bufoffs >= length || bufoffs + buflen > length) {
         return USB_RET_STALL;
+    }
 
     infobuflen = ndis_query(s, le32_to_cpu(buf->OID),
                             bufoffs + (uint8_t *) buf, buflen, infobuf,
@@ -962,8 +963,9 @@ static int rndis_set_response(USBNetState *s,
 
     bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
     buflen = le32_to_cpu(buf->InformationBufferLength);
-    if (bufoffs + buflen > length)
+    if (buflen > length || bufoffs >= length || bufoffs + buflen > length) {
         return USB_RET_STALL;
+    }
 
     ret = ndis_set(s, le32_to_cpu(buf->OID),
                     bufoffs + (uint8_t *) buf, buflen);
@@ -1213,8 +1215,9 @@ static void usb_net_handle_dataout(USBNetState *s, USBPacket *p)
     if (le32_to_cpu(msg->MessageType) == RNDIS_PACKET_MSG) {
         uint32_t offs = 8 + le32_to_cpu(msg->DataOffset);
         uint32_t size = le32_to_cpu(msg->DataLength);
-        if (offs + size <= len)
+        if (offs < len && size < len && offs + size <= len) {
             qemu_send_packet(qemu_get_queue(s->nic), s->out_buf + offs, size);
+        }
     }
     s->out_ptr -= len;
     memmove(s->out_buf, &s->out_buf[len], s->out_ptr);
-- 
1.8.3.1

[Qemu-devel] [PULL 5/6] usb: add pid check at the first of uhci_handle_td()

[Gerd Hoffmann]

From: Gonglei <arei.gonglei@huawei.com>

pid can be gotten from uhci device memory in uhci_handle_td(),
so the guest can trigger assert qemu if we get an invalid pid.
And the uhci spec 2.1.2 tells us The Host Controller sets Host
Controller Process Error bit to 1 when it detects a fatal error
and indicates that the Host Controller suffered a consistency
check failure while processing a Transfer Descriptor. An example
of a consistency check failure would be finding an illegal PID
field while processing the packet header portion of the TD.
When this error occurs, the Host Controller clears the Run/Stop
bit in the Command register to prevent further schedule execution.

We'd better to set UHCI_STS_HCPERR and kick an interrupt, check
the pid value at the first of uhci_handle_td function.

https://bugzilla.redhat.com/show_bug.cgi?id=1070027

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Message-id: 1455867238-4720-1-git-send-email-arei.gonglei@huawei.com

[ applied minor codestyle fix ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 5ccfb83..c370240 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -773,8 +773,22 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
     bool spd;
     bool queuing = (q != NULL);
     uint8_t pid = td->token & 0xff;
-    UHCIAsync *async = uhci_async_find_td(s, td_addr);
+    UHCIAsync *async;
 
+    switch (pid) {
+    case USB_TOKEN_OUT:
+    case USB_TOKEN_SETUP:
+    case USB_TOKEN_IN:
+        break;
+    default:
+        /* invalid pid : frame interrupted */
+        s->status |= UHCI_STS_HCPERR;
+        s->cmd &= ~UHCI_CMD_RS;
+        uhci_update_irq(s);
+        return TD_RESULT_STOP_FRAME;
+    }
+
+    async = uhci_async_find_td(s, td_addr);
     if (async) {
         if (uhci_queue_verify(async->queue, qh_addr, td, td_addr, queuing)) {
             assert(q == NULL || q == async->queue);
@@ -880,11 +894,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
         break;
 
     default:
-        /* invalid pid : frame interrupted */
-        uhci_async_free(async);
-        s->status |= UHCI_STS_HCPERR;
-        uhci_update_irq(s);
-        return TD_RESULT_STOP_FRAME;
+        abort(); /* Never to execute */
     }
 
     if (async->packet.status == USB_RET_ASYNC) {
-- 
1.8.3.1

[Qemu-devel] [PULL 6/6] ohci: allocate timer only once.

[Gerd Hoffmann]

Allocate timer once, at init time, instead of allocating/freeing
it all the time when starting/stopping the bus.  Simplifies the
code, also fixes bugs (memory leak) due to missing checks whenever
the time is already allocated or not.

Cc: Prasad J Pandit <pjp@fedoraproject.org>
Reported-by: Zuozhi Fzz <zuozhi.fzz@alibaba-inc.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ohci.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index bed55dd..17ed461 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1347,16 +1347,6 @@ static void ohci_frame_boundary(void *opaque)
  */
 static int ohci_bus_start(OHCIState *ohci)
 {
-    ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                    ohci_frame_boundary,
-                    ohci);
-
-    if (ohci->eof_timer == NULL) {
-        trace_usb_ohci_bus_eof_timer_failed(ohci->name);
-        ohci_die(ohci);
-        return 0;
-    }
-
     trace_usb_ohci_start(ohci->name);
 
     /* Delay the first SOF event by one frame time as
@@ -1373,11 +1363,7 @@ static int ohci_bus_start(OHCIState *ohci)
 static void ohci_bus_stop(OHCIState *ohci)
 {
     trace_usb_ohci_stop(ohci->name);
-    if (ohci->eof_timer) {
-        timer_del(ohci->eof_timer);
-        timer_free(ohci->eof_timer);
-    }
-    ohci->eof_timer = NULL;
+    timer_del(ohci->eof_timer);
 }
 
 /* Sets a flag in a port status register but only set it if the port is
@@ -1907,6 +1893,9 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
     usb_packet_init(&ohci->usb_packet);
 
     ohci->async_td = 0;
+
+    ohci->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                   ohci_frame_boundary, ohci);
 }
 
 #define TYPE_PCI_OHCI "pci-ohci"
@@ -1976,6 +1965,9 @@ static void usb_ohci_exit(PCIDevice *dev)
     if (!ohci->masterbus) {
         usb_bus_release(&s->bus);
     }
+
+    timer_del(s->eof_timer);
+    timer_free(s->eof_timer);
 }
 
 static void usb_ohci_reset_pci(DeviceState *d)
@@ -2041,23 +2033,13 @@ static bool ohci_eof_timer_needed(void *opaque)
 {
     OHCIState *ohci = opaque;
 
-    return ohci->eof_timer != NULL;
-}
-
-static int ohci_eof_timer_pre_load(void *opaque)
-{
-    OHCIState *ohci = opaque;
-
-    ohci_bus_start(ohci);
-
-    return 0;
+    return timer_pending(ohci->eof_timer);
 }
 
 static const VMStateDescription vmstate_ohci_eof_timer = {
     .name = "ohci-core/eof-timer",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_load = ohci_eof_timer_pre_load,
     .needed = ohci_eof_timer_needed,
     .fields = (VMStateField[]) {
         VMSTATE_TIMER_PTR(eof_timer, OHCIState),
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 0/6] usb: misc bugfixes

[Peter Maydell]

On 23 February 2016 at 10:54, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here comes the usb patch queue, with a collection
> of small usb bugfixes in various places.
>
> please pull,
>   Gerd
>
> The following changes since commit 8eb779e4223a18db9838a49ece1bc72cfdfb7761:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2016-02-22 16:55:41 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-usb-20160223-1
>
> for you to fetch changes up to fa1298c2d623522eda7b4f1f721fcb935abb7360:
>
>   ohci: allocate timer only once. (2016-02-23 11:13:18 +0100)
>
> ----------------------------------------------------------------
> usb: misc bugfixes.

Applied, thanks.

-- PMM