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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
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@xxxxxxxxxxxxxx>
---
 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@xxxxxxxxxxxxxx/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?

I think you have pointed it out totally clear. This is obviously the
case. It just did not trigger here. But the window is there and has to
be locked in some way.

For now we have two options to solve it.

1) Trying to avoid this double code path of the complete callback and
uvc_video_free_requests. This is what your patches are already doing.

But for now I am not so pleased with the timeout concept by waiting for
the complete interrupt to be called. This is also a shot in the dark as
the latency depends on the scheduler and the amount of potential
requests that are being handled.

2) Locking both codepathes around the resource in question so the issue
is avoided.

However, I am also not a fried of many locks.

Perhaps it is possible to use a combination of wait_for_completion in
the uvc_video_free_requests and a complete callback in
uvc_video_complete for those requests that are not listed in the
req_free list.

What do you think?

Regards,
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: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux