Hi Avichal On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote: >On 9/10/23 17:24, Michael Grzeschik wrote: >> The uvc_video_enable function of the uvc-gadget driver is dequeing and >> immediately deallocs all requests on its disable codepath. This is not >> save since the dequeue function is async and does not ensure that the >> requests are left unlinked in the controller driver. >> >> By adding the ep_free_request into the completion path of the requests >> we ensure that the request will be properly deallocated. >> >> Signed-off-by: Michael Grzeschik >> --- >> drivers/usb/gadget/function/uvc_video.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >> index 4b6e854e30c58c..52e3666b51f743 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) >> struct uvc_device *uvc = video->uvc; >> unsigned long flags; >> >> + if (uvc->state == UVC_STATE_CONNECTED) { >> + usb_ep_free_request(video->ep, ureq->req); >nit: You can probably just call usb_ep_free_request with req instead of ureq->req. Thanks, thats a good point. >> + ureq->req = NULL; >> + return; >> + } >> + >> switch (req->status) { >> case 0: >> break; > >Perhaps I am missing something here, but I am not sure how this alone >fixes the use-after-free issue. uvcg_video_enable still deallocates >_all_ usb_requests right after calling usb_ep_dequeue, so it is still >possible that an unreturned request is deallocated, and now it is >possible that the complete callback accesses a deallocated ureq :( Since the issue I saw was usually coming from the list_del_entry_valid check in the list_del_entry of the giveback function, the issue was probably just not triggered anymore as the complete function did exit early. So this fix alone is actually bogus without a second patch I had in the stack. The second patch I am refering should change the actual overall issue: https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u This early list_del and this patch here should ensure that the concurrent functions are not handling already freed memory. Thanks, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |