| Message ID | 20231024183605.908253-2-arakesh@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8,1/4] usb: gadget: uvc: prevent use of disabled endpoint | expand |
Hi Avichal - thanks for the patch On 24/10/2023 19:36, 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 patch is 1 of 2 patches addressing the use-after-free issue. > Instead of bulk allocating all uvc_requests as an array, this patch > allocates uvc_requests one at a time, which should allows for similar > granularity when deallocating the uvc_requests. This patch has no > functional changes other than allocating each uvc_request separately, > and similarly freeing each of them separately. > > Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com > Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Reviewed-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 > v2 -> v3: Fix email threading goof-up > v3 -> v4: Address review comments & re-rebase to ToT > v4 -> v5: Address more review comments. Add Reviewed-by & Tested-by. > v5 -> v6: No change > v6 -> v7: No change > v7 -> v8: No change. Getting back in review queue > > drivers/usb/gadget/function/uvc.h | 3 +- > drivers/usb/gadget/function/uvc_video.c | 89 ++++++++++++++----------- > 2 files changed, 52 insertions(+), 40 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 989bc6b4e93d..993694da0bbc 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -81,6 +81,7 @@ struct uvc_request { > struct sg_table sgt; > u8 header[UVCG_REQUEST_HEADER_LEN]; > struct uvc_buffer *last_buf; > + struct list_head list; > }; > > struct uvc_video { > @@ -102,7 +103,7 @@ struct uvc_video { > > /* Requests */ > unsigned int req_size; > - struct uvc_request *ureq; > + struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ > struct list_head req_free; > spinlock_t req_lock; > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index c334802ac0a4..c180866c8e34 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -227,6 +227,24 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > * Request handling > */ > > +static void > +uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) > +{ > + sg_free_table(&ureq->sgt); > + if (ureq->req && ep) { > + usb_ep_free_request(ep, ureq->req); > + ureq->req = NULL; > + } > + > + kfree(ureq->req_buffer); > + ureq->req_buffer = NULL; > + > + if (!list_empty(&ureq->list)) Is this conditional needed? You can only get here through the list_for_each_entry_safe() > + list_del_init(&ureq->list); > + > + kfree(ureq); > +} > + > static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) > { > int ret; > @@ -293,27 +311,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > static int > uvc_video_free_requests(struct uvc_video *video) > { > - unsigned int i; > - > - if (video->ureq) { > - for (i = 0; i < video->uvc_num_requests; ++i) { > - sg_free_table(&video->ureq[i].sgt); > + struct uvc_request *ureq, *temp; > > - if (video->ureq[i].req) { > - usb_ep_free_request(video->ep, video->ureq[i].req); > - video->ureq[i].req = NULL; > - } > - > - if (video->ureq[i].req_buffer) { > - kfree(video->ureq[i].req_buffer); > - video->ureq[i].req_buffer = NULL; > - } > - } > - > - kfree(video->ureq); > - video->ureq = NULL; > - } > + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) > + uvc_video_free_request(ureq, video->ep); > > + INIT_LIST_HEAD(&video->ureqs); > INIT_LIST_HEAD(&video->req_free); > video->req_size = 0; > return 0; > @@ -322,6 +325,7 @@ uvc_video_free_requests(struct uvc_video *video) > static int > uvc_video_alloc_requests(struct uvc_video *video) > { > + struct uvc_request *ureq; > unsigned int req_size; > unsigned int i; > int ret = -ENOMEM; > @@ -332,29 +336,34 @@ uvc_video_alloc_requests(struct uvc_video *video) > * max_t(unsigned int, video->ep->maxburst, 1) > * (video->ep->mult); > > - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); > - if (video->ureq == NULL) > - return -ENOMEM; > + INIT_LIST_HEAD(&video->ureqs); Probably unecessary here; it's done in uvc_video_free_requests() and uvcg_video_init() already > + for (i = 0; i < video->uvc_num_requests; i++) { > + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); > + if (ureq == NULL) > + goto error; > + > + INIT_LIST_HEAD(&ureq->list); > + > + list_add_tail(&ureq->list, &video->ureqs); > > - for (i = 0; i < video->uvc_num_requests; ++i) { > - video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); > - if (video->ureq[i].req_buffer == NULL) > + ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); > + if (ureq->req_buffer == NULL) > goto error; > > - video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); > - if (video->ureq[i].req == NULL) > + ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); > + if (ureq->req == NULL) > goto error; > > - video->ureq[i].req->buf = video->ureq[i].req_buffer; > - video->ureq[i].req->length = 0; > - video->ureq[i].req->complete = uvc_video_complete; > - video->ureq[i].req->context = &video->ureq[i]; > - video->ureq[i].video = video; > - video->ureq[i].last_buf = NULL; > + ureq->req->buf = ureq->req_buffer; > + ureq->req->length = 0; > + ureq->req->complete = uvc_video_complete; > + ureq->req->context = ureq; > + ureq->video = video; > + ureq->last_buf = NULL; > > - list_add_tail(&video->ureq[i].req->list, &video->req_free); > + list_add_tail(&ureq->req->list, &video->req_free); > /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ > - sg_alloc_table(&video->ureq[i].sgt, > + sg_alloc_table(&ureq->sgt, > DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, > PAGE_SIZE) + 2, GFP_KERNEL); > } > @@ -489,8 +498,8 @@ static void uvcg_video_pump(struct work_struct *work) > */ > int uvcg_video_enable(struct uvc_video *video, int enable) > { > - unsigned int i; > int ret; > + struct uvc_request *ureq; > > if (video->ep == NULL) { > uvcg_info(&video->uvc->func, > @@ -502,9 +511,10 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > cancel_work_sync(&video->pump); > uvcg_queue_cancel(&video->queue, 0); > > - for (i = 0; i < video->uvc_num_requests; ++i) > - if (video->ureq && video->ureq[i].req) > - usb_ep_dequeue(video->ep, video->ureq[i].req); > + list_for_each_entry(ureq, &video->ureqs, list) { > + if (ureq->req) > + usb_ep_dequeue(video->ep, ureq->req); > + } > > uvc_video_free_requests(video); > uvcg_queue_enable(&video->queue, 0); > @@ -536,6 +546,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > */ > int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) > { > + INIT_LIST_HEAD(&video->ureqs); > INIT_LIST_HEAD(&video->req_free); > spin_lock_init(&video->req_lock); > INIT_WORK(&video->pump, uvcg_video_pump); > -- > 2.42.0.758.gaed0368e0e-goog
Thank you for the reviews, Dan! Uploaded v9 with the comments addressed. On 10/27/23 05:57, Dan Scally wrote: > Hi Avichal - thanks for the patch > > On 24/10/2023 19:36, 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. >> >> <snip> >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >> index c334802ac0a4..c180866c8e34 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -227,6 +227,24 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, >> * Request handling >> */ >> >> +static void >> +uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) >> +{ >> + sg_free_table(&ureq->sgt); >> + if (ureq->req && ep) { >> + usb_ep_free_request(ep, ureq->req); >> + ureq->req = NULL; >> + } >> + >> + kfree(ureq->req_buffer); >> + ureq->req_buffer = NULL; >> + >> + if (!list_empty(&ureq->list)) > > > Is this conditional needed? You can only get here through the list_for_each_entry_safe() Strictly speaking, we don't need this check right now. As you said, we currently only get to this from within a list_for_each_entry_safe block. However, we end up needing the check in the very next patch. Considering this is a function with no real control over who might call it, it seemed reasonable to write this a little defensively in case of a partial revert of the patchset. > >> + list_del_init(&ureq->list); >> + >> + kfree(ureq); >> +} >> + >> <snip> >> @@ -322,6 +325,7 @@ uvc_video_free_requests(struct uvc_video *video) >> static int >> uvc_video_alloc_requests(struct uvc_video *video) >> { >> + struct uvc_request *ureq; >> unsigned int req_size; >> unsigned int i; >> int ret = -ENOMEM; >> @@ -332,29 +336,34 @@ uvc_video_alloc_requests(struct uvc_video *video) >> * max_t(unsigned int, video->ep->maxburst, 1) >> * (video->ep->mult); >> >> - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); >> - if (video->ureq == NULL) >> - return -ENOMEM; >> + INIT_LIST_HEAD(&video->ureqs); > > > Probably unecessary here; it's done in uvc_video_free_requests() and uvcg_video_init() already Ah, that is fair. Added a BUG_ON instead, like we do for video->req_size so we still catch cases where the state might be inconsistent. > >> + for (i = 0; i < video->uvc_num_requests; i++) { >> + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); >> + if (ureq == NULL) >> + goto error; >> + >> + INIT_LIST_HEAD(&ureq->list); >> + >> + list_add_tail(&ureq->list, &video->ureqs); >> >> <snip> Regards, Avi.
On Fri, Oct 27, 2023 at 01:31:26PM -0700, Avichal Rakesh wrote: > >> @@ -322,6 +325,7 @@ uvc_video_free_requests(struct uvc_video *video) > >> static int > >> uvc_video_alloc_requests(struct uvc_video *video) > >> { > >> + struct uvc_request *ureq; > >> unsigned int req_size; > >> unsigned int i; > >> int ret = -ENOMEM; > >> @@ -332,29 +336,34 @@ uvc_video_alloc_requests(struct uvc_video *video) > >> * max_t(unsigned int, video->ep->maxburst, 1) > >> * (video->ep->mult); > >> > >> - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); > >> - if (video->ureq == NULL) > >> - return -ENOMEM; > >> + INIT_LIST_HEAD(&video->ureqs); > > > > > > Probably unecessary here; it's done in uvc_video_free_requests() and uvcg_video_init() already > > Ah, that is fair. Added a BUG_ON instead, like we do for video->req_size > so we still catch cases where the state might be inconsistent. Please no, that means you just crashed a machine and all data is lost and the user will get very mad. Either handle the error properly or it's something that can never happen and so you don't need to handle it. thanks, greg k-h
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 989bc6b4e93d..993694da0bbc 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -81,6 +81,7 @@ struct uvc_request { struct sg_table sgt; u8 header[UVCG_REQUEST_HEADER_LEN]; struct uvc_buffer *last_buf; + struct list_head list; }; struct uvc_video { @@ -102,7 +103,7 @@ struct uvc_video { /* Requests */ unsigned int req_size; - struct uvc_request *ureq; + struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ struct list_head req_free; spinlock_t req_lock; diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index c334802ac0a4..c180866c8e34 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -227,6 +227,24 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, * Request handling */ +static void +uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) +{ + sg_free_table(&ureq->sgt); + if (ureq->req && ep) { + usb_ep_free_request(ep, ureq->req); + ureq->req = NULL; + } + + kfree(ureq->req_buffer); + ureq->req_buffer = NULL; + + if (!list_empty(&ureq->list)) + list_del_init(&ureq->list); + + kfree(ureq); +} + static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) { int ret; @@ -293,27 +311,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) static int uvc_video_free_requests(struct uvc_video *video) { - unsigned int i; - - if (video->ureq) { - for (i = 0; i < video->uvc_num_requests; ++i) { - sg_free_table(&video->ureq[i].sgt); + struct uvc_request *ureq, *temp; - if (video->ureq[i].req) { - usb_ep_free_request(video->ep, video->ureq[i].req); - video->ureq[i].req = NULL; - } - - if (video->ureq[i].req_buffer) { - kfree(video->ureq[i].req_buffer); - video->ureq[i].req_buffer = NULL; - } - } - - kfree(video->ureq); - video->ureq = NULL; - } + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) + uvc_video_free_request(ureq, video->ep); + INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); video->req_size = 0; return 0; @@ -322,6 +325,7 @@ uvc_video_free_requests(struct uvc_video *video) static int uvc_video_alloc_requests(struct uvc_video *video) { + struct uvc_request *ureq; unsigned int req_size; unsigned int i; int ret = -ENOMEM; @@ -332,29 +336,34 @@ uvc_video_alloc_requests(struct uvc_video *video) * max_t(unsigned int, video->ep->maxburst, 1) * (video->ep->mult); - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); - if (video->ureq == NULL) - return -ENOMEM; + INIT_LIST_HEAD(&video->ureqs); + for (i = 0; i < video->uvc_num_requests; i++) { + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); + if (ureq == NULL) + goto error; + + INIT_LIST_HEAD(&ureq->list); + + list_add_tail(&ureq->list, &video->ureqs); - for (i = 0; i < video->uvc_num_requests; ++i) { - video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); - if (video->ureq[i].req_buffer == NULL) + ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); + if (ureq->req_buffer == NULL) goto error; - video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); - if (video->ureq[i].req == NULL) + ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); + if (ureq->req == NULL) goto error; - video->ureq[i].req->buf = video->ureq[i].req_buffer; - video->ureq[i].req->length = 0; - video->ureq[i].req->complete = uvc_video_complete; - video->ureq[i].req->context = &video->ureq[i]; - video->ureq[i].video = video; - video->ureq[i].last_buf = NULL; + ureq->req->buf = ureq->req_buffer; + ureq->req->length = 0; + ureq->req->complete = uvc_video_complete; + ureq->req->context = ureq; + ureq->video = video; + ureq->last_buf = NULL; - list_add_tail(&video->ureq[i].req->list, &video->req_free); + list_add_tail(&ureq->req->list, &video->req_free); /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ - sg_alloc_table(&video->ureq[i].sgt, + sg_alloc_table(&ureq->sgt, DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, PAGE_SIZE) + 2, GFP_KERNEL); } @@ -489,8 +498,8 @@ static void uvcg_video_pump(struct work_struct *work) */ int uvcg_video_enable(struct uvc_video *video, int enable) { - unsigned int i; int ret; + struct uvc_request *ureq; if (video->ep == NULL) { uvcg_info(&video->uvc->func, @@ -502,9 +511,10 @@ int uvcg_video_enable(struct uvc_video *video, int enable) cancel_work_sync(&video->pump); uvcg_queue_cancel(&video->queue, 0); - for (i = 0; i < video->uvc_num_requests; ++i) - if (video->ureq && video->ureq[i].req) - usb_ep_dequeue(video->ep, video->ureq[i].req); + list_for_each_entry(ureq, &video->ureqs, list) { + if (ureq->req) + usb_ep_dequeue(video->ep, ureq->req); + } uvc_video_free_requests(video); uvcg_queue_enable(&video->queue, 0); @@ -536,6 +546,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) */ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) { + INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); spin_lock_init(&video->req_lock); INIT_WORK(&video->pump, uvcg_video_pump);