[PATCH] usb: gadget: fix race when gadget driver register via ioctl

[PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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

Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Greg KH]

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

Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Alan Stern]

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

Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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

Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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

[PATCH v2] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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)

Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl

[Alan Stern]

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)
> 

Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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

Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl

[Alan Stern]

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

[PATCH v3] usb: gadget: fix race when gadget driver register via ioctl

[Schspa Shi]

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)

Re: [PATCH v3] usb: gadget: fix race when gadget driver register via ioctl

[Andrey Konovalov]

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!