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

[neck3922]

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.

> 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.

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.

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.

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. Separately, an
intermediate cumulative version allowed gadgetfs_unbind() to pass the
completion-work flush before a callback had queued that work. The
resulting patch addresses all three issues and applies directly to
upstream v7.2-rc1.

I reran the exact pre-queue matrix and earlier regression reproducers,
together with native PREAD/PREADV/PWRITE/PWRITEV payload checks and
directed cross-CPU and deferred-giveback tests. I also exercised teardown
and endpoint-release orderings, module lifecycle, CPU hotplug, rebind,
io_destroy, process exit, and partial-read cancellation stress. Across
the tested GadgetFS AIO lifetime and teardown paths, I observed no KASAN
report, Oops, LOCKDEP warning, stall, duplicate completion, or userspace
failure.

Targeted DEBUG_OBJECTS runs were clean, and strict KCSAN reported no race
involving GadgetFS or these AIO work functions. The incremental changes
passed strict checkpatch; the resulting source passed a W=1 module build,
a built-in CONFIG_USB_GADGETFS=y build, and a C=2 W=1 sparse check.

Supporting files for the two diagnostics above:

  Cross-CPU reproducer:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_20260825_support/repro_candidate_unlink_copy_uaf.c

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

  Symbolized ep_unlink_worker() KASAN report:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_20260825_support/symbolized_report_ep_unlink_worker_uaf.txt

  Worker-entry delay diagnostic and module-unload result:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_20260825_support/candidate_20260822_diag_unlink_entry_delay.patch
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_20260825_support/teardown_module_unload_serial.log

  Kernel config used for these diagnostics:
    https://raw.githubusercontent.com/neck392/linux-kernel-bug-reports/main/gadgetfs_candidate_20260825_support/kernel.config.kasan_inline_dwarf5_lockdep

If I have misunderstood any part of the intended lifetime or teardown
behavior, please let me know. I would be glad to run any additional tests
if needed. :-)

With sincere appreciation and respect,
Minseo Kim

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
@@ -29,6 +29,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/moduleparam.h>
+#include <linux/workqueue.h>

 #include <linux/usb/gadgetfs.h>
 #include <linux/usb/gadget.h>
@@ -123,6 +124,8 @@
 	spinlock_t			lock;
 	refcount_t			count;
 	int				udc_usage;
+	int				aio_producers;	/* P: lock */
+	bool				aio_shutdown;	/* P: aio_lock */
 	enum ep0_state			state;		/* P: lock */
 	struct usb_gadgetfs_event	event [N_EVENT];
 	unsigned			ev_next;
@@ -236,6 +239,10 @@
 static const char *CHIP;
 static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */

+static DEFINE_SPINLOCK(aio_lock);	/* Protect GadgetFS AIO state */
+static struct workqueue_struct *gadgetfs_unlink_wq;
+static struct workqueue_struct *gadgetfs_copy_wq;
+
 /*----------------------------------------------------------------------*/

 /* NOTE:  don't use dev_printk calls before binding to the gadget
@@ -392,6 +399,8 @@
 		data->state = STATE_EP_DISABLED;
 		data->desc.bDescriptorType = 0;
 		data->hs_desc.bDescriptorType = 0;
+		/* Wait for unlink work before disabling the endpoint. */
+		flush_workqueue(gadgetfs_unlink_wq);
 		usb_ep_disable(data->ep);
 	}
 	mutex_unlock(&data->lock);
@@ -433,95 +442,189 @@

 /* 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;
+	int			status;
+	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;
+	struct usb_ep		*ep;
+	struct usb_request	*req;
+	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();
+	ep = priv->ep;
+	req = priv->req;

-	return value;
+	usb_ep_dequeue(ep, req);
+
+	spin_lock_irq(&aio_lock);
+	req_state = priv->req_state;
+	priv->cancel_state = AIO_UNLINK_DONE;
+	if (req_state >= AIO_COMPLETED)
+		priv->req = NULL;
+	spin_unlock_irq(&aio_lock);
+
+	/*
+	 * If the callback has already published AIO_COMPLETED, this worker
+	 * claims request cleanup. Free priv only after copy_work publishes
+	 * AIO_GIVEN_BACK.
+	 */
+	if (req_state >= AIO_COMPLETED) {
+		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;		/* No longer cancellable */
+	}
+	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 */
+	}
+	if (priv->epdata->dev->aio_shutdown) {
+		spin_unlock_irqrestore(&aio_lock, flags);
+		return -EINVAL;	/* Leave completion to the giveback path */
+	}
+
+	/*
+	 * The AIO core may call us with the iocb context lock held. Run the
+	 * dequeue in work context, and complete the iocb from copy_work, so a
+	 * synchronous giveback cannot re-enter the AIO core from this callback.
+	 *
+	 * Queue under aio_lock so unbind cannot set aio_shutdown and flush
+	 * the workqueue before this work is visible.
+	 */
+	priv->cancel_state = AIO_UNLINKING;
+	queue_work(gadgetfs_unlink_wq, &priv->unlink_work);
+	spin_unlock_irqrestore(&aio_lock, flags);
+	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 usb_request *req = NULL;
+	struct ep_data *epdata = NULL;
+	struct usb_ep *ep = NULL;
 	struct mm_struct *mm = priv->mm;
 	struct kiocb *iocb = priv->iocb;
-	size_t ret;
+	ssize_t ret;

-	kthread_use_mm(mm);
-	ret = copy_to_iter(priv->buf, priv->actual, &priv->to);
-	kthread_unuse_mm(mm);
-	if (!ret)
-		ret = -EFAULT;
+	/* Writes leave priv->to empty; reads retain the destination iterator. */
+	if (!iov_iter_count(&priv->to) || !priv->actual) {
+		ret = priv->actual ? (ssize_t)priv->actual :
+			(ssize_t)priv->status;
+	} else {
+		kthread_use_mm(mm);
+		ret = copy_to_iter(priv->buf, priv->actual, &priv->to);
+		kthread_unuse_mm(mm);
+		if (!ret)
+			ret = -EFAULT;
+	}
+
+	/* Claim request cleanup unless the unlink worker owns it. */
+	spin_lock_irq(&aio_lock);
+	if (priv->cancel_state != AIO_UNLINKING && priv->req) {
+		req = priv->req;
+		epdata = priv->epdata;
+		ep = priv->ep;
+		priv->req = NULL;
+	}
+	spin_unlock_irq(&aio_lock);
+
+	if (req) {
+		mutex_lock(&epdata->lock);
+		usb_ep_free_request(ep, req);
+		mutex_unlock(&epdata->lock);
+		put_ep(epdata);
+	}

-	/* completing the iocb can drop the ctx and mm, don't touch mm after */
+	/* Completing the iocb can drop the file, ctx, and mm. */
 	iocb->ki_complete(iocb, ret);

 	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;
+	struct dev_data		*dev = priv->epdata->dev;

-	/* lock against disconnect (and ideally, cancel) */
-	spin_lock(&epdata->dev->lock);
-	priv->req = NULL;
-	priv->epdata = NULL;
-
-	/* 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)) {
-		kfree(req->buf);
-		kfree(priv->to_free);
-		kfree(priv);
-		iocb->private = NULL;
-		iocb->ki_complete(iocb,
-				req->actual ? req->actual : (long)req->status);
-	} 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",
-				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);
-	}
-
-	usb_ep_free_request(ep, req);
-	spin_unlock(&epdata->dev->lock);
-	put_ep(epdata);
+	/* Prevent future cancellation */
+	spin_lock(&aio_lock);
+	iocb->private = NULL;
+	spin_unlock(&aio_lock);
+
+	/* Log status errors hidden by a nonzero actual count. */
+	if (iov_iter_count(&priv->to) && req->actual &&
+	    unlikely(req->status != 0))
+		DBG(priv->epdata->dev, "%s fault %d len %d\n",
+		    ep->name, req->status, req->actual);
+
+	priv->buf = req->buf;
+	priv->actual = req->actual;
+	priv->status = req->status;
+
+	spin_lock(&aio_lock);
+	priv->req_state = AIO_COMPLETED;
+	spin_unlock(&aio_lock);
+
+	INIT_WORK(&priv->copy_work, ep_user_copy_worker);
+	/* Publish copy work before unbind can observe the last producer. */
+	spin_lock(&dev->lock);
+	queue_work(gadgetfs_copy_wq, &priv->copy_work);
+	--dev->aio_producers;
+	spin_unlock(&dev->lock);
 }

 static ssize_t ep_aio(struct kiocb *iocb,
@@ -532,11 +635,13 @@
 {
 	struct usb_request *req;
 	ssize_t value;
+	struct usb_ep *ep;
+	bool need_unlink = false;

+	INIT_WORK(&priv->unlink_work, ep_unlink_worker);
 	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 +652,12 @@
 	 */
 	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 +667,48 @@
 	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;
+	/* Account for the callback before the request is visible to the UDC. */
+	++epdata->dev->aio_producers;
+	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);
+		--epdata->dev->aio_producers;
+		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:
@@ -1640,8 +1783,12 @@
 gadgetfs_unbind (struct usb_gadget *gadget)
 {
 	struct dev_data		*dev = get_gadget_data (gadget);
+	unsigned long		flags;

 	DBG (dev, "%s\n", __func__);
+	spin_lock_irqsave(&aio_lock, flags);
+	dev->aio_shutdown = true;
+	spin_unlock_irqrestore(&aio_lock, flags);

 	spin_lock_irq (&dev->lock);
 	dev->state = STATE_DEV_UNBOUND;
@@ -1652,7 +1799,21 @@
 	}
 	spin_unlock_irq (&dev->lock);

+	/* Wait for unlink work before disabling endpoints. */
+	flush_workqueue(gadgetfs_unlink_wq);
 	destroy_ep_files (dev);
+
+	/* Wait until completion callbacks have published their cleanup work. */
+	spin_lock_irq(&dev->lock);
+	while (dev->aio_producers > 0) {
+		spin_unlock_irq(&dev->lock);
+		usleep_range(1000, 2000);
+		spin_lock_irq(&dev->lock);
+	}
+	spin_unlock_irq(&dev->lock);
+
+	/* Wait for completion work queued by the callbacks. */
+	flush_workqueue(gadgetfs_copy_wq);
 	gadget->ep0->driver_data = NULL;
 	set_gadget_data (gadget, NULL);

@@ -1677,6 +1838,9 @@
 			shortname, CHIP, gadget->name);
 		return -ENODEV;
 	}
+	spin_lock_irq(&aio_lock);
+	dev->aio_shutdown = false;
+	spin_unlock_irq(&aio_lock);

 	set_gadget_data (gadget, dev);
 	dev->gadget = gadget;
@@ -2125,10 +2289,25 @@
 {
 	int status;

+	gadgetfs_unlink_wq = alloc_workqueue("gadgetfs_unlink", WQ_PERCPU, 0);
+	if (!gadgetfs_unlink_wq)
+		return -ENOMEM;
+
+	gadgetfs_copy_wq = alloc_workqueue("gadgetfs_copy", WQ_PERCPU, 0);
+	if (!gadgetfs_copy_wq) {
+		destroy_workqueue(gadgetfs_unlink_wq);
+		return -ENOMEM;
+	}
+
 	status = register_filesystem (&gadgetfs_type);
-	if (status == 0)
-		pr_info ("%s: %s, version " DRIVER_VERSION "\n",
-			shortname, driver_desc);
+	if (status) {
+		destroy_workqueue(gadgetfs_copy_wq);
+		destroy_workqueue(gadgetfs_unlink_wq);
+		return status;
+	}
+
+	pr_info("%s: %s, version " DRIVER_VERSION "\n",
+		shortname, driver_desc);
 	return status;
 }
 module_init (gadgetfs_init);
@@ -2137,6 +2316,7 @@
 {
 	pr_debug ("unregister %s\n", shortname);
 	unregister_filesystem (&gadgetfs_type);
+	destroy_workqueue(gadgetfs_unlink_wq);
+	destroy_workqueue(gadgetfs_copy_wq);
 }
 module_exit (gadgetfs_cleanup);
-