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

[Minseo Kim]

Hi Alan,

Thank you for the comments and for pointing out where my explanation was
unclear.

> Are you saying that this test in ep_read_iter():
>
>         if (!iter_is_ubuf(&priv->to) && !priv->to_free) {
>
> is wrong, for example, the && should be || ?

No. The && condition should remain as it is. dup_iter() copies the iterator
state into priv->to. In my tests, native PREAD and a one-segment PREADV used
ITER_UBUF. Since ITER_UBUF has no separate iovec array to duplicate,
dup_iter() returned NULL while the copied iterator remained valid. My
two-segment PREADV test used ITER_IOVEC; there, a NULL return would mean that
allocation of the duplicated iovec array failed. The iter_is_ubuf() check
therefore distinguishes the valid NULL return for ITER_UBUF from an allocation
failure in the ITER_IOVEC case. In a control build using ||, valid PREAD and
both one- and two-segment PREADV submissions failed with -ENOMEM.

The issue I observed was instead in the later completion test:

        if (priv->to_free == NULL || unlikely(req->actual == 0)) {

As I understand it, this later test treats a NULL to_free pointer as meaning
that no userspace copy is needed. In this path, however, to_free stores the
pointer returned by dup_iter() and tracks separately allocated iterator
backing, rather than whether the saved iterator has a destination.

For this issue, the relevant change was to base the later copy decision on
iov_iter_count(&priv->to); I did not change the ep_read_iter() condition. With
the original test, native PREAD reported res=511 while the destination buffer
remained unchanged. After this change, native PREAD and one-segment PREADV
copied the payload correctly, and two-segment PREADV continued to work.

My previous description of to_free as a predicate was inaccurate. I was
referring to its later completion-side use as a copy/no-copy indicator.

> I understood that it was okay to call usb_ep_free_request() after
> usb_ep_disable(). Was that wrong? Did you see it create any problems?

Your understanding matches the behavior I observed under dummy_hcd. For both
PWRITE and PREAD, I kept a completed, unqueued request allocated until after
usb_ep_disable() returned, with no intervening usb_ep_dequeue(). In each case,
the request completed exactly once with the expected result; the subsequent
usb_ep_free_request() produced no KASAN report or Oops.

Separately, in an intermediate version I observed usb_ep_free_request()
running while usb_ep_disable() was still in progress. Because that version
moved normal request cleanup into a worker, I used the existing endpoint mutex
to prevent the worker's usb_ep_free_request() call from overlapping endpoint
disable while I investigated the ordering. The mutex eliminated the overlap,
but I did not reproduce a failure without it, so these tests did not establish
that the serialization was required.

The endpoint-use ordering I could reproduce in a directed dummy_hcd diagnostic
was different: without waiting for unlink work before endpoint disable,
ep_unlink_worker() could call usb_ep_dequeue() while usb_ep_disable() was in
progress. In matched PWRITE and PREAD tests, the variant without the
pre-disable unlink-work wait produced this overlap; with the wait retained,
usb_ep_dequeue() completed before usb_ep_disable() began.

I did not reproduce a KASAN report, Oops, or userspace failure from the
concurrent dequeue/disable overlap itself. I retained the pre-disable wait in
the narrower test version as a conservative interpretation of the
usb_ep_disable() requirement that no other task be using the endpoint when it
is called.

> This is probably because you were flushing the workqueues at the wrong
> time.

Yes. In the callback-gate test, the relevant workqueue flush returned
while the completion callback was stopped before queueing its follow-up work.
In the narrower test version, I incremented aio_producers before
usb_ep_queue() and decremented it only after the callback had either queued
the required follow-up work or completed the immediate AIO result without
leaving deferred work to publish. If usb_ep_queue() failed, the submission
path decremented it directly. With that change, gadgetfs_unbind() remained
blocked in the producer wait while the callback was gated. It proceeded to
flush the completion workqueue only after the callback queued the follow-up
work.

In the delayed-worker test, module unload completed before the running
unlink_work returned. This confirmed that module unload must not complete
while such work is still running.

> For the final submission, I think the workqueue management stuff should
> go into its own separate patch.

Separating the workqueue management changes from the AIO race fixes makes
sense. Thank you also for the reminder about -p.

> Second, why did you change ep_aio_complete() to make it queue up
> ep_user_copy_worker() even when nothing needed to be copied to
> userspace?

My reason for routing every completion through ep_user_copy_worker() was to
make a common deferred completion and request-cleanup stage visible to a
workqueue flush during teardown, not because every request needed a userspace
copy. In the normal non-unlink path of that cumulative design, the worker
released the USB request and epdata reference before calling ki_complete(),
and then published AIO_GIVEN_BACK. Since ki_complete() could drop the last AIO
file reference and allow ep_release() to begin, the intent was to prevent
teardown from passing the completion-work flush before that cleanup had
finished. A gate placed between cleanup and ki_complete() confirmed that the
flush covered this no-copy PWRITE cleanup: unbind reached
flush_workqueue(gadgetfs_copy_wq) but did not return until the worker was
released.

I agree that using the worker for every completion was broader than necessary
for copy handling. The narrower behavior I tested retains aio_producers but
queues copy work only when iov_iter_count(&priv->to) and req->actual are both
nonzero. Nonzero PREAD and the one- and two-segment PREADV cases invoked the
copy worker, while PWRITE, PWRITEV, and zero-length reads and writes did not.
This indicates that no-copy paths do not need ep_user_copy_worker() merely for
copy handling. Their cleanup ordering and the separate teardown lifetime
issue can be considered independently of that routing decision.

Looking back, trying to address all of the observed issues in one cumulative
patch made it harder to separate and review the purpose of each change.
Considering the issues independently may also make it easier to identify a
simpler approach for some of them. I hope these clarifications and test
results are useful.

Thank you again for your comments.

Best regards,
Minseo Kim

2026년 8월 30일 (일) 오전 1:08, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> On Fri, Aug 28, 2026 at 03:30:10PM -0500, neck3922@xxxxxxxxx wrote:
> > Hi Alan,
> >
> > Thank you for the revised patch and for explaining the intended use of
> > the saved endpoint.
> >
> > I applied the patch as posted to upstream v7.2-rc1, commit
> > dc59e4fea9d83f03bad6bddf3fa2e52491777482.
> >
> > I first tested your revision unchanged. The IRQ-state fix and saved
> > endpoint behaved as intended. The exact pre-queue cancellation matrix
> > produced exactly one expected completion in every case, with no KASAN,
> > Oops, or LOCKDEP output. The original ep_aio_cancel() null-ptr-deref
> > and UAF signatures, the earlier ep_aio() submit-path regressions, and the
> > teardown NULL-endpoint failure did not recur in these tests.
>
> All good results.
>
> > > I think it will still be necessary to flush the workqueue after
> > > destroy_ep_files() runs.  The best way to check whether this is needed
> > > would be to put a long delay right at the start of ep_unlink_worker()
> > > and then run a test where during that delay, the test program closes its
> > > open files, unmounts the directory, and unloads the gadgetfs module.
> >
> > Using CONFIG_USB_GADGETFS=m on the posted revision, I inserted a
> > diagnostic 15-second delay at the entry of ep_unlink_worker(). While the
> > worker was delayed, the test process had no open /dev/gadget/* file
> > descriptors, GadgetFS unmounted successfully, the gadgetfs module
> > reference count reached zero, and rmmod succeeded before the delay ended.
> > The delay was diagnostic only.
> >
> > When the delayed worker resumed, the kernel warned and then panicked. The
> > serial log showed an unresolved work-function address and identified
> > gadgetfs as the last unloaded module:
> >
> >   Modules linked in: dummy_hcd [last unloaded: gadgetfs(O)]
> >
> > Thus, in this path, a running unlink_work item does not pin the gadgetfs
> > module. Teardown must quiesce pending or running GadgetFS AIO work before
> > module unload, or otherwise retain the module until that work completes.
>
> Indeed.  Quiescing won't be easy to do; it will just have to wait until
> the work is complete.  In other words, flush the workqueue.
>
> > While rerunning the directed cross-CPU test from my previous message, I
> > found that the underlying lifetime race involving ep_unlink_worker()
> > remained:
> >
> >   BUG: KASAN: slab-use-after-free in ep_unlink_worker
> >   Read of size 8
> >   drivers/usb/gadget/legacy/inode.c:489
> >
> > The faulting statement was:
> >
> >   put_ep(priv->epdata);
> >
> > After AIO_UNLINK_DONE was published, completion work could free priv
> > before the unlink worker's final accesses through priv. Saving epdata and
> > ep before usb_ep_dequeue() eliminated the failure in the directed test.
>
> Ah, yes.  I should have realized that last time.  I just didn't think
> hard enough about how the ordering could interact with those tests at
> the end of ep_unlink_worker().  (Or maybe I did and then forgot to
> include the changes into the patch -- I can't remember.)  Anyway, my
> version of the patch has been updated accordingly.
>
> > Based on our discussion and the results above, I prepared the cumulative
> > patch below, incorporating your latest revision. It also fixes an
> > existing native PREAD issue: the to_free predicate could skip copying an
> > ITER_UBUF payload while still reporting success.
>
> I'm not sure what you're referring to.  Are you saying that this test
> in ep_read_iter():
>
>                 if (!iter_is_ubuf(&priv->to) && !priv->to_free) {
>
> is wrong, for example, the && should be || ?  In fact, I don't
> understand the reason for the iter_is_ubuf() check at all.
>
> Note that to_free isn't a predicate; rather it's a pointer to a copy of
> an iov_iter structure.
>
> > During development and validation, I found two further ordering issues.
> > In the existing completion ordering, request cleanup could overlap
> > usb_ep_disable() during final endpoint release.
>
> Yes, I know.  I understood that it was okay to call
> usb_ep_free_request() after usb_ep_disable().  Was that wrong?  Did you
> see it create any problems?
>
> Note that usb_ep_disable() doesn't return until all the requests queued
> for that endpoint have completed.  So ep_aio_complete() will have run,
> but the work routines may still be pending.
>
> >  Separately, an
> > intermediate cumulative version allowed gadgetfs_unbind() to pass the
> > completion-work flush before a callback had queued that work.
>
> This is probably because you were flushing the workqueues at the wrong
> time.
>
> For the final submission, I think the workqueue management stuff should
> go into its own separate patch.  Straightening out the various AIO races
> is already complicated enough by itself.
>
> >  The
> > resulting patch addresses all three issues and applies directly to
> > upstream v7.2-rc1.
>
> I'll review the patch later.  For now, there's two things to mention.
> First, when you create your patches, it would help to add the -p
> option to the diff command.
>
> Second, why did you change ep_aio_complete() to make it queue up
> ep_user_copy_worker() even when nothing needed to be copied to
> userspace?  It's a bad idea to run a workqueue routine if it isn't
> necessary.
>
> Alan Stern