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

[Minseo Kim]

Hi Alan,

Thank you for the revised patch.

Following your explanation, I focused this round of testing on how the
revised patch handles module lifetime.

I applied your first patch followed by this revised second patch to
upstream v7.2-rc1, commit
dc59e4fea9d83f03bad6bddf3fa2e52491777482, using
CONFIG_USB_GADGETFS=m for the runtime tests.

With the unbind flush removed, I held ep_unlink_worker() at entry.
gadgetfs_unbind() returned and the module usage count reached zero, but
rmmod remained blocked in gadgetfs_cleanup()'s destroy_workqueue(). After
I released the worker, rmmod completed successfully. Holding
ep_user_copy_worker() immediately after iocb->ki_complete() likewise kept
rmmod blocked in destroy_workqueue() until I released the worker. In both
cases, the blocked rmmod stack showed __flush_workqueue(),
drain_workqueue(), destroy_workqueue(), and gadgetfs_cleanup().

To check whether module unloading could begin before a callback published
its work, I held the completion callback immediately before it queued
copy_work. At that point, the reproducer's file descriptor table contained
no GadgetFS descriptors. In two runs, each starting from a fresh boot, a
direct umount2() call failed with errno set to EBUSY while the callback was
held. The module usage count remained 2, rmmod was rejected because the
module was in use, and gadgetfs_cleanup() did not begin. After I released
the callback, it queued copy_work, the AIO request completed, and subsequent
unmount and rmmod attempts succeeded.

I also held ep_aio_complete() immediately after iocb->ki_complete() on
the PWRITE path that does not queue copy_work. In two runs, each starting
from a fresh boot, after the reproducer consumed the AIO completion event
and closed all GadgetFS descriptors, unmount remained pending, at least
one concurrent delete_module() attempt failed with errno set to
EWOULDBLOCK, and gadgetfs_cleanup() did not begin while the callback tail
was held. After I released the gate, the callback returned, unmount
completed, and module removal succeeded.

In another cancellation test, both unlink_work and copy_work were
outstanding on gadgetfs_wq when rmmod entered destroy_workqueue().
Releasing unlink_work alone did not allow cleanup to return; it returned
only after I released copy_work.

Using the GadgetFS source with both patches applied and without the
diagnostic gates or markers, I reran the original NULL pointer dereference
and UAF reproducers and the relevant AIO cancellation, payload, teardown,
CPU hotplug, rebind, module reload, and partial read stress tests. All
completed with the expected results and without a KASAN report, Oops, or
LOCKDEP warning.

In these x86-64 QEMU and dummy_hcd tests, module cleanup did not begin while
a callback still had work to publish, and destroy_workqueue() waited for the
remaining GadgetFS AIO work once module cleanup began. I did not find a new
failure attributable to the revised second patch in these tests.

Thank you for examining this issue with such care and for the time and
effort you have devoted to it.

With sincere appreciation and great respect,
Minseo Kim

2026년 9월 12일 (토) 오전 4:22, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> 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);
>
>