In current scenario if Plug-out and Plug-In performed continuously
there could be a chance while checking for dwc->gadget_driver in
dwc3_gadget_suspend, a NULL pointer dereference may occur.
Call Stack:
CPU1: CPU2:
gadget_unbind_driver dwc3_suspend_common
dw3_gadget_stop dwc3_gadget_suspend
dwc3_disconnect_gadget
CPU1 basically clears the variable and CPU2 checks the variable.
Consider CPU1 is running and right before gadget_driver is cleared
and in parallel CPU2 executes dwc3_gadget_suspend where it finds
dwc->gadget_driver which is not NULL and resumes execution and then
CPU1 completes execution. CPU2 executes dwc3_disconnect_gadget where
it checks dwc->gadget_driver is already NULL because of which the
NULL pointer deference occur.
Cc: <[email protected]>
Fixes: 9772b47a4c291 ("usb: dwc3: gadget: Fix suspend/resume during device mode")
Signed-off-by: Uttkarsh Aggarwal <[email protected]>
---
Changes in v2:
Added cc and fixes tag missing in v1.
Link to v1:
https://lore.kernel.org/linux-usb/[email protected]/T/#t
drivers/usb/dwc3/gadget.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 019368f8e9c4..564976b3e2b9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4709,15 +4709,13 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
unsigned long flags;
int ret;
- if (!dwc->gadget_driver)
- return 0;
-
ret = dwc3_gadget_soft_disconnect(dwc);
if (ret)
goto err;
spin_lock_irqsave(&dwc->lock, flags);
- dwc3_disconnect_gadget(dwc);
+ if (dwc->gadget_driver)
+ dwc3_disconnect_gadget(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
return 0;
--
2.17.1
Hi,
On Thu, Jan 18, 2024, Uttkarsh Aggarwal wrote:
> In current scenario if Plug-out and Plug-In performed continuously
> there could be a chance while checking for dwc->gadget_driver in
> dwc3_gadget_suspend, a NULL pointer dereference may occur.
>
> Call Stack:
>
> CPU1: CPU2:
> gadget_unbind_driver dwc3_suspend_common
> dw3_gadget_stop dwc3_gadget_suspend
> dwc3_disconnect_gadget
>
> CPU1 basically clears the variable and CPU2 checks the variable.
> Consider CPU1 is running and right before gadget_driver is cleared
> and in parallel CPU2 executes dwc3_gadget_suspend where it finds
> dwc->gadget_driver which is not NULL and resumes execution and then
> CPU1 completes execution. CPU2 executes dwc3_disconnect_gadget where
> it checks dwc->gadget_driver is already NULL because of which the
> NULL pointer deference occur.
>
> Cc: <[email protected]>
> Fixes: 9772b47a4c291 ("usb: dwc3: gadget: Fix suspend/resume during device mode")
> Signed-off-by: Uttkarsh Aggarwal <[email protected]>
> ---
>
> Changes in v2:
> Added cc and fixes tag missing in v1.
>
> Link to v1:
> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/T/*t__;Iw!!A4F2R9G_pg!cbcAuSJqsFGbN3YvHw8xlq0df192LTVjmAR0zo5fhzpkCkiFn_zCo5ms9LwVJlJa8kbHbRg6gDmZO6WSI6Vf_k7oRViUFQ$
>
> drivers/usb/dwc3/gadget.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 019368f8e9c4..564976b3e2b9 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4709,15 +4709,13 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
> unsigned long flags;
> int ret;
>
> - if (!dwc->gadget_driver)
> - return 0;
> -
> ret = dwc3_gadget_soft_disconnect(dwc);
> if (ret)
> goto err;
Just a side note: there's still a potential race where both the pullup()
from gadget unbind and the soft disconnect occur. However, functionally
I don't see a problem with it from the controller's point of view. If
both are cleaning up and halting the controller, it shouldn't be an
issue. It would be nicer to also prevent that from happening, but I
think that may need a bigger change. This small fix is sufficient to
resolve this issue.
>
> spin_lock_irqsave(&dwc->lock, flags);
> - dwc3_disconnect_gadget(dwc);
> + if (dwc->gadget_driver)
> + dwc3_disconnect_gadget(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> return 0;
> --
> 2.17.1
>
Please run checkpatch and fix warnings next time:
WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 9772b47a4c29 ("usb: dwc3: gadget: Fix suspend/resume during device mode")'
#34:
Fixes: 9772b47a4c291 ("usb: dwc3: gadget: Fix suspend/resume during device mode")
total: 0 errors, 1 warnings, 0 checks, 17 lines checked
After the fix above, you can add the Ack:
Acked-by: Thinh Nguyen <[email protected]>
Thanks,
Thinh
Hi,
> CPU1: CPU2:
> gadget_unbind_driver dwc3_suspend_common
> dw3_gadget_stop dwc3_gadget_suspend
> dwc3_disconnect_gadget
>
The typo hasn't been fixed.
dw3_gadget_stop -> dwc3_gadget_stop
Thanks,
Kuen-Han