[PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

[PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

[Yang, Liang1]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

Dear QEMU maintainers,

I would like to submit a patch for preventing the guest VM crash caused by the assertion failure in usb_host_ep_update() during USB hotplug/unplug on host passthrough.

QEMU issue submitted:
https://gitlab.com/qemu-project/qemu/-/issues/3189

Please help to review below patch, thanks!

===============================

Author: Liang1 Yang liang1.yang@intel.com<mailto:liang1.yang@intel.com>
Date:   Thu Oct 30 20:07:41 2025 +0800

    hw/usb/host-libusb: Do not assert when detects invalid alt

    Log warning and skip the interface instead of asserting in qemu
    host-libusb when there is invalid altsetting index during fast
    USB device hotplug/unplug.
    This is to prevent guest vm from crashing which is caused by
    QEMU task abort.

    Signed-off-by: Liang1 Yang liang1.yang@intel.com<mailto:liang1.yang@intel.com>

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 691bc881fb..3a08caafa5 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -885,6 +885,15 @@ static void usb_host_ep_update(USBHostDevice *s)
     trace_usb_host_parse_config(s->bus_num, s->addr,
                                 conf->bConfigurationValue, true);

+    /* Log and skip if configuration is NULL or has no interfaces */
+    if (!conf || conf->bNumInterfaces == 0) {
+        warn_report("usb-host: ignoring invalid configuration "
+            "for device %s (bus=%03d, addr=%03d)",
+            udev->product_desc ? udev->product_desc : "unknown",
+            s->bus_num, s->addr);
+        return;
+    }
+
     for (i = 0; i < conf->bNumInterfaces; i++) {
         /*
          * The udev->altsetting array indexes alternate settings
@@ -896,7 +905,21 @@ static void usb_host_ep_update(USBHostDevice *s)
         alt = udev->altsetting[intf->bInterfaceNumber];

         if (alt != 0) {
-            assert(alt < conf->interface[i].num_altsetting);
+            if (alt >= conf->interface[i].num_altsetting) {
+                /*
+                 * Recommend fix: sometimes libusb reports a temporary
+                 * invalid altsetting index during fast hotplug/unplug.
+                 * Instead of aborting, log a warning and skip the interface.
+                 */
+                warn_report("usb-host: ignoring invalid altsetting=%d (max=%d) "
+                    "for interface=%d on %s (bus=%03d, addr=%03d)",
+                    alt,
+                    conf->interface[i].num_altsetting ? conf->interface[i].num_altsetting - 1 : -1,
+                    i,
+                    udev->product_desc ? udev->product_desc : "unknown",
+                    s->bus_num, s->addr);
+                continue;
+            }
             intf = &conf->interface[i].altsetting[alt];
         }


Best regards,
Yang Liang


[-- Attachment #2: Type: text/html, Size: 9382 bytes --]

Re: [PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

[Peter Maydell]

On Thu, 20 Nov 2025 at 05:27, Yang, Liang1 <liang1.yang@intel.com> wrote:
>
> Dear QEMU maintainers,
>
>
>
> I would like to submit a patch for preventing the guest VM crash caused by the assertion failure in usb_host_ep_update() during USB hotplug/unplug on host passthrough.
>
>
>
> QEMU issue submitted:
>
> https://gitlab.com/qemu-project/qemu/-/issues/3189
>
>
>
> Please help to review below patch, thanks!

Hi; thanks for this patch. I don't really know our USB
subsystem, so this is more some surface-level comments
rather than an in-depth review.

(PS: for future patch submissions, it would be helpful
if you could send them as plain text, not HTML emails.)


> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 691bc881fb..3a08caafa5 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
>
> @@ -885,6 +885,15 @@ static void usb_host_ep_update(USBHostDevice *s)
>      trace_usb_host_parse_config(s->bus_num, s->addr,
>                                  conf->bConfigurationValue, true);
>
> +    /* Log and skip if configuration is NULL or has no interfaces */

This comment says what the code does, which is not very useful.
More helpful is to say *why* the code does something.

> +    if (!conf || conf->bNumInterfaces == 0) {

Why are we testing conf for NULL here? We already dereference
it in the line above, so this is too late to check if we need to.
We get conf by calling libusb_get_active_config_descriptor(),
and I don't think that will set conf to NULL if it succeeds,
so it looks to me like we don't need to test for NULL.

> +        warn_report("usb-host: ignoring invalid configuration "
> +            "for device %s (bus=%03d, addr=%03d)",
> +            udev->product_desc ? udev->product_desc : "unknown",
> +            s->bus_num, s->addr);
> +        return;

This is something we already get wrong in a lot of the
error-exit paths of this function, but we need to free
the conf descriptor with libusb_free_config_descriptor().
(If you wanted to include a patch to fix those existing
leaks that would be great.)

We already ignored a bNumInterfaces==0 config
(because the rest of the function would just do nothing), so
the only change here is to warn about it.

(1) Have you seen the bNumInterfaces==0 situation in real life?

(2) This is unrelated to the other half of the patch, so it
should be its own patch (i.e. one which just says "warn about
this kind of invalid device rather than silently ignoring it").

(3) All the other cases in this function of "the info about
the endpoint looked weird" we report via the tracepoint
trace_usb_host_parse_error(). I don't have a strong opinion
about warn_report vs a tracepoint here, but I do think we
should be consistent.

> +    }
> +
>      for (i = 0; i < conf->bNumInterfaces; i++) {
>          /*
>           * The udev->altsetting array indexes alternate settings
> @@ -896,7 +905,21 @@ static void usb_host_ep_update(USBHostDevice *s)
>          alt = udev->altsetting[intf->bInterfaceNumber];
>
>          if (alt != 0) {
> -            assert(alt < conf->interface[i].num_altsetting);
> +            if (alt >= conf->interface[i].num_altsetting) {
> +                /*
> +                 * Recommend fix: sometimes libusb reports a temporary

"Recommend fix" doesn't make sense here -- you can delete that bit.

> +                 * invalid altsetting index during fast hotplug/unplug.
> +                 * Instead of aborting, log a warning and skip the interface.
> +                 */
> +                warn_report("usb-host: ignoring invalid altsetting=%d (max=%d) "
> +                    "for interface=%d on %s (bus=%03d, addr=%03d)",
> +                    alt,
> +                    conf->interface[i].num_altsetting ? conf->interface[i].num_altsetting - 1 : -1,
> +                    i,
> +                    udev->product_desc ? udev->product_desc : "unknown",
> +                    s->bus_num, s->addr);
> +                continue;

For other errors with the intf info we "return", i.e.
skip the whole endpoint, rather than just continuing.
Should we do the same thing here ?
(Conceptually the same thing -- just "return" leaks
the config descriptor, as noted above.)

> +            }
>              intf = &conf->interface[i].altsetting[alt];
>          }

thanks
-- PMM

RE: [PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

[Yang, Liang1]

Hi Peter,

Thank you very much for the review and the detailed comments!
Please see my comments embedded below and I will update the patch accordingly.

Best regards,
Yang Liang

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org> 
Sent: Monday, November 24, 2025 8:50 PM
To: Yang, Liang1 <liang1.yang@intel.com>
Cc: qemu-devel@nongnu.org; qemu-stable@nongnu.org; kraxel@redhat.com; Kim, Dongwon <dongwon.kim@intel.com>
Subject: Re: [PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

On Thu, 20 Nov 2025 at 05:27, Yang, Liang1 <liang1.yang@intel.com> wrote:
>
> Dear QEMU maintainers,
>
>
>
> I would like to submit a patch for preventing the guest VM crash caused by the assertion failure in usb_host_ep_update() during USB hotplug/unplug on host passthrough.
>
>
>
> QEMU issue submitted:
>
> https://gitlab.com/qemu-project/qemu/-/issues/3189
>
>
>
> Please help to review below patch, thanks!

Hi; thanks for this patch. I don't really know our USB subsystem, so this is more some surface-level comments rather than an in-depth review.

(PS: for future patch submissions, it would be helpful if you could send them as plain text, not HTML emails.)

YL: Thanks, well noted.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c index 
> 691bc881fb..3a08caafa5 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
>
> @@ -885,6 +885,15 @@ static void usb_host_ep_update(USBHostDevice *s)
>      trace_usb_host_parse_config(s->bus_num, s->addr,
>                                  conf->bConfigurationValue, true);
>
> +    /* Log and skip if configuration is NULL or has no interfaces */

This comment says what the code does, which is not very useful.
More helpful is to say *why* the code does something.

YL: Agree, will update accordingly.

> +    if (!conf || conf->bNumInterfaces == 0) {

Why are we testing conf for NULL here? We already dereference it in the line above, so this is too late to check if we need to.
We get conf by calling libusb_get_active_config_descriptor(),
and I don't think that will set conf to NULL if it succeeds, so it looks to me like we don't need to test for NULL.

YL: Agree, will remove the check for conf==NULL.

> +        warn_report("usb-host: ignoring invalid configuration "
> +            "for device %s (bus=%03d, addr=%03d)",
> +            udev->product_desc ? udev->product_desc : "unknown",
> +            s->bus_num, s->addr);
> +        return;

This is something we already get wrong in a lot of the error-exit paths of this function, but we need to free the conf descriptor with libusb_free_config_descriptor().
(If you wanted to include a patch to fix those existing leaks that would be great.)

YL: I see, let me check and see if can create a patch to fix those leaks.

We already ignored a bNumInterfaces==0 config (because the rest of the function would just do nothing), so the only change here is to warn about it.

(1) Have you seen the bNumInterfaces==0 situation in real life?

YL: No, I have not seen (or noticed) this in real testing, but I think it's good to have especially during very fast USB hot-unplug/hot-replug.

(2) This is unrelated to the other half of the patch, so it should be its own patch (i.e. one which just says "warn about this kind of invalid device rather than silently ignoring it").

YL: This is for preventing the potential assert, but we can separate it into another patch or just remove it if it is really not required.

(3) All the other cases in this function of "the info about the endpoint looked weird" we report via the tracepoint trace_usb_host_parse_error(). I don't have a strong opinion about warn_report vs a tracepoint here, but I do think we should be consistent.

YL: Understand, I think it depends. For normal QEMU USB event tracing or debugging, it's better to use the tracepoint functions. But in this case, it is reporting an unexpected condition or behavior.

> +    }
> +
>      for (i = 0; i < conf->bNumInterfaces; i++) {
>          /*
>           * The udev->altsetting array indexes alternate settings @@ 
> -896,7 +905,21 @@ static void usb_host_ep_update(USBHostDevice *s)
>          alt = udev->altsetting[intf->bInterfaceNumber];
>
>          if (alt != 0) {
> -            assert(alt < conf->interface[i].num_altsetting);
> +            if (alt >= conf->interface[i].num_altsetting) {
> +                /*
> +                 * Recommend fix: sometimes libusb reports a 
> + temporary

"Recommend fix" doesn't make sense here -- you can delete that bit.

YL: Agree, will remove it.

> +                 * invalid altsetting index during fast hotplug/unplug.
> +                 * Instead of aborting, log a warning and skip the interface.
> +                 */
> +                warn_report("usb-host: ignoring invalid altsetting=%d (max=%d) "
> +                    "for interface=%d on %s (bus=%03d, addr=%03d)",
> +                    alt,
> +                    conf->interface[i].num_altsetting ? conf->interface[i].num_altsetting - 1 : -1,
> +                    i,
> +                    udev->product_desc ? udev->product_desc : "unknown",
> +                    s->bus_num, s->addr);
> +                continue;

For other errors with the intf info we "return", i.e.
skip the whole endpoint, rather than just continuing.
Should we do the same thing here ?
(Conceptually the same thing -- just "return" leaks the config descriptor, as noted above.)

YL: In this case, only one interface has an inconsistent altsetting index (caused by hot unplug/replug) while the others remain valid, so I use "continue" for skipping only the invalid interface avoids losing valid endpoint information. Returning early would drop all remaining interfaces, which seems undesirable.  Thanks.

> +            }
>              intf = &conf->interface[i].altsetting[alt];
>          }

thanks
-- PMM

Re: [PATCH] hw/usb/host-libusb: Do not assert when detects invalid altsetting

[Peter Maydell]

On Wed, 26 Nov 2025 at 11:20, Yang, Liang1 <liang1.yang@intel.com> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>

> > +        warn_report("usb-host: ignoring invalid configuration "
> > +            "for device %s (bus=%03d, addr=%03d)",
> > +            udev->product_desc ? udev->product_desc : "unknown",
> > +            s->bus_num, s->addr);
> > +        return;
>
> This is something we already get wrong in a lot of the error-exit paths of this function, but we need to free the conf descriptor with libusb_free_config_descriptor().
> (If you wanted to include a patch to fix those existing leaks that would be great.)
>
> YL: I see, let me check and see if can create a patch to fix those leaks.
>
> We already ignored a bNumInterfaces==0 config (because the rest of the function would just do nothing), so the only change here is to warn about it.
>
> (1) Have you seen the bNumInterfaces==0 situation in real life?
>
> YL: No, I have not seen (or noticed) this in real testing, but I think it's good to have especially during very fast USB hot-unplug/hot-replug.

If you haven't seen it, why worry about it? We won't crash if it
happens, we'll just ignore the device.

> (2) This is unrelated to the other half of the patch, so it should be its own patch (i.e. one which just says "warn about this kind of invalid device rather than silently ignoring it").
>
> YL: This is for preventing the potential assert, but we can separate it into another patch or just remove it if it is really not required.
>
> (3) All the other cases in this function of "the info about the endpoint looked weird" we report via the tracepoint trace_usb_host_parse_error(). I don't have a strong opinion about warn_report vs a tracepoint here, but I do think we should be consistent.
>
> YL: Understand, I think it depends. For normal QEMU USB event tracing or debugging, it's better to use the tracepoint functions. But in this case, it is reporting an unexpected condition or behavior.

All of these error cases are the same thing, though -- the device (via
libusb) is reporting something to us that seems to be invalid.

> > +                 * invalid altsetting index during fast hotplug/unplug.
> > +                 * Instead of aborting, log a warning and skip the interface.
> > +                 */
> > +                warn_report("usb-host: ignoring invalid altsetting=%d (max=%d) "
> > +                    "for interface=%d on %s (bus=%03d, addr=%03d)",
> > +                    alt,
> > +                    conf->interface[i].num_altsetting ? conf->interface[i].num_altsetting - 1 : -1,
> > +                    i,
> > +                    udev->product_desc ? udev->product_desc : "unknown",
> > +                    s->bus_num, s->addr);
> > +                continue;
>
> For other errors with the intf info we "return", i.e.
> skip the whole endpoint, rather than just continuing.
> Should we do the same thing here ?
> (Conceptually the same thing -- just "return" leaks the config descriptor, as noted above.)
>
> YL: In this case, only one interface has an inconsistent altsetting index (caused by hot unplug/replug) while the others remain valid, so I use "continue" for skipping only the invalid interface avoids losing valid endpoint information. Returning early would drop all remaining interfaces, which seems undesirable.  Thanks.

But this USB device is just (temporarily) broken, isn't it? Doesn't it all come
back with the right information later?

If it's reporting garbage to us then ignoring it entirely until it's
consistently sensible seems safer to me.

thanks
-- PMM