On Tue, 4 Nov 2025 at 09:42, zoudongjie <[email protected]> wrote:
>
> The EHCI controller in QEMU seems to cause a UAF (Use-After-Free) issue when
> handling packets from abnormal USB devices. Specifically, when the USB
> device's
> firmware behaves abnormally and causes some initialization requests to block
> and time out, passing that USB device through to QEMU's EHCI controller
> appears
> to make the do-while loop in ehci_advance_state repeatedly submit multiple
> requests to handle the same packet (this do-while loop is quite complex;
> I confirmed the issue by adding logs in usb_host_cancel_packet).
Hi; thanks for this patch. I'm not a USB expert so my comments
below are just some surface level questions. Like you, I wonder
if the correct fix ought instead to be to not have multiple in-flight
requests for the same packet, but I don't know our USB subsystem
at all well enough to answer that.
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index b74670ae25..b5aab12aee 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -406,18 +406,6 @@ static void usb_host_req_free(USBHostRequest *r)
> g_free(r);
> }
>
> -static USBHostRequest *usb_host_req_find(USBHostDevice *s, USBPacket *p)
> -{
> - USBHostRequest *r;
> -
> - QTAILQ_FOREACH(r, &s->requests, next) {
> - if (r->p == p) {
> - return r;
> - }
> - }
> - return NULL;
> -}
> -
> static void LIBUSB_CALL usb_host_req_complete_ctrl(struct libusb_transfer
> *xfer)
> {
> USBHostRequest *r = xfer->user_data;
> @@ -1276,7 +1264,7 @@ static void usb_host_unrealize(USBDevice *udev)
> static void usb_host_cancel_packet(USBDevice *udev, USBPacket *p)
> {
> USBHostDevice *s = USB_HOST_DEVICE(udev);
> - USBHostRequest *r;
> + USBHostRequest *r, *next_entry;
>
> if (p->combined) {
> usb_combined_packet_cancel(udev, p);
> @@ -1285,10 +1273,17 @@ static void usb_host_cancel_packet(USBDevice *udev,
> USBPacket *p)
>
> trace_usb_host_req_canceled(s->bus_num, s->addr, p);
>
> - r = usb_host_req_find(s, p);
> - if (r && r->p) {
> - r->p = NULL; /* mark as dead */
> - libusb_cancel_transfer(r->xfer);
> + QTAILQ_FOREACH_SAFE(r, &s->requests, next, next_entry) {
> + if (r->p == p) {
> + if (unlikely(r && r->fake_in_flight)) {
What is this fake_in_flight field ? r is a USBHostRequest
but the definition of this struct doesn't seem to have that
field in it.
> + usb_host_req_free(r);
> + continue;
> + }
> + if (r && r->p) {
We're inside the "if (r->p == p)" check here, so we
know that r and r->p are the same. And the
QTAILQ_FOREACH_SAFE() macro that iterates 'r' through
the request list will only give us non-NULL pointers;
it terminates when it hits the NULL. So I don't think
we need to test this, it's always true.
> + r->p = NULL; /* mark as dead */
> + libusb_cancel_transfer(r->xfer);
> + }
> + }
> }
> }
thanks
-- PMM
