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 <[email protected]>
---
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
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
Shuah Khan <[email protected]> 于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
Friendly ping about the issue.
Sorry that I couldn't make test about the driver.
Thanks,
Zheng
Zheng Hacker <[email protected]> 于2023年3月18日周六 15:39写道：
>
> Shuah Khan <[email protected]> 于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
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 <[email protected]> 于2023年3月18日周六 15:39写道：
>>
>> Shuah Khan <[email protected]> 于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
Hi Shuah,
I got it. I'll try it.
Best regards,
Zheng
Shuah Khan <[email protected]> 于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 <[email protected]> 于2023年3月18日周六 15:39写道：
> >>
> >> Shuah Khan <[email protected]> 于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
>