Re: [PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: core: hcd: cancel_work_sync() in usb_stop_hcd() strands pending BH URBs
--

commit 6e7c5c14d78d88d1d1fec5ad2d48ff9cf59b253d
Author: Igor Putko <igorpetindev@gmail.com>

usb: core: hcd: cancel giveback work synchronously on stop

This commit adds cancel_work_sync() calls for both high and low priority
giveback BH work items in usb_stop_hcd() to prevent use-after-free accesses
to struct usb_hcd when usb_giveback_urb_bh() is still running during HCD
teardown.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a865a15ea3863..e0f00d68598a1 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2788,6 +2788,9 @@ static void usb_stop_hcd(struct usb_hcd *hcd)
>  	/* In case the HCD restarted the timer, stop it again. */
>  	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
>  	timer_delete_sync(&hcd->rh_timer);
> +
> +	cancel_work_sync(&hcd->high_prio_bh.bh);
> +	cancel_work_sync(&hcd->low_prio_bh.bh);

[Severity: High]
Could calling cancel_work_sync() instead of draining the BH work items leave
URBs stranded on hcd->high_prio_bh.head and hcd->low_prio_bh.head, leaking
struct urb references and DMA mappings or hanging callers in usb_kill_urb()?

Unlike flush_work(), cancel_work_sync() steals the pending bit if &bh->bh has
not started executing yet, so usb_giveback_urb_bh() never runs for URBs
already queued on bh->head. In addition, if usb_giveback_urb_bh() is already
running a snapshot when new URBs are added to bh->head, it relies on
re-queueing &bh->bh before returning:

drivers/usb/core/hcd.c:usb_giveback_urb_bh() {
	...
	spin_lock_irq(&bh->lock);
	if (!list_empty(&bh->head)) {
		if (bh->high_prio)
			queue_work(system_bh_highpri_wq, &bh->bh);
		else
			queue_work(system_bh_wq, &bh->bh);
	}
	bh->running = false;
	spin_unlock_irq(&bh->lock);
}

Because cancel_work_sync() sets WORK_CANCEL_DISABLE while waiting for the
current execution to finish, that queue_work() call is silently dropped.

This can be reached in two ways during HCD teardown:

1. In usb_stop_hcd(), hcd->driver->stop(hcd) runs right before
cancel_work_sync(). For HCD drivers with HCD_BH set that complete remaining
URBs in their stop callback, such as in drivers/usb/dwc2/hcd.c:

_dwc2_hcd_stop()
  -> dwc2_hcd_disconnect()
    -> dwc2_kill_all_urbs()
      -> dwc2_host_complete()
        -> usb_hcd_giveback_urb()

URBs are added to bh->head and &bh->bh is queued immediately before
cancel_work_sync() cancels the pending work item.

2. During usb_remove_hcd() -> usb_disconnect() -> usb_disable_device() ->
usb_hcd_flush_endpoint(), usb_hcd_unlink_urb_from_ep() removes the URB from
ep->urb_list before usb_hcd_giveback_urb() appends it to bh->head. In
drivers/usb/core/hcd.c:usb_hcd_flush_endpoint():

	/* Wait until the endpoint queue is completely empty */
	while (!list_empty (&ep->urb_list)) {

this loop can observe an empty ep->urb_list and return without calling
usb_kill_urb() while the URB is still sitting on bh->head, allowing
usb_stop_hcd() to cancel &bh->bh before usb_giveback_urb_bh() processes it.

Any URB stranded on bh->head never reaches __usb_hcd_giveback_urb(), so it
is never unmapped via unmap_urb_for_dma(), its struct urb reference from
usb_get_urb() is leaked, urb->complete() is skipped, and urb->use_count
remains non-zero (which hangs any caller waiting in usb_kill_urb() or
usb_poison_urb()).

Also, should the comment in drivers/usb/core/hcd.c:usb_remove_hcd() right
above usb_stop_hcd(hcd):

	/*
	 * flush_work() isn't needed here because:
	 * - driver's disconnect() called from usb_disconnect() should
	 *   make sure its URBs are completed during the disconnect()
	 *   callback
	 *
	 * - it is too late to run complete() here since driver may have
	 *   been removed already now
	 */

be updated now that usb_stop_hcd() synchronously waits on the BH work items?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023174015.54793-1-igorpetindev@gmail.com?part=1