| 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: <20230930184821.310143-4-arakesh@google.com>
Date: Sat, 30 Sep 2023 11:48:21 -0700
From: Avichal Rakesh <arakesh@...gle.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Daniel Scally <dan.scally@...asonboard.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Michael Grzeschik <m.grzeschik@...gutronix.de>
Cc: jchowdhary@...gle.com, etalvala@...gle.com,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
Avichal Rakesh <arakesh@...gle.com>,
Michael Grzeschik <mgr@...gutronix.de>
Subject: [PATCH v1 3/3] 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_request to mark it as 'abandoned'. When disabling the video
stream, instead of de-allocating all uvc_requests and usb_requests, the
gadget driver only de-allocates those usb_requests that are currently
owned by the gadget driver (as present in req_free). Other usb_requests
have their corresponding 'is_abandoned' flag tripped, and the
usb_requests complete handler takes care of freeing the usb_request and
its corresponding uvc_request.
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 <mgr@...gutronix.de>
Signed-off-by: Avichal Rakesh <arakesh@...gle.com>
---
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_video.c | 106 ++++++++++++++++++++----
2 files changed, 91 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 993694da0bbc..e69cfb7cced1 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -82,6 +82,7 @@ struct uvc_request {
u8 header[UVCG_REQUEST_HEADER_LEN];
struct uvc_buffer *last_buf;
struct list_head list;
+ bool is_abandoned;
};
struct uvc_video {
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index ffecd7a140dc..aad7dcba46ee 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -271,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
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;
+ bool is_abandoned;
+
+ spin_lock_irqsave(&video->req_lock, flags);
+ is_abandoned = ureq->is_abandoned;
+ last_buf = ureq->last_buf;
+ ureq->last_buf = NULL;
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ if (is_abandoned) {
+ uvcg_dbg(&video->uvc->func, "Freeing abandoned usb_request\n");
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
switch (req->status) {
case 0:
@@ -294,15 +308,29 @@ 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(&video->queue.irqlock, flags);
+ uvcg_complete_buffer(&video->queue, last_buf);
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
}
+ /*
+ * request might have been abandoned while being processed.
+ * do a last minute check before queueing the request back.
+ */
spin_lock_irqsave(&video->req_lock, flags);
- list_add_tail(&req->list, &video->req_free);
+ is_abandoned = ureq->is_abandoned;
+ if (!is_abandoned)
+ list_add_tail(&req->list, &video->req_free);
spin_unlock_irqrestore(&video->req_lock, flags);
+ if (is_abandoned) {
+ uvcg_dbg(&video->uvc->func,
+ "usb_request abandoned mid-processing - freeing.\n");
+ uvc_video_free_request(ureq, ep);
+ return;
+ }
+
if (uvc->state == UVC_STATE_STREAMING)
queue_work(video->async_wq, &video->pump);
}
@@ -366,7 +394,6 @@ uvc_video_alloc_requests(struct uvc_video *video)
}
video->req_size = req_size;
-
return 0;
error:
@@ -490,13 +517,69 @@ static void uvcg_video_pump(struct work_struct *work)
return;
}
+/*
+ * Disable video stream. This ensures that any inflight usb requests are marked
+ * for clean up and all video buffers are dropped before returning.
+ */
+static void uvcg_video_disable(struct uvc_video *video)
+{
+ struct uvc_buffer *buf, *tmp_buf;
+ struct uvc_request *ureq, *temp;
+ struct list_head buf_list; /* track in-flight video buffers */
+ struct usb_request *req;
+ unsigned long flags;
+
+ cancel_work_sync(&video->pump);
+ uvcg_queue_cancel(&video->queue, 0);
+
+ INIT_LIST_HEAD(&buf_list);
+ spin_lock_irqsave(&video->req_lock, flags);
+ /* abandon all usb requests */
+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) {
+ ureq->is_abandoned = true;
+ if (ureq->last_buf) {
+ list_add(&ureq->last_buf->queue, &buf_list);
+ ureq->last_buf = NULL;
+ }
+ list_del_init(&ureq->list);
+ if (ureq->req)
+ usb_ep_dequeue(video->ep, ureq->req);
+ }
+ /*
+ * re-add uvc_requests currently owned by the gadget to
+ * video->ureqs to be deallocated
+ */
+ list_for_each_entry(req, &video->req_free, list) {
+ ureq = req->context;
+ list_add_tail(&ureq->list, &video->ureqs);
+ }
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ /*
+ * drop abandoned uvc_buffers, as the completion handler
+ * no longer will
+ */
+ if (!list_empty(&buf_list)) {
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ list_for_each_entry_safe(buf, tmp_buf,
+ &buf_list, queue) {
+ video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ uvcg_complete_buffer(&video->queue, buf);
+ list_del(&buf->queue);
+ }
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
+ }
+
+ uvc_video_free_requests(video);
+ uvcg_queue_enable(&video->queue, 0);
+}
+
/*
* Enable or disable the video stream.
*/
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,
@@ -505,16 +588,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
}
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);
+ uvcg_video_disable(video);
return 0;
}
--
2.42.0.582.g8ccd20d70d-goog
Powered by blists - more mailing lists