Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4500 bytes --]

On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>On 9/15/23 16:32, Michael Grzeschik wrote:
>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>> immediately deallocs all requests on its disable codepath. This is not
>>>> save since the dequeue function is async and does not ensure that the
>>>> requests are left unlinked in the controller driver.
>>>>
>>>> By adding the ep_free_request into the completion path of the requests
>>>> we ensure that the request will be properly deallocated.
>>>>
>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>> ---
>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>      struct uvc_device *uvc = video->uvc;
>>>>      unsigned long flags;
>>>>
>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>
>> Thanks, thats a good point.
>>
>>>> +        ureq->req = NULL;
>>>> +        return;
>>>> +    }
>>>> +
>>>>      switch (req->status) {
>>>>      case 0:
>>>>          break;
>>>
>>> Perhaps I am missing something here, but I am not sure how this alone
>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>> possible that an unreturned request is deallocated, and now it is
>>> possible that the complete callback accesses a deallocated ureq :(
>>
>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>> the list_del_entry of the giveback function, the issue was probably just not
>> triggered anymore as the complete function did exit early.
>>
>> So this fix alone is actually bogus without a second patch I had in the stack.
>> The second patch I am refering should change the actual overall issue:
>>
>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>
>> This early list_del and this patch here should ensure that the
>> concurrent functions are not handling already freed memory.
>
>Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>list at all. However, this setup is still prone to errors. For example, there is now
>a chance that gadget_ep_free_request is called twice for one request. A scheduling
>like the following might cause double kfree:
>
>1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>   calling the complete callbacks.
>3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>   calls gadget_ep_free_request (calling kfree).
>
>There is currently (even in your patches) no synchronization between calls to
>gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>inevitably call usb_ep_free_request twice for one request.
>
>Does that make sense, or am I misunderstanding some part of the patch?

The overall concept is correct. But in detail the
uvc_video_free_requests is checking that video->ureq[i].req is not NULL.

With our previous call of ep_free_request in the complete handler, the
ureq->req pointer in focus was already set to NULL. So the
uvc_video_free_requests function will skip that extra free.

Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]