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]

 



Hi Avichal

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.

Thanks,
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