Re: [BUG] usb: gadgetfs: KASAN null-ptr-deref and intermittent UAF in ep_aio_cancel()

[Alan Stern]

On Fri, Sep 11, 2026 at 11:00:00PM +0900, Minseo Kim wrote:
> Hi Alan,
> 
> Thank you for the questions. I reran the tests to distinguish the
> request's endpoint-queue state from the callback's execution state, and
> the unbind flush from workqueue destruction during module cleanup.
> 
> > I didn't think this was possible.  gadgetfs_unbind() calls
> > destroy_ep_files() before doing the flush, and destroy_ep_files() calls
> > usb_ep_disable() for each active endpoint.  usb_ep_disable() doesn't
> > return until the completion handlers for all the outstanding requests on
> > the endpoint have finished.  This means no copy_work items should have
> > been left to queue when the flush occurred.
> 
> With both patches applied, this ordering was possible in the dummy_hcd
> test because the target request had already been removed from its
> endpoint queue before ep_aio_complete() ran. When gadgetfs_unbind()
> subsequently called destroy_ep_files(), dummy_disable() found the
> endpoint queue empty. The request associated with the already-running
> callback was no longer on that queue, so nuke() had no queued request to
> give back and dummy_disable() returned while the callback was still held
> at the gate. The unbind flush then returned before the callback queued
> copy_work.

Aha!  In other words, usb_ep_disable() doesn't guarantee that the 
completion handlers for all outstanding requests on the endpoint have 
finished -- it only guarantees that they have started.  That's an 
important difference.  Guess I should update the documentation for 
that routine.

> I confirmed this with markers recording the request and endpoint
> pointers. The gate was inside ep_aio_complete(), after the request had
> already been removed from its endpoint queue. It used a bounded,
> non-sleeping loop. dummy_hcd had already released dum->lock before
> calling usb_gadget_giveback_request().
> 
> In a control with the host-side transfer delayed, the request remained
> queued when usb_ep_disable() began. dummy_disable() called nuke(), which
> removed it and invoked its completion callback through
> usb_gadget_giveback_request(). While I held the callback at entry,
> neither usb_ep_disable() nor the ep0 close returned. After I released the
> gate, the callback returned before usb_ep_disable() did, matching your
> expectation for this pending-request case. The AIO request produced
> exactly one completion with res=-ESHUTDOWN.
> 
> In a separate run with GadgetFS still mounted, the unbind flush and ep0
> close returned while the callback was held before queuing copy_work. The
> reproducer had no GadgetFS file descriptors left open in userspace, but
> the module usage count was 2. rmmod was rejected because gadgetfs was in
> use, and gadgetfs_cleanup() had not begun. After I released the callback
> and the reproducer exited, the count was 1; subsequent unmount and rmmod
> succeeded.
> 
> > What prevented rmmod from completing after iocb->ki_complete() had
> > finished?  Was it waiting for the flush to finish?  If any additional
> > work items were added to the queue after the flush started, they should
> > not have blocked the flush.
> 
> In a separate worker-tail run using the same late-queueing ordering, the
> unbind flush had already returned before the callback queued copy_work.
> I then held the worker immediately after iocb->ki_complete() returned.
> The module usage count was zero before rmmod started. When rmmod was
> started, it waited in destroy_workqueue() during module cleanup, not in
> the earlier unbind flush. The relevant part of the blocked rmmod task's
> stack was:
> 
>     __flush_workqueue
>     drain_workqueue
>     destroy_workqueue
>     gadgetfs_cleanup [gadgetfs]
>     __do_sys_delete_module
> 
> The __flush_workqueue frame came from drain_workqueue(), which was
> called by destroy_workqueue(). Thus, the late copy_work did not block
> the earlier flush_workqueue() in gadgetfs_unbind(); it was already
> running when gadgetfs_cleanup() called destroy_workqueue(), and
> drain_workqueue() waited for it instead. Markers confirmed that
> destroy_workqueue() returned only after I released the worker, and rmmod
> then completed successfully.
> 
> This is consistent with the entry-gated ep_unlink_worker result in my
> previous message. In that test, unlink_work had already been queued
> before the unbind flush began, so the flush and the ep0 close remained
> blocked until I released the worker. In the late-queueing run above,
> copy_work was not queued until after the unbind flush had returned. A
> separate control with copy_work queued before the flush likewise kept
> the unbind flush and the ep0 close blocked until I released the worker.
> 
> Thank you for asking me to check this more closely.

These results explain a lot.  They mean that flushing the workqueue in 
gadgetfs_unbind() not only doesn't do what we want, it also isn't 
necessary -- because destroy_workqueue() will automatically do a flush 
for us.

Also, it isn't really necessary to flush the workqueue when the user 
program closes ep0 or unmounts the gadgetfs filesystem.  We merely have 
to make sure that when the module is unloaded, no work items are running 
or will be started.

Here's a revised version of the second patch, which omits the 
flush_workqueue() call from gadgetfs_unbind().  I expect it will 
eliminate the "module unloaded while work items are still running" 
problem just as well as the original version did.

Alan Stern



Index: usb-devel/drivers/usb/gadget/legacy/inode.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/legacy/inode.c
+++ usb-devel/drivers/usb/gadget/legacy/inode.c
@@ -237,6 +237,7 @@ static const char *CHIP;
 static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
 
 static DEFINE_SPINLOCK(aio_lock);	/* Protect aio cancellation info */
+static struct workqueue_struct *gadgetfs_wq;
 
 /*----------------------------------------------------------------------*/
 
@@ -514,9 +515,7 @@ static int ep_aio_cancel(struct kiocb *i
 		spin_unlock_irqrestore(&aio_lock, flags);
 		return 0;	/* ep_aio() will call us again if needed */
 	}
-
 	priv->cancel_state = AIO_UNLINKING;
-	spin_unlock_irqrestore(&aio_lock, flags);
 
 	/*
 	 * We are called with the aio core holding iocb's context lock.
@@ -527,7 +526,9 @@ static int ep_aio_cancel(struct kiocb *i
 	 * For this reason, do the dequeue operation in a work routine.
 	 */
 	INIT_WORK(&priv->unlink_work, ep_unlink_worker);
-	schedule_work(&priv->unlink_work);
+	queue_work(gadgetfs_wq, &priv->unlink_work);
+
+	spin_unlock_irqrestore(&aio_lock, flags);
 	return 0;
 }
 
@@ -597,7 +598,7 @@ static void ep_aio_complete(struct usb_e
 
 	if (new_req_state == AIO_COMPLETED) {
 		INIT_WORK(&priv->copy_work, ep_user_copy_worker);
-		schedule_work(&priv->copy_work);
+		queue_work(gadgetfs_wq, &priv->copy_work);
 	}
 	if (cancel_state != AIO_UNLINKING) {
 		usb_ep_free_request(ep, req);
@@ -2245,10 +2246,17 @@ static int __init gadgetfs_init (void)
 {
 	int status;
 
+	gadgetfs_wq = alloc_workqueue("gadgetfs", WQ_PERCPU, 0);
+	if (!gadgetfs_wq)
+		return -ENOMEM;
+
 	status = register_filesystem (&gadgetfs_type);
-	if (status == 0)
-		pr_info ("%s: %s, version " DRIVER_VERSION "\n",
-			shortname, driver_desc);
+	if (status) {
+		destroy_workqueue(gadgetfs_wq);
+		return status;
+	}
+
+	pr_info("%s: %s, version " DRIVER_VERSION "\n", shortname, driver_desc);
 	return status;
 }
 module_init (gadgetfs_init);
@@ -2257,6 +2265,7 @@ static void __exit gadgetfs_cleanup (voi
 {
 	pr_debug ("unregister %s\n", shortname);
 	unregister_filesystem (&gadgetfs_type);
+	destroy_workqueue(gadgetfs_wq);
 }
 module_exit (gadgetfs_cleanup);