From d710e1e7bd3d5bfc26b631f02ae87901ebe646b0 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 7 Feb 2017 18:42:55 -0800 Subject: usb: ehci: fix memory leak in ehci 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 Message-id: 589a85b8.3c2b9d0a.b8e6.1434@mx.google.com Signed-off-by: Gerd Hoffmann --- 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 56577051e2..6dedcb8989 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 7622a3ae72..50ef817f93 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 3fd7038658..938d8aa284 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); -- cgit v1.2.3 From 26f670a244982335cc08943fb1ec099a2c81e42d Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 7 Feb 2017 03:15:03 -0800 Subject: usb: ohci: fix error return code in servicing iso td 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 Message-id: 5899ac3e.1033240a.944d5.9a2d@mx.google.com Signed-off-by: Gerd Hoffmann --- 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 c82a92fff7..2cba3e3b5b 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); -- cgit v1.2.3 From 95ed56939eb2eaa4e2f349fe6dcd13ca4edfd8fb Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 7 Feb 2017 02:23:33 -0800 Subject: usb: ohci: limit the number of link eds 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 Message-id: 5899a02e.45ca240a.6c373.93c1@mx.google.com Signed-off-by: Gerd Hoffmann --- 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 2cba3e3b5b..21c93e0372 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. */ -- cgit v1.2.3 From f89b60f6e5fee3923bedf80e82b4e5efc1bb156b Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 6 Feb 2017 13:21:09 +0100 Subject: xhci: apply limits to loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 李强 Signed-off-by: Gerd Hoffmann 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 54b3901c8c..f3f95796ba 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 fdd1d29030..0c323d4cac 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" -- cgit v1.2.3 From 898248a32915024a4f01ce4f0c3519509fb703cb Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 6 Feb 2017 12:55:36 +0100 Subject: xhci: drop ER_FULL_HACK workaround 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 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 f3f95796ba..cfb5f740b7 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, -- cgit v1.2.3 From 72a810f411abaabc55f375533220adf69e059c89 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 6 Feb 2017 12:55:37 +0100 Subject: xhci: add qemu xhci controller 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 Reviewed-by: Marcel Apfelbaum 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 16fdb0c93f..95adee07d6 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 cfb5f740b7..c534b437b5 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 cbc1fdfb5b..05ef14b6f5 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 -- cgit v1.2.3 From 2992d6b49ce7ca2d4c02ff6baf23fc815879eef3 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 6 Feb 2017 12:55:38 +0100 Subject: xhci: fix nec vendor quirk handling Only the TYPE_NEC_XHCI controller will have the nec vendor quirks. Signed-off-by: Gerd Hoffmann 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 c534b437b5..4ac67ae906 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; } -- cgit v1.2.3 From 558ff1b6efcebd7f919bae3e36b97fa6f9139f42 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 6 Feb 2017 12:55:39 +0100 Subject: xhci: drop via vendor command handling Seems pretty pointless, we don't emulate an via xhci controller. Signed-off-by: Gerd Hoffmann 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 4ac67ae906..28dd2f2c9a 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 */ -- cgit v1.2.3 From 0aeebc73b7976bae5cb7e9768e3d9a0fd9d634e8 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:37 +0100 Subject: usb-ccid: better bulk_out error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Marc-André Lureau 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 1325ea1659..badcfcbf7d 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) -- cgit v1.2.3 From 7569c54642e8aa9fa03e250c7c578bd4d3747f00 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:38 +0100 Subject: usb-ccid: move header size check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Marc-André Lureau 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 badcfcbf7d..1acc1fb37a 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, -- cgit v1.2.3 From 31fb4444a485a348f8e2699d7c3dd15e1819ad2c Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 16 Feb 2017 14:13:39 +0100 Subject: usb-ccid: add check message size checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Marc-André Lureau 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 1acc1fb37a..7cd4ed0d17 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, -- cgit v1.2.3