Re: usb: A use-after-free Write in put_dev

[Salvatore Bonaccorso <carnil@debian.org>]

Hi,

On Tue, Dec 13, 2022 at 09:02:53AM +0800, Gerald Lee wrote:
> Hi Alan,
> 
> I've tested this patch. It resolves the problem.
> 
> Thanks.
> 
> 
> On Tue, Dec 13, 2022 at 12:24 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Mon, Dec 12, 2022 at 04:07:43PM +0800, Gerald Lee wrote:
> > > Hi, all
> > >
> > > I found a vulnerability when fuzzing linux kernel by syzkaller. The
> > > KASAN reports that use-after-free Write in put_dev. Then I tried to
> > > reproduce and got the C source file. I compiled it and executed the
> > > binary program, but the kernel crashed as expected. This vulnerability
> > > could be used to LPE as UAF, I thought.
> >
> > I think this use-after-free violation is caused by a race among the
> > superblock operations in the gadgetfs driver.  Please try running your
> > test after applying the patch below, to see if it fixes the problem.
> >
> > Alan Stern
> >
> >
> > --- usb-devel.orig/drivers/usb/gadget/legacy/inode.c
> > +++ usb-devel/drivers/usb/gadget/legacy/inode.c
> > @@ -229,6 +229,7 @@ static void put_ep (struct ep_data *data
> >   */
> >
> >  static const char *CHIP;
> > +static DEFINE_MUTEX(sb_mutex);         /* Serialize superblock maintenance */
> >
> >  /*----------------------------------------------------------------------*/
> >
> > @@ -2010,13 +2011,20 @@ gadgetfs_fill_super (struct super_block
> >  {
> >         struct inode    *inode;
> >         struct dev_data *dev;
> > +       int             rc;
> >
> > -       if (the_device)
> > -               return -ESRCH;
> > +       mutex_lock(&sb_mutex);
> > +
> > +       if (the_device) {
> > +               rc = -ESRCH;
> > +               goto Done;
> > +       }
> >
> >         CHIP = usb_get_gadget_udc_name();
> > -       if (!CHIP)
> > -               return -ENODEV;
> > +       if (!CHIP) {
> > +               rc = -ENODEV;
> > +               goto Done;
> > +       }
> >
> >         /* superblock */
> >         sb->s_blocksize = PAGE_SIZE;
> > @@ -2053,13 +2061,17 @@ gadgetfs_fill_super (struct super_block
> >          * from binding to a controller.
> >          */
> >         the_device = dev;
> > -       return 0;
> > +       rc = 0;
> > +       goto Done;
> >
> > -Enomem:
> > + Enomem:
> >         kfree(CHIP);
> >         CHIP = NULL;
> > +       rc = -ENOMEM;
> >
> > -       return -ENOMEM;
> > + Done:
> > +       mutex_unlock(&sb_mutex);
> > +       return rc;
> >  }
> >
> >  /* "mount -t gadgetfs path /dev/gadget" ends up here */
> > @@ -2081,6 +2093,7 @@ static int gadgetfs_init_fs_context(stru
> >  static void
> >  gadgetfs_kill_sb (struct super_block *sb)
> >  {
> > +       mutex_lock(&sb_mutex);
> >         kill_litter_super (sb);
> >         if (the_device) {
> >                 put_dev (the_device);
> > @@ -2088,6 +2101,7 @@ gadgetfs_kill_sb (struct super_block *sb
> >         }
> >         kfree(CHIP);
> >         CHIP = NULL;
> > +       mutex_unlock(&sb_mutex);
> >  }
> >
> >  /*----------------------------------------------------------------------*/

AFAICS, this patch has not yet been applied in mainline, is this
correct?

Regards,
Salvatore