Re: [PATCH QEMU HCI-8.0 v6 01/12] usb-host: don't leak hostdev FD on open failure #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb-host: usb_host_open() stores hostfd before early checks that skip the new close
--

commit 0fc31623dd0ee74837ec2c01d3f3ced2e30b7f95
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: don't leak hostdev FD on open failure #VSTOR-137800

This patch stores the caller-supplied hostfd in s->hostfd at the top of
usb_host_open() when no libusb_device is given. It also closes s->hostfd
on the fail: path, so the FD is released when libusb_wrap_sys_device() or
a later open step fails.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index b74670ae25696..83645fbe0ced0 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -954,6 +954,11 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      int rc;
>      Error *local_err = NULL;
>  
> +    /* Make sure failure path frees caller-owned FD */
> +    if (!dev) {
> +        s->hostfd = hostfd;
> +    }
> +
>      if (s->bh_postld_pending) {
>          return -1;
>      }
>      if (s->dh != NULL) {
>          goto fail;
>      }

[Severity: Medium]
Can this leak the FD when usb_host_open() returns early?

The store of s->hostfd happens before the bh_postld_pending check:

    if (!dev) {
        s->hostfd = hostfd;
    }

    if (s->bh_postld_pending) {
        return -1;
    }

That early return bypasses the fail: label and its new close(s->hostfd)
block. The caller-owned FD is then neither closed nor released, and
s->hostfd keeps holding it.

usb_host_realize() does not close the FD either. It only calls
error_setg() and returns:

    rc = usb_host_open(s, NULL, fd);
    if (rc < 0) {
        error_setg(errp, "failed to open host usb device %s", s->hostdevice);
        return;
    }

The s->dh != NULL case has a related problem. It does jump to fail:, but
s->hostfd has already been overwritten. The FD that s->hostfd previously
owned is lost, and the new caller's FD is closed instead.

Today the only caller that passes a real FD is usb_host_realize(). There
s->hostfd starts at -1 and s->dh is NULL, so both early exits look
unreachable and the problem is latent. I also checked the end of the
series. The store is still placed before the bh_postld_pending check, so
a later patch does not fix this.

Would it be better to assign s->hostfd only after those two early checks?
Alternatively, the early checks could be routed through the fail: path in
a way that does not clobber an FD that is already owned.

> @@ -978,7 +983,6 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>          if (rc != 0) {
>              goto fail;
>          }
> -        s->hostfd  = hostfd;
>          dev = libusb_get_device(s->dh);

[ ... ]

> @@ -1066,6 +1070,10 @@ fail:
>          s->dh = NULL;
>          s->dev = NULL;
>      }
> +    if (s->hostfd != -1) {
> +        close(s->hostfd);
> +        s->hostfd = -1;
> +    }
>      return -1;
>  }

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