| Message ID | 20230317100954.2626573-1-zyytlz.wz@163.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usbip: vudc: Fix use after free bug in vudc_remove due to race condition | expand |
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
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
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
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 >
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; }
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(+)