| Message ID | 20250301003452.2675360-2-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix use after freed situation in UDC core | expand |
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
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
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; }
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(-)