Re: [PATCH 1/2] usbip: stub: reference count struct stub_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usbip: stub: hung stub_tx, leaks, and URB UAF when shutdown is
  missed
- [Medium] usbip: use-after-free in event_handler() reading eh_ops.put
  after wake_up()
--

commit cd9a80911da15b88ceb306a5e8d623b311108647
Author: Michael Grzeschik <grzeschik@puzzle.ch>

usbip: stub: reference count struct stub_device

This commit adds kref reference counting to struct stub_device and holds
references across device binding, the rx/tx kthreads, and queued events so
that stub_device and its usb_device are not freed while still in use.

> diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
> index abfa11d6bde79..27d1e9c8db176 100644
> --- a/drivers/usb/usbip/stub_dev.c
> +++ b/drivers/usb/usbip/stub_dev.c
> @@ -11,6 +11,42 @@
>  #include "usbip_common.h"
>  #include "stub.h"
>  
> +/*
> + * A stub_device is referenced by its binding to the USB device (from probe
> + * until disconnect), by the rx and tx threads while they run, and by the
> + * event handler while an event for it is queued or being handled. It must
> + * not be freed before all of them are done with it: stopping the threads
> + * relies on the event handler, which can miss a shutdown request or still
> + * have an event queued when disconnect wants to free the device.
> + */
> +static void stub_device_release(struct kref *kref)
> +{
> +	struct stub_device *sdev = container_of(kref, struct stub_device, kref);
> +
> +	usb_put_dev(sdev->udev);
> +	kfree(sdev);
> +}

[Severity: High]
If the rx and tx threads outlive stub_disconnect() because
stub_shutdown_connection() was missed, does stub_tx_loop() hang forever on
sdev->tx_waitq, or does stub_device_release() free sdev while in-flight
URBs and connection resources are still active?

Once shutdown_busid() queues SDEV_EVENT_REMOVED during stub_disconnect(),
USBIP_EH_BYE is set in ud->event, which causes any later usbip_event_add()
call to return early without scheduling stub_shutdown_connection():

drivers/usb/usbip/usbip_event.c:usbip_event_add() {
	if (ud->event & USBIP_EH_BYE)
		return;
}

Without stub_shutdown_connection() calling kthread_stop_put(ud->tcp_tx),
stub_tx_loop() blocks indefinitely in wait_event_interruptible() on
sdev->tx_waitq because neither socket closure nor usbip_event_add() wakes
that queue.

Even if both threads exit on a socket error and drop their references,
stub_device_release() only drops sdev->udev and frees sdev without running
the cleanup from stub_shutdown_connection().  That leaks the ud->tcp_rx and
ud->tcp_tx task structs, ud->tcp_socket, and the sdev->unlink_tx and
sdev->unlink_free lists, and leaves in-flight URBs on sdev->priv_init to
dereference the freed sdev in stub_complete().  In addition, letting
stub_disconnect() return while threads or events are still running allows
usbip_host_exit() to destroy stub_priv_cache and finish unloading the
module while sdev is still in use.

[ ... ]

> diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
> index 0e00c2d000f89..eea9a71f0eac2 100644
> --- a/drivers/usb/usbip/usbip_event.c
> +++ b/drivers/usb/usbip/usbip_event.c
> @@ -94,6 +94,10 @@ static void event_handler(struct work_struct *work)
>  		mutex_unlock(&ud->sysfs_lock);
>  
>  		wake_up(&ud->eh_waitq);
> +
> +		/* drop the reference taken by usbip_event_add() */
> +		if (ud->eh_ops.put)
> +			ud->eh_ops.put(ud);
>  	}
>  }

[Severity: Medium]
Can waking ud->eh_waitq before reading ud->eh_ops.put cause a
use-after-free on ud?

vhci and vudc do not set eh_ops.get or eh_ops.put, and their removal paths
free the enclosing device as soon as usbip_stop_eh() returns from waiting
on ud->eh_waitq:

CPU0 (event_handler)              CPU1 (vudc_remove)
unset_event(ud, ...)
wake_up(&ud->eh_waitq)
                                  usb_del_gadget_udc()
                                    vgadget_pullup()
                                      usbip_stop_eh(&udc->ud)  /* returns */
                                  kfree(udc)
if (ud->eh_ops.put)  <-- use-after-free

For stub_device, waking usbip_stop_eh() before ud->eh_ops.put(ud) runs (or
while a second event queued for the same ud is still on event_list) also
lets stub_disconnect() and usbip_host_exit() finish unloading usbip-host.ko
while event_handler() in usbip-core.ko is still about to call
stub_eh_put().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-usbip-v1-0-da5217ac71d7@puzzle.ch?part=1