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: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Schspa Shi <[email protected]>
---
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;
/* 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) {
--
2.29.0
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: [email protected]
> > Link: https://lore.kernel.org/all/[email protected]/
> >
> > Signed-off-by: Schspa Shi <[email protected]>
> > ---
> > 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
Greg KH <[email protected]> writes:
> 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: [email protected]
>> Link: https://lore.kernel.org/all/[email protected]/
>>
>> Signed-off-by: Schspa Shi <[email protected]>
>> ---
>> 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?
>
We can't use the existing lock, because it's a spinlock, and can't call
usb_gadget_register_driver() in its critical section, it will hold
"udc_lock" which is a mutex_lock. Moreover, a deeper, it will call
driver_register(), which can't be called by atomic context too.
>> + mutex_lock(&dev->driver_lock);
>> ret = usb_gadget_register_driver(&dev->driver);
>> + mutex_unlock(&dev->driver_lock);
>
> How can unregister race with register?
>
I'm sorry for the confused race explanation, it's not unregister race
with register, this lock around usb_gadget_unregister_driver() can be
I will remove this lock in a new patchset if no other change needs to
be made.
> What ioctl is causing this race? What userspace program is doing this?
> Only one userspace program should be accessing this at once, right?
>
> confused,
It's because of two processes calling to register the same driver
instance, which causes the race condition.
The ioctl is USB_RAW_IOCTL_RUN, which can be called from userspace
multi times or we should add protection to ioctl calls to avoid multi
time device register?
In this usb gadget, the driver property should be passed from
USB_RAW_IOCTL_INIT ioctl, which leave here a device_register by
userspace.
For more details about this race.
Process 0 Process 1
USB_RAW_IOCTL_INIT
USB_RAW_IOCTL_RUN USB_RAW_IOCTL_RUN
usb_gadget_register_driver usb_gadget_register_driver
driver_register driver_register
>
> greg k-h
---
BRs
Schspa Shi
Alan Stern <[email protected]> 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: [email protected]
>> > Link: https://lore.kernel.org/all/[email protected]/
>> >
>> > Signed-off-by: Schspa Shi <[email protected]>
>> > ---
>> > 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
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: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Schspa Shi <[email protected]>
> ---
> 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?
confused,
greg k-h
The usb_gadget_register_driver can be called multi time by to
threads via USB_RAW_IOCTL_RUN ioctl syscall, which will lead
to multiple registrations.
Call trace:
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
ioctl USB_RAW_IOCTL_RUN
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 new STATE_DEV_REGISTERING device state to
avoid double register.
Reported-by: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Schspa Shi <[email protected]>
---
drivers/usb/gadget/legacy/raw_gadget.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index b3be8db1ff63..b75f8f7b7b46 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -146,6 +146,7 @@ enum dev_state {
STATE_DEV_OPENED,
STATE_DEV_INITIALIZED,
STATE_DEV_RUNNING,
+ STATE_DEV_REGISTERING,
STATE_DEV_CLOSED,
STATE_DEV_FAILED
};
@@ -508,6 +509,7 @@ static int raw_ioctl_run(struct raw_dev *dev, unsigned long value)
ret = -EINVAL;
goto out_unlock;
}
+ dev->state = STATE_DEV_REGISTERING;
spin_unlock_irqrestore(&dev->lock, flags);
ret = usb_gadget_register_driver(&dev->driver);
--
2.24.3 (Apple Git-128)