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. Alan Stern
