* [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
@ 2024-01-10 9:55 Uttkarsh Aggarwal
2024-01-17 1:19 ` Thinh Nguyen
2024-01-17 7:17 ` Kuen-Han Tsai
0 siblings, 2 replies; 10+ messages in thread
From: Uttkarsh Aggarwal @ 2024-01-10 9:55 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman
Cc: linux-kernel, linux-usb, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-10 9:55 [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend Uttkarsh Aggarwal
@ 2024-01-17 1:19 ` Thinh Nguyen
2024-01-17 6:36 ` UTTKARSH AGGARWAL
2024-01-17 7:17 ` Kuen-Han Tsai
1 sibling, 1 reply; 10+ messages in thread
From: Thinh Nguyen @ 2024-01-17 1:19 UTC (permalink / raw)
To: Uttkarsh Aggarwal
Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 1:19 ` Thinh Nguyen
@ 2024-01-17 6:36 ` UTTKARSH AGGARWAL
2024-01-17 6:49 ` Kuen-Han Tsai
0 siblings, 1 reply; 10+ messages in thread
From: UTTKARSH AGGARWAL @ 2024-01-17 6:36 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 6:36 ` UTTKARSH AGGARWAL
@ 2024-01-17 6:49 ` Kuen-Han Tsai
0 siblings, 0 replies; 10+ messages in thread
From: Kuen-Han Tsai @ 2024-01-17 6:49 UTC (permalink / raw)
To: UTTKARSH AGGARWAL
Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-10 9:55 [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend Uttkarsh Aggarwal
2024-01-17 1:19 ` Thinh Nguyen
@ 2024-01-17 7:17 ` Kuen-Han Tsai
2024-01-17 9:22 ` UTTKARSH AGGARWAL
1 sibling, 1 reply; 10+ messages in thread
From: Kuen-Han Tsai @ 2024-01-17 7:17 UTC (permalink / raw)
To: Uttkarsh Aggarwal
Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 7:17 ` Kuen-Han Tsai
@ 2024-01-17 9:22 ` UTTKARSH AGGARWAL
2024-01-17 9:35 ` UTTKARSH AGGARWAL
0 siblings, 1 reply; 10+ messages in thread
From: UTTKARSH AGGARWAL @ 2024-01-17 9:22 UTC (permalink / raw)
To: Kuen-Han Tsai; +Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 9:22 ` UTTKARSH AGGARWAL
@ 2024-01-17 9:35 ` UTTKARSH AGGARWAL
2024-01-17 12:16 ` Kuen-Han Tsai
2024-01-18 0:56 ` Thinh Nguyen
0 siblings, 2 replies; 10+ messages in thread
From: UTTKARSH AGGARWAL @ 2024-01-17 9:35 UTC (permalink / raw)
To: Kuen-Han Tsai; +Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 9:35 ` UTTKARSH AGGARWAL
@ 2024-01-17 12:16 ` Kuen-Han Tsai
2024-01-18 0:56 ` Thinh Nguyen
1 sibling, 0 replies; 10+ messages in thread
From: Kuen-Han Tsai @ 2024-01-17 12:16 UTC (permalink / raw)
To: UTTKARSH AGGARWAL
Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-17 9:35 ` UTTKARSH AGGARWAL
2024-01-17 12:16 ` Kuen-Han Tsai
@ 2024-01-18 0:56 ` Thinh Nguyen
2024-01-18 8:46 ` UTTKARSH AGGARWAL
1 sibling, 1 reply; 10+ messages in thread
From: Thinh Nguyen @ 2024-01-18 0:56 UTC (permalink / raw)
To: UTTKARSH AGGARWAL
Cc: Kuen-Han Tsai, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend
2024-01-18 0:56 ` Thinh Nguyen
@ 2024-01-18 8:46 ` UTTKARSH AGGARWAL
0 siblings, 0 replies; 10+ messages in thread
From: UTTKARSH AGGARWAL @ 2024-01-18 8:46 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: Kuen-Han Tsai, Greg Kroah-Hartman, linux-kernel, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-01-18 8:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-10 9:55 [RFC PATCH] usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend Uttkarsh Aggarwal
2024-01-17 1:19 ` Thinh Nguyen
2024-01-17 6:36 ` UTTKARSH AGGARWAL
2024-01-17 6:49 ` Kuen-Han Tsai
2024-01-17 7:17 ` Kuen-Han Tsai
2024-01-17 9:22 ` UTTKARSH AGGARWAL
2024-01-17 9:35 ` UTTKARSH AGGARWAL
2024-01-17 12:16 ` Kuen-Han Tsai
2024-01-18 0:56 ` Thinh Nguyen
2024-01-18 8:46 ` UTTKARSH AGGARWAL
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).