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

On Wed, Aug 05, 2026 at 08:12:51AM +0900, Minseo Kim wrote:
> Hi Alan,
> 
> Thank you for the updated patch. I applied it as posted to upstream
> v7.2-rc1, commit dc59e4fea9d83f03bad6bddf3fa2e52491777482.
> 
> In my runs, the post-queue ep_aio() UAF reported against the previous
> revision did not recur with this update, including on a diagnostic build
> that deliberately widened the same window. I also did not observe the
> original ep_aio_cancel() null-ptr-deref or UAF signatures in the timings
> I tested.

But as you have seen, there are still other problems in that driver.

However, I'm not sure it's worth working on them.  Greg KH is talking 
about getting rid of almost all the drivers in the gadget/legacy 
directory.  I don't know if that will include the gadgetfs driver.  If 
it does, trying to fix up the driver will be a waste of time.

> > The possibility I was worried about was that gadgetfs_unbind() might
> > complete while ep_aio_cancel() was still running.
> 
> I was able to reach this ordering in a diagnostic build. I paused
> ep_aio_cancel() after usb_ep_dequeue() returned and before the
> cancel-side usb_ep_free_request() and put_ep() calls. The
> pointer-correlated trace showed gadgetfs_unbind(),
> usb_gadget_unregister_driver(), and dev_release() returning before those
> cleanup calls and before ep_aio_cancel() returned.
> 
> No KASAN or Oops occurred under dummy_hcd. The trace establishes that,
> in this instrumented run, the unbind and unregister paths returned before
> the cancel-side cleanup finished.

Yeah, that's not good.  The unbind path, and in particular, 
destroy_ep_files(), should wait until an endpoint being destroyed is no 
longer in use.

> > Even worse, the aio file descriptor might get closed and the entire
> > module unloaded from memory while ep_aio_cancel() is running on another
> > CPU.
> 
> I also tested this case with CONFIG_USB_GADGETFS=m. The only difference
> between the built-in and modular test configurations was
> CONFIG_USB_GADGETFS=y versus CONFIG_USB_GADGETFS=m. In a diagnostic run
> with a longer pause, gadgetfs_unbind() returned while ep_aio_cancel() was
> paused. At that point, the trace harness found no file descriptors
> referring to /dev/gadget/* in the process's file descriptor table, and I
> then started the unmount. While the unmount was still blocked and
> ep_aio_cancel() had not returned, the reported reference count for the
> gadgetfs module was 2 and an rmmod attempt failed with:
> 
>   rmmod: ERROR: Module gadgetfs is in use

That's a relief.

> dev_release() and the unmount completed after ep_aio_cancel() returned,
> and rmmod then succeeded. I therefore did not reproduce the module being
> unloaded while ep_aio_cancel() was running in this path.
> 
> Several existing references are relevant here: the AIO request holds a
> file reference in ki_filp until iocb_destroy(), the endpoint file
> operations and GadgetFS filesystem type both specify
> .owner = THIS_MODULE, and ep_aio() takes an ep_data reference through
> get_ep(). The reference taken by get_ep() keeps ep_data alive at least
> until its matching put_ep(); the final put_ep() also drops the dev_data
> reference held by ep_data.

The ep_data and dev_data references don't help pin the module, but the 
ki_filp reference does.

On the other hand, if ep_cancel() gets split into two threads (which 
will be necessary to eliminate the locking cycle mentioned below), its 
second half could end up running after iocb_destroy() is finished.  This 
is yet another reason for making destroy_ep_files() wait until the 
endpoint is no longer in use.

> These references do not by themselves establish whether the UDC-provided
> usb_ep object and the request allocated from it remain valid long enough
> for the later cancel-side usb_ep_free_request() call to be safe after
> gadgetfs_unbind() and usb_gadget_unregister_driver() have returned. The
> outstanding-AIO reference count you suggested, or an equivalent teardown
> barrier, may therefore still be relevant to this narrower lifetime
> question, unless the USB core already guarantees those object lifetimes
> through this ordering. Any such wait would need to avoid blocking a
> disable or giveback operation needed for an outstanding AIO to finish.

AIO doesn't wait for disable operations, only giveback.  I don't think 
this will cause any new problems.

> Separately, in the matched LOCKDEP run I did not observe a cycle
> involving aio_lock. The patched kernel reported:
> 
>   &ctx->ctx_lock -> &dev->lock#2 -> &ctx->ctx_lock
> 
> In that report, one recorded dev->lock-to-ctx->ctx_lock dependency comes
> from kiocb_set_cancel_fn() being called while dev->lock is held. The
> reverse direction is exercised when free_ioctx_users() holds
> ctx->ctx_lock and a synchronous dummy_hcd giveback enters
> ep_aio_complete(), which acquires dev->lock. The cancel-function
> registration placement was already present in the preceding patch
> revision and was unchanged by the one-line update.

Fixing this will require moving ep_aio()'s calls to 
kiocb_set_cancel_fn() and usb_ep_queue() outside the scope of dev->lock.  
There are several other places in the driver where the code does 
something similar, so this should be straightforward.

> The matched unpatched control reported a direct recursive attempt to
> acquire ctx->ctx_lock in the same synchronous-giveback call chain. I
> therefore regard the synchronous ctx_lock re-entry itself as pre-existing
> rather than as a regression introduced by the one-line update.

To fix this, I will have to put the second half of ep_cancel() 
(everything from the usb_ep_dequeue() call to the end) into a workqueue 
routine, rather like ep_user_copy_worker().  And to avoid other locking 
cycles, the rule should be to allow dev->lock to be acquired while 
ctx->ctx_lock is held, but not the reverse.

> These reports confirm that, under dummy_hcd, the dequeue giveback can
> re-enter the AIO completion path synchronously. They do not by themselves
> establish that GadgetFS AIO completion must be deferred to another
> thread, but they identify the synchronous callback chain that such
> deferral would be intended to avoid. I have included both reports because
> this ordering may also be relevant to the design of a teardown barrier.

Yes, thank you, it did help.

Alan Stern