On 10/18/23 15:06, Michael Grzeschik wrote: > On Wed, Oct 18, 2023 at 02:50:08PM -0700, Avichal Rakesh wrote: >> >> >> 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. > > I think you misunderstood. The second structure is all right. > > What I did want you to do is as follows. > > Lets look at your series: > > patch 0/3 > patch 1/3 > patch 2/3 > > <--- add a patch here that does the refactoring of the separate > function uvcg_video_disable without changing the functional > content of it: > > uvcg_video_disable(video) { > // ... > // disable impl > // ... > } > > uvcg_video_enable(video, enable) { > // ep test > > if (!enable) > return uvcg_video_disable(video); > > // ... > // enable impl > // ... > } > > patch 3/3 > > This way in the patch 3/3 the functional changes you introduce to the > uvcg_video_diable will get better to review. I see! I did indeed misunderstand. Sent out v6 with 4 patches! Thank you! - Avi.
