| Message ID | 20250122024452.50289-1-royluo@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] usb: dwc3: gadget: fix gadget workqueue use-after-free | expand |
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
Cc Alan On Fri, Jan 31, 2025, Roy Luo wrote: > 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) > Thanks for the details! The UDC core is initiating and handling the gadget->work, so the flushing of the gadget->work should also be handled there. Since the usb_gadget_disconnect_locked() may trigger a state change work on unbind, the flushing of the gadget->work should to be moved to gadget_unbind_driver() instead: diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index f8c1ef465e45..9e4abd6e40f8 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -1568,7 +1568,6 @@ void usb_del_gadget(struct usb_gadget *gadget) kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); sysfs_remove_link(&udc->dev.kobj, "gadget"); - flush_work(&gadget->work); device_del(&gadget->dev); ida_free(&gadget_id_numbers, gadget->id_number); cancel_work_sync(&udc->vbus_work); @@ -1694,6 +1693,8 @@ 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); BR, Thinh
On Fri, Jan 31, 2025 at 11:44:17PM +0000, Thinh Nguyen wrote: > Cc Alan > > On Fri, Jan 31, 2025, Roy Luo wrote: > > 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) > > > > Thanks for the details! > > The UDC core is initiating and handling the gadget->work, so the > flushing of the gadget->work should also be handled there. > > Since the usb_gadget_disconnect_locked() may trigger a state change work > on unbind, the flushing of the gadget->work should to be moved to > gadget_unbind_driver() instead: > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index f8c1ef465e45..9e4abd6e40f8 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -1568,7 +1568,6 @@ void usb_del_gadget(struct usb_gadget *gadget) > > kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); > sysfs_remove_link(&udc->dev.kobj, "gadget"); > - flush_work(&gadget->work); > device_del(&gadget->dev); > ida_free(&gadget_id_numbers, gadget->id_number); > cancel_work_sync(&udc->vbus_work); > @@ -1694,6 +1693,8 @@ 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); What about instead moving the flush_work() call down just one line, after the device_del(&gadget->dev) call rather than before it? The work queue doesn't need to be flushed every time a driver unbinds from the gadget, only when the gadget is about to be deallocated. Alan Stern
On Sat, Feb 01, 2025, Alan Stern wrote: > On Fri, Jan 31, 2025 at 11:44:17PM +0000, Thinh Nguyen wrote: > > Cc Alan > > > > On Fri, Jan 31, 2025, Roy Luo wrote: > > > 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) > > > > > > > Thanks for the details! > > > > The UDC core is initiating and handling the gadget->work, so the > > flushing of the gadget->work should also be handled there. > > > > Since the usb_gadget_disconnect_locked() may trigger a state change work > > on unbind, the flushing of the gadget->work should to be moved to > > gadget_unbind_driver() instead: > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index f8c1ef465e45..9e4abd6e40f8 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -1568,7 +1568,6 @@ void usb_del_gadget(struct usb_gadget *gadget) > > > > kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); > > sysfs_remove_link(&udc->dev.kobj, "gadget"); > > - flush_work(&gadget->work); > > device_del(&gadget->dev); > > ida_free(&gadget_id_numbers, gadget->id_number); > > cancel_work_sync(&udc->vbus_work); > > @@ -1694,6 +1693,8 @@ 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); > > What about instead moving the flush_work() call down just one line, > after the device_del(&gadget->dev) call rather than before it? > > The work queue doesn't need to be flushed every time a driver unbinds > from the gadget, only when the gadget is about to be deallocated. > That sounds good to me. Thanks, Thinh
On Mon, Feb 3, 2025 at 3:54 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Sat, Feb 01, 2025, Alan Stern wrote: > > On Fri, Jan 31, 2025 at 11:44:17PM +0000, Thinh Nguyen wrote: > > > Cc Alan > > > > > > On Fri, Jan 31, 2025, Roy Luo wrote: > > > > 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) > > > > > > > > > > Thanks for the details! > > > > > > The UDC core is initiating and handling the gadget->work, so the > > > flushing of the gadget->work should also be handled there. > > > > > > Since the usb_gadget_disconnect_locked() may trigger a state change work > > > on unbind, the flushing of the gadget->work should to be moved to > > > gadget_unbind_driver() instead: > > > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > > index f8c1ef465e45..9e4abd6e40f8 100644 > > > --- a/drivers/usb/gadget/udc/core.c > > > +++ b/drivers/usb/gadget/udc/core.c > > > @@ -1568,7 +1568,6 @@ void usb_del_gadget(struct usb_gadget *gadget) > > > > > > kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); > > > sysfs_remove_link(&udc->dev.kobj, "gadget"); > > > - flush_work(&gadget->work); > > > device_del(&gadget->dev); > > > ida_free(&gadget_id_numbers, gadget->id_number); > > > cancel_work_sync(&udc->vbus_work); > > > @@ -1694,6 +1693,8 @@ 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); > > > > What about instead moving the flush_work() call down just one line, > > after the device_del(&gadget->dev) call rather than before it? > > > > The work queue doesn't need to be flushed every time a driver unbinds > > from the gadget, only when the gadget is about to be deallocated. > > > > That sounds good to me. > > Thanks, > Thinh Thank both of you for the review! I've sent out https://lore.kernel.org/linux-usb/20250204000102.3989779-1-royluo@google.com per Alan's suggestion. Regards, Roy
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); }
`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(+) base-commit: f066b5a6c7a06adfb666b7652cc99b4ff264f4ed