From 80eecda8e5d09c442c24307f340840a5b70ea3b9 Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Thu, 11 Feb 2016 16:31:20 +0530 Subject: [PATCH 1/6] usb: check USB configuration descriptor object 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 Signed-off-by: Prasad J Pandit Message-id: 1455188480-14688-1-git-send-email-ppandit@redhat.com Signed-off-by: Gerd Hoffmann --- 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 985a6298bf..5dc45383d3 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, From 14ec7b2c5bc0e7b0f7097211d349b57450f09d02 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 19 Feb 2016 12:03:24 +0000 Subject: [PATCH 2/6] tusb6010: move from hw/timer to hw/usb 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 Message-id: 1455883404-10976-1-git-send-email-peter.maydell@linaro.org Signed-off-by: Gerd Hoffmann --- 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 133bd0d455..5cfea6e0da 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 8f00fbd8f6..2717027d34 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 From 64c9bc181fc78275596649f591302d72df2d3071 Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Wed, 17 Feb 2016 00:23:40 +0530 Subject: [PATCH 3/6] usb: check RNDIS message length 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 Message-id: 1455648821-17340-2-git-send-email-ppandit@redhat.com Signed-off-by: Gerd Hoffmann --- 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 bea5e1ee8b..45fa00c517 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)) { From fe3c546c5ff2a6210f9a4d8561cc64051ca8603e Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Wed, 17 Feb 2016 00:23:41 +0530 Subject: [PATCH 4/6] usb: check RNDIS buffer offsets & length 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 Signed-off-by: Prasad J Pandit Message-id: 1455648821-17340-3-git-send-email-ppandit@redhat.com Signed-off-by: Gerd Hoffmann --- 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 5dc45383d3..c6abd38c2a 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); From 5f77e06baa84323e5bbc96c2c7f4fe627078b210 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Fri, 19 Feb 2016 15:33:58 +0800 Subject: [PATCH 5/6] usb: add pid check at the first of uhci_handle_td() 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 Message-id: 1455867238-4720-1-git-send-email-arei.gonglei@huawei.com [ applied minor codestyle fix ] Signed-off-by: Gerd Hoffmann --- 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 5ccfb8395a..c370240be2 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) { From fa1298c2d623522eda7b4f1f721fcb935abb7360 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 22 Feb 2016 09:50:11 +0100 Subject: [PATCH 6/6] ohci: allocate timer only once. 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 Reported-by: Zuozhi Fzz Signed-off-by: Gerd Hoffmann --- 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 bed55dda78..17ed4617ef 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),