[PATCH] USB: misc: iowarrior: fix use-after-free in release

[PATCH] USB: misc: iowarrior: fix use-after-free in release

[Yue Sun]

From: Sam Sun <samsun1006219@gmail.com>

iowarrior_release() clears dev->opened while holding dev->mutex and then
unlocks the mutex before freeing the device when it has already been
disconnected. If iowarrior_disconnect() is waiting for the same mutex, it
can acquire it while mutex_unlock() in release is still in progress,
observe dev->opened == 0, and free the same object. This violates the
mutex_unlock() lifetime rule because the mutex storage can be freed before
mutex_unlock() has returned.

Fix this by adding a kref to struct iowarrior and holding one reference
for the USB interface, one for each opened file, and one for each
in-flight asynchronous write URB. Release, disconnect and write completion
now only drop their references after they are done using the object, and
the object is freed only after all users have released it.

Reported-by: Sam Sun <samsun1006219@gmail.com>
Closes: https://lore.kernel.org/r/20260618080204.38322-1-samsun1006219@gmail.com
Signed-off-by: Sam Sun <samsun1006219@gmail.com>
---
 drivers/usb/misc/iowarrior.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 22504c0a2841..abeed76379d7 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/mutex.h>
+#include <linux/kref.h>
 #include <linux/poll.h>
 #include <linux/hid.h>
 #include <linux/usb/iowarrior.h>
@@ -73,6 +74,7 @@ static struct usb_driver iowarrior_driver;
 /* Structure to hold all of our device specific stuff */
 struct iowarrior {
 	struct mutex mutex;			/* locks this structure */
+	struct kref kref;
 	struct usb_device *udev;		/* save off the usb device pointer */
 	struct usb_interface *interface;	/* the interface for this device */
 	struct usb_endpoint_descriptor *int_out_endpoint;	/* endpoint for reading (needed for IOW56 only) */
@@ -95,6 +97,10 @@ struct iowarrior {
 	struct usb_anchor submitted;
 };
 
+#define to_iowarrior_dev(d) container_of(d, struct iowarrior, kref)
+
+static void iowarrior_delete(struct kref *kref);
+
 /*--------------*/
 /*    globals   */
 /*--------------*/
@@ -235,13 +241,16 @@ static void iowarrior_write_callback(struct urb *urb)
 	/* tell a waiting writer the interrupt-out-pipe is available again */
 	atomic_dec(&dev->write_busy);
 	wake_up_interruptible(&dev->write_wait);
+	kref_put(&dev->kref, iowarrior_delete);
 }
 
 /*
  *	iowarrior_delete
  */
-static inline void iowarrior_delete(struct iowarrior *dev)
+static void iowarrior_delete(struct kref *kref)
 {
+	struct iowarrior *dev = to_iowarrior_dev(kref);
+
 	kfree(dev->int_in_buffer);
 	usb_free_urb(dev->int_in_urb);
 	kfree(dev->read_queue);
@@ -454,12 +463,14 @@ static ssize_t iowarrior_write(struct file *file,
 			goto error;
 		}
 		usb_anchor_urb(int_out_urb, &dev->submitted);
+		kref_get(&dev->kref);
 		retval = usb_submit_urb(int_out_urb, GFP_KERNEL);
 		if (retval) {
 			dev_dbg(&dev->interface->dev,
 				"submit error %d for urb nr.%d\n",
 				retval, atomic_read(&dev->write_busy));
 			usb_unanchor_urb(int_out_urb);
+			kref_put(&dev->kref, iowarrior_delete);
 			goto error;
 		}
 		/* submit was ok */
@@ -637,6 +648,7 @@ static int iowarrior_open(struct inode *inode, struct file *file)
 	}
 	/* increment our usage count for the driver */
 	++dev->opened;
+	kref_get(&dev->kref);
 	/* save our object in the file's private structure */
 	file->private_data = dev;
 	retval = 0;
@@ -657,13 +669,13 @@ static int iowarrior_release(struct inode *inode, struct file *file)
 	dev = file->private_data;
 	if (!dev)
 		return -ENODEV;
+	file->private_data = NULL;
 
 	/* lock our device */
 	mutex_lock(&dev->mutex);
 
 	if (dev->opened <= 0) {
 		retval = -ENODEV;	/* close called more than once */
-		mutex_unlock(&dev->mutex);
 	} else {
 		dev->opened = 0;	/* we're closing now */
 		retval = 0;
@@ -675,13 +687,11 @@ static int iowarrior_release(struct inode *inode, struct file *file)
 			usb_kill_urb(dev->int_in_urb);
 			wake_up_interruptible(&dev->read_wait);
 			wake_up_interruptible(&dev->write_wait);
-			mutex_unlock(&dev->mutex);
-		} else {
-			/* The device was unplugged, cleanup resources */
-			mutex_unlock(&dev->mutex);
-			iowarrior_delete(dev);
 		}
 	}
+	mutex_unlock(&dev->mutex);
+
+	kref_put(&dev->kref, iowarrior_delete);
 	return retval;
 }
 
@@ -768,6 +778,7 @@ static int iowarrior_probe(struct usb_interface *interface,
 		return retval;
 
 	mutex_init(&dev->mutex);
+	kref_init(&dev->kref);
 
 	atomic_set(&dev->intr_idx, 0);
 	atomic_set(&dev->read_idx, 0);
@@ -885,7 +896,7 @@ static int iowarrior_probe(struct usb_interface *interface,
 	return retval;
 
 error:
-	iowarrior_delete(dev);
+	kref_put(&dev->kref, iowarrior_delete);
 	return retval;
 }
 
@@ -918,8 +929,8 @@ static void iowarrior_disconnect(struct usb_interface *interface)
 	} else {
 		/* no process is using the device, cleanup now */
 		mutex_unlock(&dev->mutex);
-		iowarrior_delete(dev);
 	}
+	kref_put(&dev->kref, iowarrior_delete);
 }
 
 /* usb specific object needed to register this driver with the usb subsystem */
-- 
2.34.1

Re: [PATCH] USB: misc: iowarrior: fix use-after-free in release

[Johan Hovold]

On Fri, Jun 19, 2026 at 11:03:37PM +0800, Yue Sun wrote:
> From: Sam Sun <samsun1006219@gmail.com>
> 
> iowarrior_release() clears dev->opened while holding dev->mutex and then
> unlocks the mutex before freeing the device when it has already been
> disconnected. If iowarrior_disconnect() is waiting for the same mutex, it
> can acquire it while mutex_unlock() in release is still in progress,
> observe dev->opened == 0, and free the same object.

The above description is not correct as it seems to suggests that there
is a double-free here, which there is not.

Either release() or disconnect() will free the driver data, but the
other one may still be executing mutex_unlock() when that happens.

> This violates the
> mutex_unlock() lifetime rule because the mutex storage can be freed before
> mutex_unlock() has returned.

Indeed. Thanks for catching this.
 
> Fix this by adding a kref to struct iowarrior and holding one reference
> for the USB interface, one for each opened file, and one for each
> in-flight asynchronous write URB. Release, disconnect and write completion
> now only drop their references after they are done using the object, and
> the object is freed only after all users have released it.

You only need a reference for each open file (and one while the driver
is bound). All I/O must cease on disconnect so there is no need to keep
an additional reference for each outstanding write.

Note that there is another bug here for which the fix has not yet been
applied though:

	https://lore.kernel.org/all/20260523170523.1074563-1-johan@kernel.org/

> Reported-by: Sam Sun <samsun1006219@gmail.com>

You shouldn't add a reported-by tag for issues you fix yourself. A Link
to the syzbot report is good to have though.

> Closes: https://lore.kernel.org/r/20260618080204.38322-1-samsun1006219@gmail.com

I only saw your report last week and missed that you had posted this fix
until today when I was about to submit a series fixing this driver and a
few others that got this wrong:

	https://lore.kernel.org/all/20260622152612.116422-1-johan@kernel.org/

With the commit message fixed and the write references dropped they are
mostly equivalent.

> Signed-off-by: Sam Sun <samsun1006219@gmail.com>
> ---

> @@ -95,6 +97,10 @@ struct iowarrior {
>  	struct usb_anchor submitted;
>  };
>  
> +#define to_iowarrior_dev(d) container_of(d, struct iowarrior, kref)

I know we have a few of these in the tree already, but it's not really
needed for a single user and the "d" is a bit misleading as these macros
are typically used to cast from a struct device (not a kref).

> +
> +static void iowarrior_delete(struct kref *kref);
> +
>  /*--------------*/
>  /*    globals   */
>  /*--------------*/
> @@ -235,13 +241,16 @@ static void iowarrior_write_callback(struct urb *urb)
>  	/* tell a waiting writer the interrupt-out-pipe is available again */
>  	atomic_dec(&dev->write_busy);
>  	wake_up_interruptible(&dev->write_wait);
> +	kref_put(&dev->kref, iowarrior_delete);
>  }

> @@ -454,12 +463,14 @@ static ssize_t iowarrior_write(struct file *file,
>  			goto error;
>  		}
>  		usb_anchor_urb(int_out_urb, &dev->submitted);
> +		kref_get(&dev->kref);
>  		retval = usb_submit_urb(int_out_urb, GFP_KERNEL);
>  		if (retval) {
>  			dev_dbg(&dev->interface->dev,
>  				"submit error %d for urb nr.%d\n",
>  				retval, atomic_read(&dev->write_busy));
>  			usb_unanchor_urb(int_out_urb);
> +			kref_put(&dev->kref, iowarrior_delete);
>  			goto error;
>  		}
>  		/* submit was ok */

All outstanding writes should be stopped on disconnect (and soon will be
when the fix I mentioned above is merged), so this is not needed.

> @@ -637,6 +648,7 @@ static int iowarrior_open(struct inode *inode, struct file *file)
>  	}
>  	/* increment our usage count for the driver */
>  	++dev->opened;
> +	kref_get(&dev->kref);
>  	/* save our object in the file's private structure */
>  	file->private_data = dev;
>  	retval = 0;
> @@ -657,13 +669,13 @@ static int iowarrior_release(struct inode *inode, struct file *file)
>  	dev = file->private_data;
>  	if (!dev)
>  		return -ENODEV;
> +	file->private_data = NULL;

This looks like an unrelated change.

>  	/* lock our device */
>  	mutex_lock(&dev->mutex);
>  
>  	if (dev->opened <= 0) {
>  		retval = -ENODEV;	/* close called more than once */
> -		mutex_unlock(&dev->mutex);
>  	} else {
>  		dev->opened = 0;	/* we're closing now */
>  		retval = 0;

> @@ -918,8 +929,8 @@ static void iowarrior_disconnect(struct usb_interface *interface)
>  	} else {
>  		/* no process is using the device, cleanup now */
>  		mutex_unlock(&dev->mutex);
> -		iowarrior_delete(dev);
>  	}

The unlock should now be moved after the conditional.

> +	kref_put(&dev->kref, iowarrior_delete);
>  }
>  
>  /* usb specific object needed to register this driver with the usb subsystem */

Johan