Re: [PATCH usb-next v2] USB: gadgetfs: Fix use-after-free in gadgetfs_kill_sb

[Rafael Alejandro Díaz Cruz]

Hello,
Sorry for the confusion. I assume I can just address the
concerns here and create another patch once the message
is clear.

I suppose there are actually two errors but because they
are triggered by the same syzkaller reproducer, I'm
treating them as one. The initial problem is the UAF on
the_device pointer when put_dev() is called both by the
error path of gadgetfs_fill_super() and gadgetfs_kill_sb().
This is triggered by the fault_injection in the reproducer.
I'll assume that one is clear.

The second underlying issue is still related to the_device
but not itself, rather the_device->count variable keeping
the references. The syzkaller reproducer still triggers the
same fault_injection in the reproducer but the
fault_injection might fail which leads to the success path
being taken. This means that the gadgetfs_fill_super()
will succeed and the the_device->count will be initialized
with 1 as you had mentioned. However, at the end of
the reproducer close() and umount() will each be called.
This leads to the following:

close() -> dev_releave() -> put_dev()
umount -> gadgetfs_kill_sb() -> put_dev()

the_device->count == 1 at the start but is decremented
twice. refcount < 0 triggers second UAF.

This is fixed by incrementing the reference on the success
path of gadgetfs_fill_super(). That is what the get_dev(dev);
call is for.

As a side note, I'm part of the Linux Kernel Mentorship
and I appreciate your feedback! Please don't hold back on
the suggestions. Every bit helps me!

Thanks,
Rafael.

On Sat, Aug 29, 2026 at 8:05 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Aug 28, 2026 at 06:19:01PM -0700, Rafael Alejandro Diaz Cruz wrote:
> > When gadgetfs_fill_super() fails, it's error path calls
> > put_dev() which drops refcount inside the_device to 0
> > and frees the objet. But the_device pointer is not
> > cleared, leading to point at freed memory.
> >
> > VFS will then call gadgetfs_kill_sb() after mount
> > failure leading to put_dev() to be called on the
> > already freed pointer.
> >
> > Fix by setting the_device = NULL during error path
> > before calling put_dev() inside gadgetfs_fill_super()
> > so that gadgetfs_kill_sb() skips put_dev().
> >
> > However, if the fault injection from syzbot failed
> > and it began the open()/write()/close()/unmount()
> > sequence then close() and umount() will each trigger
> > the refcount to drop once via put_dev(). This will
> > still cause UAF since refcount is only incremented
> > once on creation but decremented twice.
>
> I don't understand this last paragraph at all.  What fault injection
> from syzbot are you talking about?  The earlier part of the description
> doesn't mention syzbot at all.
>
> Why do you spell "unmount" the first time with an 'n' but "umount" the
> second time without an 'n'?
>
> Is there any reason why close and unmount shouldn't both do a
> put_dev()?  Doesn't the open increment the refcount to 2, so close
> and unmount will set it to 0, causing a deallocation but not a UAF?
>
> Why is the refcount incremented upon creation?  Normally refcounts are
> created with an initial value of 1 so they don't need to be incremented.
>
> > Reported-by: syzbot+4a5c87a01894ca37f25c@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Link: https://syzkaller.appspot.com/bug?extid=4a5c87a01894ca37f25c
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Rafael Alejandro Diaz Cruz <rafad900@xxxxxxxxx>
> > ---
> >  drivers/usb/gadget/legacy/inode.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> > index d87a8ab51510..77efa984ce84 100644
> > --- a/drivers/usb/gadget/legacy/inode.c
> > +++ b/drivers/usb/gadget/legacy/inode.c
> > @@ -2059,6 +2059,7 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
> >       rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
> >       if (rc) {
> >               put_dev(dev);
> > +             the_device = NULL;
> >               goto Enomem;
> >       }
> >
> > @@ -2066,6 +2067,7 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
> >        * from binding to a controller.
> >        */
> >       the_device = dev;
> > +     get_dev(dev);
>
> Does this have something to do with that mysterious last paragraph in
> the description?  I can't see any relation between the two.  In
> particular, that paragraph doesn't say anything about adding a
> get_dev().
>
> Alan Stern
>
> >       rc = 0;
> >       goto Done;
> >
> > --
> > 2.43.0