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
