diff mbox series

[v11,4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests

Message ID 20231102201939.4171214-4-arakesh@google.com
State New
Headers show
Series [v11,1/4] usb: gadget: uvc: prevent use of disabled endpoint | expand

Commit Message

Avichal Rakesh Nov. 2, 2023, 8:19 p.m. UTC
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: Michael Grzeschik <m.grzeschik@pengutronix.de>
Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Avichal Rakesh <arakesh@google.com>
---
v1  -> v2  : Rebased to ToT, and fixed deadlock reported in
             https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/
v2  -> v3  : Fix email threading goof-up
v3  -> v4  : re-rebase to ToT & moved to a uvc_video level lock
             as discussed in
             https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/
v4  -> v5  : Address review comments. Add Reviewed-by & Tested-by.
v5  -> v6  : Added another patch before this one to make uvcg_video_disable
             easier to review.
v6  -> v7  : Fix warning reported in
             https://lore.kernel.org/202310200457.GwPPFuHX-lkp@intel.com/
v7  -> v8  : No change. Getting back in review queue
v8  -> v9  : No change.
v9  -> v10 : Address review comments. Rebase to ToT (usb-next)
v10 -> v11 : Address review comments

 drivers/usb/gadget/function/uvc.h       |   1 +
 drivers/usb/gadget/function/uvc_v4l2.c  |  10 +-
 drivers/usb/gadget/function/uvc_video.c | 130 ++++++++++++++++++++----
 3 files changed, 112 insertions(+), 29 deletions(-)

--
2.42.0.869.gea05f2083d-goog

Comments

Daniel Scally Nov. 8, 2023, 2:15 p.m. UTC | #1
Hi Avichal

On 02/11/2023 20:19, Avichal Rakesh wrote:
> 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: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Avichal Rakesh <arakesh@google.com>
> ---


Thanks for the update. Let's leave the locking as it is; I think albeit not strictly necessary on 
that occasion it certainly is necessary to take the lock to protect the flags elsewhere, and 
probably better to be consistent with it.


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> v1  -> v2  : Rebased to ToT, and fixed deadlock reported in
>               https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/
> v2  -> v3  : Fix email threading goof-up
> v3  -> v4  : re-rebase to ToT & moved to a uvc_video level lock
>               as discussed in
>               https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/
> v4  -> v5  : Address review comments. Add Reviewed-by & Tested-by.
> v5  -> v6  : Added another patch before this one to make uvcg_video_disable
>               easier to review.
> v6  -> v7  : Fix warning reported in
>               https://lore.kernel.org/202310200457.GwPPFuHX-lkp@intel.com/
> v7  -> v8  : No change. Getting back in review queue
> v8  -> v9  : No change.
> v9  -> v10 : Address review comments. Rebase to ToT (usb-next)
> v10 -> v11 : Address review comments
>
>   drivers/usb/gadget/function/uvc.h       |   1 +
>   drivers/usb/gadget/function/uvc_v4l2.c  |  10 +-
>   drivers/usb/gadget/function/uvc_video.c | 130 ++++++++++++++++++++----
>   3 files changed, 112 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 993694da0bbc..be0d012aa244 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -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;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 904dd283cbf7..c7e5fa4f29e0 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -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;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index c3e8c48f46a9..164bdeb7f2a9 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -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);
> -	list_add_tail(&req->list, &video->req_free);
> -	spin_unlock_irqrestore(&video->req_lock, flags);
> -
> -	if (uvc->state == UVC_STATE_STREAMING)
> +	/*
> +	 * 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);
>   		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);
> -	list_add_tail(&req->list, &video->req_free);
> +	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);
> --
> 2.42.0.869.gea05f2083d-goog
Avichal Rakesh Nov. 9, 2023, 1 a.m. UTC | #2
On 11/8/23 06:15, Dan Scally wrote:
> Hi Avichal
> 
> On 02/11/2023 20:19, Avichal Rakesh wrote:
>> 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: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> Signed-off-by: Avichal Rakesh <arakesh@google.com>
>> ---
> 
> 
> Thanks for the update. Let's leave the locking as it is; I think albeit not strictly necessary on that occasion it certainly is necessary to take the lock to protect the flags elsewhere, and probably better to be consistent with it.
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Thank you for reviewing, Dan!

Greg, I just sent out v12 with the Reviewed-by tag:
https://lore.kernel.org/all/20231109004104.3467968-1-arakesh@google.com/ 
They should be ready to submit now. Thank you!

Regards,
Avi.

> 
>> <snip>
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 993694da0bbc..be0d012aa244 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -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;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 904dd283cbf7..c7e5fa4f29e0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -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;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index c3e8c48f46a9..164bdeb7f2a9 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -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);
-	list_add_tail(&req->list, &video->req_free);
-	spin_unlock_irqrestore(&video->req_lock, flags);
-
-	if (uvc->state == UVC_STATE_STREAMING)
+	/*
+	 * 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);
 		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);
-	list_add_tail(&req->list, &video->req_free);
+	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);