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

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7957 bytes --]

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 <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.

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]