[Qemu-devel] [PULL 00/11] usb patch queue

[Qemu-devel] [PULL 00/11] usb patch queue

[Gerd Hoffmann]

  Hi,

Here is the usb patch queue, bringing the usual share of bugfixes.
Also a generic xhci device variant (qemu-xhci).

please pull,
  Gerd

The following changes since commit 56f9e46b841c7be478ca038d8d4085d776ab4b0d:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2017-02-20' into staging (2017-02-20 17:42:47 +0000)

are available in the git repository at:


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

for you to fetch changes up to 31fb4444a485a348f8e2699d7c3dd15e1819ad2c:

  usb-ccid: add check message size checks (2017-02-21 08:11:43 +0100)

----------------------------------------------------------------
xhci: add qemu-xhci device, some followup cleanups.
ccid: better sanity checking.
ehci: fix memory leak
ohci: bugfixes.

----------------------------------------------------------------
Gerd Hoffmann (8):
      xhci: apply limits to loops
      xhci: drop ER_FULL_HACK workaround
      xhci: add qemu xhci controller
      xhci: fix nec vendor quirk handling
      xhci: drop via vendor command handling
      usb-ccid: better bulk_out error handling
      usb-ccid: move header size check
      usb-ccid: add check message size checks

Li Qiang (3):
      usb: ehci: fix memory leak in ehci
      usb: ohci: fix error return code in servicing iso td
      usb: ohci: limit the number of link eds

 docs/specs/pci-ids.txt        |   1 +
 hw/usb/dev-smartcard-reader.c | 140 +++++++++++++-----------
 hw/usb/hcd-ehci-pci.c         |   9 ++
 hw/usb/hcd-ehci.c             |   5 +
 hw/usb/hcd-ehci.h             |   1 +
 hw/usb/hcd-ohci.c             |  11 +-
 hw/usb/hcd-xhci.c             | 247 +++++++++++++++---------------------------
 hw/usb/trace-events           |   1 +
 include/hw/pci/pci.h          |   1 +
 9 files changed, 193 insertions(+), 223 deletions(-)

[Qemu-devel] [PULL 01/11] usb: ehci: fix memory leak in ehci

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

In usb_ehci_init function, it initializes 's->ipacket', but there
is no corresponding function to free this. As the ehci can be hotplug
and unplug, this will leak host memory leak. In order to make the
hierarchy clean, we should add a ehci pci finalize function, then call
the clean function in ehci device.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 589a85b8.3c2b9d0a.b8e6.1434@mx.google.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci-pci.c | 9 +++++++++
 hw/usb/hcd-ehci.c     | 5 +++++
 hw/usb/hcd-ehci.h     | 1 +
 3 files changed, 15 insertions(+)

diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 5657705..6dedcb8 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -89,6 +89,14 @@ static void usb_ehci_pci_init(Object *obj)
     usb_ehci_init(s, DEVICE(obj));
 }
 
+static void usb_ehci_pci_finalize(Object *obj)
+{
+    EHCIPCIState *i = PCI_EHCI(obj);
+    EHCIState *s = &i->ehci;
+
+    usb_ehci_finalize(s);
+}
+
 static void usb_ehci_pci_exit(PCIDevice *dev)
 {
     EHCIPCIState *i = PCI_EHCI(dev);
@@ -159,6 +167,7 @@ static const TypeInfo ehci_pci_type_info = {
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(EHCIPCIState),
     .instance_init = usb_ehci_pci_init,
+    .instance_finalize = usb_ehci_pci_finalize,
     .abstract = true,
     .class_init = ehci_class_init,
 };
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 7622a3a..50ef817 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2545,6 +2545,11 @@ void usb_ehci_init(EHCIState *s, DeviceState *dev)
                                 &s->mem_ports);
 }
 
+void usb_ehci_finalize(EHCIState *s)
+{
+    usb_packet_cleanup(&s->ipacket);
+}
+
 /*
  * vim: expandtab ts=4
  */
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 3fd7038..938d8aa 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -323,6 +323,7 @@ struct EHCIState {
 extern const VMStateDescription vmstate_ehci;
 
 void usb_ehci_init(EHCIState *s, DeviceState *dev);
+void usb_ehci_finalize(EHCIState *s);
 void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp);
 void usb_ehci_unrealize(EHCIState *s, DeviceState *dev, Error **errp);
 void ehci_reset(void *opaque);
-- 
1.8.3.1

[Qemu-devel] [PULL 02/11] usb: ohci: fix error return code in servicing iso td

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

It should return 1 if an error occurs when reading iso td.
This will avoid an infinite loop issue in ohci_service_ed_list.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 5899ac3e.1033240a.944d5.9a2d@mx.google.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ohci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index c82a92f..2cba3e3 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -725,7 +725,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     if (ohci_read_iso_td(ohci, addr, &iso_td)) {
         trace_usb_ohci_iso_td_read_failed(addr);
         ohci_die(ohci);
-        return 0;
+        return 1;
     }
 
     starting_frame = OHCI_BM(iso_td.flags, TD_SF);
-- 
1.8.3.1

[Qemu-devel] [PULL 03/11] usb: ohci: limit the number of link eds

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

The guest may builds an infinite loop with link eds. This patch
limit the number of linked ed to avoid this.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
Message-id: 5899a02e.45ca240a.6c373.93c1@mx.google.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ohci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 2cba3e3..21c93e0 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -42,6 +42,8 @@
 
 #define OHCI_MAX_PORTS 15
 
+#define ED_LINK_LIMIT 4
+
 static int64_t usb_frame_time;
 static int64_t usb_bit_time;
 
@@ -1184,7 +1186,7 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
     uint32_t next_ed;
     uint32_t cur;
     int active;
-
+    uint32_t link_cnt = 0;
     active = 0;
 
     if (head == 0)
@@ -1199,6 +1201,11 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
 
         next_ed = ed.next & OHCI_DPTR_MASK;
 
+        if (++link_cnt > ED_LINK_LIMIT) {
+            ohci_die(ohci);
+            return 0;
+        }
+
         if ((ed.head & OHCI_ED_H) || (ed.flags & OHCI_ED_K)) {
             uint32_t addr;
             /* Cancel pending packets for ED that have been paused.  */
-- 
1.8.3.1

[Qemu-devel] [PULL 04/11] xhci: apply limits to loops

[Gerd Hoffmann]

Limits should be big enough that normal guest should not hit it.
Add a tracepoint to log them, just in case.  Also, while being
at it, log the existing link trb limit too.

Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486383669-6421-1-git-send-email-kraxel@redhat.com
---
 hw/usb/hcd-xhci.c   | 15 ++++++++++++++-
 hw/usb/trace-events |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 54b3901..f3f9579 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -54,6 +54,8 @@
 #define ER_FULL_HACK
 
 #define TRB_LINK_LIMIT  4
+#define COMMAND_LIMIT   256
+#define TRANSFER_LIMIT  256
 
 #define LEN_CAP         0x40
 #define LEN_OPER        (0x400 + 0x10 * MAXPORTS)
@@ -1032,6 +1034,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
             return type;
         } else {
             if (++link_cnt > TRB_LINK_LIMIT) {
+                trace_usb_xhci_enforced_limit("trb-link");
                 return 0;
             }
             ring->dequeue = xhci_mask64(trb->parameter);
@@ -2150,6 +2153,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
     XHCIRing *ring;
     USBEndpoint *ep = NULL;
     uint64_t mfindex;
+    unsigned int count = 0;
     int length;
     int i;
 
@@ -2262,6 +2266,10 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
             epctx->retry = xfer;
             break;
         }
+        if (count++ > TRANSFER_LIMIT) {
+            trace_usb_xhci_enforced_limit("transfers");
+            break;
+        }
     }
     epctx->kick_active--;
 
@@ -2734,7 +2742,7 @@ static void xhci_process_commands(XHCIState *xhci)
     TRBType type;
     XHCIEvent event = {ER_COMMAND_COMPLETE, CC_SUCCESS};
     dma_addr_t addr;
-    unsigned int i, slotid = 0;
+    unsigned int i, slotid = 0, count = 0;
 
     DPRINTF("xhci_process_commands()\n");
     if (!xhci_running(xhci)) {
@@ -2848,6 +2856,11 @@ static void xhci_process_commands(XHCIState *xhci)
         }
         event.slotid = slotid;
         xhci_event(xhci, &event, 0);
+
+        if (count++ > COMMAND_LIMIT) {
+            trace_usb_xhci_enforced_limit("commands");
+            return;
+        }
     }
 }
 
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index fdd1d29..0c323d4 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -174,6 +174,7 @@ usb_xhci_xfer_retry(void *xfer) "%p"
 usb_xhci_xfer_success(void *xfer, uint32_t bytes) "%p: len %d"
 usb_xhci_xfer_error(void *xfer, uint32_t ret) "%p: ret %d"
 usb_xhci_unimplemented(const char *item, int nr) "%s (0x%x)"
+usb_xhci_enforced_limit(const char *item) "%s"
 
 # hw/usb/desc.c
 usb_desc_device(int addr, int len, int ret) "dev %d query device, len %d, ret %d"
-- 
1.8.3.1

[Qemu-devel] [PULL 05/11] xhci: drop ER_FULL_HACK workaround

[Gerd Hoffmann]

The nec/renesas driver problems have finally been debugged and root
caused, see commit "7da76e1 xhci: fix event queue IRQ handling".

It's pretty clear now that
 (a) The whole "driver can't handle ring full" story is most likely
     wrong.
 (b) The ER_FULL_HACK workaround based on the false assumtion doesn't
     much.  It avoids the driver crashing (without commit 7da76e1), but
     it doesn't make usb work.
 (c) With 7da76e1 applied it doesn't trigger any more.

So, lets kill it.  Or, to be exact, lets almost kill it.  Some data
fields are kept unused in the state struct, for live migration backward
compatibility.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486382139-30630-2-git-send-email-kraxel@redhat.com
---
 hw/usb/hcd-xhci.c | 117 +++++-------------------------------------------------
 1 file changed, 11 insertions(+), 106 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index f3f9579..cfb5f74 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -49,9 +49,6 @@
 
 /* Very pessimistic, let's hope it's enough for all cases */
 #define EV_QUEUE (((3 * 24) + 16) * MAXSLOTS)
-/* Do not deliver ER Full events. NEC's driver does some things not bound
- * to the specs when it gets them */
-#define ER_FULL_HACK
 
 #define TRB_LINK_LIMIT  4
 #define COMMAND_LIMIT   256
@@ -433,12 +430,14 @@ typedef struct XHCIInterrupter {
     uint32_t erdp_low;
     uint32_t erdp_high;
 
-    bool msix_used, er_pcs, er_full;
+    bool msix_used, er_pcs;
 
     dma_addr_t er_start;
     uint32_t er_size;
     unsigned int er_ep_idx;
 
+    /* kept for live migration compat only */
+    bool er_full_unused;
     XHCIEvent ev_buffer[EV_QUEUE];
     unsigned int ev_buffer_put;
     unsigned int ev_buffer_get;
@@ -828,7 +827,7 @@ static void xhci_intr_raise(XHCIState *xhci, int v)
 
 static inline int xhci_running(XHCIState *xhci)
 {
-    return !(xhci->usbsts & USBSTS_HCH) && !xhci->intr[0].er_full;
+    return !(xhci->usbsts & USBSTS_HCH);
 }
 
 static void xhci_die(XHCIState *xhci)
@@ -867,74 +866,6 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v)
     }
 }
 
-static void xhci_events_update(XHCIState *xhci, int v)
-{
-    XHCIInterrupter *intr = &xhci->intr[v];
-    dma_addr_t erdp;
-    unsigned int dp_idx;
-    bool do_irq = 0;
-
-    if (xhci->usbsts & USBSTS_HCH) {
-        return;
-    }
-
-    erdp = xhci_addr64(intr->erdp_low, intr->erdp_high);
-    if (erdp < intr->er_start ||
-        erdp >= (intr->er_start + TRB_SIZE*intr->er_size)) {
-        DPRINTF("xhci: ERDP out of bounds: "DMA_ADDR_FMT"\n", erdp);
-        DPRINTF("xhci: ER[%d] at "DMA_ADDR_FMT" len %d\n",
-                v, intr->er_start, intr->er_size);
-        xhci_die(xhci);
-        return;
-    }
-    dp_idx = (erdp - intr->er_start) / TRB_SIZE;
-    assert(dp_idx < intr->er_size);
-
-    /* NEC didn't read section 4.9.4 of the spec (v1.0 p139 top Note) and thus
-     * deadlocks when the ER is full. Hack it by holding off events until
-     * the driver decides to free at least half of the ring */
-    if (intr->er_full) {
-        int er_free = dp_idx - intr->er_ep_idx;
-        if (er_free <= 0) {
-            er_free += intr->er_size;
-        }
-        if (er_free < (intr->er_size/2)) {
-            DPRINTF("xhci_events_update(): event ring still "
-                    "more than half full (hack)\n");
-            return;
-        }
-    }
-
-    while (intr->ev_buffer_put != intr->ev_buffer_get) {
-        assert(intr->er_full);
-        if (((intr->er_ep_idx+1) % intr->er_size) == dp_idx) {
-            DPRINTF("xhci_events_update(): event ring full again\n");
-#ifndef ER_FULL_HACK
-            XHCIEvent full = {ER_HOST_CONTROLLER, CC_EVENT_RING_FULL_ERROR};
-            xhci_write_event(xhci, &full, v);
-#endif
-            do_irq = 1;
-            break;
-        }
-        XHCIEvent *event = &intr->ev_buffer[intr->ev_buffer_get];
-        xhci_write_event(xhci, event, v);
-        intr->ev_buffer_get++;
-        do_irq = 1;
-        if (intr->ev_buffer_get == EV_QUEUE) {
-            intr->ev_buffer_get = 0;
-        }
-    }
-
-    if (do_irq) {
-        xhci_intr_raise(xhci, v);
-    }
-
-    if (intr->er_full && intr->ev_buffer_put == intr->ev_buffer_get) {
-        DPRINTF("xhci_events_update(): event ring no longer full\n");
-        intr->er_full = 0;
-    }
-}
-
 static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
 {
     XHCIInterrupter *intr;
@@ -947,19 +878,6 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
     }
     intr = &xhci->intr[v];
 
-    if (intr->er_full) {
-        DPRINTF("xhci_event(): ER full, queueing\n");
-        if (((intr->ev_buffer_put+1) % EV_QUEUE) == intr->ev_buffer_get) {
-            DPRINTF("xhci: event queue full, dropping event!\n");
-            return;
-        }
-        intr->ev_buffer[intr->ev_buffer_put++] = *event;
-        if (intr->ev_buffer_put == EV_QUEUE) {
-            intr->ev_buffer_put = 0;
-        }
-        return;
-    }
-
     erdp = xhci_addr64(intr->erdp_low, intr->erdp_high);
     if (erdp < intr->er_start ||
         erdp >= (intr->er_start + TRB_SIZE*intr->er_size)) {
@@ -973,21 +891,12 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
     dp_idx = (erdp - intr->er_start) / TRB_SIZE;
     assert(dp_idx < intr->er_size);
 
-    if ((intr->er_ep_idx+1) % intr->er_size == dp_idx) {
-        DPRINTF("xhci_event(): ER full, queueing\n");
-#ifndef ER_FULL_HACK
+    if ((intr->er_ep_idx + 2) % intr->er_size == dp_idx) {
+        DPRINTF("xhci: ER %d full, send ring full error\n", v);
         XHCIEvent full = {ER_HOST_CONTROLLER, CC_EVENT_RING_FULL_ERROR};
-        xhci_write_event(xhci, &full);
-#endif
-        intr->er_full = 1;
-        if (((intr->ev_buffer_put+1) % EV_QUEUE) == intr->ev_buffer_get) {
-            DPRINTF("xhci: event queue full, dropping event!\n");
-            return;
-        }
-        intr->ev_buffer[intr->ev_buffer_put++] = *event;
-        if (intr->ev_buffer_put == EV_QUEUE) {
-            intr->ev_buffer_put = 0;
-        }
+        xhci_write_event(xhci, &full, v);
+    } else if ((intr->er_ep_idx + 1) % intr->er_size == dp_idx) {
+        DPRINTF("xhci: ER %d full, drop event\n", v);
     } else {
         xhci_write_event(xhci, event, v);
     }
@@ -1127,7 +1036,6 @@ static void xhci_er_reset(XHCIState *xhci, int v)
 
     intr->er_ep_idx = 0;
     intr->er_pcs = 1;
-    intr->er_full = 0;
 
     DPRINTF("xhci: event ring[%d]:" DMA_ADDR_FMT " [%d]\n",
             v, intr->er_start, intr->er_size);
@@ -2991,7 +2899,6 @@ static void xhci_reset(DeviceState *dev)
 
         xhci->intr[i].er_ep_idx = 0;
         xhci->intr[i].er_pcs = 1;
-        xhci->intr[i].er_full = 0;
         xhci->intr[i].ev_buffer_put = 0;
         xhci->intr[i].ev_buffer_get = 0;
     }
@@ -3381,7 +3288,6 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
         break;
     case 0x1c: /* ERDP high */
         intr->erdp_high = val;
-        xhci_events_update(xhci, v);
         break;
     default:
         trace_usb_xhci_unimplemented("oper write", reg);
@@ -3879,8 +3785,7 @@ static const VMStateDescription vmstate_xhci_event = {
 
 static bool xhci_er_full(void *opaque, int version_id)
 {
-    struct XHCIInterrupter *intr = opaque;
-    return intr->er_full;
+    return false;
 }
 
 static const VMStateDescription vmstate_xhci_intr = {
@@ -3904,7 +3809,7 @@ static const VMStateDescription vmstate_xhci_intr = {
         VMSTATE_UINT32(er_ep_idx,     XHCIInterrupter),
 
         /* event queue (used if ring is full) */
-        VMSTATE_BOOL(er_full,         XHCIInterrupter),
+        VMSTATE_BOOL(er_full_unused,  XHCIInterrupter),
         VMSTATE_UINT32_TEST(ev_buffer_put, XHCIInterrupter, xhci_er_full),
         VMSTATE_UINT32_TEST(ev_buffer_get, XHCIInterrupter, xhci_er_full),
         VMSTATE_STRUCT_ARRAY_TEST(ev_buffer, XHCIInterrupter, EV_QUEUE,
-- 
1.8.3.1

[Qemu-devel] [PULL 06/11] xhci: add qemu xhci controller

[Gerd Hoffmann]

Turn existing TYPE_XHCI into an abstract base class.
Create two child classes, TYPE_NEC_XHCI (same name as old xhci
controller) and TYPE_QEMU_XHCI (using an ID from our namespace).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Message-id: 1486382139-30630-3-git-send-email-kraxel@redhat.com
---
 docs/specs/pci-ids.txt |  1 +
 hw/usb/hcd-xhci.c      | 40 ++++++++++++++++++++++++++++++++++++----
 include/hw/pci/pci.h   |  1 +
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index 16fdb0c..95adee0 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -61,6 +61,7 @@ PCI devices (other than virtio):
 1b36:0009  PCI Expander Bridge (-device pxb)
 1b36:000a  PCI-PCI bridge (multiseat)
 1b36:000b  PCIe Expander Bridge (-device pxb-pcie)
+1b36:000d  PCI xhci usb host adapter
 
 All these devices are documented in docs/specs.
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index cfb5f74..c534b43 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -489,7 +489,9 @@ struct XHCIState {
     XHCIRing cmd_ring;
 };
 
-#define TYPE_XHCI "nec-usb-xhci"
+#define TYPE_XHCI "base-xhci"
+#define TYPE_NEC_XHCI "nec-usb-xhci"
+#define TYPE_QEMU_XHCI "qemu-xhci"
 
 #define XHCI(obj) \
     OBJECT_CHECK(XHCIState, (obj), TYPE_XHCI)
@@ -3881,10 +3883,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     k->realize      = usb_xhci_realize;
     k->exit         = usb_xhci_exit;
-    k->vendor_id    = PCI_VENDOR_ID_NEC;
-    k->device_id    = PCI_DEVICE_ID_NEC_UPD720200;
     k->class_id     = PCI_CLASS_SERIAL_USB;
-    k->revision     = 0x03;
     k->is_express   = 1;
 }
 
@@ -3893,11 +3892,44 @@ static const TypeInfo xhci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(XHCIState),
     .class_init    = xhci_class_init,
+    .abstract      = true,
+};
+
+static void nec_xhci_class_init(ObjectClass *klass, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->vendor_id    = PCI_VENDOR_ID_NEC;
+    k->device_id    = PCI_DEVICE_ID_NEC_UPD720200;
+    k->revision     = 0x03;
+}
+
+static const TypeInfo nec_xhci_info = {
+    .name          = TYPE_NEC_XHCI,
+    .parent        = TYPE_XHCI,
+    .class_init    = nec_xhci_class_init,
+};
+
+static void qemu_xhci_class_init(ObjectClass *klass, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->vendor_id    = PCI_VENDOR_ID_REDHAT;
+    k->device_id    = PCI_DEVICE_ID_REDHAT_XHCI;
+    k->revision     = 0x01;
+}
+
+static const TypeInfo qemu_xhci_info = {
+    .name          = TYPE_QEMU_XHCI,
+    .parent        = TYPE_XHCI,
+    .class_init    = qemu_xhci_class_init,
 };
 
 static void xhci_register_types(void)
 {
     type_register_static(&xhci_info);
+    type_register_static(&nec_xhci_info);
+    type_register_static(&qemu_xhci_info);
 }
 
 type_init(xhci_register_types)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index cbc1fdf..05ef14b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -97,6 +97,7 @@
 #define PCI_DEVICE_ID_REDHAT_BRIDGE_SEAT 0x000a
 #define PCI_DEVICE_ID_REDHAT_PXB_PCIE    0x000b
 #define PCI_DEVICE_ID_REDHAT_PCIE_RP     0x000c
+#define PCI_DEVICE_ID_REDHAT_XHCI        0x000d
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
 
 #define FMT_PCIBUS                      PRIx64
-- 
1.8.3.1

[Qemu-devel] [PULL 07/11] xhci: fix nec vendor quirk handling

[Gerd Hoffmann]

Only the TYPE_NEC_XHCI controller will have the nec vendor quirks.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486382139-30630-4-git-send-email-kraxel@redhat.com
---
 hw/usb/hcd-xhci.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index c534b43..4ac67ae 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -487,6 +487,8 @@ struct XHCIState {
     XHCIInterrupter intr[MAXINTRS];
 
     XHCIRing cmd_ring;
+
+    bool nec_quirks;
 };
 
 #define TYPE_XHCI "base-xhci"
@@ -2745,20 +2747,26 @@ static void xhci_process_commands(XHCIState *xhci)
             xhci_via_challenge(xhci, trb.parameter);
             break;
         case CR_VENDOR_NEC_FIRMWARE_REVISION:
-            event.type = 48; /* NEC reply */
-            event.length = 0x3025;
+            if (xhci->nec_quirks) {
+                event.type = 48; /* NEC reply */
+                event.length = 0x3025;
+            } else {
+                event.ccode = CC_TRB_ERROR;
+            }
             break;
         case CR_VENDOR_NEC_CHALLENGE_RESPONSE:
-        {
-            uint32_t chi = trb.parameter >> 32;
-            uint32_t clo = trb.parameter;
-            uint32_t val = xhci_nec_challenge(chi, clo);
-            event.length = val & 0xFFFF;
-            event.epid = val >> 16;
-            slotid = val >> 24;
-            event.type = 48; /* NEC reply */
-        }
-        break;
+            if (xhci->nec_quirks) {
+                uint32_t chi = trb.parameter >> 32;
+                uint32_t clo = trb.parameter;
+                uint32_t val = xhci_nec_challenge(chi, clo);
+                event.length = val & 0xFFFF;
+                event.epid = val >> 16;
+                slotid = val >> 24;
+                event.type = 48; /* NEC reply */
+            } else {
+                event.ccode = CC_TRB_ERROR;
+            }
+            break;
         default:
             trace_usb_xhci_unimplemented("command", type);
             event.ccode = CC_TRB_ERROR;
@@ -3265,9 +3273,12 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
         intr->erstsz = val & 0xffff;
         break;
     case 0x10: /* ERSTBA low */
-        /* XXX NEC driver bug: it doesn't align this to 64 bytes
-        intr->erstba_low = val & 0xffffffc0; */
-        intr->erstba_low = val & 0xfffffff0;
+        if (xhci->nec_quirks) {
+            /* NEC driver bug: it doesn't align this to 64 bytes */
+            intr->erstba_low = val & 0xfffffff0;
+        } else {
+            intr->erstba_low = val & 0xffffffc0;
+        }
         break;
     case 0x14: /* ERSTBA high */
         intr->erstba_high = val;
@@ -3562,6 +3573,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
     dev->config[0x60] = 0x30; /* release number */
 
+    if (strcmp(object_get_typename(OBJECT(dev)), TYPE_NEC_XHCI) == 0) {
+        xhci->nec_quirks = true;
+    }
     if (xhci->numintrs > MAXINTRS) {
         xhci->numintrs = MAXINTRS;
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 08/11] xhci: drop via vendor command handling

[Gerd Hoffmann]

Seems pretty pointless, we don't emulate an via xhci controller.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1486382139-30630-5-git-send-email-kraxel@redhat.com
---
 hw/usb/hcd-xhci.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4ac67ae..28dd2f2 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -198,7 +198,6 @@ typedef enum TRBType {
     ER_DEVICE_NOTIFICATION,
     ER_MFINDEX_WRAP,
     /* vendor specific bits */
-    CR_VENDOR_VIA_CHALLENGE_RESPONSE = 48,
     CR_VENDOR_NEC_FIRMWARE_REVISION  = 49,
     CR_VENDOR_NEC_CHALLENGE_RESPONSE = 50,
 } TRBType;
@@ -554,7 +553,6 @@ static const char *TRBType_names[] = {
     [ER_HOST_CONTROLLER]               = "ER_HOST_CONTROLLER",
     [ER_DEVICE_NOTIFICATION]           = "ER_DEVICE_NOTIFICATION",
     [ER_MFINDEX_WRAP]                  = "ER_MFINDEX_WRAP",
-    [CR_VENDOR_VIA_CHALLENGE_RESPONSE] = "CR_VENDOR_VIA_CHALLENGE_RESPONSE",
     [CR_VENDOR_NEC_FIRMWARE_REVISION]  = "CR_VENDOR_NEC_FIRMWARE_REVISION",
     [CR_VENDOR_NEC_CHALLENGE_RESPONSE] = "CR_VENDOR_NEC_CHALLENGE_RESPONSE",
 };
@@ -2622,32 +2620,6 @@ static uint32_t xhci_nec_challenge(uint32_t hi, uint32_t lo)
     return ~val;
 }
 
-static void xhci_via_challenge(XHCIState *xhci, uint64_t addr)
-{
-    PCIDevice *pci_dev = PCI_DEVICE(xhci);
-    uint32_t buf[8];
-    uint32_t obuf[8];
-    dma_addr_t paddr = xhci_mask64(addr);
-
-    pci_dma_read(pci_dev, paddr, &buf, 32);
-
-    memcpy(obuf, buf, sizeof(obuf));
-
-    if ((buf[0] & 0xff) == 2) {
-        obuf[0] = 0x49932000 + 0x54dc200 * buf[2] + 0x7429b578 * buf[3];
-        obuf[0] |=  (buf[2] * buf[3]) & 0xff;
-        obuf[1] = 0x0132bb37 + 0xe89 * buf[2] + 0xf09 * buf[3];
-        obuf[2] = 0x0066c2e9 + 0x2091 * buf[2] + 0x19bd * buf[3];
-        obuf[3] = 0xd5281342 + 0x2cc9691 * buf[2] + 0x2367662 * buf[3];
-        obuf[4] = 0x0123c75c + 0x1595 * buf[2] + 0x19ec * buf[3];
-        obuf[5] = 0x00f695de + 0x26fd * buf[2] + 0x3e9 * buf[3];
-        obuf[6] = obuf[2] ^ obuf[3] ^ 0x29472956;
-        obuf[7] = obuf[2] ^ obuf[3] ^ 0x65866593;
-    }
-
-    pci_dma_write(pci_dev, paddr, &obuf, 32);
-}
-
 static void xhci_process_commands(XHCIState *xhci)
 {
     XHCITRB trb;
@@ -2743,9 +2715,6 @@ static void xhci_process_commands(XHCIState *xhci)
         case CR_GET_PORT_BANDWIDTH:
             event.ccode = xhci_get_port_bandwidth(xhci, trb.parameter);
             break;
-        case CR_VENDOR_VIA_CHALLENGE_RESPONSE:
-            xhci_via_challenge(xhci, trb.parameter);
-            break;
         case CR_VENDOR_NEC_FIRMWARE_REVISION:
             if (xhci->nec_quirks) {
                 event.type = 48; /* NEC reply */
-- 
1.8.3.1

[Qemu-devel] [PULL 09/11] usb-ccid: better bulk_out error handling

[Gerd Hoffmann]

Add err goto label where we can jump to from all error conditions.
STALL request on all errors.  Reset position on all errors.

Normal request processing is not in a else branch any more, so this code
is reintended, there are no code changes in that part of the code
though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1487250819-23764-2-git-send-email-kraxel@redhat.com
---
 hw/usb/dev-smartcard-reader.c | 116 ++++++++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 1325ea1..badcfcb 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1001,8 +1001,7 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
     CCID_Header *ccid_header;
 
     if (p->iov.size + s->bulk_out_pos > BULK_OUT_DATA_SIZE) {
-        p->status = USB_RET_STALL;
-        return;
+        goto err;
     }
     ccid_header = (CCID_Header *)s->bulk_out_data;
     usb_packet_copy(p, s->bulk_out_data + s->bulk_out_pos, p->iov.size);
@@ -1017,64 +1016,71 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
         DPRINTF(s, 1,
                 "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n",
                 __func__);
-    } else {
-        DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__,
-                ccid_header->bMessageType,
-                ccid_message_type_to_str(ccid_header->bMessageType));
-        switch (ccid_header->bMessageType) {
-        case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus:
-            ccid_write_slot_status(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
-            DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
+        goto err;
+    }
+
+    DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__,
+            ccid_header->bMessageType,
+            ccid_message_type_to_str(ccid_header->bMessageType));
+    switch (ccid_header->bMessageType) {
+    case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus:
+        ccid_write_slot_status(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn:
+        DPRINTF(s, 1, "%s: PowerOn: %d\n", __func__,
                 ((CCID_IccPowerOn *)(ccid_header))->bPowerSelect);
-            s->powered = true;
-            if (!ccid_card_inserted(s)) {
-                ccid_report_error_failed(s, ERROR_ICC_MUTE);
-            }
-            /* atr is written regardless of error. */
-            ccid_write_data_block_atr(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff:
-            ccid_reset_error_status(s);
-            s->powered = false;
-            ccid_write_slot_status(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock:
-            ccid_on_apdu_from_guest(s, (CCID_XferBlock *)s->bulk_out_data);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters:
-            ccid_reset_error_status(s);
-            ccid_set_parameters(s, ccid_header);
-            ccid_write_parameters(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters:
-            ccid_reset_error_status(s);
-            ccid_reset_parameters(s);
-            ccid_write_parameters(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters:
-            ccid_reset_error_status(s);
-            ccid_write_parameters(s, ccid_header);
-            break;
-        case CCID_MESSAGE_TYPE_PC_to_RDR_Mechanical:
-            ccid_report_error_failed(s, 0);
-            ccid_write_slot_status(s, ccid_header);
-            break;
-        default:
-            DPRINTF(s, 1,
+        s->powered = true;
+        if (!ccid_card_inserted(s)) {
+            ccid_report_error_failed(s, ERROR_ICC_MUTE);
+        }
+        /* atr is written regardless of error. */
+        ccid_write_data_block_atr(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff:
+        ccid_reset_error_status(s);
+        s->powered = false;
+        ccid_write_slot_status(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock:
+        ccid_on_apdu_from_guest(s, (CCID_XferBlock *)s->bulk_out_data);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters:
+        ccid_reset_error_status(s);
+        ccid_set_parameters(s, ccid_header);
+        ccid_write_parameters(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters:
+        ccid_reset_error_status(s);
+        ccid_reset_parameters(s);
+        ccid_write_parameters(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters:
+        ccid_reset_error_status(s);
+        ccid_write_parameters(s, ccid_header);
+        break;
+    case CCID_MESSAGE_TYPE_PC_to_RDR_Mechanical:
+        ccid_report_error_failed(s, 0);
+        ccid_write_slot_status(s, ccid_header);
+        break;
+    default:
+        DPRINTF(s, 1,
                 "handle_data: ERROR: unhandled message type %Xh\n",
                 ccid_header->bMessageType);
-            /*
-             * The caller is expecting the device to respond, tell it we
-             * don't support the operation.
-             */
-            ccid_report_error_failed(s, ERROR_CMD_NOT_SUPPORTED);
-            ccid_write_slot_status(s, ccid_header);
-            break;
-        }
+        /*
+         * The caller is expecting the device to respond, tell it we
+         * don't support the operation.
+         */
+        ccid_report_error_failed(s, ERROR_CMD_NOT_SUPPORTED);
+        ccid_write_slot_status(s, ccid_header);
+        break;
     }
     s->bulk_out_pos = 0;
+    return;
+
+err:
+    p->status = USB_RET_STALL;
+    s->bulk_out_pos = 0;
+    return;
 }
 
 static void ccid_bulk_in_copy_to_guest(USBCCIDState *s, USBPacket *p)
-- 
1.8.3.1

[Qemu-devel] [PULL 10/11] usb-ccid: move header size check

[Gerd Hoffmann]

Move up header size check, so we can use header fields in sanity checks
(in followup patches).  Also reword the debug message.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1487250819-23764-3-git-send-email-kraxel@redhat.com
---
 hw/usb/dev-smartcard-reader.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index badcfcb..1acc1fb 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1003,21 +1003,20 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
     if (p->iov.size + s->bulk_out_pos > BULK_OUT_DATA_SIZE) {
         goto err;
     }
-    ccid_header = (CCID_Header *)s->bulk_out_data;
     usb_packet_copy(p, s->bulk_out_data + s->bulk_out_pos, p->iov.size);
     s->bulk_out_pos += p->iov.size;
+    if (s->bulk_out_pos < 10) {
+        DPRINTF(s, 1, "%s: header incomplete\n", __func__);
+        goto err;
+    }
+
+    ccid_header = (CCID_Header *)s->bulk_out_data;
     if (p->iov.size == CCID_MAX_PACKET_SIZE) {
         DPRINTF(s, D_VERBOSE,
             "usb-ccid: bulk_in: expecting more packets (%zd/%d)\n",
             p->iov.size, ccid_header->dwLength);
         return;
     }
-    if (s->bulk_out_pos < 10) {
-        DPRINTF(s, 1,
-                "%s: bad USB_TOKEN_OUT length, should be at least 10 bytes\n",
-                __func__);
-        goto err;
-    }
 
     DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__,
             ccid_header->bMessageType,
-- 
1.8.3.1

[Qemu-devel] [PULL 11/11] usb-ccid: add check message size checks

[Gerd Hoffmann]

Check message size too when figuring whenever we should expect more data.
Fix debug message to show useful data, p->iov.size is fixed anyway if we
land there, print how much we got meanwhile instead.

Also check announced message size against actual message size.  That
is a more general fix for CVE-2017-5898 than commit "c7dfbf3 usb: ccid:
check ccid apdu length".

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1487250819-23764-4-git-send-email-kraxel@redhat.com
---
 hw/usb/dev-smartcard-reader.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 1acc1fb..7cd4ed0 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1011,12 +1011,19 @@ static void ccid_handle_bulk_out(USBCCIDState *s, USBPacket *p)
     }
 
     ccid_header = (CCID_Header *)s->bulk_out_data;
-    if (p->iov.size == CCID_MAX_PACKET_SIZE) {
+    if ((s->bulk_out_pos - 10 < ccid_header->dwLength) &&
+        (p->iov.size == CCID_MAX_PACKET_SIZE)) {
         DPRINTF(s, D_VERBOSE,
-            "usb-ccid: bulk_in: expecting more packets (%zd/%d)\n",
-            p->iov.size, ccid_header->dwLength);
+                "usb-ccid: bulk_in: expecting more packets (%d/%d)\n",
+                s->bulk_out_pos - 10, ccid_header->dwLength);
         return;
     }
+    if (s->bulk_out_pos - 10 != ccid_header->dwLength) {
+        DPRINTF(s, 1,
+                "usb-ccid: bulk_in: message size mismatch (got %d, expected %d)\n",
+                s->bulk_out_pos - 10, ccid_header->dwLength);
+        goto err;
+    }
 
     DPRINTF(s, D_MORE_INFO, "%s %x %s\n", __func__,
             ccid_header->bMessageType,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 00/11] usb patch queue

[Peter Maydell]

On 21 February 2017 at 07:16, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here is the usb patch queue, bringing the usual share of bugfixes.
> Also a generic xhci device variant (qemu-xhci).
>
> please pull,
>   Gerd
>
> The following changes since commit 56f9e46b841c7be478ca038d8d4085d776ab4b0d:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2017-02-20' into staging (2017-02-20 17:42:47 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-usb-20170221-1
>
> for you to fetch changes up to 31fb4444a485a348f8e2699d7c3dd15e1819ad2c:
>
>   usb-ccid: add check message size checks (2017-02-21 08:11:43 +0100)
>
> ----------------------------------------------------------------
> xhci: add qemu-xhci device, some followup cleanups.
> ccid: better sanity checking.
> ehci: fix memory leak
> ohci: bugfixes.

Applied, thanks.

-- PMM