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

[Avichal Rakesh <arakesh@google.com>]

On 9/16/23 16:23, Michael Grzeschik wrote:
> 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.
> 

Is there any form of synchronization between uvc_video_request and the 
complete callback? As I see it, the dwc3 interrupt thread and the v4l2 
ioctl thread (which calls uvcg_video_enable) are fully independent, so
the calls made by them are free to be interleaved arbitrarily, so an
interleaving like this is technically possible:

+------+------------------------------------+---------------------------------------------+
| time |            ioctl_thread            |            dwc3 interrupt handler           |
+======+====================================+=============================================+
|   1  | -uvc_v4l2_streamoff                |                                             |
|   2  | |-uvcg_video_enable                |                                             |
|   3  | ||-usb_ep_dequeue                  |                                             |
|   4  | ||                                 | -dwc3_process_event_buf                     |
|   5  | ||-uvc_video_free_requests         | |                                           |
|   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
|   7  | |||                                | ||-dwc3_gadget_giveback                     |
|   8  | |||                                | |||-uvc_video_complete                      |
|   9  | |||-check ureq->req != NULL [true] | ||||                                        |
|  10  | ||||-usb_ep_free_request           | ||||                                        |
|  11  | |||||-dwc3_ep_free_request         | ||||                                        |
|  12  | ||||||-kfree [first call]          | ||||                                        |
|  13  | ||||                               | ||||-usb_ep_free_request                    |
|  14  | ||||                               | |||||-dwc3_ep_free_request                  |
|  15  | ||||                               | ||||||-kfree [second call]                  |
|  16  | ||||                               | ||||-set ureq->req = NULL                   |
|  17  | ||||-set ureq->req = NULL          |                                             |
+------+------------------------------------+---------------------------------------------+

A situation like this means that dwc3_ep_free_request can be called
twice for a particular usb_request. This is obviously low probability,
but a race condition here means we'll start seeing very vague and hard 
to repro crashes or memory inconsistencies when using the uvc gadget.

I do apologize if I've missed something obvious with your changes that 
prevents such interleaving. I don't currently see any locking or 
other synchronization mechanism in your changes. Is there something
in dwc3 that prevents this situation?

Best Regards,
Avi.