From: Avichal Rakesh <arakesh@google.com>
To: Michael Grzeschik <mgr@pengutronix.de>
Cc: dan.scally@ideasonboard.com, laurent.pinchart@ideasonboard.com,
etalvala@google.com, gregkh@linuxfoundation.org,
jchowdhary@google.com, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
Subject: Re: [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests
Date: Thu, 19 Oct 2023 11:54:56 -0700 [thread overview]
Message-ID: <b9f9ff09-efad-4c1a-9d27-6dde5c89852f@google.com> (raw)
In-Reply-To: <ZTBW/o3tT/ukdbbi@pengutronix.de>
On 10/18/23 15:06, Michael Grzeschik wrote:
> On Wed, Oct 18, 2023 at 02:50:08PM -0700, Avichal Rakesh wrote:
>>
>>
>> On 10/18/23 06:10, Michael Grzeschik wrote:
>>> On Wed, Oct 11, 2023 at 05:24:51PM -0700, 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 accomodate 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
>>>> Suggested-by: Michael Grzeschik <m.grzeschik@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
>>>> 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/
>>>
>>> I tested this and I no longer saw any use after free
>>> errors anymore! :)
>>
>> Yay! Glad to hear!
>>
>>>
>>> Here comes some more review:
>>>
>>>> drivers/usb/gadget/function/uvc.h | 1 +
>>>> drivers/usb/gadget/function/uvc_v4l2.c | 12 +-
>>>> drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++-----
>>>> 3 files changed, 128 insertions(+), 41 deletions(-)
>>>>
>>
>>>> +
>>>> +/*
>>>> + * Disable video stream
>>>> + */
>>>> +static int
>>>> +uvcg_video_disable(struct uvc_video *video) {
>>>> + unsigned long flags;
>>>> + struct list_head inflight_bufs;
>>>> + struct usb_request *req, *temp;
>>>> + struct uvc_buffer *buf, *btemp;
>>>> + struct uvc_request *ureq, *utemp;
>>>> +
>>>> + 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);
>>>> - return;
>>>> +
>>>> + cancel_work_sync(&video->pump);
>>>> + uvcg_queue_cancel(&video->queue, 0);
>>>> +
>>>> + spin_lock_irqsave(&video->req_lock, flags);
>>>> + /*
>>>> + * Remove all uvc_reqeusts from 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);
>>>> + }
>>>> +
>>>> + 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;
>>>> }
>>>>
>>>> /*
>>>> @@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work)
>>>> 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,
>>>> "Video enable failed, device is uninitialized.\n");
>>>> return -ENODEV;
>>>> }
>>>> -
>>>> - 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);
>>>> - return 0;
>>>> - }
>>>> -
>>>> + if (!enable)
>>>> + return uvcg_video_disable(video);
>>>
>>> Could you refactor this code as it is to an separate
>>> function and prepand this change as an extra patch
>>> to this one? It would make the changes in the functions
>>> more obvious and better to review.
>>
>> Sure I can send a follow up patch, but I am curious why you think this
>> needs to be a separate function? Refactoring into a function would
>> have the functions structured something like:
>>
>> uvcg_video_disable(video) {
>> // ...
>> // disable impl
>> // ...
>> }
>>
>> uvcg_video_enable(video) {
>> // ...
>> // enable impl
>> // ...
>> }
>>
>> uvcg_video_enable(video, enable) {
>> // ep test
>>
>> if (!enable)
>> return uvcg_video_disable(video);
>>
>> return uvc_video_enable(video);
>> }
>>
>> instead of the current structure:
>>
>> uvcg_video_disable(video) {
>> // ...
>> // disable impl
>> // ...
>> }
>>
>> uvcg_video_enable(video, enable) {
>> // ep test
>>
>> if (!enable)
>> return uvcg_video_disable(video);
>>
>> // ...
>> // enable impl
>> // ...
>> }
>>
>> I am not sure if one is more readable than the other.
>
> I think you misunderstood. The second structure is all right.
>
> What I did want you to do is as follows.
>
> Lets look at your series:
>
> patch 0/3
> patch 1/3
> patch 2/3
>
> <--- add a patch here that does the refactoring of the separate
> function uvcg_video_disable without changing the functional
> content of it:
>
> uvcg_video_disable(video) {
> // ...
> // disable impl
> // ...
> }
>
> uvcg_video_enable(video, enable) {
> // ep test
>
> if (!enable)
> return uvcg_video_disable(video);
>
> // ...
> // enable impl
> // ...
> }
>
> patch 3/3
>
> This way in the patch 3/3 the functional changes you introduce to the
> uvcg_video_diable will get better to review.
I see! I did indeed misunderstand. Sent out v6 with 4 patches!
Thank you!
- Avi.
next prev parent reply other threads:[~2023-10-19 18:55 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 ` [PATCH v3 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb_requests Avichal Rakesh
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 [this message]
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=b9f9ff09-efad-4c1a-9d27-6dde5c89852f@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=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).