usb: gadget: uvc: Fix use-after-free for inflight usb_requests

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: Daniel Scally <dan.scally@ideasonboard.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>
Link: https://lore.kernel.org/r/20231109004104.3467968-4-arakesh@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 993694da0bbcd2a54b25f2660a26b14dbb5a0e39..be0d012aa2444c004deb19b8e0549f4c1a21865b 100644 (file)
--- 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 904dd283cbf721735fd943973b54c0980e71ab0c..c7e5fa4f29e038e7737bfd13c90ee3a2c577da30 100644 (file)
--- 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 c3e8c48f46a9514fa116d0910b063320ebc95067..164bdeb7f2a9331df5e888392451ccbe0aee50b5 100644 (file)
--- 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);