| 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 |
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
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 --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);