| Message ID | 20220507160243.35304-1-schspa@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: gadget: fix race when gadget driver register via ioctl | expand |
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) >
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
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
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!
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);
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(+)