Re: [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests

[Avichal Rakesh <arakesh@google.com>]

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 <m.grzeschik@pengutronix.de>
>> Signed-off-by: Avichal Rakesh <arakesh@google.com>
>> ---
>> 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.

> 
>> +
>> +    /*
>> +     * 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@pengutronix.de>
> Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
>