Alan Stern <[email protected]> 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
On Sun, May 08, 2022 at 12:08:35PM +0800, Schspa Shi wrote:
> Alan Stern <[email protected]> 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 <[email protected]>
Alan Stern
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: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Schspa Shi <[email protected]>
---
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)
On Sun, May 8, 2022 at 5:03 PM Schspa Shi <[email protected]> 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: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Schspa Shi <[email protected]>
>
> ---
>
> 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 <[email protected]>
Thanks, Schspa!
Thanks for the review, Alan!