Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:

> On Sat, May 07, 2022 at 04:27:14PM +0200, Greg KH wrote:
>> On Sat, May 07, 2022 at 08:08:51PM +0800, Schspa Shi wrote:
>> > The usb_gadget_register_driver doesn't have inside locks to protect the
>> > driver, and If there is two threads are registered at the same time via
>> > the ioctl syscall, the system will crash as syzbot reported.
>> >
>> > Call trace as:
>> >   driver_register+0x220/0x3a0 drivers/base/driver.c:171
>> >   usb_gadget_register_driver_owner+0xfb/0x1e0
>> >     drivers/usb/gadget/udc/core.c:1546
>> >   raw_ioctl_run drivers/usb/gadget/legacy/raw_gadget.c:513 [inline]
>> >   raw_ioctl+0x1883/0x2730 drivers/usb/gadget/legacy/raw_gadget.c:1220
>> >
>> > This routine allows two processes to register the same driver instance
>> > via ioctl syscall. which lead to a race condition.
>> >
>> > We can fix it by adding a driver_lock to avoid double register.
>> >
>> > Reported-by: syzbot+dc7c3ca638e773db07f6@xxxxxxxxxxxxxxxxxxxxxxxxx
>> > Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@xxxxxxxxxx/
>> >
>> > Signed-off-by: Schspa Shi <schspa@xxxxxxxxx>
>> > ---
>> >  drivers/usb/gadget/legacy/raw_gadget.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
>> > index b3be8db1ff63..d7ff9c2b5397 100644
>> > --- a/drivers/usb/gadget/legacy/raw_gadget.c
>> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
>> > @@ -155,7 +155,9 @@ struct raw_dev {
>> >    spinlock_t                      lock;
>> >
>> >    const char                      *udc_name;
>> > +  /* Protected by driver_lock for reentrant registration */
>> >    struct usb_gadget_driver        driver;
>> > +  struct mutex                    driver_lock;
>>
>> Why are you adding another lock here?  What's wrong with the existing
>> lock in this structure that requires an additional one?
>>
>> >
>> >    /* Reference to misc device: */
>> >    struct device                   *dev;
>> > @@ -188,6 +190,8 @@ static struct raw_dev *dev_new(void)
>> >    spin_lock_init(&dev->lock);
>> >    init_completion(&dev->ep0_done);
>> >    raw_event_queue_init(&dev->queue);
>> > +  mutex_init(&dev->driver_lock);
>> > +
>> >    return dev;
>> >  }
>> >
>> > @@ -398,7 +402,9 @@ static int raw_release(struct inode *inode, struct file *fd)
>> >    spin_unlock_irqrestore(&dev->lock, flags);
>> >
>> >    if (unregister) {
>> > +          mutex_lock(&dev->driver_lock);
>> >            ret = usb_gadget_unregister_driver(&dev->driver);
>> > +          mutex_unlock(&dev->driver_lock);
>> >            if (ret != 0)
>> >                    dev_err(dev->dev,
>> >                            "usb_gadget_unregister_driver() failed with %d\n",
>> > @@ -510,7 +516,9 @@ static int raw_ioctl_run(struct raw_dev *dev, unsigned long value)
>> >    }
>> >    spin_unlock_irqrestore(&dev->lock, flags);
>> >
>> > +  mutex_lock(&dev->driver_lock);
>> >    ret = usb_gadget_register_driver(&dev->driver);
>> > +  mutex_unlock(&dev->driver_lock);
>>
>> How can unregister race with register?
>>
>> What ioctl is causing this race?  What userspace program is doing this?
>> Only one userspace program should be accessing this at once, right?
>
> These questions are on the right track.
>
> The problem here is not insufficient locking.  The problem is that
> dev->state does not have a special state to indicate that the driver is
> being registered.
>
> Before calling usb_gadget_register_driver(), while still holding
> dev->lock, the code should change dev->state to STATE_DEV_REGISTERING.
> Then no race can occur, because the second thread to acquire the
> spinlock will see that dev->state is not equal to STATE_DEV_INITIALIZED.
>

Yes, it's a good suggestion, I will upload a new patch set to use this
method.

> Alan Stern

---
BRs

Schspa Shi