lkml.org 
[lkml]   [2023]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v11 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests
From
Hi Avichal

On 02/11/2023 20:19, 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 accommodate 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
> Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Avichal Rakesh <arakesh@google.com>
> ---


Thanks for the update. Let's leave the locking as it is; I think albeit not strictly necessary on
that occasion it certainly is necessary to take the lock to protect the flags elsewhere, and
probably better to be consistent with it.


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.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/
> v4 -> v5 : Address review comments. Add Reviewed-by & Tested-by.
> v5 -> v6 : Added another patch before this one to make uvcg_video_disable
> easier to review.
> v6 -> v7 : Fix warning reported in
> https://lore.kernel.org/202310200457.GwPPFuHX-lkp@intel.com/
> v7 -> v8 : No change. Getting back in review queue
> v8 -> v9 : No change.
> v9 -> v10 : Address review comments. Rebase to ToT (usb-next)
> v10 -> v11 : Address review comments
>
> drivers/usb/gadget/function/uvc.h | 1 +
> drivers/usb/gadget/function/uvc_v4l2.c | 10 +-
> drivers/usb/gadget/function/uvc_video.c | 130 ++++++++++++++++++++----
> 3 files changed, 112 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 993694da0bbc..be0d012aa244 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -102,6 +102,7 @@ struct uvc_video {
> unsigned int uvc_num_requests;
>
> /* Requests */
> + bool is_enabled; /* tracks whether video stream is enabled */
> unsigned int req_size;
> struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
> struct list_head req_free;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 904dd283cbf7..c7e5fa4f29e0 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -468,11 +468,11 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
> if (type != video->queue.queue.type)
> return -EINVAL;
>
> - uvc->state = UVC_STATE_CONNECTED;
> ret = uvcg_video_disable(video);
> if (ret < 0)
> return ret;
>
> + uvc->state = UVC_STATE_CONNECTED;
> uvc_function_setup_continue(uvc, 1);
> return 0;
> }
> @@ -507,14 +507,6 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
> static void uvc_v4l2_disable(struct uvc_device *uvc)
> {
> uvc_function_disconnect(uvc);
> - /*
> - * Drop uvc->state to CONNECTED if it was streaming before.
> - * This ensures that the usb_requests are no longer queued
> - * to the controller.
> - */
> - if (uvc->state == UVC_STATE_STREAMING)
> - uvc->state = UVC_STATE_CONNECTED;
> -
> uvcg_video_disable(&uvc->video);
> uvcg_free_buffers(&uvc->video.queue);
> uvc->func_connected = false;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index c3e8c48f46a9..164bdeb7f2a9 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -227,6 +227,10 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
> * Request handling
> */
>
> +/*
> + * Callers must take care to hold req_lock when this function may be called
> + * from multiple threads. For example, when frames are streaming to the host.
> + */
> static void
> uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep)
> {
> @@ -271,9 +275,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> struct uvc_request *ureq = req->context;
> struct uvc_video *video = ureq->video;
> struct uvc_video_queue *queue = &video->queue;
> - struct uvc_device *uvc = video->uvc;
> + struct uvc_buffer *last_buf;
> unsigned long flags;
>
> + spin_lock_irqsave(&video->req_lock, flags);
> + if (!video->is_enabled) {
> + /*
> + * When is_enabled is false, uvcg_video_disable() ensures
> + * that in-flight uvc_buffers are returned, so we can
> + * safely call free_request without worrying about
> + * last_buf.
> + */
> + uvc_video_free_request(ureq, ep);
> + spin_unlock_irqrestore(&video->req_lock, flags);
> + return;
> + }
> +
> + last_buf = ureq->last_buf;
> + ureq->last_buf = NULL;
> + spin_unlock_irqrestore(&video->req_lock, flags);
> +
> switch (req->status) {
> case 0:
> break;
> @@ -295,17 +316,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> uvcg_queue_cancel(queue, 0);
> }
>
> - if (ureq->last_buf) {
> - uvcg_complete_buffer(&video->queue, ureq->last_buf);
> - ureq->last_buf = NULL;
> + if (last_buf) {
> + spin_lock_irqsave(&queue->irqlock, flags);
> + uvcg_complete_buffer(queue, last_buf);
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> }
>
> spin_lock_irqsave(&video->req_lock, flags);
> - list_add_tail(&req->list, &video->req_free);
> - spin_unlock_irqrestore(&video->req_lock, flags);
> -
> - if (uvc->state == UVC_STATE_STREAMING)
> + /*
> + * Video stream might have been disabled while we were
> + * processing the current usb_request. So make sure
> + * we're still streaming before queueing the usb_request
> + * back to req_free
> + */
> + if (video->is_enabled) {
> + list_add_tail(&req->list, &video->req_free);
> queue_work(video->async_wq, &video->pump);
> + } else {
> + uvc_video_free_request(ureq, ep);
> + }
> + spin_unlock_irqrestore(&video->req_lock, flags);
> }
>
> static int
> @@ -392,20 +422,22 @@ static void uvcg_video_pump(struct work_struct *work)
> struct uvc_video_queue *queue = &video->queue;
> /* video->max_payload_size is only set when using bulk transfer */
> bool is_bulk = video->max_payload_size;
> - struct uvc_device *uvc = video->uvc;
> struct usb_request *req = NULL;
> struct uvc_buffer *buf;
> unsigned long flags;
> bool buf_done;
> int ret;
>
> - while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) {
> + while (true) {
> + if (!video->ep->enabled)
> + return;
> +
> /*
> - * Retrieve the first available USB request, protected by the
> - * request lock.
> + * Check is_enabled and retrieve the first available USB
> + * request, protected by the request lock.
> */
> spin_lock_irqsave(&video->req_lock, flags);
> - if (list_empty(&video->req_free)) {
> + if (!video->is_enabled || list_empty(&video->req_free)) {
> spin_unlock_irqrestore(&video->req_lock, flags);
> return;
> }
> @@ -487,9 +519,11 @@ static void uvcg_video_pump(struct work_struct *work)
> return;
>
> spin_lock_irqsave(&video->req_lock, flags);
> - list_add_tail(&req->list, &video->req_free);
> + if (video->is_enabled)
> + list_add_tail(&req->list, &video->req_free);
> + else
> + uvc_video_free_request(req->context, video->ep);
> spin_unlock_irqrestore(&video->req_lock, flags);
> - return;
> }
>
> /*
> @@ -498,7 +532,11 @@ static void uvcg_video_pump(struct work_struct *work)
> int
> uvcg_video_disable(struct uvc_video *video)
> {
> - struct uvc_request *ureq;
> + unsigned long flags;
> + struct list_head inflight_bufs;
> + struct usb_request *req, *temp;
> + struct uvc_buffer *buf, *btemp;
> + struct uvc_request *ureq, *utemp;
>
> if (video->ep == NULL) {
> uvcg_info(&video->uvc->func,
> @@ -506,15 +544,58 @@ uvcg_video_disable(struct uvc_video *video)
> return -ENODEV;
> }
>
> + 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);
> +
> 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);
> + spin_lock_irqsave(&video->req_lock, flags);
> + /*
> + * Remove all uvc_requests 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);
> }
>
> - uvc_video_free_requests(video);
> + 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;
> }
> @@ -532,6 +613,14 @@ int uvcg_video_enable(struct uvc_video *video)
> return -ENODEV;
> }
>
> + /*
> + * 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;
> +
> if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0)
> return ret;
>
> @@ -557,6 +646,7 @@ int uvcg_video_enable(struct uvc_video *video)
> */
> 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.869.gea05f2083d-goog

\
 
 \ /
  Last update: 2023-11-20 13:53    [W:0.643 / U:0.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site