On 10/18/23 06:10, Michael Grzeschik wrote: > On Wed, Oct 11, 2023 at 05:24:51PM -0700, Avichal Rakesh wrote: >> Currently, the uvc gadget driver allocates all uvc_requests as one array >> and deallocates them all when the video stream stops. This includes >> de-allocating all the usb_requests associated with those uvc_requests. >> This can lead to use-after-free issues if any of those de-allocated >> usb_requests were still owned by the usb controller. >> >> This is patch 2 of 2 in fixing the use-after-free issue. It adds a new >> flag to uvc_video to track when frames and requests should be flowing. >> When disabling the video stream, the flag is tripped and, instead >> of de-allocating all uvc_requests and usb_requests, the gadget >> driver only de-allocates those usb_requests that are currently >> owned by it (as present in req_free). Other usb_requests are left >> untouched until their completion handler is called which takes care >> of freeing the usb_request and its corresponding uvc_request. >> >> Now that uvc_video does not depends on uvc->state, this patch removes >> unnecessary upates to uvc->state that were made to accomodate uvc_video >> logic. This should ensure that uvc gadget driver never accidentally >> de-allocates a usb_request that it doesn't own. >> >> Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@xxxxxxxxxx >> Suggested-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> >> Signed-off-by: Avichal Rakesh <arakesh@xxxxxxxxxx> >> --- >> v1 -> v2: Rebased to ToT, and fixed deadlock reported in >> https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@xxxxxxxxxxxxxx/ >> v2 -> v3: Fix email threading goof-up >> v3 -> v4: re-rebase to ToT & moved to a uvc_video level lock >> as discussed in >> https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@xxxxxxxxxx/ > > I tested this and I no longer saw any use after free > errors anymore! :) Yay! Glad to hear! > > Here comes some more review: > >> drivers/usb/gadget/function/uvc.h | 1 + >> drivers/usb/gadget/function/uvc_v4l2.c | 12 +- >> drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++----- >> 3 files changed, 128 insertions(+), 41 deletions(-) >> >> + >> +/* >> + * Disable video stream >> + */ >> +static int >> +uvcg_video_disable(struct uvc_video *video) { >> + unsigned long flags; >> + struct list_head inflight_bufs; >> + struct usb_request *req, *temp; >> + struct uvc_buffer *buf, *btemp; >> + struct uvc_request *ureq, *utemp; >> + >> + INIT_LIST_HEAD(&inflight_bufs); >> + spin_lock_irqsave(&video->req_lock, flags); >> + video->is_enabled = false; >> + >> + /* >> + * Remove any in-flight buffers from the uvc_requests >> + * because we want to return them before cancelling the >> + * queue. This ensures that we aren't stuck waiting for >> + * all complete callbacks to come through before disabling >> + * vb2 queue. >> + */ >> + list_for_each_entry(ureq, &video->ureqs, list) { >> + if (ureq->last_buf) { >> + list_add_tail(&ureq->last_buf->queue, &inflight_bufs); >> + ureq->last_buf = NULL; >> + } >> + } >> spin_unlock_irqrestore(&video->req_lock, flags); >> - return; >> + >> + cancel_work_sync(&video->pump); >> + uvcg_queue_cancel(&video->queue, 0); >> + >> + spin_lock_irqsave(&video->req_lock, flags); >> + /* >> + * Remove all uvc_reqeusts from from ureqs with list_del_init >> + * This lets uvc_video_free_request correctly identify >> + * if the uvc_request is attached to a list or not when freeing >> + * memory. >> + */ >> + list_for_each_entry_safe(ureq, utemp, &video->ureqs, list) >> + list_del_init(&ureq->list); >> + >> + list_for_each_entry_safe(req, temp, &video->req_free, list) { >> + list_del(&req->list); >> + uvc_video_free_request(req->context, video->ep); >> + } >> + >> + INIT_LIST_HEAD(&video->ureqs); >> + INIT_LIST_HEAD(&video->req_free); >> + video->req_size = 0; >> + spin_unlock_irqrestore(&video->req_lock, flags); >> + >> + /* >> + * Return all the video buffers before disabling the queue. >> + */ >> + spin_lock_irqsave(&video->queue.irqlock, flags); >> + list_for_each_entry_safe(buf, btemp, &inflight_bufs, queue) { >> + list_del(&buf->queue); >> + uvcg_complete_buffer(&video->queue, buf); >> + } >> + spin_unlock_irqrestore(&video->queue.irqlock, flags); >> + >> + uvcg_queue_enable(&video->queue, 0); >> + return 0; >> } >> >> /* >> @@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work) >> int uvcg_video_enable(struct uvc_video *video, int enable) >> { >> int ret; >> - struct uvc_request *ureq; >> >> if (video->ep == NULL) { >> uvcg_info(&video->uvc->func, >> "Video enable failed, device is uninitialized.\n"); >> return -ENODEV; >> } >> - >> - if (!enable) { >> - cancel_work_sync(&video->pump); >> - uvcg_queue_cancel(&video->queue, 0); >> - >> - list_for_each_entry(ureq, &video->ureqs, list) { >> - if (ureq->req) >> - usb_ep_dequeue(video->ep, ureq->req); >> - } >> - >> - uvc_video_free_requests(video); >> - uvcg_queue_enable(&video->queue, 0); >> - return 0; >> - } >> - >> + if (!enable) >> + return uvcg_video_disable(video); > > Could you refactor this code as it is to an separate > function and prepand this change as an extra patch > to this one? It would make the changes in the functions > more obvious and better to review. Sure I can send a follow up patch, but I am curious why you think this needs to be a separate function? Refactoring into a function would have the functions structured something like: uvcg_video_disable(video) { // ... // disable impl // ... } uvcg_video_enable(video) { // ... // enable impl // ... } uvcg_video_enable(video, enable) { // ep test if (!enable) return uvcg_video_disable(video); return uvc_video_enable(video); } instead of the current structure: uvcg_video_disable(video) { // ... // disable impl // ... } uvcg_video_enable(video, enable) { // ep test if (!enable) return uvcg_video_disable(video); // ... // enable impl // ... } I am not sure if one is more readable than the other. > >> + >> + /* >> + * Safe to access request related fields without req_lock because >> + * this is the only thread currently active, and no other >> + * request handling thread will become active until this function >> + * returns. >> + */ >> + video->is_enabled = true; > > Add an extra empty line. > >> if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0) >> return ret; >> >> @@ -544,6 +637,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) >> */ >> int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) >> { >> + video->is_enabled = false; >> INIT_LIST_HEAD(&video->ureqs); >> INIT_LIST_HEAD(&video->req_free); >> spin_lock_init(&video->req_lock); >> -- >> 2.42.0.609.gbb76f46606-goog >> >> > > Reviewed-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > Tested-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > >
