drivers/usb/dwc3/gadget.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
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> 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
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
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);
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
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
> 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
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
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
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
© 2016 - 2025 Red Hat, Inc.