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

On Sun, Aug 02, 2026 at 03:08:11PM +0900, Minseo Kim wrote:
> Hi Alan,
> 
> Thank you for the new patch. I applied it as posted to upstream v7.2-rc1,
> commit dc59e4fea9d83f03bad6bddf3fa2e52491777482.
> 
> Across the timing ranges I tested, I did not reproduce the originally
> reported ep_aio_cancel() stale-priv failure on the kernel with the new
> patch applied. In a targeted diagnostic comparison, the original
> ep_aio_cancel() UAF was reproduced on the unpatched kernel. Using the
> same kernel configuration, reproducer, and arguments, the diagnostic
> runs on the patched kernel produced no KASAN or Oops. I also did not
> observe the original null-ptr-deref or UAF signatures in non-diagnostic
> runs of the patched kernel.
> 
> However, I observed a different slab-use-after-free in ep_aio() on the
> patched kernel:
> 
>   BUG: KASAN: slab-use-after-free in ep_aio+0x573/0x660
>   Read of size 4
>   drivers/usb/gadget/legacy/inode.c:647
> 
> This UAF was repeatedly reproduced on the patched kernel. It was not
> observed in control runs on the unpatched kernel using the same kernel
> configuration, reproducer, workload, and timing settings.
> 
> The ordering appears to be:
> 
> 1. ep_aio() registers cancellation and successfully queues the USB request.
> 2. ep_aio() releases epdata->dev->lock before acquiring aio_lock.
> 3. ep_aio_complete() can run the immediate-completion branch, clear
>    iocb->private, call iocb->ki_complete(), set priv->req_state to
>    AIO_GIVEN_BACK, observe that cancel_state is not AIO_UNLINKING, and
>    reach the path that frees priv.
> 4. ep_aio() then acquires aio_lock and reads priv->req_state at line 647.
> 
> Thus aio_lock serializes the post-queue state check and update, but it
> does not keep priv alive until ep_aio() has finished that transition.
> This is not the original ep_aio_cancel() stale-pointer window. The
> observed UAF and the code ordering are consistent with ep_aio()
> dereferencing priv after the completion path has freed it.
> 
> This suggests that the submitting path may need an additional lifetime
> handoff. Either priv must remain alive until ep_aio() has finished its
> post-queue work, or ep_aio() must no longer access priv after the point at
> which the completion path may free it.

In fact it's simpler than that; I didn't think to add a test in one 
spot.  The updated patch is below.  It differs from the previous patch 
only in that one line.

> In diagnostic runs, I also tested cancellation while the request was in
> AIO_SUBMITTING. In the queue-success ordering, I observed one completion
> event with res=-ECONNRESET; in the forced queue-failure ordering, one
> event with res=-EINVAL. Neither case produced KASAN or an Oops.
> 
> I also rechecked the potential unbind race you mentioned earlier. Using
> pointer-correlated tracing on the patched build, I observed an
> ep_aio_cancel() operation overlapping the gadgetfs_unbind() path while
> that path called usb_ep_disable() on the request's endpoint. The
> ep_aio_cancel() entry and ep_aio_complete() records referred to the same
> iocb, priv, and usb_request, and the corresponding usb_ep_dequeue()
> record referred to that same usb_request and endpoint.
> ep_user_copy_worker() also ran for the same priv while gadgetfs_unbind()
> was still in progress.

That's okay.  The possibility I was worried about was that 
gadgetfs_unbind() might complete while ep_aio_cancel() was still 
running.  If that happened, the usb_ep_free_request() and put_ep() calls 
near the end of ep_aio_cancel() might run into trouble.  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 don't know if that's possible, but I also don't see anything to 
prevent it from happening.  It might be necessary to add a reference 
counter for the number of outstanding aio operations.

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,6 +435,19 @@ 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;
@@ -443,24 +458,53 @@ struct kiocb_priv {
 	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)
 {
-	struct kiocb_priv	*priv = iocb->private;
+	struct kiocb_priv	*priv;
 	struct ep_data		*epdata;
+	struct usb_ep		*ep;
 	int			value;
+	enum aio_req_state	req_state;
 
-	local_irq_disable();
-	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();
+	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 */
+	}
 
+	epdata = priv->epdata;
+	ep = epdata->ep;
+	priv->cancel_state = AIO_UNLINKING;
+	spin_unlock_irq(&aio_lock);
+
+	value = usb_ep_dequeue(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(ep, priv->req);
+		put_ep(epdata);
+		if (req_state == AIO_GIVEN_BACK)
+			kfree(priv);
+	}
 	return value;
 }
 
@@ -482,7 +526,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 +539,16 @@ 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) */
+	/* lock against unbind */
 	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 +557,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))
@@ -516,12 +569,23 @@ 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)
+		schedule_work(&priv->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 +596,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 +612,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 +626,37 @@ 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;
+	kiocb_set_cancel_fn(iocb, ep_aio_cancel);
+
+	value = usb_ep_queue(ep, req, GFP_ATOMIC);
 	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_SUBMITTING) {
+		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: