Re: [PATCH QEMU HCI-8.0 v6 10/12] usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] usb-host: CPR post_load fails on an unconfigured device (NOT_FOUND treated as fatal)
- [Medium] usb-host: re-claim loop uses array index instead of bInterfaceNumber
- [Medium] usb-host: device-supplied bInterfaceNumber indexes altsetting[16] unchecked; ninterfaces unclamped
- [Low] usb-host: usb_host_ep_update() result ignored on CPR path, so partial rebuild reports success
--

commit 7fc1fbcbe45c4e899a6393d8acc4e2e9151dac29
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: reconfigure endpoints on CPR incoming #VSTOR-137800

This commit makes the CPR incoming path in usb_host_post_load() re-claim the
interfaces through libusb and call usb_host_ep_update(). The goal is to
rebuild the endpoints for a non-zero altsetting restored by vmload. It also
adds an "alt >= 0" check to an assert in usb_host_ep_update().

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> +static int usb_host_cpr_reclaim_interfaces(USBHostDevice *s)
> +{
> +    USBDevice *udev = USB_DEVICE(s);
> +    struct libusb_config_descriptor *conf;
> +    int n, rc;
> +
> +    rc = libusb_get_active_config_descriptor(s->dev, &conf);
> +    if (rc != 0) {
> +        usb_host_libusb_error("libusb_get_active_config_descriptor", rc);
> +        return -1;
> +    }

[Severity: Medium]
Can this abort the whole incoming CPR vmload for a device that has no active
configuration?

For a device in the address state (for example after SET_CONFIGURATION(0) or
a device reset), libusb_get_active_config_descriptor() returns
LIBUSB_ERROR_NOT_FOUND.  Here any rc != 0 returns -1, and
usb_host_post_load() propagates it:

    if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
        return -1;
    }

Before this patch the CPR branch always returned 0.  The existing
usb_host_claim_interfaces() treats the same condition as benign:

    if (rc == LIBUSB_ERROR_NOT_FOUND) {
        /* address state - ignore */
        return USB_RET_SUCCESS;
    }

and usb_host_ep_update() simply returns.  Should NOT_FOUND be tolerated here
as well?

Also, if the host still has an active configuration but the guest never
configured the device, does this function claim every interface and then
overwrite the guest-visible state?

    udev->ninterfaces = conf->bNumInterfaces;
    udev->configuration = conf->bConfigurationValue;

The source had both of these at 0.  The later patch "usb-host: make CPR
migration blocker conditional" only changes the blocker and does not touch
this helper.

> +
> +    for (int i = 0; i < conf->bNumInterfaces; i++) {
> +        int ifnum = conf->interface[i].altsetting[0].bInterfaceNumber;
> +        int alt = udev->altsetting[ifnum];

[Severity: Medium]
Can this read past the end of udev->altsetting[]?

ifnum comes from the device's bInterfaceNumber (a uint8_t, up to 255), while
altsetting[] has USB_MAX_INTERFACES (16) entries.  This validation loop runs
over all conf->bNumInterfaces entries with no bound on ifnum.

The claim loop below is clamped with MIN(conf->bNumInterfaces,
USB_MAX_INTERFACES), but the function then stores the unclamped value and
returns success:

    udev->ninterfaces = conf->bNumInterfaces;

Can a device with more than 16 interfaces then trip this assert in a later
usb_desc_set_config()?

    assert(dev->ninterfaces <= USB_MAX_INTERFACES);

usb_host_claim_interfaces() would have returned a stall in that case.

usb_host_ep_update(), which is now also called from the CPR path, has the
same unchecked access:

    alt = udev->altsetting[intf->bInterfaceNumber];

This isn't a bug introduced by this patch, but the new call site makes it
reachable on the CPR path.  The patch only adds "alt >= 0" to the assert
there.

> +
> +        if (alt < 0 || alt >= conf->interface[i].num_altsetting) {
> +            error_report("usb-host: migrated altsetting %d out of range for "
> +                         "interface %d", alt, ifnum);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +    }
> +
> +    n = MIN(conf->bNumInterfaces, USB_MAX_INTERFACES);
> +    for (int i = 0; i < n; i++) {
> +        rc = libusb_claim_interface(s->dh, i);
> +        if (rc != 0) {
> +            usb_host_libusb_error("libusb_claim_interface", rc);
> +            libusb_free_config_descriptor(conf);
> +            return -1;
> +        }
> +        s->ifs[i].claimed = true;
> +    }

[Severity: Medium]
Is the loop index i the right value to pass to libusb_claim_interface()?

i is the position in conf->interface[], but libusb and usbfs expect the
interface number, which is altsetting[0].bInterfaceNumber.  The validation
loop above already uses bInterfaceNumber for this.  The two can differ for
devices whose interface numbers don't start at 0 or aren't contiguous, such
as {0,2} or {1,2}.

Claiming a non-existent number fails, so the function returns -1 and the
migration aborts.  Where the claim does succeed, the real interfaces are not
recorded in libusb's claimed set, so a later SET_INTERFACE can still fail
with LIBUSB_ERROR_NOT_FOUND.  That is the problem this patch sets out to fix.

s->ifs[i].claimed is also indexed by position, so usb_host_release_interfaces()
would act on the wrong numbers.

By contrast, usb_host_claim_interfaces() probes by interface number and
tolerates gaps:

    for (i = 0; i < USB_MAX_INTERFACES; i++) {
        trace_usb_host_claim_interface(s->bus_num, s->addr, configuration, i);
        rc = libusb_claim_interface(s->dh, i);
        if (rc == 0) {
            s->ifs[i].claimed = true;

Should the new code claim by bInterfaceNumber and index ifs[] the same way?
The later patches in the series do not change this.

[ ... ]

>      if (cpr_is_incoming()) {
> +        /*
> +         * The device stayed open, so rebuild libusb's claims and endpoints,
> +         * and start reaping now that the source has handed the fd off.
> +         */
> +        if (usb_host_cpr_reclaim_interfaces(dev) < 0) {
> +            return -1;
> +        }
> +        usb_host_ep_update(dev);
>          usb_host_cpr_restart_events();
>          return 0;
>      }

[Severity: Low]
Should the result of usb_host_ep_update() be checked here?

usb_host_ep_update() is void.  After usb_ep_reset() it returns silently on
"invalid endpoint address", "duplicate endpoint address", or a missing
active configuration, which leaves a partial endpoint table.
usb_host_post_load() then returns 0, so guest transfers to the missing
endpoints would stall without any report.

usb_host_ep_update() also calls usb_ep_set_halted(udev, pid, ep, 0) for every
endpoint, so any halted state the guest had on the source is lost, since it
is not migrated.  This second point may be speculative.  The failure
conditions would normally already have occurred in usb_host_open() on the
same device, so this is probably a rare edge case.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v6-0-8688ec9576d1@virtuozzo.com?part=10