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@google.com >>> Suggested-by: Michael Grzeschik >>> Signed-off-by: Avichal Rakesh >>> --- >>> v1 -> v2: Rebased to ToT, and fixed deadlock reported in >>>          https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/ >>> 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@google.com/ >> >> 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. Regards, 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 |