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

[Johan Hovold <johan@kernel.org>]

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