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

[Minseo Kim]

Hi Alan,

Thank you for the updated patch.

> If you trim your patch down to the parts that only target the AIO races,
> do you get anything significantly different from my version?

I compared your current patch with my AIO-only version and found no
significant difference in the AIO cancellation state machine or IRQ-state
handling.

Both versions snapshot epdata and ep alongside req before
usb_ep_dequeue(). In the directed cross-CPU test, a control based on your
current patch but without the local snapshots of epdata and ep reproduced
the earlier ep_unlink_worker() UAF; I did not reproduce it with your
current patch.

The only substantive implementation difference I found in the AIO-only
comparison was the criterion used alongside the req->actual check to
decide whether to queue copy work. My version used
iov_iter_count(&priv->to), whereas your current patch uses the explicit
is_read flag. I agree that is_read makes this decision more explicit and
avoids requiring the reader to infer it from how priv->to is initialized.

With your current patch, native PREAD and one- and two-segment PREADV
copied their payloads correctly. PWRITE, PWRITEV, and zero-length PREAD
and PWRITE completed without invoking ep_user_copy_worker().

I also reran the original null-ptr-deref and UAF reproducers, the exact
pre-queue cancellation matrix, and the reproducers for the earlier
candidate-patch regressions. I separately tested deferred giveback,
forced dequeue failure, and the earlier NULL-endpoint case. All of these
tests produced the expected results, and none of the corresponding
earlier failure signatures recurred.

One small point in the ep_aio_cancel() comment: as I understand it, the
completion function pointer in struct kiocb is named ki_complete, which
is also the name I used in my previous message. I wondered whether
iocb->kio_complete() was intended to be iocb->ki_complete().

Thank you again for updating the patch.

With great respect and appreciation,
Minseo Kim

2026년 9월 1일 (화) 오전 11:47, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> On Mon, Aug 31, 2026 at 10:30:08PM +0900, Minseo Kim wrote:
> > 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.
>
> Okay, I see.  I had assumed that the information about the userspace
> buffer was contained in the region that to_free points to.
>
> > 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.
>
> While that's a valid solution, it would be more straightforward to add
> an "is_read" flag to the priv structure.  If an aio operation isn't a
> read then there certainly won't be anything to copy back to userspace
> upon completion.  This doesn't require people reading the source to know
> anything about the internal details of struct iov_iter.  And it is a
> better match to the immediately preceding comment.
>
> > 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.
>
> Good, so we don't need to worry about that.
>
> > 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.
>
> We don't need to be that conservative here.  And in fact, I'm not
> entirely sure what that comment in usb_ep_disable() was intended to
> mean.  Since the function doesn't cause the UDC driver's endpoint data
> structure to be deallocated, there isn't much to be careful of.
>
> > > 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.
>
> So let's worry about the workqueue stuff later and concentrate for now
> just on fixing the AIO races.
>
> > > 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.
>
> And since your decision was based on handling the workqueue flushing, it
> can be put off until later.
>
> If you trim your patch down to the parts that only target the AIO races,
> do you get anything significantly different from my version?  I did see
> that you added or changed a few comments, and I have updated my version
> of the patch in a couple of respects, but overall yours must end up
> being pretty similar to mine.  Please point out any differences you
> believe are worth mentioning.  My current version is below.
>
> > 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.
>
> I'm sure they will be.
>
> 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
> @@ -236,6 +236,8 @@ static void put_ep (struct ep_data *data
>  static const char *CHIP;
>  static DEFINE_MUTEX(sb_mutex);         /* Serialize superblock operations */
>
> +static DEFINE_SPINLOCK(aio_lock);      /* Protect aio cancellation info */
> +
>  /*----------------------------------------------------------------------*/
>
>  /* NOTE:  don't use dev_printk calls before binding to the gadget
> @@ -433,40 +435,105 @@ static long ep_ioctl(struct file *fd, un
>
>  /* ASYNCHRONOUS ENDPOINT I/O OPERATIONS (bulk/intr/iso) */
>
> +enum aio_req_state {
> +       AIO_SUBMITTING,
> +       AIO_RUNNING,
> +       AIO_COMPLETED,
> +       AIO_GIVEN_BACK,
> +};
> +
> +enum aio_cancel_state {
> +       AIO_NOT_CANCELLED,
> +       AIO_UNLINKING,
> +       AIO_UNLINK_DONE,
> +};
> +
>  struct kiocb_priv {
>         struct usb_request      *req;
>         struct ep_data          *epdata;
> +       struct usb_ep           *ep;
>         struct kiocb            *iocb;
>         struct mm_struct        *mm;
> -       struct work_struct      work;
> +       struct work_struct      copy_work;
> +       struct work_struct      unlink_work;
>         void                    *buf;
>         struct iov_iter         to;
>         const void              *to_free;
>         unsigned                actual;
> +       enum aio_req_state      req_state;
> +       enum aio_cancel_state   cancel_state;
> +       bool                    is_read;
>  };
>
> -static int ep_aio_cancel(struct kiocb *iocb)
> +static void ep_unlink_worker(struct work_struct *work)
>  {
> -       struct kiocb_priv       *priv = iocb->private;
> +       struct kiocb_priv       *priv;
> +       struct usb_request      *req;
>         struct ep_data          *epdata;
> -       int                     value;
> +       struct usb_ep           *ep;
> +       enum aio_req_state      req_state;
>
> -       local_irq_disable();
> +       priv = container_of(work, struct kiocb_priv, unlink_work);
> +       req = priv->req;
>         epdata = priv->epdata;
> -       // spin_lock(&epdata->dev->lock);
> -       if (likely(epdata && epdata->ep && priv->req))
> -               value = usb_ep_dequeue (epdata->ep, priv->req);
> -       else
> -               value = -EINVAL;
> -       // spin_unlock(&epdata->dev->lock);
> -       local_irq_enable();
> +       ep = priv->ep;
>
> -       return value;
> +       usb_ep_dequeue(ep, req);
> +
> +       spin_lock_irq(&aio_lock);
> +       req_state = priv->req_state;
> +       priv->cancel_state = AIO_UNLINK_DONE;
> +       spin_unlock_irq(&aio_lock);
> +
> +       /*
> +        * req and epdata are freed after unlinking and completion are both done.
> +        * priv is freed after unlinking and giveback are both done.
> +        */
> +       if (req_state >= AIO_COMPLETED) {
> +               /* priv may have been freed already */
> +               usb_ep_free_request(ep, req);
> +               put_ep(epdata);
> +               if (req_state == AIO_GIVEN_BACK)
> +                       kfree(priv);
> +       }
> +}
> +
> +static int ep_aio_cancel(struct kiocb *iocb)
> +{
> +       struct kiocb_priv       *priv;
> +       unsigned long           flags;
> +
> +       spin_lock_irqsave(&aio_lock, flags);
> +       priv = iocb->private;
> +       if (!priv || priv->cancel_state != AIO_NOT_CANCELLED) {
> +               spin_unlock_irqrestore(&aio_lock, flags);
> +               return -EINVAL;         /* Already completed or cancelled */
> +       }
> +       if (priv->req_state == AIO_SUBMITTING) {
> +               priv->cancel_state = AIO_UNLINK_DONE;
> +               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.
> +        * usb_ep_dequeue() is allowed to run synchronously, calling the
> +        * completion handler ep_aio_complete() before it returns.
> +        * But ep_aio_complete() may call iocb->kio_complete(), which
> +        * tries to acquire the context lock, leading to deadlock.
> +        * For this reason, do the dequeue operation in a work routine.
> +        */
> +       INIT_WORK(&priv->unlink_work, ep_unlink_worker);
> +       schedule_work(&priv->unlink_work);
> +       return 0;
>  }
>
>  static void ep_user_copy_worker(struct work_struct *work)
>  {
> -       struct kiocb_priv *priv = container_of(work, struct kiocb_priv, work);
> +       struct kiocb_priv *priv = container_of(work, struct kiocb_priv, copy_work);
>         struct mm_struct *mm = priv->mm;
>         struct kiocb *iocb = priv->iocb;
>         size_t ret;
> @@ -482,46 +549,62 @@ static void ep_user_copy_worker(struct w
>
>         kfree(priv->buf);
>         kfree(priv->to_free);
> -       kfree(priv);
> +
> +       spin_lock_irq(&aio_lock);
> +       priv->req_state = AIO_GIVEN_BACK;
> +       if (priv->cancel_state != AIO_UNLINKING)
> +               kfree(priv);
> +       spin_unlock_irq(&aio_lock);
>  }
>
>  static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
>  {
>         struct kiocb            *iocb = req->context;
>         struct kiocb_priv       *priv = iocb->private;
> -       struct ep_data          *epdata = priv->epdata;
> +       enum aio_req_state      new_req_state;
> +       enum aio_cancel_state   cancel_state;
>
> -       /* lock against disconnect (and ideally, cancel) */
> -       spin_lock(&epdata->dev->lock);
> -       priv->req = NULL;
> -       priv->epdata = NULL;
> +       /* Prevent future cancellation */
> +       spin_lock(&aio_lock);
> +       iocb->private = NULL;
> +       spin_unlock(&aio_lock);
>
>         /* if this was a write or a read returning no data then we
>          * don't need to copy anything to userspace, so we can
>          * complete the aio request immediately.
>          */
> -       if (priv->to_free == NULL || unlikely(req->actual == 0)) {
> +       if (!priv->is_read || unlikely(req->actual == 0)) {
>                 kfree(req->buf);
>                 kfree(priv->to_free);
> -               kfree(priv);
> -               iocb->private = NULL;
>                 iocb->ki_complete(iocb,
>                                 req->actual ? req->actual : (long)req->status);
> +               new_req_state = AIO_GIVEN_BACK;
>         } else {
>                 /* ep_copy_to_user() won't report both; we hide some faults */
>                 if (unlikely(0 != req->status))
> -                       DBG(epdata->dev, "%s fault %d len %d\n",
> +                       DBG(priv->epdata->dev, "%s fault %d len %d\n",
>                                 ep->name, req->status, req->actual);
>
>                 priv->buf = req->buf;
>                 priv->actual = req->actual;
> -               INIT_WORK(&priv->work, ep_user_copy_worker);
> -               schedule_work(&priv->work);
> +               new_req_state = AIO_COMPLETED;
>         }
>
> -       usb_ep_free_request(ep, req);
> -       spin_unlock(&epdata->dev->lock);
> -       put_ep(epdata);
> +       spin_lock(&aio_lock);
> +       priv->req_state = new_req_state;
> +       cancel_state = priv->cancel_state;
> +       spin_unlock(&aio_lock);
> +
> +       if (new_req_state == AIO_COMPLETED) {
> +               INIT_WORK(&priv->copy_work, ep_user_copy_worker);
> +               schedule_work(&priv->copy_work);
> +       }
> +       if (cancel_state != AIO_UNLINKING) {
> +               usb_ep_free_request(ep, req);
> +               put_ep(priv->epdata);
> +               if (new_req_state == AIO_GIVEN_BACK)
> +                       kfree(priv);
> +       }
>  }
>
>  static ssize_t ep_aio(struct kiocb *iocb,
> @@ -532,11 +615,12 @@ static ssize_t ep_aio(struct kiocb *iocb
>  {
>         struct usb_request *req;
>         ssize_t value;
> +       struct usb_ep *ep;
> +       bool need_unlink = false;
>
>         iocb->private = priv;
>         priv->iocb = iocb;
>
> -       kiocb_set_cancel_fn(iocb, ep_aio_cancel);
>         get_ep(epdata);
>         priv->epdata = epdata;
>         priv->actual = 0;
> @@ -547,10 +631,12 @@ static ssize_t ep_aio(struct kiocb *iocb
>          */
>         spin_lock_irq(&epdata->dev->lock);
>         value = -ENODEV;
> -       if (unlikely(epdata->ep == NULL))
> +       ep = epdata->ep;
> +       if (unlikely(ep == NULL))
>                 goto fail;
> +       priv->ep = ep;
>
> -       req = usb_ep_alloc_request(epdata->ep, GFP_ATOMIC);
> +       req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>         value = -ENOMEM;
>         if (unlikely(!req))
>                 goto fail;
> @@ -560,12 +646,45 @@ static ssize_t ep_aio(struct kiocb *iocb
>         req->length = len;
>         req->complete = ep_aio_complete;
>         req->context = iocb;
> -       value = usb_ep_queue(epdata->ep, req, GFP_ATOMIC);
> +
> +       priv->req_state = AIO_SUBMITTING;
> +       priv->cancel_state = AIO_NOT_CANCELLED;
> +
> +       /* Not allowed to manipulate the aio context while holding dev->lock */
> +       ++epdata->dev->udc_usage;
> +       spin_unlock_irq(&epdata->dev->lock);
> +
> +       kiocb_set_cancel_fn(iocb, ep_aio_cancel);
> +       value = usb_ep_queue(ep, req, GFP_KERNEL);
> +
> +       spin_lock_irq(&epdata->dev->lock);
> +       --epdata->dev->udc_usage;
> +
>         if (unlikely(0 != value)) {
> -               usb_ep_free_request(epdata->ep, req);
> +               spin_lock(&aio_lock);
> +               iocb->private = NULL;
> +               spin_unlock(&aio_lock);
> +
> +               usb_ep_free_request(ep, req);
>                 goto fail;
>         }
>         spin_unlock_irq(&epdata->dev->lock);
> +
> +       spin_lock_irq(&aio_lock);
> +       if (iocb->private != NULL) {
> +               priv->req_state = AIO_RUNNING;
> +
> +               /* Cancelled before or just after submission? */
> +               if (priv->cancel_state == AIO_UNLINK_DONE) {
> +                       priv->cancel_state = AIO_NOT_CANCELLED;
> +                       need_unlink = true;
> +               }
> +       }                       /* Otherwise already completed */
> +       spin_unlock_irq(&aio_lock);
> +
> +       if (need_unlink)        /* Redo cancel that was too early */
> +               ep_aio_cancel(iocb);
> +
>         return -EIOCBQUEUED;
>
>  fail:
> @@ -618,6 +737,7 @@ ep_read_iter(struct kiocb *iocb, struct
>                 value = -ENOMEM;
>                 if (!priv)
>                         goto fail;
> +               priv->is_read = true;
>                 priv->to_free = dup_iter(&priv->to, to, GFP_KERNEL);
>                 if (!iter_is_ubuf(&priv->to) && !priv->to_free) {
>                         kfree(priv);