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

[Minseo Kim]

Hi Alan,

Thank you very much for taking the time to prepare this patch. I applied
the patch as posted in your message to upstream v7.2-rc1, commit
dc59e4fea9d83f03bad6bddf3fa2e52491777482.

I repeatedly tested the proposed patch with the original null-ptr-deref
and UAF reproducers, as well as several reduced variants. During those
tests, I did not observe either of the original signatures.

While continuing the tests, I observed a new null-ptr-deref in
ep_aio_cancel() on the kernel with the proposed patch applied. It appears
to be associated with the initialization window in ep_aio(). Running the
reproducer without command-line arguments triggered:

  KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
  RIP: ep_aio_cancel+0xd0/0x2d0
  drivers/usb/gadget/legacy/inode.c:473

The corresponding source statement is:

  ep = epdata->ep;

where epdata is NULL. The faulting task has UID 1000.

One detail that may be relevant is the initialization order in ep_aio().
iocb->private is assigned and kiocb_set_cancel_fn() registers the
cancellation callback before priv->epdata is initialized. Because priv is
zero-initialized and AIO_REQ_RUNNING is the zero-valued enumerator in the
proposed patch, the state check can treat priv as RUNNING before the
explicit state assignment. The observed crash is consistent with
cancellation reaching this initialization window, because priv->epdata
was NULL at the fault.

I also ran the reproducer on a LOCKDEP-enabled build. LOCKDEP reported the
following possible circular locking dependency involving the newly added
aio_lock:

  &ctx->ctx_lock -> aio_lock -> &dev->lock#2 -> &ctx->ctx_lock

The cycle appears to arise because io_cancel() calls ep_aio_cancel() while
holding ctx->ctx_lock, and ep_aio_cancel() then acquires aio_lock. In the
other direction, the patched completion path holds aio_lock and dev->lock
while calling iocb->ki_complete(). In this path, the completion callback
is aio_complete_rw(), which can acquire ctx->ctx_lock.

The reported cycle involving aio_lock appears specific to the proposed
patch, since aio_lock is newly introduced there.

Supporting files:

  C reproducer for the new null-ptr-deref:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_validation_20260713/repro_candidate_setup_ndr.c

  Build:
    gcc -O2 -Wall -Wextra -pthread -o repro_candidate_setup_ndr
repro_candidate_setup_ndr.c

  Clean symbolized report for the new null-ptr-deref:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_validation_20260713/clean_report_candidate_setup_ndr.txt

  Kernel config used for the KASAN reproducer:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_validation_20260713/kernel.config.kasan_inline_dwarf5

  LOCKDEP report for the aio_lock cycle:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_patch_validation_20260713/lockdep_report_aio_lock_cycle.txt

I wanted to share these observations in case they are useful. Please let
me know if I have misunderstood any part of the initialization or locking
sequence.

Best regards,
Minseo Kim

2026년 7월 8일 (수) 오전 2:31, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>님이 작성:
>
> On Mon, Jul 06, 2026 at 10:18:11AM +0900, 김민서 wrote:
> > I can help validate candidate fixes under KASAN against the reproducer and
> > the intermittent UAF variant. I can also build a lockdep-enabled kernel and
> > run the reproducer against candidate fixes if that would be useful.
> >
> > The clearest conclusion I can draw is still the lifetime invariant:
> > ep_aio_cancel() should not be able to use iocb->private as a
> > struct kiocb_priv pointer after the completion path has cleared
> > iocb->private or freed the underlying struct kiocb_priv. I would be happy
> > to test any candidate fix or approach you think would be worth trying.
>
> Here is a possible fix for you to test.  It's a little complicated,
> which is to be expected since this is a somewhat complicated problem.
>
> I believe it will solve the problems you are seeing but I have not tried
> using it myself.
>
> Alan Stern
>
>
> ---
>  drivers/usb/gadget/legacy/inode.c |   80 +++++++++++++++++++++++++++++---------
>  1 file changed, 63 insertions(+), 17 deletions(-)
>
> 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,6 +435,13 @@ static long ep_ioctl(struct file *fd, un
>
>  /* ASYNCHRONOUS ENDPOINT I/O OPERATIONS (bulk/intr/iso) */
>
> +enum aio_req_state {
> +       AIO_REQ_RUNNING,
> +       AIO_REQ_CANCELLING,
> +       AIO_REQ_CANCELLED,
> +       AIO_REQ_COMPLETED,
> +};
> +
>  struct kiocb_priv {
>         struct usb_request      *req;
>         struct ep_data          *epdata;
> @@ -443,24 +452,46 @@ struct kiocb_priv {
>         struct iov_iter         to;
>         const void              *to_free;
>         unsigned                actual;
> +       enum aio_req_state      aio_state;
>  };
>
>  static int ep_aio_cancel(struct kiocb *iocb)
>  {
> -       struct kiocb_priv       *priv = iocb->private;
> +       struct kiocb_priv       *priv;
>         struct ep_data          *epdata;
> -       int                     value;
> +       struct usb_ep           *ep;
> +       struct dev_data         *dev;
> +       int                     value = -EINVAL;
> +
> +       spin_lock_irq(&aio_lock);
> +       priv = iocb->private;
> +       if (!priv || priv->aio_state != AIO_REQ_RUNNING)
> +               goto Done;      /* Already completed or cancelled */
>
> -       local_irq_disable();
> +       priv->aio_state = AIO_REQ_CANCELLING;
>         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 = epdata->ep;
> +       dev = epdata->dev;
> +       spin_lock(&dev->lock);
> +       ++dev->udc_usage;
> +       spin_unlock(&dev->lock);
> +       spin_unlock_irq(&aio_lock);
> +
> +       value = usb_ep_dequeue(ep, priv->req);
> +
> +       spin_lock_irq(&aio_lock);
> +       if (priv->aio_state == AIO_REQ_CANCELLING) {
> +               priv->aio_state = AIO_REQ_CANCELLED;
> +       } else {                /* Must be AIO_REQ_COMPLETED */
> +               usb_ep_free_request(ep, priv->req);
> +               kfree(priv);
> +       }
> +       spin_lock(&dev->lock);
> +       --dev->udc_usage;
> +       spin_unlock(&dev->lock);
>
> + Done:
> +       spin_unlock_irq(&aio_lock);
>         return value;
>  }
>
> @@ -482,7 +513,13 @@ static void ep_user_copy_worker(struct w
>
>         kfree(priv->buf);
>         kfree(priv->to_free);
> -       kfree(priv);
> +
> +       spin_lock_irq(&aio_lock);
> +       if (priv->aio_state == AIO_REQ_CANCELLING)
> +               priv->aio_state = AIO_REQ_COMPLETED;
> +       else
> +               kfree(priv);
> +       spin_unlock_irq(&aio_lock);
>  }
>
>  static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
> @@ -490,11 +527,15 @@ 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;
> +       bool                    cancelling;
> +
> +       /* lock against cancellation */
> +       spin_lock(&aio_lock);
> +       cancelling = (priv->aio_state == AIO_REQ_CANCELLING);
>
> -       /* lock against disconnect (and ideally, cancel) */
> +       /* lock against unbind */
>         spin_lock(&epdata->dev->lock);
> -       priv->req = NULL;
> -       priv->epdata = NULL;
> +       iocb->private = NULL;           /* Prevent future cancellation */
>
>         /* 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,8 +544,10 @@ 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;
> +               if (cancelling)
> +                       priv->aio_state = AIO_REQ_COMPLETED;
> +               else
> +                       kfree(priv);
>                 iocb->ki_complete(iocb,
>                                 req->actual ? req->actual : (long)req->status);
>         } else {
> @@ -519,8 +562,10 @@ static void ep_aio_complete(struct usb_e
>                 schedule_work(&priv->work);
>         }
>
> -       usb_ep_free_request(ep, req);
> +       if (!cancelling)
> +               usb_ep_free_request(ep, req);
>         spin_unlock(&epdata->dev->lock);
> +       spin_unlock(&aio_lock);
>         put_ep(epdata);
>  }
>
> @@ -541,6 +586,7 @@ static ssize_t ep_aio(struct kiocb *iocb
>         priv->epdata = epdata;
>         priv->actual = 0;
>         priv->mm = current->mm; /* mm teardown waits for iocbs in exit_aio() */
> +       priv->aio_state = AIO_REQ_RUNNING;
>
>         /* each kiocb is coupled to one usb_request, but we can't
>          * allocate or submit those if the host disconnected.