| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZTBW/o3tT/ukdbbi@pengutronix.de>
Date: Thu, 19 Oct 2023 00:06:54 +0200
From: Michael Grzeschik <mgr@...gutronix.de>
To: Avichal Rakesh <arakesh@...gle.com>
Cc: dan.scally@...asonboard.com, laurent.pinchart@...asonboard.com,
etalvala@...gle.com, gregkh@...uxfoundation.org,
jchowdhary@...gle.com, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight
usb_requests
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@...gutronix.de>
>>> Signed-off-by: Avichal Rakesh <arakesh@...gle.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 |
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists