linux-kernel - Re: [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight usb

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8d290d0-643c-4a41-8dfa-c56a98a8a02d@google.com>
Date:   Wed, 11 Oct 2023 17:42:15 -0700
From:   Avichal Rakesh <arakesh@...gle.com>
To:     dan.scally@...asonboard.com, laurent.pinchart@...asonboard.com,
        m.grzeschik@...gutronix.de
Cc:     etalvala@...gle.com, gregkh@...uxfoundation.org,
        jchowdhary@...gle.com, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v4 3/3] usb: gadget: uvc: Fix use-after-free for inflight
 usb_requests



On 10/11/23 17:24, 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@...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
> 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/
> 
>  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(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h

Apologies, I realized I forgot to run checkpatch on this patch. Will fix 
the lint issues in the next version. This patch is functionally okay, but 
has 2 minor formatting issues. Feel free to review the patch, and I will
fix the formatting as I am addressing the comments.

- Avi.