Messages in this thread | | | Date | Thu, 19 Oct 2023 00:06:54 +0200 | From | Michael Grzeschik <> | 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@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 | [unhandled content-type:application/pgp-signature] | |