[PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Peter Maydell]

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: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
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

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Peter Maydell]

On Thu, 1 Oct 2026 at 20:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.

Side note -- compare 45684127cd which fixes an assert we hit
in hcd-xhci when the guest configures an isoch transfer on a
non-isoch endpoint, and the code in the hcd-ehci model which
catches a similar case. An alternative approach here would be
to add an argument to usb_handle_packet() which was a bool for
"is isochronous transfer" (or some enum, for better ergonomics),
and have usb_handle_packet() complete the packet with an error
response in the case of an endpoint mismatch. That would allow
us to deal with this in one place and make it harder for the
controller models to forget about it, but it would also be more
invasive as a change, which is a bit unappealing given USB's
current unmaintained status and a lack of tests for it.

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Thomas Huth]

On 01/10/2026 21.47, Peter Maydell wrote:
> 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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729
Looks like this issue got assigned with a CVE number, so please add:

CVE: CVE-2026-7180

With that:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Peter Maydell]

On Fri, 2 Oct 2026 at 11:22, Thomas Huth <thuth@redhat.com> wrote:
>
> On 01/10/2026 21.47, Peter Maydell wrote:
> > 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: qemu-stable@nongnu.org
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729
> Looks like this issue got assigned with a CVE number, so please add:
>
> CVE: CVE-2026-7180

I guess we might as well, but honestly I don't think this is CVE worthy,
and I can't find any record of what that CVE number is supposed to cover
from a websearch.

This is in OHCI, which I thought was agreed to be outside
the security envelope, and in any case it's only an assertion failure.

The use-after-free that was the original subject of the bug report
has already been fixed by 7b1eba96aeca (which downgraded it to
"fails assertion").

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Daniel P. Berrangé]

On Fri, Oct 02, 2026 at 11:33:10AM +0100, Peter Maydell wrote:
> On Fri, 2 Oct 2026 at 11:22, Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 01/10/2026 21.47, Peter Maydell wrote:
> > > 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: qemu-stable@nongnu.org
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729

Change that to 'Related', since the assert fix you mention below is
what actually Resolves that issue initially

> > Looks like this issue got assigned with a CVE number, so please add:
> >
> > CVE: CVE-2026-7180
> 
> I guess we might as well, but honestly I don't think this is CVE worthy,
> and I can't find any record of what that CVE number is supposed to cover
> from a websearch.

This issue originated on the qemu-security list, and we declared it
a CVE because it was a guest triggerable use-after-free originally.

> This is in OHCI, which I thought was agreed to be outside
> the security envelope, and in any case it's only an assertion failure.
> 
> The use-after-free that was the original subject of the bug report
> has already been fixed by 7b1eba96aeca (which downgraded it to
> "fails assertion").

Ok, that commit is the one that should have been tagged with the CVE.

This commit is just a nice followup.

> 
> thanks
> -- PMM
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Philippe Mathieu-Daudé]

On 2026-10-01 21:47, Peter Maydell wrote:
> 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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

These functions are huge...

Side note, I don't get why we copy packet data *then* return
error if device not found (trace_usb_ohci_td_dev_error), this
is sub-optimal.

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Peter Maydell]

On Fri, 2 Oct 2026 at 11:31, Philippe Mathieu-Daudé
<philmd@oss.qualcomm.com> wrote:
> Side note, I don't get why we copy packet data *then* return
> error if device not found (trace_usb_ohci_td_dev_error), this
> is sub-optimal.

Yeah, that's a bit silly, but it will never happen in normal
operation, because a guest is never going to set up a transfer
to a non-existent endpoint.

-- PMM

Re: [PATCH] hw/usb/hcd-ohci: Fail iso packets sent to non-iso endpoints and vice-versa

[Philippe Mathieu-Daudé]

On 1/10/26 21:47, Peter Maydell wrote:
> 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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3729
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> 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(-)

Queued via hw-misc, thanks.