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

[Minseo Kim]

Hi Alan,

Thank you for letting me know that the earlier results were helpful.

I applied the patch as posted to upstream v7.2-rc1, commit
dc59e4fea9d83f03bad6bddf3fa2e52491777482.

I reran the original null-ptr-deref and UAF reproducers, along with the
reproducers for the earlier candidate-patch regressions. I did not observe
the corresponding KASAN signatures with this patch.

A separate cross-CPU concurrency diagnostic appears to expose a remaining
lifetime race in the deferred-read cancellation path. As I understand it,
copy_work and unlink_work are distinct work items queued through
schedule_work() and may execute concurrently on different CPUs. I did not
see an ordering mechanism in the posted patch that would serialize the two
work items. I therefore treated their concurrent execution as a possible
interleaving and modified only dummy_hcd to exercise it directly.

On dummy_hcd's successful dequeue path, the calling path released
dummy_hcd's lock and restored its IRQ state before invoking work_on_cpu().
The diagnostic ran the giveback on another online CPU and waited for it to
complete before usb_ep_dequeue() returned. GadgetFS remained exactly as in
the posted patch. The target-CPU helper disabled local IRQs around
usb_gadget_giveback_request() and restored the previous IRQ state after
the function returned. The VM used a fixed four-CPU configuration and did
not perform CPU hotplug.

With this diagnostic, the C workload repeatedly triggered:

  BUG: KASAN: slab-use-after-free in ep_unlink_worker+0x1d1/0x1f0
  Read of size 8
  drivers/usb/gadget/legacy/inode.c:488

The faulting source statement is:

  usb_ep_free_request(epdata->ep, priv->req);

I did not reproduce this UAF with unmodified dummy_hcd under the same
kernel configuration, reproducer, and arguments. Those runs completed
without KASAN or an Oops and produced one AIO completion event with
res=512 for each request.

The observed UAF is consistent with the following ordering.
ep_unlink_worker() calls usb_ep_dequeue(), and the giveback enters
ep_aio_complete(), sets req_state to AIO_COMPLETED, and queues
ep_user_copy_worker(). After usb_ep_dequeue() returns, the unlink worker
observes AIO_COMPLETED, sets cancel_state to AIO_UNLINK_DONE, and releases
aio_lock. The copy worker can then set req_state to AIO_GIVEN_BACK,
observe that cancel_state is no longer AIO_UNLINKING, and free priv before
the unlink worker reaches the access above.

aio_lock serializes these state changes, but it does not keep priv alive
after the unlink worker releases the lock. This suggests that the final
free must be deferred until every queued or running work item that may
access priv has finished accessing it. For example, a lifetime reference
could be taken for unlink_work before it is queued and released after
ep_unlink_worker() has finished its final access, or an equivalent
last-user mechanism could release priv only after both work items have
finished accessing it.

The reproducer does not close the GadgetFS files or trigger unbind during
the request loop, and it consumes all AIO completion events before
teardown. A workqueue flush in gadgetfs_unbind() would therefore address
teardown ordering, but it would not serialize unlink_work and copy_work
during this race.

If I have misunderstood whether the USB gadget API permits a UDC to
deliver a dequeue giveback on another CPU before usb_ep_dequeue() returns,
please let me know.

Supporting files:

  C reproducer:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_20260814_followup_20260817/repro_candidate_unlink_copy_uaf.c

  Build:
    gcc -O2 -Wall -Wextra -pthread -o repro_candidate_unlink_copy_uaf \
      repro_candidate_unlink_copy_uaf.c

  Run:
    ./repro_candidate_unlink_copy_uaf 1000 32

  Diagnostic-only dummy_hcd patch:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_20260814_followup_20260817/dummy_hcd_crosscpu_giveback_diagnostic.patch

  Symbolized KASAN report:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_20260814_followup_20260817/symbolized_report_ep_unlink_worker_uaf.txt

  Kernel config used for the unmodified and diagnostic runs:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_20260814_followup_20260817/kernel.config.kasan_inline_dwarf5_lockdep

I hope this helps.

Best regards,
Minseo Kim

2026년 8월 15일 (토) 오전 1:17, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> On Wed, Aug 05, 2026 at 12:17:22PM -0400, Alan Stern wrote:
> > 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.
>
> Not having heard anything from Greg, I'll assume that the driver is not
> in any imminent danger.
>
> Accordingly, below is a new patch implementing the updates mentioned
> last time.  In particular, the ep_aio_cancel() carries out its dequeue
> operation in a separate thread, and ep_aio() drops dev->lock before
> calling kiocb_set_cancel_fn() and usb_ep_queue().
>
> It will probably be necessary to flush the workqueue in
> gadgetfs_unbind() after calling destroy_ep_files(), to make sure that
> any work routines scheduled for aio requests are no longer running.  We
> can worry about that later.
>
> 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,98 @@ 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 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;
>  };
>
> -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 ep_data          *epdata;
> -       int                     value;
> +       enum aio_req_state      req_state;
>
> -       local_irq_disable();
> +       priv = container_of(work, struct kiocb_priv, unlink_work);
>         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();
>
> -       return value;
> +       usb_ep_dequeue(epdata->ep, priv->req);
> +
> +       spin_lock_irq(&aio_lock);
> +       req_state = priv->req_state;
> +       priv->cancel_state = AIO_UNLINK_DONE;
> +       spin_unlock_irq(&aio_lock);
> +
> +       /*
> +        * priv->req and epdata are freed after unlinking and completion are
> +        * both done.
> +        * priv itself is freed after unlinking and giveback are both done.
> +        */
> +       if (req_state >= AIO_COMPLETED) {
> +               usb_ep_free_request(epdata->ep, priv->req);
> +               put_ep(epdata);
> +               if (req_state == AIO_GIVEN_BACK)
> +                       kfree(priv);
> +       }
> +}
> +
> +static int ep_aio_cancel(struct kiocb *iocb)
> +{
> +       struct kiocb_priv       *priv;
> +
> +       spin_lock_irq(&aio_lock);
> +       priv = iocb->private;
> +       if (!priv || priv->cancel_state != AIO_NOT_CANCELLED) {
> +               spin_unlock_irq(&aio_lock);
> +               return -EINVAL;         /* Already completed or cancelled */
> +       }
> +       if (priv->req_state == AIO_SUBMITTING) {
> +               priv->cancel_state = AIO_UNLINK_DONE;
> +               spin_unlock_irq(&aio_lock);
> +               return 0;       /* ep_aio() will call us again if needed */
> +       }
> +
> +       priv->cancel_state = AIO_UNLINKING;
> +       spin_unlock_irq(&aio_lock);
> +
> +       /*
> +        * usb_ep_dequeue() is allowed to run synchronously, calling the
> +        * request's completion handler before it returns.
> +        * We are called with the aio core holding the aio's context lock,
> +        * and ep_aio_complete() below may call iocb->kio_complete(), which
> +        * tries to acquire the context lock, which would cause 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,7 +542,12 @@ 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)
> @@ -490,11 +555,13 @@ static void ep_aio_complete(struct usb_e
>         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
> @@ -503,10 +570,9 @@ static void ep_aio_complete(struct usb_e
>         if (priv->to_free == NULL || 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))
> @@ -515,13 +581,24 @@ static void ep_aio_complete(struct usb_e
>
>                 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(epdata);
> +               if (new_req_state == AIO_GIVEN_BACK)
> +                       kfree(priv);
> +       }
>  }
>
>  static ssize_t ep_aio(struct kiocb *iocb,
> @@ -532,11 +609,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 +625,11 @@ 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;
>
> -       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 +639,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:
>