* [PATCH] usb: gadget: fix race when gadget driver register via ioctl
@ 2022-05-07 12:08 Schspa Shi
2022-05-07 14:27 ` Greg KH
0 siblings, 1 reply; 11+ messages in thread
From: Schspa Shi @ 2022-05-07 12:08 UTC (permalink / raw)
To: andreyknvl, balbi, gregkh
Cc: jj251510319013, stern, jannh, Julia.Lawall, linux-usb,
linux-kernel, schspa, syzbot+dc7c3ca638e773db07f6
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;
/* 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 12:08 [PATCH] usb: gadget: fix race when gadget driver register via ioctl Schspa Shi
@ 2022-05-07 14:27 ` Greg KH
2022-05-07 15:06 ` Alan Stern
2022-05-07 15:43 ` [PATCH] " Schspa Shi
0 siblings, 2 replies; 11+ messages in thread
From: Greg KH @ 2022-05-07 14:27 UTC (permalink / raw)
To: Schspa Shi
Cc: andreyknvl, balbi, jj251510319013, stern, jannh, Julia.Lawall,
linux-usb, linux-kernel, syzbot+dc7c3ca638e773db07f6
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?
confused,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 14:27 ` Greg KH
@ 2022-05-07 15:06 ` Alan Stern
2022-05-07 15:50 ` Schspa Shi
2022-05-07 15:43 ` [PATCH] " Schspa Shi
1 sibling, 1 reply; 11+ messages in thread
From: Alan Stern @ 2022-05-07 15:06 UTC (permalink / raw)
To: Greg KH
Cc: Schspa Shi, andreyknvl, balbi, jj251510319013, jannh,
Julia.Lawall, linux-usb, linux-kernel,
syzbot+dc7c3ca638e773db07f6
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 14:27 ` Greg KH
2022-05-07 15:06 ` Alan Stern
@ 2022-05-07 15:43 ` Schspa Shi
1 sibling, 0 replies; 11+ messages in thread
From: Schspa Shi @ 2022-05-07 15:43 UTC (permalink / raw)
To: Greg KH
Cc: andreyknvl, balbi, jj251510319013, stern, jannh, Julia.Lawall,
linux-usb, Linux Kernel Mailing List,
syzbot+dc7c3ca638e773db07f6
Greg KH <gregkh@linuxfoundation.org> 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: 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?
>
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 15:06 ` Alan Stern
@ 2022-05-07 15:50 ` Schspa Shi
2022-05-07 16:02 ` [PATCH v2] " Schspa Shi
0 siblings, 1 reply; 11+ messages in thread
From: Schspa Shi @ 2022-05-07 15:50 UTC (permalink / raw)
To: Alan Stern
Cc: Greg KH, andreyknvl, balbi, jj251510319013, jannh, Julia.Lawall,
linux-usb, Linux Kernel Mailing List,
syzbot+dc7c3ca638e773db07f6
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 15:50 ` Schspa Shi
@ 2022-05-07 16:02 ` Schspa Shi
2022-05-07 17:56 ` Alan Stern
0 siblings, 1 reply; 11+ messages in thread
From: Schspa Shi @ 2022-05-07 16:02 UTC (permalink / raw)
To: schspa
Cc: Julia.Lawall, andreyknvl, balbi, gregkh, jannh, jj251510319013,
linux-kernel, linux-usb, stern, syzbot+dc7c3ca638e773db07f6
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: 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 | 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)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 16:02 ` [PATCH v2] " Schspa Shi
@ 2022-05-07 17:56 ` Alan Stern
2022-05-08 4:08 ` Schspa Shi
0 siblings, 1 reply; 11+ messages in thread
From: Alan Stern @ 2022-05-07 17:56 UTC (permalink / raw)
To: Schspa Shi
Cc: Julia.Lawall, andreyknvl, balbi, gregkh, jannh, jj251510319013,
linux-kernel, linux-usb, syzbot+dc7c3ca638e773db07f6
On Sun, May 08, 2022 at 12:02:43AM +0800, Schspa Shi wrote:
> 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.
Are you sure that this patch will fix the problem found by syzbot? That
is, are you sure that the problem really was caused by two threads
registering the same driver concurrently?
The fact that the error was "use after free" suggests that the problem
might be something else. It looks like one of the threads was trying to
access the driver structure after the other thread had done something
that caused it to be deallocated, which suggests an error in reference
counting.
Yes, this could be caused by two threads both registering the same
driver. But the evidence doesn't prove that this is what happened, as
far as I can see.
Alan Stern
> 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 | 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)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl
2022-05-07 17:56 ` Alan Stern
@ 2022-05-08 4:08 ` Schspa Shi
2022-05-08 14:34 ` Alan Stern
0 siblings, 1 reply; 11+ messages in thread
From: Schspa Shi @ 2022-05-08 4:08 UTC (permalink / raw)
To: Alan Stern
Cc: Julia.Lawall, andreyknvl, balbi, gregkh, jannh, jj251510319013,
linux-kernel, linux-usb, syzbot+dc7c3ca638e773db07f6
Alan Stern <stern@rowland.harvard.edu> writes:
> On Sun, May 08, 2022 at 12:02:43AM +0800, Schspa Shi wrote:
>> 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.
>
> Are you sure that this patch will fix the problem found by syzbot? That
> is, are you sure that the problem really was caused by two threads
> registering the same driver concurrently?
>
Yes, from the console log from syzbot.
T8324 alloced driver_private was released by T8326.
> The fact that the error was "use after free" suggests that the problem
> might be something else. It looks like one of the threads was trying to
> access the driver structure after the other thread had done something
> that caused it to be deallocated, which suggests an error in reference
> counting.
>
The direct cause of this place is because of the refcount error, but the
root cause is still caused by multiple registrations
Please refer to the following scenarios.
T1 T2
------------------------------------------------------------------
usb_gadget_register_driver_owner
driver_register driver_register
driver_find driver_find
bus_add_driver bus_add_driver
priv alloced <context switch>
drv->p = priv;
<schedule out>
kobject_init_and_add // refcount = 1;
//couldn't find an available UDC or it's busy
<context switch>
priv alloced
drv->priv = priv;
kobject_init_and_add
---> refcount = 1 <------
// register success
<context switch>
===================== another ioctl/process ======================
driver_register
driver_find
k = kset_find_obj()
---> refcount = 2 <------
<context out>
driver_unregister
// drv->p become T2's priv
---> refcount = 1 <------
<context switch>
kobject_put(k)
---> refcount = 0 <------
return priv->driver;
--------UAF here----------
There will be UAF in this scenario.
And all the logs reported by syzbot can be matched to this scenario.
> Yes, this could be caused by two threads both registering the same
> driver. But the evidence doesn't prove that this is what happened, as
> far as I can see.
>
> Alan Stern
>
BRs
Schspa Shi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl
2022-05-08 4:08 ` Schspa Shi
@ 2022-05-08 14:34 ` Alan Stern
2022-05-08 15:02 ` [PATCH v3] " Schspa Shi
0 siblings, 1 reply; 11+ messages in thread
From: Alan Stern @ 2022-05-08 14:34 UTC (permalink / raw)
To: Schspa Shi
Cc: Julia.Lawall, andreyknvl, balbi, gregkh, jannh, jj251510319013,
linux-kernel, linux-usb, syzbot+dc7c3ca638e773db07f6
On Sun, May 08, 2022 at 12:08:35PM +0800, Schspa Shi wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> >
> > Are you sure that this patch will fix the problem found by syzbot? That
> > is, are you sure that the problem really was caused by two threads
> > registering the same driver concurrently?
> >
>
> Yes, from the console log from syzbot.
> T8324 alloced driver_private was released by T8326.
That is a smoking gun.
> > The fact that the error was "use after free" suggests that the problem
> > might be something else. It looks like one of the threads was trying to
> > access the driver structure after the other thread had done something
> > that caused it to be deallocated, which suggests an error in reference
> > counting.
> >
>
> The direct cause of this place is because of the refcount error, but the
> root cause is still caused by multiple registrations
>
> Please refer to the following scenarios.
>
> T1 T2
> ------------------------------------------------------------------
> usb_gadget_register_driver_owner
> driver_register driver_register
> driver_find driver_find
> bus_add_driver bus_add_driver
> priv alloced <context switch>
> drv->p = priv;
> <schedule out>
> kobject_init_and_add // refcount = 1;
> //couldn't find an available UDC or it's busy
> <context switch>
> priv alloced
> drv->priv = priv;
> kobject_init_and_add
> ---> refcount = 1 <------
> // register success
> <context switch>
> ===================== another ioctl/process ======================
> driver_register
> driver_find
> k = kset_find_obj()
> ---> refcount = 2 <------
> <context out>
> driver_unregister
> // drv->p become T2's priv
> ---> refcount = 1 <------
> <context switch>
> kobject_put(k)
> ---> refcount = 0 <------
> return priv->driver;
> --------UAF here----------
It looks like you've got T2 calling driver_register and driver_find
twice, but the overall idea is pretty clear.
> There will be UAF in this scenario.
> And all the logs reported by syzbot can be matched to this scenario.
And in any case it is obvious that the patch is necessary. (Although I
would have put the new state before the RUNNING state, to reflect the
actual order in which the states occur.)
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Alan Stern
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] usb: gadget: fix race when gadget driver register via ioctl
2022-05-08 14:34 ` Alan Stern
@ 2022-05-08 15:02 ` Schspa Shi
2022-05-08 23:51 ` Andrey Konovalov
0 siblings, 1 reply; 11+ messages in thread
From: Schspa Shi @ 2022-05-08 15:02 UTC (permalink / raw)
To: stern
Cc: Julia.Lawall, andreyknvl, balbi, gregkh, jannh, jj251510319013,
linux-kernel, linux-usb, schspa, syzbot+dc7c3ca638e773db07f6
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.
Please refer to the following scenarios.
T1 T2
------------------------------------------------------------------
usb_gadget_register_driver_owner
driver_register driver_register
driver_find driver_find
bus_add_driver bus_add_driver
priv alloced <context switch>
drv->p = priv;
<schedule out>
kobject_init_and_add // refcount = 1;
//couldn't find an available UDC or it's busy
<context switch>
priv alloced
drv->priv = priv;
kobject_init_and_add
---> refcount = 1 <------
// register success
<context switch>
===================== another ioctl/process ======================
driver_register
driver_find
k = kset_find_obj()
---> refcount = 2 <------
<context out>
driver_unregister
// drv->p become T2's priv
---> refcount = 1 <------
<context switch>
kobject_put(k)
---> refcount = 0 <------
return priv->driver;
--------UAF here----------
There will be UAF in this scenario.
We can fix it by adding a new STATE_DEV_REGISTERING device state 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>
---
Changelog:
v1 -> v2:
- Add a STATE_DEV_REGISTERING as Alan Stern suggested.
v2 -> v3:
- Adjust STATE_DEV_REGISTERING position to reflect the actual
order in which the states occur.
- Add the fault scenarios to comments.
---
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..241740024c50 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -145,6 +145,7 @@ enum dev_state {
STATE_DEV_INVALID = 0,
STATE_DEV_OPENED,
STATE_DEV_INITIALIZED,
+ STATE_DEV_REGISTERING,
STATE_DEV_RUNNING,
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)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] usb: gadget: fix race when gadget driver register via ioctl
2022-05-08 15:02 ` [PATCH v3] " Schspa Shi
@ 2022-05-08 23:51 ` Andrey Konovalov
0 siblings, 0 replies; 11+ messages in thread
From: Andrey Konovalov @ 2022-05-08 23:51 UTC (permalink / raw)
To: Schspa Shi
Cc: stern, Julia Lawall, Felipe Balbi, Greg Kroah-Hartman, Jann Horn,
Wei Ming Chen, LKML, USB list, syzbot+dc7c3ca638e773db07f6
On Sun, May 8, 2022 at 5:03 PM Schspa Shi <schspa@gmail.com> wrote:
>
> 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.
>
> Please refer to the following scenarios.
>
> T1 T2
> ------------------------------------------------------------------
> usb_gadget_register_driver_owner
> driver_register driver_register
> driver_find driver_find
> bus_add_driver bus_add_driver
> priv alloced <context switch>
> drv->p = priv;
> <schedule out>
> kobject_init_and_add // refcount = 1;
> //couldn't find an available UDC or it's busy
> <context switch>
> priv alloced
> drv->priv = priv;
> kobject_init_and_add
> ---> refcount = 1 <------
> // register success
> <context switch>
> ===================== another ioctl/process ======================
> driver_register
> driver_find
> k = kset_find_obj()
> ---> refcount = 2 <------
> <context out>
> driver_unregister
> // drv->p become T2's priv
> ---> refcount = 1 <------
> <context switch>
> kobject_put(k)
> ---> refcount = 0 <------
> return priv->driver;
> --------UAF here----------
>
> There will be UAF in this scenario.
>
> We can fix it by adding a new STATE_DEV_REGISTERING device state 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>
>
> ---
>
> Changelog:
> v1 -> v2:
> - Add a STATE_DEV_REGISTERING as Alan Stern suggested.
> v2 -> v3:
> - Adjust STATE_DEV_REGISTERING position to reflect the actual
> order in which the states occur.
> - Add the fault scenarios to comments.
> ---
> 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..241740024c50 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -145,6 +145,7 @@ enum dev_state {
> STATE_DEV_INVALID = 0,
> STATE_DEV_OPENED,
> STATE_DEV_INITIALIZED,
> + STATE_DEV_REGISTERING,
> STATE_DEV_RUNNING,
> 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)
>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
Thanks, Schspa!
Thanks for the review, Alan!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-05-09 1:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-07 12:08 [PATCH] usb: gadget: fix race when gadget driver register via ioctl Schspa Shi
2022-05-07 14:27 ` Greg KH
2022-05-07 15:06 ` Alan Stern
2022-05-07 15:50 ` Schspa Shi
2022-05-07 16:02 ` [PATCH v2] " Schspa Shi
2022-05-07 17:56 ` Alan Stern
2022-05-08 4:08 ` Schspa Shi
2022-05-08 14:34 ` Alan Stern
2022-05-08 15:02 ` [PATCH v3] " Schspa Shi
2022-05-08 23:51 ` Andrey Konovalov
2022-05-07 15:43 ` [PATCH] " Schspa Shi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).