From 7b84b90966568da0e05655ecaa78c209300aae6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Wed, 7 Aug 2019 12:40:48 +0400 Subject: [PATCH 1/5] usbredir: fix buffer-overflow on vmload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If interface_count is NO_INTERFACE_INFO, let's not access the arrays out-of-bounds. ==994==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625000243930 at pc 0x5642068086a8 bp 0x7f0b6f9ffa50 sp 0x7f0b6f9ffa40 READ of size 1 at 0x625000243930 thread T0 #0 0x5642068086a7 in usbredir_check_bulk_receiving /home/elmarco/src/qemu/hw/usb/redirect.c:1503 #1 0x56420681301c in usbredir_post_load /home/elmarco/src/qemu/hw/usb/redirect.c:2154 #2 0x5642068a56c2 in vmstate_load_state /home/elmarco/src/qemu/migration/vmstate.c:168 #3 0x56420688e2ac in vmstate_load /home/elmarco/src/qemu/migration/savevm.c:829 #4 0x5642068980cb in qemu_loadvm_section_start_full /home/elmarco/src/qemu/migration/savevm.c:2211 #5 0x564206899645 in qemu_loadvm_state_main /home/elmarco/src/qemu/migration/savevm.c:2395 #6 0x5642068998cf in qemu_loadvm_state /home/elmarco/src/qemu/migration/savevm.c:2467 #7 0x56420685f3e9 in process_incoming_migration_co /home/elmarco/src/qemu/migration/migration.c:449 #8 0x564207106c47 in coroutine_trampoline /home/elmarco/src/qemu/util/coroutine-ucontext.c:115 #9 0x7f0c0604e37f (/lib64/libc.so.6+0x4d37f) Signed-off-by: Marc-André Lureau Reviewed-by: Liam Merwick Reviewed-by: Li Qiang Reviewed-by: Philippe Mathieu-Daudé Message-id: 20190807084048.4258-1-marcandre.lureau@redhat.com Signed-off-by: Gerd Hoffmann --- hw/usb/redirect.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index fc9fe0c00f..be15b9f303 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -1499,6 +1499,11 @@ static void usbredir_check_bulk_receiving(USBRedirDevice *dev) for (i = EP2I(USB_DIR_IN); i < MAX_ENDPOINTS; i++) { dev->endpoint[i].bulk_receiving_enabled = 0; } + + if (dev->interface_info.interface_count == NO_INTERFACE_INFO) { + return; + } + for (i = 0; i < dev->interface_info.interface_count; i++) { quirks = usb_get_quirks(dev->device_info.vendor_id, dev->device_info.product_id, From baeed705081be1919d6929dfdb405c37b2df9cd5 Mon Sep 17 00:00:00 2001 From: Martin Cerveny Date: Wed, 24 Jul 2019 14:58:59 +0200 Subject: [PATCH 2/5] usb-redir: merge interrupt packets Interrupt packets (limited by wMaxPacketSize) should be buffered and merged by algorithm described in USB spec. (see usb_20.pdf/5.7.3 Interrupt Transfer Packet Size Constraints). Signed-off-by: Martin Cerveny Message-id: 20190724125859.14624-2-M.Cerveny@computer.org Signed-off-by: Gerd Hoffmann --- hw/usb/redirect.c | 69 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index be15b9f303..e0f5ca6f81 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -819,8 +819,8 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev, USBPacket *p, uint8_t ep) { /* Input interrupt endpoint, buffered packet input */ - struct buf_packet *intp; - int status, len; + struct buf_packet *intp, *intp_to_free; + int status, len, sum; if (!dev->endpoint[EP2I(ep)].interrupt_started && !dev->endpoint[EP2I(ep)].interrupt_error) { @@ -839,9 +839,17 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev, dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0; } - intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq); + /* check for completed interrupt message (with all fragments) */ + sum = 0; + QTAILQ_FOREACH(intp, &dev->endpoint[EP2I(ep)].bufpq, next) { + sum += intp->len; + if (intp->len < dev->endpoint[EP2I(ep)].max_packet_size || + sum >= p->iov.size) + break; + } + if (intp == NULL) { - DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep); + DPRINTF2("interrupt-token-in ep %02X, no intp, buffered %d\n", ep, sum); /* Check interrupt_error for stream errors */ status = dev->endpoint[EP2I(ep)].interrupt_error; dev->endpoint[EP2I(ep)].interrupt_error = 0; @@ -852,18 +860,42 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev, } return; } - DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep, - intp->status, intp->len); - status = intp->status; - len = intp->len; - if (len > p->iov.size) { - ERROR("received int data is larger then packet ep %02X\n", ep); - len = p->iov.size; - status = usb_redir_babble; + /* copy of completed interrupt message */ + sum = 0; + status = usb_redir_success; + intp_to_free = NULL; + QTAILQ_FOREACH(intp, &dev->endpoint[EP2I(ep)].bufpq, next) { + if (intp_to_free) { + bufp_free(dev, intp_to_free, ep); + } + DPRINTF("interrupt-token-in ep %02X fragment status %d len %d\n", ep, + intp->status, intp->len); + + sum += intp->len; + len = intp->len; + if (status == usb_redir_success) { + status = intp->status; + } + if (sum > p->iov.size) { + ERROR("received int data is larger then packet ep %02X\n", ep); + len -= (sum - p->iov.size); + sum = p->iov.size; + status = usb_redir_babble; + } + + usb_packet_copy(p, intp->data, len); + + intp_to_free = intp; + if (intp->len < dev->endpoint[EP2I(ep)].max_packet_size || + sum >= p->iov.size) + break; } - usb_packet_copy(p, intp->data, len); - bufp_free(dev, intp, ep); + if (intp_to_free) { + bufp_free(dev, intp_to_free, ep); + } + DPRINTF("interrupt-token-in ep %02X summary status %d len %d\n", ep, + status, sum); usbredir_handle_status(dev, p, status); } @@ -2041,22 +2073,17 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id, } if (ep & USB_DIR_IN) { - bool q_was_empty; - if (dev->endpoint[EP2I(ep)].interrupt_started == 0) { DPRINTF("received int packet while not started ep %02X\n", ep); free(data); return; } - q_was_empty = QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq); - /* bufp_alloc also adds the packet to the ep queue */ bufp_alloc(dev, data, data_len, interrupt_packet->status, ep, data); - if (q_was_empty) { - usb_wakeup(usb_ep_get(&dev->dev, USB_TOKEN_IN, ep & 0x0f), 0); - } + /* insufficient data solved with USB_RET_NAK */ + usb_wakeup(usb_ep_get(&dev->dev, USB_TOKEN_IN, ep & 0x0f), 0); } else { /* * We report output interrupt packets as completed directly upon From dc2c037fd23ea3dcf2e13afda22c1c64ab56f96b Mon Sep 17 00:00:00 2001 From: Hikaru Nishida Date: Sat, 20 Jul 2019 15:04:27 +0900 Subject: [PATCH 3/5] xhci: Add No Op Command This commit adds No Op Command (23) to xHC for verifying the operation of the Command Ring mechanisms. No Op Command is defined in XHCI spec (4.6.2) and just reports Command Completion Event with Completion Code == Success. Before this commit, No Op Command is not implemented so xHC reports Command Completion Event with Completion Code == TRB Error. This commit fixes this behaviour to report Completion Code correctly. Signed-off-by: Hikaru Nishida Message-id: 20190720060427.50457-1-hikarupsp@gmail.com Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-xhci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index f698224c8a..f578264948 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -2543,6 +2543,9 @@ static void xhci_process_commands(XHCIState *xhci) case CR_GET_PORT_BANDWIDTH: event.ccode = xhci_get_port_bandwidth(xhci, trb.parameter); break; + case CR_NOOP: + event.ccode = CC_SUCCESS; + break; case CR_VENDOR_NEC_FIRMWARE_REVISION: if (xhci->nec_quirks) { event.type = 48; /* NEC reply */ From 73f46fef7400dc1dc6cb5e8914d3d1b3b673459f Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 15 Aug 2019 15:14:28 +0100 Subject: [PATCH 4/5] usb: reword -usb command-line option and mention xHCI The -usb section of the man page is not very clear on what exactly -usb does and fails to mention xHCI as a modern alternative (-device nec-usb-xhci). Signed-off-by: Stefan Hajnoczi Reviewed-by: Thomas Huth Message-id: 20190815141428.29080-1-stefanha@redhat.com Signed-off-by: Gerd Hoffmann --- qemu-options.hx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index 9621e934c0..1fb362f06f 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1436,12 +1436,15 @@ STEXI ETEXI DEF("usb", 0, QEMU_OPTION_usb, - "-usb enable the USB driver (if it is not used by default yet)\n", + "-usb enable on-board USB host controller (if not enabled by default)\n", QEMU_ARCH_ALL) STEXI @item -usb @findex -usb -Enable the USB driver (if it is not used by default yet). +Enable USB emulation on machine types with an on-board USB host controller (if +not enabled by default). Note that on-board USB host controllers may not +support USB 3.0. In this case @option{-device qemu-xhci} can be used instead +on machines with PCI. ETEXI DEF("usbdevice", HAS_ARG, QEMU_OPTION_usbdevice, From 1be344b7ad25d572dadeee46d80f0103354352b2 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 21 Aug 2019 10:53:19 +0200 Subject: [PATCH 5/5] ehci: fix queue->dev null ptr dereference In case we don't have a device for an active queue, just skip processing the queue (same we do for inactive queues) and log a guest bug. Reported-by: Guenter Roeck Signed-off-by: Gerd Hoffmann Tested-by: Guenter Roeck Message-id: 20190821085319.13711-1-kraxel@redhat.com --- hw/usb/hcd-ehci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 9ca7b87a80..56ab2f457f 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1838,6 +1838,9 @@ static int ehci_state_fetchqtd(EHCIQueue *q) ehci_set_state(q->ehci, q->async, EST_EXECUTING); break; } + } else if (q->dev == NULL) { + ehci_trace_guest_bug(q->ehci, "no device attached to queue"); + ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH); } else { p = ehci_alloc_packet(q); p->qtdaddr = q->qtdaddr;