| 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: <20231005180814.3278050-3-arakesh@google.com>
Date: Thu, 5 Oct 2023 11:08:14 -0700
From: Avichal Rakesh <arakesh@...gle.com>
To: arakesh@...gle.com, dan.scally@...asonboard.com,
gregkh@...uxfoundation.org, laurent.pinchart@...asonboard.com,
m.grzeschik@...gutronix.de
Cc: etalvala@...gle.com, jchowdhary@...gle.com,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
Michael Grzeschik <mgr@...gutronix.de>
Subject: [PATCH v3 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>
---
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
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_video.c | 118 ++++++++++++++++++++----
2 files changed, 102 insertions(+), 17 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 3c4d286d81c0..69521886d599 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;
+ }
if (uvc->state == UVC_STATE_CONNECTED) {
usb_ep_free_request(video->ep, ureq->req);
@@ -300,15 +314,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);
}
@@ -372,7 +400,6 @@ uvc_video_alloc_requests(struct uvc_video *video)
}
video->req_size = req_size;
-
return 0;
error:
@@ -504,13 +531,80 @@ 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;
+
+ INIT_LIST_HEAD(&buf_list);
+
+ 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);
+ }
+
+ /**
+ * acquiring req_lock here should prevent any more complete
+ * callbacks from processing until we've abandoned the
+ * requests that are still in-flight and let the complete
+ * callback handle cleanup
+ */
+ 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);
+ }
+ /*
+ * re-add uvc_requests currently owned by the gadget to
+ * video->ureqs to be deallocated. This effectively leaves
+ * video->ureqs with the requests that we currently own.
+ */
+ 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)
{
struct uvc_device *uvc = video->uvc;
- struct uvc_request *ureq;
int ret;
if (video->ep == NULL) {
@@ -521,17 +615,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
if (!enable) {
uvc->state = UVC_STATE_CONNECTED;
-
- 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.609.gbb76f46606-goog
Powered by blists - more mailing lists