In renesas_usb3_probe, role_work is bound with renesas_usb3_role_work.
renesas_usb3_start will be called to start the work.
If we remove the driver which will call usbhs_remove, there may be
an unfinished work. The possible sequence is as follows:
CPU0 CPU1
renesas_usb3_role_work
renesas_usb3_remove
usb_role_switch_unregister
device_unregister
kfree(sw)
//free usb3->role_sw
usb_role_switch_set_role
//use usb3->role_sw
The usb3->role_sw could be freed under such circumstance and use in usb_role_switch_set_role.
This bug was found by static analysis. And note that removing a driver is a root-only operation, and should never
happen in normal case. But the attacker can directly remove the device which will also triggering remove function.
Fix it by canceling the work before cleanup in the renesas_usb3_remove.
Fixes: 39facfa01c9f ("usb: gadget: udc: renesas_usb3: Add register of usb role switch")
Signed-off-by: Zheng Wang <[email protected]>
Reviewed-by: Yoshihiro Shimoda <[email protected]>
---
v9:
- append with more information suggested by Greg KH
v8:
- replace | with spaces to make line up suggested by Greg KH
v7:
- add more details about how the bug was found suggested by Shuah
v6:
- beautify the format and add note suggested by Greg KH
v5:
- fix typo
v4:
- add Reviewed-by label and resubmit v4 suggested by Greg KH
v3:
- modify the commit message to make it clearer suggested by Yoshihiro Shimoda
v2:
- fix typo, use clearer commit message and only cancel the UAF-related work suggested by Yoshihiro Shimoda
---
drivers/usb/gadget/udc/renesas_usb3.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index bee6bceafc4f..a301af66bd91 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -2661,6 +2661,7 @@ static int renesas_usb3_remove(struct platform_device *pdev)
debugfs_remove_recursive(usb3->dentry);
device_remove_file(&pdev->dev, &dev_attr_role);
+ cancel_work_sync(&usb3->role_work);
usb_role_switch_unregister(usb3->role_sw);
usb_del_gadget_udc(&usb3->gadget);
--
2.25.1
Hi Zheng,
> From: Zheng Wang <[email protected]>, Sent: Friday, March 17, 2023 6:55 PM
>
> In renesas_usb3_probe, role_work is bound with renesas_usb3_role_work.
> renesas_usb3_start will be called to start the work.
>
> If we remove the driver which will call usbhs_remove, there may be
> an unfinished work. The possible sequence is as follows:
>
> CPU0 CPU1
>
> renesas_usb3_role_work
> renesas_usb3_remove
> usb_role_switch_unregister
> device_unregister
> kfree(sw)
> //free usb3->role_sw
> usb_role_switch_set_role
> //use usb3->role_sw
>
> The usb3->role_sw could be freed under such circumstance and use in usb_role_switch_set_role.
The checkpatch.pl said:
---
./scripts/checkpatch.pl this.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#75:
The usb3->role_sw could be freed under such circumstance and use in usb_role_switch_set_role.
total: 0 errors, 1 warnings, 7 lines checked
---
> This bug was found by static analysis. And note that removing a driver is a root-only operation, and should never
> happen in normal case. But the attacker can directly remove the device which will also triggering remove function.
I think you should fix them about 75 chars per line) too.
And, I don't know why "attacker" is related to this issue.
I think "the root user" is better than "attacker".
Best regards,
Yoshihiro Shimoda
> Fix it by canceling the work before cleanup in the renesas_usb3_remove.
>
> Fixes: 39facfa01c9f ("usb: gadget: udc: renesas_usb3: Add register of usb role switch")
> Signed-off-by: Zheng Wang <[email protected]>
> Reviewed-by: Yoshihiro Shimoda <[email protected]>
> ---
> v9:
> - append with more information suggested by Greg KH
> v8:
> - replace | with spaces to make line up suggested by Greg KH
> v7:
> - add more details about how the bug was found suggested by Shuah
> v6:
> - beautify the format and add note suggested by Greg KH
> v5:
> - fix typo
> v4:
> - add Reviewed-by label and resubmit v4 suggested by Greg KH
> v3:
> - modify the commit message to make it clearer suggested by Yoshihiro Shimoda
> v2:
> - fix typo, use clearer commit message and only cancel the UAF-related work suggested by Yoshihiro Shimoda
> ---
> drivers/usb/gadget/udc/renesas_usb3.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> index bee6bceafc4f..a301af66bd91 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -2661,6 +2661,7 @@ static int renesas_usb3_remove(struct platform_device *pdev)
> debugfs_remove_recursive(usb3->dentry);
> device_remove_file(&pdev->dev, &dev_attr_role);
>
> + cancel_work_sync(&usb3->role_work);
> usb_role_switch_unregister(usb3->role_sw);
>
> usb_del_gadget_udc(&usb3->gadget);
> --
> 2.25.1
Yoshihiro Shimoda <[email protected]> 于2023年3月17日周五 20:19写道：
>
> Hi Zheng,
>
> > From: Zheng Wang <[email protected]>, Sent: Friday, March 17, 2023 6:55 PM
> >
> > In renesas_usb3_probe, role_work is bound with renesas_usb3_role_work.
> > renesas_usb3_start will be called to start the work.
> >
> > If we remove the driver which will call usbhs_remove, there may be
> > an unfinished work. The possible sequence is as follows:
> >
> > CPU0 CPU1
> >
> > renesas_usb3_role_work
> > renesas_usb3_remove
> > usb_role_switch_unregister
> > device_unregister
> > kfree(sw)
> > //free usb3->role_sw
> > usb_role_switch_set_role
> > //use usb3->role_sw
> >
> > The usb3->role_sw could be freed under such circumstance and use in usb_role_switch_set_role.
>
> The checkpatch.pl said:
> ---
> ./scripts/checkpatch.pl this.patch
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #75:
> The usb3->role_sw could be freed under such circumstance and use in usb_role_switch_set_role.
>
> total: 0 errors, 1 warnings, 7 lines checked
> ---
>
> > This bug was found by static analysis. And note that removing a driver is a root-only operation, and should never
> > happen in normal case. But the attacker can directly remove the device which will also triggering remove function.
>
> I think you should fix them about 75 chars per line) too.
>
> And, I don't know why "attacker" is related to this issue.
> I think "the root user" is better than "attacker".
>
Thanks for your detailed check and advice. I'll apply it in the next verion.
Best regards,
Zheng
> Best regards,
> Yoshihiro Shimoda
>
> > Fix it by canceling the work before cleanup in the renesas_usb3_remove.
> >
> > Fixes: 39facfa01c9f ("usb: gadget: udc: renesas_usb3: Add register of usb role switch")
> > Signed-off-by: Zheng Wang <[email protected]>
> > Reviewed-by: Yoshihiro Shimoda <[email protected]>
> > ---
> > v9:
> > - append with more information suggested by Greg KH
> > v8:
> > - replace | with spaces to make line up suggested by Greg KH
> > v7:
> > - add more details about how the bug was found suggested by Shuah
> > v6:
> > - beautify the format and add note suggested by Greg KH
> > v5:
> > - fix typo
> > v4:
> > - add Reviewed-by label and resubmit v4 suggested by Greg KH
> > v3:
> > - modify the commit message to make it clearer suggested by Yoshihiro Shimoda
> > v2:
> > - fix typo, use clearer commit message and only cancel the UAF-related work suggested by Yoshihiro Shimoda
> > ---
> > drivers/usb/gadget/udc/renesas_usb3.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
> > index bee6bceafc4f..a301af66bd91 100644
> > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> > @@ -2661,6 +2661,7 @@ static int renesas_usb3_remove(struct platform_device *pdev)
> > debugfs_remove_recursive(usb3->dentry);
> > device_remove_file(&pdev->dev, &dev_attr_role);
> >
> > + cancel_work_sync(&usb3->role_work);
> > usb_role_switch_unregister(usb3->role_sw);
> >
> > usb_del_gadget_udc(&usb3->gadget);
> > --
> > 2.25.1
>