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

[Sam Sun]

On Mon, Jun 22, 2026 at 11:49 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> On Fri, Jun 19, 2026 at 11:03:37PM +0800, Yue Sun wrote:
> > From: Sam Sun <samsun1006219@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx/
>
> > Reported-by: Sam Sun <samsun1006219@xxxxxxxxx>
>
> 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@xxxxxxxxx
>
> 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@xxxxxxxxxx/
>
> With the commit message fixed and the write references dropped they are
> mostly equivalent.
>

Thanks for taking a look and for the detailed explanation!

I agree that the per-write URB references are unnecessary once the
outstanding write URBs are killed unconditionally on disconnect. Your
series is a better and smaller fix.

I'll drop my version in favour of your series.

Best regards,
Yue

> > Signed-off-by: Sam Sun <samsun1006219@xxxxxxxxx>
> > ---
>
> > @@ -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