[PATCH 0/2] Fix use after freed situation in UDC core

[PATCH 0/2] Fix use after freed situation in UDC core

[Wesley Cheng]

commit 1ff24d40b3c3 ("usb: dwc3: gadget: Fix incorrect UDC state after
manual deconfiguration")

Added a use after freed situation due to the fact that the
usb_gadget_set_state() API queuing asynchronous work to be run at a later
time.  When moving from device to host mode, the UDC is removed, and this
is the condition where this is seen.  Relocate this fix from the DWC3
gadget, and apply it within the UDC core, so that we can properly manage
the gadget->work.  It also makes sense, as during soft disconnection
sequence, the USB link is unavailable, so any UDC should reflect the
proper state.

Wesley Cheng (2):
  Revert "usb: dwc3: gadget: Fix incorrect UDC state after manual
    deconfiguration"
  usb: gadget: udc: Update USB gadget state during soft disconnect

 drivers/usb/dwc3/gadget.c     | 2 --
 drivers/usb/gadget/udc/core.c | 5 ++++-
 2 files changed, 4 insertions(+), 3 deletions(-)

[PATCH 1/2] Revert "usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration"

[Wesley Cheng]

This reverts commit 1ff24d40b3c3c673d833c546f898133b80dffc39.

The following change creates a potential use after freed scenario within
the UDC core level.  When transitioning from peripheral to host mode, the
DWC3 DRD will remove the USB UDC using usb_del_gadget().  As part of the
UDC removal routine, the UDC core will issue a pullup disable call back to
the DWC3 gadget driver, which will now call usb_gadget_set_state(), and
queues gadget->work.  If gadget->work is unable to complete before the USB
gadget is freed, then we will run into the use after freed situation.

Below is the callstack in question:

dwc3_gadget_exit()
 --> usb_del_gadget()
 --> gadget_unbind_driver()
  --> device_del()
   --> gadget_unbind_driver()
    --> usb_gadget_disconnect_locked()
     --> pullup(0)
      --> dwc3_gadget_soft_disconnect()
       --> usb_gadget_set_state(UNATTACHED)
        --> queue_work(gadget->work) --> async
  --> device_unregister()

Unfortunately, the flush_work() call within usb_del_gadget() doesn't work
in this situation, as it happens before the device_del() and soft
disconnect sequence.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5156fafbda6c..921f72995dbf 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2781,8 +2781,6 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
 	__dwc3_gadget_stop(dwc);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
-
 	return ret;
 }
 

Re: [PATCH 1/2] Revert "usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration"

[Thinh Nguyen]

Hi,

On Fri, Feb 28, 2025, Wesley Cheng wrote:
> This reverts commit 1ff24d40b3c3c673d833c546f898133b80dffc39.
> 
> The following change creates a potential use after freed scenario within
> the UDC core level.  When transitioning from peripheral to host mode, the
> DWC3 DRD will remove the USB UDC using usb_del_gadget().  As part of the
> UDC removal routine, the UDC core will issue a pullup disable call back to
> the DWC3 gadget driver, which will now call usb_gadget_set_state(), and
> queues gadget->work.  If gadget->work is unable to complete before the USB
> gadget is freed, then we will run into the use after freed situation.
> 
> Below is the callstack in question:
> 
> dwc3_gadget_exit()
>  --> usb_del_gadget()
>  --> gadget_unbind_driver()
>   --> device_del()
>    --> gadget_unbind_driver()
>     --> usb_gadget_disconnect_locked()
>      --> pullup(0)
>       --> dwc3_gadget_soft_disconnect()
>        --> usb_gadget_set_state(UNATTACHED)
>         --> queue_work(gadget->work) --> async
>   --> device_unregister()
> 
> Unfortunately, the flush_work() call within usb_del_gadget() doesn't work
> in this situation, as it happens before the device_del() and soft
> disconnect sequence.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5156fafbda6c..921f72995dbf 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2781,8 +2781,6 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
>  	__dwc3_gadget_stop(dwc);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> -	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
> -
>  	return ret;
>  }
>  

Have you tried with this fix?
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=399a45e5237ca14037120b1b895bd38a3b4492ea

BR,
Thinh

Re: [PATCH 1/2] Revert "usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration"

[Wesley Cheng]

Hi Thinh,

On 2/28/2025 6:01 PM, Thinh Nguyen wrote:
> Hi,
> 
> On Fri, Feb 28, 2025, Wesley Cheng wrote:
>> This reverts commit 1ff24d40b3c3c673d833c546f898133b80dffc39.
>>
>> The following change creates a potential use after freed scenario within
>> the UDC core level.  When transitioning from peripheral to host mode, the
>> DWC3 DRD will remove the USB UDC using usb_del_gadget().  As part of the
>> UDC removal routine, the UDC core will issue a pullup disable call back to
>> the DWC3 gadget driver, which will now call usb_gadget_set_state(), and
>> queues gadget->work.  If gadget->work is unable to complete before the USB
>> gadget is freed, then we will run into the use after freed situation.
>>
>> Below is the callstack in question:
>>
>> dwc3_gadget_exit()
>>  --> usb_del_gadget()
>>  --> gadget_unbind_driver()
>>   --> device_del()
>>    --> gadget_unbind_driver()
>>     --> usb_gadget_disconnect_locked()
>>      --> pullup(0)
>>       --> dwc3_gadget_soft_disconnect()
>>        --> usb_gadget_set_state(UNATTACHED)
>>         --> queue_work(gadget->work) --> async
>>   --> device_unregister()
>>
>> Unfortunately, the flush_work() call within usb_del_gadget() doesn't work
>> in this situation, as it happens before the device_del() and soft
>> disconnect sequence.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>  drivers/usb/dwc3/gadget.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5156fafbda6c..921f72995dbf 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2781,8 +2781,6 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
>>  	__dwc3_gadget_stop(dwc);
>>  	spin_unlock_irqrestore(&dwc->lock, flags);
>>  
>> -	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>> -
>>  	return ret;
>>  }
>>  
> 
> Have you tried with this fix?
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=399a45e5237ca14037120b1b895bd38a3b4492ea
> 

Thanks for pointing that out.  Looks like its working fine for me.  Bit odd
that we're still potentially utilizing the usb gadget after calling
device_del(), but I guess that should be fine.

Thanks
Wesley Cheng

Re: [PATCH 1/2] Revert "usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration"

[Thinh Nguyen]

On Fri, Feb 28, 2025, Wesley Cheng wrote:
> Hi Thinh,
> 
> On 2/28/2025 6:01 PM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Fri, Feb 28, 2025, Wesley Cheng wrote:
> >> This reverts commit 1ff24d40b3c3c673d833c546f898133b80dffc39.
> >>
> >> The following change creates a potential use after freed scenario within
> >> the UDC core level.  When transitioning from peripheral to host mode, the
> >> DWC3 DRD will remove the USB UDC using usb_del_gadget().  As part of the
> >> UDC removal routine, the UDC core will issue a pullup disable call back to
> >> the DWC3 gadget driver, which will now call usb_gadget_set_state(), and
> >> queues gadget->work.  If gadget->work is unable to complete before the USB
> >> gadget is freed, then we will run into the use after freed situation.
> >>
> >> Below is the callstack in question:
> >>
> >> dwc3_gadget_exit()
> >>  --> usb_del_gadget()
> >>  --> gadget_unbind_driver()
> >>   --> device_del()
> >>    --> gadget_unbind_driver()
> >>     --> usb_gadget_disconnect_locked()
> >>      --> pullup(0)
> >>       --> dwc3_gadget_soft_disconnect()
> >>        --> usb_gadget_set_state(UNATTACHED)
> >>         --> queue_work(gadget->work) --> async
> >>   --> device_unregister()
> >>
> >> Unfortunately, the flush_work() call within usb_del_gadget() doesn't work
> >> in this situation, as it happens before the device_del() and soft
> >> disconnect sequence.
> >>
> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> >> ---
> >>  drivers/usb/dwc3/gadget.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> index 5156fafbda6c..921f72995dbf 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -2781,8 +2781,6 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> >>  	__dwc3_gadget_stop(dwc);
> >>  	spin_unlock_irqrestore(&dwc->lock, flags);
> >>  
> >> -	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
> >> -
> >>  	return ret;
> >>  }
> >>  
> > 
> > Have you tried with this fix?
> > https://urldefense.com/v3/__https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=399a45e5237ca14037120b1b895bd38a3b4492ea__;!!A4F2R9G_pg!c0-XJOQST8G_jMPRaU0NxsP4zRPkygId8mQEsUGwxzQEsz2XuX4JsnesMTuaGuyVgN7hNmYJ3gg-cUeowVZaG_m7DP4$ 
> > 
> 
> Thanks for pointing that out.  Looks like its working fine for me.  Bit odd
> that we're still potentially utilizing the usb gadget after calling
> device_del(), but I guess that should be fine.
> 

Yeah, but device_del() isn't usb_del_gadget(). We just need to make sure
the usb gadget isn't used after usb_del_gadget().

BR,
Thinh

[PATCH 2/2] usb: gadget: udc: Update USB gadget state during soft disconnect

[Wesley Cheng]

In the soft disconnect scenarios, the USB connection will be lost
momentarily, so the proper gadget state should be reflected during the time
the connection is unavailable.  Add a flush_work() call, to ensure that
gadget->work is completed before continuing with the UDC unbind sequence.
Since usb_gadget_set_state() queues work to a workqueue, depending on when
the queue is scheduled, this avoids a possible use after freed scenario as
the USB gadget will most likely be freed shortly after the UDC driver is
unbounded.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/gadget/udc/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4b3d5075621a..7e401cb5a265 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -775,8 +775,10 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
 	}
 
 	ret = gadget->ops->pullup(gadget, 0);
-	if (!ret)
+	if (!ret) {
 		gadget->connected = 0;
+		usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+	}
 
 	mutex_lock(&udc_lock);
 	if (gadget->udc->driver)
@@ -1669,6 +1671,7 @@ static void gadget_unbind_driver(struct device *dev)
 		synchronize_irq(gadget->irq);
 	mutex_unlock(&udc->connect_lock);
 
+	flush_work(&gadget->work);
 	udc->driver->unbind(gadget);
 
 	mutex_lock(&udc->connect_lock);