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

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月17日周五 01:57写道：
>
> On 3/16/23 11:44, 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:
> >
>
> When you resend the patch as you indicated would, please add details
> on how you found this proble,m.
>

Get it, I found it by static analysis. So this might be false postive.

Best regards,
Zheng

> > Fix it by shutdown the timer work before cleanup in vudc_remove.
> >
>
> thanks,
> -- Shuah
>

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

[Shuah Khan]

On 3/16/23 11:44, 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:
> 

When you resend the patch as you indicated would, please add details
on how you found this proble,m.

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

thanks,
-- Shuah

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

[Zheng Hacker]

Sorry this is not v3 but v1, I will resubmit v1 again. Sorry again for
my mistake.

Thanks,
Zheng

Zheng Wang <zyytlz.wz@163.com> 于2023年3月17日周五 01:44写道：
>
> 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.
>
> CPU0                  CPU1
>
>                      |v_timer
> vudc_remove          |
> kfree(udc);          |
> //free shost         |
>                      |udc->gadget
>                      |//use
>
> Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
>  drivers/usb/usbip/vudc_dev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 2bc428f2e261..33d0991755bb 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);
>
> +       timer_shutdown_sync(&udc->timer);
>         usb_del_gadget_udc(&udc->gadget);
>         cleanup_vudc_hw(udc);
>         kfree(udc);
> --
> 2.25.1
>

[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.

CPU0                  CPU1

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

Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
 drivers/usb/usbip/vudc_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 2bc428f2e261..33d0991755bb 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);
 
+	timer_shutdown_sync(&udc->timer);
 	usb_del_gadget_udc(&udc->gadget);
 	cleanup_vudc_hw(udc);
 	kfree(udc);
-- 
2.25.1