linux-kernel - Re: [PATCH v11 4/4] 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: <e29802cf-8387-41d1-a5b1-ab77254cd5cc@google.com>
Date:   Wed, 8 Nov 2023 17:00:07 -0800
From:   Avichal Rakesh <arakesh@...gle.com>
To:     Dan Scally <dan.scally@...asonboard.com>,
        gregkh@...uxfoundation.org
Cc:     etalvala@...gle.com, jchowdhary@...gle.com,
        laurent.pinchart@...asonboard.com, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, m.grzeschik@...gutronix.de
Subject: Re: [PATCH v11 4/4] usb: gadget: uvc: Fix use-after-free for inflight
 usb_requests



On 11/8/23 06:15, Dan Scally wrote:
> Hi Avichal
> 
> On 02/11/2023 20:19, 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 accommodate 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
>> Reviewed-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>> Suggested-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>> Tested-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>> Signed-off-by: Avichal Rakesh <arakesh@...gle.com>
>> ---
> 
> 
> Thanks for the update. Let's leave the locking as it is; I think albeit not strictly necessary on that occasion it certainly is necessary to take the lock to protect the flags elsewhere, and probably better to be consistent with it.
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>

Thank you for reviewing, Dan!

Greg, I just sent out v12 with the Reviewed-by tag:
https://lore.kernel.org/all/20231109004104.3467968-1-arakesh@google.com/ 
They should be ready to submit now. Thank you!

Regards,
Avi.

> 
>> <snip>