[PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Zheng Wang]

In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.

When it calls usbip_sockfd_store, it will call v_start_timer to start the
timer work.

When we call vudc_remove to remove the driver, theremay be a sequence as
follows:

Fix it by shutdown the timer work before cleanup in vudc_remove.

Note that removing a driver is a root-only operation, and should never
happen. But the attacker can directly unplug the usb to trigger the remove
function.

CPU0                  CPU1

                     |v_timer
vudc_remove          |
kfree(udc);          |
//free shost         |
                     |udc->gadget
                     |//use

The udc might be removed before v_timer finished, and UAF happens.

This bug was found by Codeql static analysis and might by false positive.

Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
v3:
- fix the issue by adding del_timer_sync in v_stop_timer and
invoke it in vudc_remove

v2:
- add more details about how the bug was found suggested by Shuah
---
 drivers/usb/usbip/vudc_dev.c      | 1 +
 drivers/usb/usbip/vudc_transfer.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 2bc428f2e261..dcbfed30806d 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -633,6 +633,7 @@ int vudc_remove(struct platform_device *pdev)
 {
 	struct vudc *udc = platform_get_drvdata(pdev);
 
+	v_stop_timer(udc);
 	usb_del_gadget_udc(&udc->gadget);
 	cleanup_vudc_hw(udc);
 	kfree(udc);
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index 7e801fee33bf..562ea7b6ea2e 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -492,5 +492,7 @@ void v_stop_timer(struct vudc *udc)
 
 	/* timer itself will take care of stopping */
 	dev_dbg(&udc->pdev->dev, "timer stop");
+	
+	del_timer_sync(&t->timer);
 	t->state = VUDC_TR_STOPPED;
 }
-- 
2.25.1

Re: [PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Shuah Khan]

On 3/17/23 04:09, Zheng Wang wrote:
> In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.
> 
> When it calls usbip_sockfd_store, it will call v_start_timer to start the
> timer work.
> 
> When we call vudc_remove to remove the driver, theremay be a sequence as
> follows:
> 
> Fix it by shutdown the timer work before cleanup in vudc_remove.
> 
> Note that removing a driver is a root-only operation, and should never
> happen. But the attacker can directly unplug the usb to trigger the remove
> function.
> 
> CPU0                  CPU1
> 
>                       |v_timer
> vudc_remove          |
> kfree(udc);          |
> //free shost         |
>                       |udc->gadget
>                       |//use
> 
> The udc might be removed before v_timer finished, and UAF happens.
> 
> This bug was found by Codeql static analysis and might by false positive.

This statement that this could be a false positive makes me hesitate
taking this patch.

What kind of testing have you done with this fix? Were you able to test
the scenario of unplugging usb?

thanks,
-- Shuah

Re: [PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Zheng Hacker]

Shuah Khan <skhan@linuxfoundation.org> 于2023年3月18日周六 06:53写道：
>
> On 3/17/23 04:09, Zheng Wang wrote:
> > In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.
> >
> > When it calls usbip_sockfd_store, it will call v_start_timer to start the
> > timer work.
> >
> > When we call vudc_remove to remove the driver, theremay be a sequence as
> > follows:
> >
> > Fix it by shutdown the timer work before cleanup in vudc_remove.
> >
> > Note that removing a driver is a root-only operation, and should never
> > happen. But the attacker can directly unplug the usb to trigger the remove
> > function.
> >
> > CPU0                  CPU1
> >
> >                       |v_timer
> > vudc_remove          |
> > kfree(udc);          |
> > //free shost         |
> >                       |udc->gadget
> >                       |//use
> >
> > The udc might be removed before v_timer finished, and UAF happens.
> >
> > This bug was found by Codeql static analysis and might by false positive.
>
> This statement that this could be a false positive makes me hesitate
> taking this patch.
>
> What kind of testing have you done with this fix? Were you able to test
> the scenario of unplugging usb?
>

Sorry I did't make a full test for I did't have the device. The
attacking scenario if based on other cases.

Best regads,
Zheng

> thanks,
> -- Shuah

Re: [PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Zheng Hacker]

Friendly ping about the issue.
Sorry that I couldn't make test about the driver.

Thanks,
Zheng

Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月18日周六 15:39写道：
>
> Shuah Khan <skhan@linuxfoundation.org> 于2023年3月18日周六 06:53写道：
> >
> > On 3/17/23 04:09, Zheng Wang wrote:
> > > In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.
> > >
> > > When it calls usbip_sockfd_store, it will call v_start_timer to start the
> > > timer work.
> > >
> > > When we call vudc_remove to remove the driver, theremay be a sequence as
> > > follows:
> > >
> > > Fix it by shutdown the timer work before cleanup in vudc_remove.
> > >
> > > Note that removing a driver is a root-only operation, and should never
> > > happen. But the attacker can directly unplug the usb to trigger the remove
> > > function.
> > >
> > > CPU0                  CPU1
> > >
> > >                       |v_timer
> > > vudc_remove          |
> > > kfree(udc);          |
> > > //free shost         |
> > >                       |udc->gadget
> > >                       |//use
> > >
> > > The udc might be removed before v_timer finished, and UAF happens.
> > >
> > > This bug was found by Codeql static analysis and might by false positive.
> >
> > This statement that this could be a false positive makes me hesitate
> > taking this patch.
> >
> > What kind of testing have you done with this fix? Were you able to test
> > the scenario of unplugging usb?
> >
>
> Sorry I did't make a full test for I did't have the device. The
> attacking scenario if based on other cases.
>
> Best regads,
> Zheng
>
> > thanks,
> > -- Shuah

Re: [PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Shuah Khan]

On 4/13/23 02:09, Zheng Hacker wrote:
> Friendly ping about the issue.
> Sorry that I couldn't make test about the driver.
> 
> Thanks,
> Zheng
> 
> Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月18日周六 15:39写道：
>>
>> Shuah Khan <skhan@linuxfoundation.org> 于2023年3月18日周六 06:53写道：
>>>
>>> On 3/17/23 04:09, Zheng Wang wrote:
>>>> In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.
>>>>
>>>> When it calls usbip_sockfd_store, it will call v_start_timer to start the
>>>> timer work.
>>>>
>>>> When we call vudc_remove to remove the driver, theremay be a sequence as
>>>> follows:
>>>>
>>>> Fix it by shutdown the timer work before cleanup in vudc_remove.
>>>>
>>>> Note that removing a driver is a root-only operation, and should never
>>>> happen. But the attacker can directly unplug the usb to trigger the remove
>>>> function.
>>>>
>>>> CPU0                  CPU1
>>>>
>>>>                        |v_timer
>>>> vudc_remove          |
>>>> kfree(udc);          |
>>>> //free shost         |
>>>>                        |udc->gadget
>>>>                        |//use
>>>>
>>>> The udc might be removed before v_timer finished, and UAF happens.
>>>>
>>>> This bug was found by Codeql static analysis and might by false positive.
>>>
>>> This statement that this could be a false positive makes me hesitate
>>> taking this patch.
>>>
>>> What kind of testing have you done with this fix? Were you able to test
>>> the scenario of unplugging usb?
>>>
>>
>> Sorry I did't make a full test for I did't have the device. The
>> attacking scenario if based on other cases.
>>

Sorry. I really need for you test this and provide information on how
it was tested.

thanks,
-- Shuah

Re: [PATCH v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Zheng Hacker]

Hi Shuah,

I got it. I'll try it.

Best regards,
Zheng

Shuah Khan <skhan@linuxfoundation.org> 于2023年4月14日周五 02:01写道：
>
> On 4/13/23 02:09, Zheng Hacker wrote:
> > Friendly ping about the issue.
> > Sorry that I couldn't make test about the driver.
> >
> > Thanks,
> > Zheng
> >
> > Zheng Hacker <hackerzheng666@gmail.com> 于2023年3月18日周六 15:39写道：
> >>
> >> Shuah Khan <skhan@linuxfoundation.org> 于2023年3月18日周六 06:53写道：
> >>>
> >>> On 3/17/23 04:09, Zheng Wang wrote:
> >>>> In vudc_probe, it calls init_vudc_hw, which bound &udc->timer with v_timer.
> >>>>
> >>>> When it calls usbip_sockfd_store, it will call v_start_timer to start the
> >>>> timer work.
> >>>>
> >>>> When we call vudc_remove to remove the driver, theremay be a sequence as
> >>>> follows:
> >>>>
> >>>> Fix it by shutdown the timer work before cleanup in vudc_remove.
> >>>>
> >>>> Note that removing a driver is a root-only operation, and should never
> >>>> happen. But the attacker can directly unplug the usb to trigger the remove
> >>>> function.
> >>>>
> >>>> CPU0                  CPU1
> >>>>
> >>>>                        |v_timer
> >>>> vudc_remove          |
> >>>> kfree(udc);          |
> >>>> //free shost         |
> >>>>                        |udc->gadget
> >>>>                        |//use
> >>>>
> >>>> The udc might be removed before v_timer finished, and UAF happens.
> >>>>
> >>>> This bug was found by Codeql static analysis and might by false positive.
> >>>
> >>> This statement that this could be a false positive makes me hesitate
> >>> taking this patch.
> >>>
> >>> What kind of testing have you done with this fix? Were you able to test
> >>> the scenario of unplugging usb?
> >>>
> >>
> >> Sorry I did't make a full test for I did't have the device. The
> >> attacking scenario if based on other cases.
> >>
>
> Sorry. I really need for you test this and provide information on how
> it was tested.
>
> thanks,
> -- Shuah
>

[PATCH] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

[Michael Bommarito]

This patch follows up Zheng Wang's 2023 report of a use-after-free in
vudc_remove(). The original thread stalled on Shuah Khan's request for
runtime testing of the unplug/unbind path. This patch supplies that
testing and keeps Zheng's original fix shape.

In vudc_probe(), v_init_timer() binds udc->tr_timer.timer to v_timer().
usbip_sockfd_store() starts the timer via v_start_timer()/v_kick_timer().
vudc_remove() can then free the containing struct vudc while the timer is
still pending or executing.

KASAN confirms the race on an unpatched x86_64 QEMU guest with
CONFIG_KASAN=y, CONFIG_USBIP_VUDC=y, CONFIG_USB_ZERO=y, and a tight loop
that repeatedly writes a socket fd to usbip_sockfd, closes the socket
pair, and unbinds/rebinds usbip-vudc.0:

  BUG: KASAN: slab-use-after-free in __run_timer_base.part.0+0x8ba/0x8e0
  Write of size 8 at addr ffff888001b80740 by task trigger_and_unb/239
  Allocated by task 239:
    vudc_probe+0x4d/0xaa0
  Freed by task 239:
    kfree+0x18f/0x520
    device_release_driver_internal+0x388/0x540
    unbind_store+0xd9/0x100

This lands in the timer core rather than v_timer() itself because the
embedded timer_list is being walked after its containing struct vudc has
already been freed. The underlying lifetime bug is the same one Zheng
reported.

With v_stop_timer() called from vudc_remove() and the timer deleted
synchronously, the same harness completed 5000 bind/unbind iterations
with no KASAN report.

Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
Reported-by: Zheng Wang <zyytlz.wz@163.com>
Closes: https://lore.kernel.org/linux-usb/20230317100954.2626573-1-zyytlz.wz@163.com/
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/usb/usbip/vudc_dev.c      | 1 +
 drivers/usb/usbip/vudc_transfer.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 90383107b660..c5f079c5a1ea 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -632,6 +632,7 @@ void vudc_remove(struct platform_device *pdev)
 {
 	struct vudc *udc = platform_get_drvdata(pdev);
 
+	v_stop_timer(udc);
 	usb_del_gadget_udc(&udc->gadget);
 	cleanup_vudc_hw(udc);
 	kfree(udc);
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index a4f02ea3e3ef..d4ce85c4c6a2 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -490,7 +490,8 @@ void v_stop_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
-	/* timer itself will take care of stopping */
+	/* Delete the timer synchronously before teardown frees udc. */
 	dev_dbg(&udc->pdev->dev, "timer stop");
+	timer_delete_sync(&t->timer);
 	t->state = VUDC_TR_STOPPED;
 }
-- 
2.53.0