`dwc3_gadget_soft_disconnect` function, called as part of
`device_del(&gadget->dev)`, queues a new work item to the gadget workqueue
after the workqueue has been flushed in `usb_del_gadget`.
This leads to a potential use-after-free issue.

To fix this, flush the workqueue in the `release` function before freeing
the gadget. This ensures that all work items are processed before the
gadget is destroyed.

Fixes: 1ff24d40b3c3 ("usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration")
Signed-off-by: Roy Luo <royluo@google.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d27af65eb08a..12055e3af622 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4580,6 +4580,7 @@ static void dwc_gadget_release(struct device *dev)
 {
 	struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev);
 
+	flush_work(&gadget->work);
 	kfree(gadget);
 }
 

base-commit: f066b5a6c7a06adfb666b7652cc99b4ff264f4ed
-- 
2.48.0.rc2.279.g1de40edade-goog

On Wed, Jan 22, 2025, Roy Luo wrote:
> `dwc3_gadget_soft_disconnect` function, called as part of

The dwc3_gadget_soft_disconnect() isn't directly part of
device_del(&gadget->dev). It should be part of disconnect.

Can you provide the full sequence of events so I can have more context?
The handling of the flushing of gadget->work should not be part of the
dwc3.

> `device_del(&gadget->dev)`, queues a new work item to the gadget workqueue
> after the workqueue has been flushed in `usb_del_gadget`.
> This leads to a potential use-after-free issue.
> 
> To fix this, flush the workqueue in the `release` function before freeing
> the gadget. This ensures that all work items are processed before the
> gadget is destroyed.
> 
> Fixes: 1ff24d40b3c3 ("usb: dwc3: gadget: Fix incorrect UDC state after manual deconfiguration")

Since the patch above is now in the mainline, may need to add a stable
tag. 

Thanks,
Thinh

> Signed-off-by: Roy Luo <royluo@google.com>
> ---
>  drivers/usb/dwc3/gadget.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d27af65eb08a..12055e3af622 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4580,6 +4580,7 @@ static void dwc_gadget_release(struct device *dev)
>  {
>  	struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev);
>  
> +	flush_work(&gadget->work);
>  	kfree(gadget);
>  }
>  
> 
> base-commit: f066b5a6c7a06adfb666b7652cc99b4ff264f4ed
> -- 
> 2.48.0.rc2.279.g1de40edade-goog
> 

On Mon, Jan 27, 2025 at 5:44 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Wed, Jan 22, 2025, Roy Luo wrote:
> > `dwc3_gadget_soft_disconnect` function, called as part of
>
> The dwc3_gadget_soft_disconnect() isn't directly part of
> device_del(&gadget->dev). It should be part of disconnect.
>
> Can you provide the full sequence of events so I can have more context?
> The handling of the flushing of gadget->work should not be part of the
> dwc3.


Yes, it's a part of disconnect, and disconnect is a part of gadget unbind.
Let me try my best to explain. Here's the call stack for usb_del_gadget:
-> usb_del_gadget
    -> flush_work(&gadget->work)
    -> device_del
        -> bus_remove_device
        -> device_release_driver
        -> gadget_unbind_driver
        -> usb_gadget_disconnect_locked
        -> dwc3_gadget_pullup
        -> dwc3_gadget_soft_disconnect
        -> usb_gadget_set_state
        -> schedule_work(&gadget->work)

Then when usb_put_gadget is called, gadget could be freed before
gadget->work is executed.
-> usb_put_gadget
-> put_device
-> kobject_put
-> device_release
-> dwc_gadget_release
-> kfree(gadget)

>
> Since the patch above is now in the mainline, may need to add a stable
> tag.

Ack, will cc stable in the next revision.

Regards,
Roy Luo