| Message ID | 20220507120851.29948-1-schspa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: fix race when gadget driver register via ioctl | expand |
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@syzkaller.appspotmail.com > > Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/ > > > > Signed-off-by: Schspa Shi <schspa@gmail.com> > > --- > > 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
Alan Stern <stern@rowland.harvard.edu> 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@syzkaller.appspotmail.com >> > Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/ >> > >> > Signed-off-by: Schspa Shi <schspa@gmail.com> >> > --- >> > 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
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; /* 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); spin_lock_irqsave(&dev->lock, flags); if (ret) {
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@syzkaller.appspotmail.com Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/ Signed-off-by: Schspa Shi <schspa@gmail.com> --- drivers/usb/gadget/legacy/raw_gadget.c | 8 ++++++++ 1 file changed, 8 insertions(+)