The OHCI USB controller requires software to set up an Endpoint Descriptor data structure. This specifies (among other things) both an EndpointNumber telling the controller which USB endpoint to talk to, and a Format (F) bit which is 1 for Isochronous transfers and 0 for General transfers.
The specification requires that software matches the F bit with the endpoint it's setting up the descriptor for, so that Control, Bulk and Interrupt endpoints use F=0, and Isochronous endpoints use F=1. It doesn't require the controller to check whether software gets this right. The spec isn't totally clear about what happens if software creates an ED with a mismatch, but I believe that the controller just trusts the F bit, and tries to talk to the endpoint with the wrong sequence of USB tokens (notably, isochronous transactions have no handshaking and the others do). This probably results in the transfer failing. In hcd-ehci.c we check for the "iso transfer to non-iso endpoint" and make it fail, but we don't do this in hcd-ohci.c. The result is that a guest that deliberately sets up this situation can cause us to hit the assertion added in commit 7b1eba96aeca, where the endpoint is not isochronous and returns USB_RET_ASYNC to an iso TD handling function that isn't prepared to deal with that. Add checks for both "iso transfer to non-iso endpoint" and "non-iso transfer to iso endpoint" and make those TDs fail rather than blindly sending them to the endpoint. Cc: [email protected] Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729 Signed-off-by: Peter Maydell <[email protected]> --- The diff looks a little complex because of the re-indenting; looking at an "ignore whitespace changes" diff might be clearer Only testing here has been running the repro in the bug tracker plus "make check-functional", which probably doesn't exercise USB very much. --- hw/usb/hcd-ohci.c | 61 +++++++++++++++++++++++++++------------------ hw/usb/trace-events | 2 ++ 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 5e5fe41f473..4d38150a845 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -579,7 +579,6 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed) int i; USBDevice *dev; USBEndpoint *ep; - USBPacket *pkt; QEMU_UNINITIALIZED uint8_t buf[8192]; bool int_req; struct ohci_iso_td iso_td; @@ -742,24 +741,31 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed) return 1; } ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN)); - pkt = g_new0(USBPacket, 1); - usb_packet_init(pkt); - int_req = relative_frame_number == frame_count && - OHCI_BM(iso_td.flags, TD_DI) == 0; - usb_packet_setup(pkt, pid, ep, 0, addr, false, int_req); - usb_packet_addbuf(pkt, buf, len); - usb_handle_packet(dev, pkt); + if (ep->type == USB_ENDPOINT_XFER_ISOC) { + USBPacket *pkt = g_new0(USBPacket, 1); - /* The USB core guarantees to never defer ISO TDs. */ - assert(pkt->status != USB_RET_ASYNC); + usb_packet_init(pkt); + int_req = relative_frame_number == frame_count && + OHCI_BM(iso_td.flags, TD_DI) == 0; + usb_packet_setup(pkt, pid, ep, 0, addr, false, int_req); + usb_packet_addbuf(pkt, buf, len); + usb_handle_packet(dev, pkt); - if (pkt->status == USB_RET_SUCCESS) { - ret = pkt->actual_length; + /* The USB core guarantees to never defer ISO TDs. */ + assert(pkt->status != USB_RET_ASYNC); + + if (pkt->status == USB_RET_SUCCESS) { + ret = pkt->actual_length; + } else { + ret = pkt->status; + } + usb_packet_cleanup(pkt); + g_free(pkt); } else { - ret = pkt->status; + /* Attempted iso transfer to non-iso endpoint */ + ret = USB_RET_NAK; + trace_usb_ohci_iso_td_to_non_iso_endpoint(ep->nr); } - usb_packet_cleanup(pkt); - g_free(pkt); trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr, str, len, ret); @@ -1022,16 +1028,23 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed) trace_usb_ohci_td_too_many_pending(ep->nr); return 1; } - usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r, - OHCI_BM(td.flags, TD_DI) == 0); - usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen); - usb_handle_packet(dev, &ohci->usb_packet); - trace_usb_ohci_td_packet_status(ohci->usb_packet.status); + if (ep->type != USB_ENDPOINT_XFER_ISOC) { + usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r, + OHCI_BM(td.flags, TD_DI) == 0); + usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen); + usb_handle_packet(dev, &ohci->usb_packet); + trace_usb_ohci_td_packet_status(ohci->usb_packet.status); - if (ohci->usb_packet.status == USB_RET_ASYNC) { - usb_device_flush_ep_queue(dev, ep); - ohci->async_td = addr; - return 1; + if (ohci->usb_packet.status == USB_RET_ASYNC) { + usb_device_flush_ep_queue(dev, ep); + ohci->async_td = addr; + return 1; + } + } else { + /* Attempted non-iso transfer to iso endpoint */ + ohci->usb_packet.status = USB_RET_NAK; + ohci->usb_packet.actual_length = 0; + trace_usb_ohci_td_to_iso_endpoint(ep->nr); } } if (ohci->usb_packet.status == USB_RET_SUCCESS) { diff --git a/hw/usb/trace-events b/hw/usb/trace-events index 80ead23358f..15f2d27dbfd 100644 --- a/hw/usb/trace-events +++ b/hw/usb/trace-events @@ -23,6 +23,7 @@ usb_ohci_iso_td_bad_direction(int dir) "Bad direction %d" usb_ohci_iso_td_bad_bp_be(uint32_t bp, uint32_t be) "ISO_TD bp 0x%.8x be 0x%.8x" usb_ohci_iso_td_bad_cc_not_accessed(uint32_t start, uint32_t next) "ISO_TD cc != not accessed 0x%.8x 0x%.8x" usb_ohci_iso_td_bad_cc_overrun(uint32_t start, uint32_t next) "ISO_TD start_offset=0x%.8x > next_offset=0x%.8x" +usb_ohci_iso_td_to_non_iso_endpoint(int ep) "ep=%d" usb_ohci_iso_td_so(uint32_t so, uint32_t eo, uint32_t s, uint32_t e, const char *str, ssize_t len, int ret) "0x%.8x eo 0x%.8x sa 0x%.8x ea 0x%.8x dir %s len %zu ret %d" usb_ohci_iso_td_data_overrun(int ret, ssize_t len) "DataOverrun %d > %zu" usb_ohci_iso_td_data_underrun(int ret) "DataUnderrun %d" @@ -55,6 +56,7 @@ usb_ohci_td_pkt_short(const char *dir, const char *buf) "%s data: %s" usb_ohci_td_pkt_full(const char *dir, const char *buf) "%s data: %s" usb_ohci_td_too_many_pending(int ep) "ep=%d" usb_ohci_td_packet_status(int status) "status=%d" +usb_ohci_td_to_iso_endpoint(int ep) "ep=%d" usb_ohci_ed_read_error(uint32_t addr) "ED read error at 0x%x" usb_ohci_ed_pkt(uint32_t cur, int h, int c, uint32_t head, uint32_t tail, uint32_t next) "ED @ 0x%.8x h=%u c=%u head=0x%.8x tailp=0x%.8x next=0x%.8x" usb_ohci_ed_pkt_flags(uint32_t fa, uint32_t en, uint32_t d, int s, int k, int f, uint32_t mps) "fa=%u en=%u d=%u s=%u k=%u f=%u mps=%u" -- 2.43.0
