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

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

[Uttkarsh Aggarwal]

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.

To prevent this, moving NULL pointer check inside of spin_lock.

Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.com>
---
 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

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

[Thinh Nguyen]

Hi,

On Wed, Jan 10, 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.
> 
> To prevent this, moving NULL pointer check inside of spin_lock.
> 
> Signed-off-by: Uttkarsh Aggarwal <quic_uaggarwa@quicinc.com>
> ---
>  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
> 

Do you have the dmesg log of this NULL pointer dereference?

Thanks,
Thinh

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

[UTTKARSH AGGARWAL]

On 1/17/2024 6:49 AM, Thinh Nguyen wrote:
> Do you have the dmesg log of this NULL pointer dereference?
> Thanks,
> Thinh

Hi Thinh,

Here is the dmesg log:

[  149.524338][  T843] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000028
…
[  149.525872][  T843] Workqueue: pm pm_runtime_work
[  149.525886][  T843] pstate: 824000c5 (Nzcv daIF +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
[  149.525893][  T843] pc : dwc3_gadget_suspend+0x4c/0xb8
[  149.525900][  T843] lr : dwc3_gadget_suspend+0x34/0xb8
…
[  149.526003][  T843] Call trace:
[  149.526008][  T843]  dwc3_gadget_suspend+0x4c/0xb8
[  149.526015][  T843]  dwc3_suspend_common+0x58/0x230
[  149.526021][  T843]  dwc3_runtime_suspend+0x34/0x50
[  149.526027][  T843]  pm_generic_runtime_suspend+0x40/0x58
[  149.526034][  T843]  __rpm_callback+0x94/0x3e0
[  149.526040][  T843]  rpm_suspend+0x2e4/0x720
[  149.526047][  T843]  __pm_runtime_suspend+0x6c/0x100
[  149.526054][  T843]  dwc3_runtime_idle+0x48/0x64
[  149.526060][  T843]  rpm_idle+0x20c/0x310
[  149.526067][  T843]  pm_runtime_work+0x80/0xac
[  149.526075][  T843]  process_one_work+0x1e4/0x43c
[  149.526084][  T843]  worker_thread+0x25c/0x430
[  149.526091][  T843]  kthread+0x104/0x1d4
[  149.526099][  T843]  ret_from_fork+0x10/0x20
  
=======================================================
Process: adbd, [affinity: 0xff] cpu: 6 pid: 4907 start: 0xffffff888079b840
=====================================================
    Task name: adbd [affinity: 0xff] pid: 4907 cpu: 6 prio: 120 start: ffffff888079b840
    state: 0x2[D] exit_state: 0x0 stack base: 0xffffffc02db20000
    Last_enqueued_ts:     149.523808841 Last_sleep_ts:     149.523859362
    Stack:
    [<ffffffc0091cd558>] __switch_to+0x174
    [<ffffffc0091cdd40>] __schedule+0x5ec
    [<ffffffc0091ce19c>] schedule+0x7c
    [<ffffffc0091d7438>] schedule_timeout+0x44
    [<ffffffc0091ce858>] wait_for_common+0xd8
    [<ffffffc0091ce774>] wait_for_completion+0x18
    [<ffffffc0082b23dc>] kthread_stop+0x78
    [<ffffffc0083134a0>] free_irq+0x184
    [<ffffffc008bc7438>] dwc3_gadget_stop+0x40
    [<ffffffc008c12228>] gadget_unbind_driver+0xfc
    [<ffffffc008ab76ac>] device_release_driver_internal[jt]+0x1d4
    [<ffffffc008ab78dc>] driver_detach+0x90
    [<ffffffc008ab519c>] bus_remove_driver+0x78
    [<ffffffc008ab9170>] driver_unregister[jt]+0x44
    [<ffffffc008c11838>] usb_gadget_unregister_driver+0x20
    [<ffffffc008c0c1e0>] unregister_gadget_item+0x30
    [<ffffffc008c256a8>] ffs_data_clear[jt]+0x88

Thanks,

Uttkarsh

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

[Kuen-Han Tsai]

Hi Uttkarsh and Thinh,

>> Call Stack:
>> CPU1:                             CPU2:
>> gadget_unbind_driver     dwc3_suspend_common
>> dw3_gadget_stop           dwc3_gadget_suspend
>>                                        dwc3_disconnect_gadget

typo: dw3_gadget_stop/dwc3_gadget_stop

> Do you have the dmesg log of this NULL pointer dereference?
> Thanks,
> Thinh

We also encountered similar stack traces.

[    5.130593][  T100] Unable to handle kernel NULL pointer
dereference at virtual address 0000000000000028
[    5.130912][  T100] Call trace:
[    5.130914][  T100]  dwc3_gadget_suspend+0x88/0xf0
[    5.130918][  T100]  dwc3_suspend_common+0x58/0x230
[    5.130921][  T100]  dwc3_runtime_suspend+0x34/0x50
[    5.130925][  T100]  pm_generic_runtime_suspend+0x40/0x58
[    5.130928][  T100]  __rpm_callback+0x94/0x3e0
[    5.130931][  T100]  rpm_suspend+0x2e4/0x720
[    5.130934][  T100]  __pm_runtime_suspend+0x6c/0x100
[    5.130937][  T100]  dwc3_runtime_idle+0x48/0x64
[    5.130941][  T100]  rpm_idle+0x20c/0x310
[    5.130944][  T100]  pm_runtime_work+0x80/0xac
[    5.130947][  T100]  process_one_work+0x1e4/0x43c
[    5.130952][  T100]  worker_thread+0x25c/0x430
[    5.130956][  T100]  kthread+0x104/0x1d4
[    5.130959][  T100]  ret_from_fork+0x10/0x20

(gdb) list *dwc3_gadget_suspend+0x88
0xffffffc0089b2318 is in dwc3_gadget_suspend (drivers/usb/dwc3/gadget.c:3729).
...
3729            if (dwc->async_callbacks && dwc->gadget_driver->disconnect) {
...

Thanks,
Kuen-Han

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

[Kuen-Han Tsai]

>         ret = dwc3_gadget_soft_disconnect(dwc);
>         if (ret)
>                 goto err;

For improved readability, we can remove the goto statement and move
the error handling logic here.

Thanks,
Kuen-Han

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

[UTTKARSH AGGARWAL]

On 1/17/2024 12:47 PM, Kuen-Han Tsai wrote:
>>          ret = dwc3_gadget_soft_disconnect(dwc);
>>          if (ret)
>>                  goto err;
> For improved readability, we can remove the goto statement and move
> the error handling logic here.

Hi Kuen-Han,

Thanks for the suggestion.
Does this looks good to you ?

    int ret = dwc3_gadget_soft_disconnect(dwc);    if (ret) {        if 
(dwc->softconnect)            dwc3_gadget_soft_connect(dwc);

        return ret;    }    spin_lock_irqsave(&dwc->lock, flags);    if 
(dwc->gadget_driver)        dwc3_disconnect_gadget(dwc);   
  spin_unlock_irqrestore(&dwc->lock, flags);

Thanks,

Uttkarsh

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

[UTTKARSH AGGARWAL]

On 1/17/2024 2:52 PM, UTTKARSH AGGARWAL wrote:
>
> On 1/17/2024 12:47 PM, Kuen-Han Tsai wrote:
>>>          ret = dwc3_gadget_soft_disconnect(dwc);
>>>          if (ret)
>>>                  goto err;
>> For improved readability, we can remove the goto statement and move
>> the error handling logic here.
>
> Hi Kuen-Han,
>
> Thanks for the suggestion.
> Does this looks good to you ?
>
>    int ret = dwc3_gadget_soft_disconnect(dwc);if (ret) {        if 
> (dwc->softconnect)            dwc3_gadget_soft_connect(dwc);
>
>        return ret;    }    spin_lock_irqsave(&dwc->lock, flags);    if 
> (dwc->gadget_driver)  dwc3_disconnect_gadget(dwc); 
>  spin_unlock_irqrestore(&dwc->lock, flags);

Sorry for the mistake.

int ret = dwc3_gadget_soft_disconnect(dwc);

if (ret) {

       if (dwc->softconnect)

                  dwc3_gadget_soft_connect(dwc);

       return ret;

}

spin_lock_irqsave(&dwc->lock, flags);

if (dwc->gadget_driver)

        dwc3_disconnect_gadget(dwc);

spin_unlock_irqrestore(&dwc->lock, flags);

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

[Kuen-Han Tsai]

> int ret = dwc3_gadget_soft_disconnect(dwc);

I'm not sure if the coding style in this line, where the declaration
and assignment of a variable are combined, is considered good
practice.
The other parts look good to me.

Thanks,
Kuen-Han

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

[Thinh Nguyen]

On Wed, Jan 17, 2024, UTTKARSH AGGARWAL wrote:
> 
> On 1/17/2024 2:52 PM, UTTKARSH AGGARWAL wrote:
> > 
> > On 1/17/2024 12:47 PM, Kuen-Han Tsai wrote:
> > > >          ret = dwc3_gadget_soft_disconnect(dwc);
> > > >          if (ret)
> > > >                  goto err;
> > > For improved readability, we can remove the goto statement and move
> > > the error handling logic here.
> > 
> > Hi Kuen-Han,
> > 
> > Thanks for the suggestion.
> > Does this looks good to you ?
> > 
> >    int ret = dwc3_gadget_soft_disconnect(dwc);if (ret) {        if
> > (dwc->softconnect)            dwc3_gadget_soft_connect(dwc);
> > 
> >        return ret;    }    spin_lock_irqsave(&dwc->lock, flags);    if
> > (dwc->gadget_driver)  dwc3_disconnect_gadget(dwc);
> >  spin_unlock_irqrestore(&dwc->lock, flags);
> 
> Sorry for the mistake.
> 
> int ret = dwc3_gadget_soft_disconnect(dwc);
> 
> if (ret) {
> 
>       if (dwc->softconnect)
> 
>                  dwc3_gadget_soft_connect(dwc);
> 
>       return ret;
> 
> }
> 
> spin_lock_irqsave(&dwc->lock, flags);
> 
> if (dwc->gadget_driver)
> 
>        dwc3_disconnect_gadget(dwc);
> 
> spin_unlock_irqrestore(&dwc->lock, flags);
> 

Please only make one logical fix per change. If any unrelated refactor
or style change is needed, keep it to a separate commit.

Thanks,
Thinh

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

[UTTKARSH AGGARWAL]

On 1/18/2024 6:26 AM, Thinh Nguyen wrote:
> On Wed, Jan 17, 2024, UTTKARSH AGGARWAL wrote:
>> On 1/17/2024 2:52 PM, UTTKARSH AGGARWAL wrote:
>>> On 1/17/2024 12:47 PM, Kuen-Han Tsai wrote:
>>>>>           ret = dwc3_gadget_soft_disconnect(dwc);
>>>>>           if (ret)
>>>>>                   goto err;
>>>> For improved readability, we can remove the goto statement and move
>>>> the error handling logic here.
>>> Hi Kuen-Han,
>>>
>>> Thanks for the suggestion.
>>> Does this looks good to you ?
>>>
>>>     int ret = dwc3_gadget_soft_disconnect(dwc);if (ret) {        if
>>> (dwc->softconnect)            dwc3_gadget_soft_connect(dwc);
>>>
>>>         return ret;    }    spin_lock_irqsave(&dwc->lock, flags);    if
>>> (dwc->gadget_driver)  dwc3_disconnect_gadget(dwc);
>>>   spin_unlock_irqrestore(&dwc->lock, flags);
>> Sorry for the mistake.
>>
>> int ret = dwc3_gadget_soft_disconnect(dwc);
>>
>> if (ret) {
>>
>>        if (dwc->softconnect)
>>
>>                   dwc3_gadget_soft_connect(dwc);
>>
>>        return ret;
>>
>> }
>>
>> spin_lock_irqsave(&dwc->lock, flags);
>>
>> if (dwc->gadget_driver)
>>
>>         dwc3_disconnect_gadget(dwc);
>>
>> spin_unlock_irqrestore(&dwc->lock, flags);
>>
> Please only make one logical fix per change. If any unrelated refactor
> or style change is needed, keep it to a separate commit.
>
> Thanks,
> Thinh

Sure Thinh,I’ll only push fix in v2, not refactoring.

Thanks,

Uttkarsh