Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220507120851.29948-1-schspa@gmail.com> <YnaBwkhIxZ1wtIQX@kroah.com>
In-Reply-To: <YnaBwkhIxZ1wtIQX@kroah.com>
From:   Schspa Shi <schspa@gmail.com>
Date:   Sat, 7 May 2022 23:43:41 +0800
Message-ID: <CAMA88TpXqRj+nhH8ty_FJqoe_yE12GvCHkL5aC+YPjHBB6=RAg@mail.gmail.com>
Subject: Re: [PATCH] usb: gadget: fix race when gadget driver register via ioctl
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     andreyknvl@gmail.com, balbi@kernel.org, jj251510319013@gmail.com,
        stern@rowland.harvard.edu, jannh@google.com, Julia.Lawall@inria.fr,
        linux-usb@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        syzbot+dc7c3ca638e773db07f6@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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