Re: [PATCH v2] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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: <stable@vger.kernel.org>
> Fixes: 9772b47a4c291 ("usb: dwc3: gadget: Fix suspend/resume during device mode")
> Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.com>
> ---
> 
> Changes in v2:
> Added cc and fixes tag missing in v1.
> 
> Link to v1:
> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0ef3fb11-a207-2db4-1714-b3bca2ce2cea@quicinc.com/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 <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh