Commit da324ffc authored by Avichal Rakesh's avatar Avichal Rakesh Committed by Greg Kroah-Hartman
Browse files

usb: gadget: uvc: Fix use-after-free for inflight usb_requests 

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: default avatarDaniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: default avatarMichael Grzeschik <m.grzeschik@pengutronix.de>
Suggested-by: default avatarMichael Grzeschik <m.grzeschik@pengutronix.de>
Tested-by: default avatarMichael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: default avatarAvichal Rakesh <arakesh@google.com>
Link: https://lore.kernel.org/r/20231109004104.3467968-4-arakesh@google.com


Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 2079b60b

drivers/usb/gadget/function/uvc.h

+1 −0
Original line number Diff line number Diff line
@@ -102,6 +102,7 @@ struct uvc_video { 
	unsigned int uvc_num_requests;



	/* Requests */

	bool is_enabled; /* tracks whether video stream is enabled */

	unsigned int req_size;

	struct list_head ureqs; /* all uvc_requests allocated by uvc_video */

	struct list_head req_free;

drivers/usb/gadget/function/uvc_v4l2.c

+1 −9
Original line number Diff line number Diff line
@@ -468,11 +468,11 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type) 
	if (type != video->queue.queue.type)

		return -EINVAL;



	uvc->state = UVC_STATE_CONNECTED;

	ret = uvcg_video_disable(video);

	if (ret < 0)

		return ret;



	uvc->state = UVC_STATE_CONNECTED;

	uvc_function_setup_continue(uvc, 1);

	return 0;

}
@@ -507,14 +507,6 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh, 
static void uvc_v4l2_disable(struct uvc_device *uvc)

{

	uvc_function_disconnect(uvc);

	/*

	 * Drop uvc->state to CONNECTED if it was streaming before.

	 * This ensures that the usb_requests are no longer queued

	 * to the controller.

	 */

	if (uvc->state == UVC_STATE_STREAMING)

		uvc->state = UVC_STATE_CONNECTED;



	uvcg_video_disable(&uvc->video);

	uvcg_free_buffers(&uvc->video.queue);

	uvc->func_connected = false;

drivers/usb/gadget/function/uvc_video.c

+110 −20
Original line number Diff line number Diff line
@@ -227,6 +227,10 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, 
 * Request handling

 */



/*

 * Callers must take care to hold req_lock when this function may be called

 * from multiple threads. For example, when frames are streaming to the host.

 */

static void

uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep)

{
@@ -271,9 +275,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) 
	struct uvc_request *ureq = req->context;

	struct uvc_video *video = ureq->video;

	struct uvc_video_queue *queue = &video->queue;

	struct uvc_device *uvc = video->uvc;

	struct uvc_buffer *last_buf;

	unsigned long flags;



	spin_lock_irqsave(&video->req_lock, flags);

	if (!video->is_enabled) {

		/*

		 * When is_enabled is false, uvcg_video_disable() ensures

		 * that in-flight uvc_buffers are returned, so we can

		 * safely call free_request without worrying about

		 * last_buf.

		 */

		uvc_video_free_request(ureq, ep);

		spin_unlock_irqrestore(&video->req_lock, flags);

		return;

	}



	last_buf = ureq->last_buf;

	ureq->last_buf = NULL;

	spin_unlock_irqrestore(&video->req_lock, flags);



	switch (req->status) {

	case 0:

		break;
@@ -295,17 +316,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) 
		uvcg_queue_cancel(queue, 0);

	}



	if (ureq->last_buf) {

		uvcg_complete_buffer(&video->queue, ureq->last_buf);

		ureq->last_buf = NULL;

	if (last_buf) {

		spin_lock_irqsave(&queue->irqlock, flags);

		uvcg_complete_buffer(queue, last_buf);

		spin_unlock_irqrestore(&queue->irqlock, flags);

	}



	spin_lock_irqsave(&video->req_lock, flags);

	/*

	 * Video stream might have been disabled while we were

	 * processing the current usb_request. So make sure

	 * we're still streaming before queueing the usb_request

	 * back to req_free

	 */

	if (video->is_enabled) {

		list_add_tail(&req->list, &video->req_free);

	spin_unlock_irqrestore(&video->req_lock, flags);



	if (uvc->state == UVC_STATE_STREAMING)

		queue_work(video->async_wq, &video->pump);

	} else {

		uvc_video_free_request(ureq, ep);

	}

	spin_unlock_irqrestore(&video->req_lock, flags);

}



static int
@@ -392,20 +422,22 @@ static void uvcg_video_pump(struct work_struct *work) 
	struct uvc_video_queue *queue = &video->queue;

	/* video->max_payload_size is only set when using bulk transfer */

	bool is_bulk = video->max_payload_size;

	struct uvc_device *uvc = video->uvc;

	struct usb_request *req = NULL;

	struct uvc_buffer *buf;

	unsigned long flags;

	bool buf_done;

	int ret;



	while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) {

	while (true) {

		if (!video->ep->enabled)

			return;



		/*

		 * Retrieve the first available USB request, protected by the

		 * request lock.

		 * Check is_enabled and retrieve the first available USB

		 * request, protected by the request lock.

		 */

		spin_lock_irqsave(&video->req_lock, flags);

		if (list_empty(&video->req_free)) {

		if (!video->is_enabled || list_empty(&video->req_free)) {

			spin_unlock_irqrestore(&video->req_lock, flags);

			return;

		}
@@ -487,9 +519,11 @@ static void uvcg_video_pump(struct work_struct *work) 
		return;



	spin_lock_irqsave(&video->req_lock, flags);

	if (video->is_enabled)

		list_add_tail(&req->list, &video->req_free);

	else

		uvc_video_free_request(req->context, video->ep);

	spin_unlock_irqrestore(&video->req_lock, flags);

	return;

}



/*
@@ -498,7 +532,11 @@ static void uvcg_video_pump(struct work_struct *work) 
int

uvcg_video_disable(struct uvc_video *video)

{

	struct uvc_request *ureq;

	unsigned long flags;

	struct list_head inflight_bufs;

	struct usb_request *req, *temp;

	struct uvc_buffer *buf, *btemp;

	struct uvc_request *ureq, *utemp;



	if (video->ep == NULL) {

		uvcg_info(&video->uvc->func,
@@ -506,15 +544,58 @@ uvcg_video_disable(struct uvc_video *video) 
		return -ENODEV;

	}



	INIT_LIST_HEAD(&inflight_bufs);

	spin_lock_irqsave(&video->req_lock, flags);

	video->is_enabled = false;



	/*

	 * Remove any in-flight buffers from the uvc_requests

	 * because we want to return them before cancelling the

	 * queue. This ensures that we aren't stuck waiting for

	 * all complete callbacks to come through before disabling

	 * vb2 queue.

	 */

	list_for_each_entry(ureq, &video->ureqs, list) {

		if (ureq->last_buf) {

			list_add_tail(&ureq->last_buf->queue, &inflight_bufs);

			ureq->last_buf = NULL;

		}

	}

	spin_unlock_irqrestore(&video->req_lock, flags);



	cancel_work_sync(&video->pump);

	uvcg_queue_cancel(&video->queue, 0);



	list_for_each_entry(ureq, &video->ureqs, list) {

		if (ureq->req)

			usb_ep_dequeue(video->ep, ureq->req);

	spin_lock_irqsave(&video->req_lock, flags);

	/*

	 * Remove all uvc_requests from ureqs with list_del_init

	 * This lets uvc_video_free_request correctly identify

	 * if the uvc_request is attached to a list or not when freeing

	 * memory.

	 */

	list_for_each_entry_safe(ureq, utemp, &video->ureqs, list)

		list_del_init(&ureq->list);



	list_for_each_entry_safe(req, temp, &video->req_free, list) {

		list_del(&req->list);

		uvc_video_free_request(req->context, video->ep);

	}



	uvc_video_free_requests(video);

	INIT_LIST_HEAD(&video->ureqs);

	INIT_LIST_HEAD(&video->req_free);

	video->req_size = 0;

	spin_unlock_irqrestore(&video->req_lock, flags);



	/*

	 * Return all the video buffers before disabling the queue.

	 */

	spin_lock_irqsave(&video->queue.irqlock, flags);

	list_for_each_entry_safe(buf, btemp, &inflight_bufs, queue) {

		list_del(&buf->queue);

		uvcg_complete_buffer(&video->queue, buf);

	}

	spin_unlock_irqrestore(&video->queue.irqlock, flags);



	uvcg_queue_enable(&video->queue, 0);

	return 0;

}
@@ -532,6 +613,14 @@ int uvcg_video_enable(struct uvc_video *video) 
		return -ENODEV;

	}



	/*

	 * Safe to access request related fields without req_lock because

	 * this is the only thread currently active, and no other

	 * request handling thread will become active until this function

	 * returns.

	 */

	video->is_enabled = true;



	if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0)

		return ret;
@@ -557,6 +646,7 @@ int uvcg_video_enable(struct uvc_video *video) 
 */

int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)

{

	video->is_enabled = false;

	INIT_LIST_HEAD(&video->ureqs);

	INIT_LIST_HEAD(&video->req_free);

	spin_lock_init(&video->req_lock);