From: Avichal Rakesh <arakesh@google.com>
To: arakesh@google.com, dan.scally@ideasonboard.com,
gregkh@linuxfoundation.org, laurent.pinchart@ideasonboard.com,
m.grzeschik@pengutronix.de
Cc: etalvala@google.com, jchowdhary@google.com,
linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
Michael Grzeschik <mgr@pengutronix.de>
Subject: [PATCH v3 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests
Date: Thu, 5 Oct 2023 11:08:14 -0700 [thread overview]
Message-ID: <20231005180814.3278050-3-arakesh@google.com> (raw)
In-Reply-To: <20231005180814.3278050-1-arakesh@google.com>
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@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
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
next prev parent reply other threads:[~2023-10-05 18:09 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-30 18:48 [PATCH v1 0/3] usb: gadget: uvc: stability fixes on STREAMOFF Avichal Rakesh
2023-09-30 18:48 ` [PATCH v1 1/3] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-03 23:18 ` [PATCH v2 " Avichal Rakesh
2023-09-30 18:48 ` [PATCH v1 2/3] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-03 23:19 ` [PATCH v2 " Avichal Rakesh
2023-09-30 18:48 ` [PATCH v1 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-03 23:21 ` [PATCH v2 " Avichal Rakesh
2023-10-03 11:09 ` [PATCH v1 0/3] usb: gadget: uvc: stability fixes on STREAMOFF Michael Grzeschik
2023-10-03 23:16 ` Avichal Rakesh
2023-10-05 7:11 ` Greg Kroah-Hartman
2023-10-05 18:09 ` Avichal Rakesh
2023-10-05 8:23 ` Laurent Pinchart
2023-10-05 10:14 ` Michael Grzeschik
2023-10-05 18:30 ` Avichal Rakesh
2023-10-05 22:05 ` Michael Grzeschik
2023-10-06 17:00 ` Avichal Rakesh
2023-10-06 22:53 ` Michael Grzeschik
2023-10-06 23:48 ` Avichal Rakesh
2023-10-08 19:48 ` Michael Grzeschik
2023-10-12 0:33 ` Avichal Rakesh
2023-10-05 18:08 ` [PATCH v3 1/3] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-05 18:08 ` [PATCH v3 2/3] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-06 22:11 ` Michael Grzeschik
2023-10-05 18:08 ` Avichal Rakesh [this message]
2023-10-06 22:04 ` [PATCH v3 1/3] usb: gadget: uvc: prevent use of disabled endpoint Michael Grzeschik
2023-10-12 0:24 ` [PATCH v4 " Avichal Rakesh
2023-10-12 0:24 ` [PATCH v4 2/3] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-18 13:03 ` Michael Grzeschik
2023-10-18 19:53 ` Avichal Rakesh
2023-10-12 0:24 ` [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-12 0:42 ` Avichal Rakesh
2023-10-18 13:10 ` Michael Grzeschik
2023-10-18 21:50 ` Avichal Rakesh
2023-10-18 22:06 ` Michael Grzeschik
2023-10-19 18:54 ` Avichal Rakesh
2023-10-18 19:46 ` [PATCH v5 1/3] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-18 19:46 ` [PATCH v5 2/3] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-18 19:46 ` [PATCH v5 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-19 18:53 ` [PATCH v6 0/4] usb: gadget: uvc: stability fixes on STREAMOFF Avichal Rakesh
2023-10-19 18:53 ` [PATCH v6 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-19 18:53 ` [PATCH v6 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-19 18:53 ` [PATCH v6 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-10-19 18:53 ` [PATCH v6 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-19 20:32 ` kernel test robot
2023-10-19 22:30 ` Avichal Rakesh
2023-10-21 10:05 ` Greg KH
2023-10-23 21:25 ` Avichal Rakesh
2023-10-24 9:27 ` Greg KH
2023-10-24 20:00 ` Avichal Rakesh
2023-10-19 18:59 ` [PATCH v6 0/4] usb: gadget: uvc: stability fixes on STREAMOFF Avichal Rakesh
2023-10-27 20:19 ` [PATCH v9 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-27 20:19 ` [PATCH v9 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-28 10:31 ` Greg KH
2023-10-28 20:13 ` Dan Scally
2023-10-30 20:26 ` Avichal Rakesh
2023-10-27 20:19 ` [PATCH v9 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-10-28 20:16 ` Dan Scally
2023-10-27 20:19 ` [PATCH v9 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-28 20:56 ` Dan Scally
2023-10-30 20:56 ` Avichal Rakesh
2023-11-02 13:29 ` Dan Scally
2023-11-02 20:39 ` Avichal Rakesh
2023-11-07 21:15 ` Avichal Rakesh
2023-10-30 20:22 ` [PATCH v10 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-30 20:22 ` [PATCH v10 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-11-01 11:06 ` Dan Scally
2023-11-01 22:13 ` Avichal Rakesh
2023-11-02 11:38 ` Dan Scally
2023-10-30 20:22 ` [PATCH v10 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-10-30 20:22 ` [PATCH v10 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-11-02 20:19 ` [PATCH v11 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-11-02 20:19 ` [PATCH v11 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-11-02 20:19 ` [PATCH v11 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-11-02 20:19 ` [PATCH v11 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-11-08 14:15 ` Dan Scally
2023-11-09 1:00 ` Avichal Rakesh
2023-11-09 0:41 ` [PATCH v12 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-11-09 0:41 ` [PATCH v12 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-11-09 0:41 ` [PATCH v12 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-11-09 0:41 ` [PATCH v12 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-11-14 21:04 ` [PATCH v12 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-20 17:36 ` [PATCH v7 " Avichal Rakesh
2023-10-20 17:36 ` [PATCH v7 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
2023-10-20 17:36 ` [PATCH v7 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-10-20 17:36 ` [PATCH v7 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-24 18:36 ` [PATCH v8 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-24 18:36 ` [PATCH v8 2/4] usb: gadget: uvc: Allocate uvc_requests one at a time Avichal Rakesh
[not found] ` <421d1996-8544-45ac-9f31-551ef597546c@ideasonboard.com>
2023-10-27 20:31 ` Avichal Rakesh
2023-10-28 5:30 ` Greg KH
2023-10-24 18:36 ` [PATCH v8 3/4] usb: gadget: uvc: move video disable logic to its own function Avichal Rakesh
2023-10-24 18:36 ` [PATCH v8 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
2023-10-26 20:23 ` [PATCH v8 1/4] usb: gadget: uvc: prevent use of disabled endpoint Avichal Rakesh
2023-10-27 10:51 ` Greg KH
2023-10-27 10:52 ` Dan Scally
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231005180814.3278050-3-arakesh@google.com \
--to=arakesh@google.com \
--cc=dan.scally@ideasonboard.com \
--cc=etalvala@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jchowdhary@google.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.grzeschik@pengutronix.de \
--cc=mgr@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).