| Message ID | 20231215210746.821494-1-arakesh@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] usb: gadget: uvc: Fix use are free during STREAMOFF | expand |
On Fri, Dec 15, 2023 at 01:07:44PM -0800, Avichal Rakesh wrote: > There is a path that may lead to freed memory being referenced, > and causing kernel panics. > > The kernel panic has the following stack trace: > > Workqueue: uvcgadget uvcg_video_pump.c51fb85fece46625450f86adbf92c56c.cfi_jt > pstate: 60c00085 (nZCv daIf +PAN +UAO -TCO BTYPE=--) > pc : __list_del_entry_valid+0xc0/0xd4 > lr : __list_del_entry_valid+0xc0/0xd4 > Call trace: > __list_del_entry_valid+0xc0/0xd4 > uvc_video_free_request+0x60/0x98 > uvcg_video_pump+0x1cc/0x204 > process_one_work+0x21c/0x4b8 > worker_thread+0x29c/0x574 > kthread+0x158/0x1b0 > ret_from_fork+0x10/0x30 > > The root cause is that uvcg_video_usb_req_queue frees the uvc_request > if is_enabled is false and returns an error status. video_pump also > frees the associated request if uvcg_video_usb_req_queue returns an e > rror status, leading to double free and accessing garbage memory. Odd line wrapping :( > > To fix the issue, this patch removes freeing logic from > uvcg_video_usb_req_queue, and lets the callers to the function handle > queueing errors as they see fit. > > Signed-off-by: Avichal Rakesh <arakesh@google.com> > --- > drivers/usb/gadget/function/uvc_video.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) What commit id does this fix? > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index 98ba524c27f5..e5db1be14ca3 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -277,8 +277,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video, > struct list_head *list = NULL; > > if (!video->is_enabled) { > - uvc_video_free_request(req->context, video->ep); > - return -ENODEV; > + return -EINVAL; Isn't this a separate change? And does it actually matter? thanks, greg k-h
Thank you for the review, Greg! Sent out V2 with the comments addressed. On 1/4/24 06:19, Greg KH wrote: > On Fri, Dec 15, 2023 at 01:07:44PM -0800, Avichal Rakesh wrote: >> There is a path that may lead to freed memory being referenced, >> and causing kernel panics. >> >> The kernel panic has the following stack trace: >> >> Workqueue: uvcgadget uvcg_video_pump.c51fb85fece46625450f86adbf92c56c.cfi_jt >> pstate: 60c00085 (nZCv daIf +PAN +UAO -TCO BTYPE=--) >> pc : __list_del_entry_valid+0xc0/0xd4 >> lr : __list_del_entry_valid+0xc0/0xd4 >> Call trace: >> __list_del_entry_valid+0xc0/0xd4 >> uvc_video_free_request+0x60/0x98 >> uvcg_video_pump+0x1cc/0x204 >> process_one_work+0x21c/0x4b8 >> worker_thread+0x29c/0x574 >> kthread+0x158/0x1b0 >> ret_from_fork+0x10/0x30 >> >> The root cause is that uvcg_video_usb_req_queue frees the uvc_request >> if is_enabled is false and returns an error status. video_pump also >> frees the associated request if uvcg_video_usb_req_queue returns an e >> rror status, leading to double free and accessing garbage memory. > > Odd line wrapping :( Sigh! Fixed. Apologies. > >> >> To fix the issue, this patch removes freeing logic from >> uvcg_video_usb_req_queue, and lets the callers to the function handle >> queueing errors as they see fit. >> >> Signed-off-by: Avichal Rakesh <arakesh@google.com> >> --- >> drivers/usb/gadget/function/uvc_video.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) > > What commit id does this fix? The commit that caused the bug is not in linus' branch yet. Added the "Fixes" tag with the commit SHA from usb-next branch. Let me know if I should be using some other SHA. > > >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >> index 98ba524c27f5..e5db1be14ca3 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -277,8 +277,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video, >> struct list_head *list = NULL; >> >> if (!video->is_enabled) { >> - uvc_video_free_request(req->context, video->ep); >> - return -ENODEV; >> + return -EINVAL; > > Isn't this a separate change? And does it actually matter? It was meant to differentiate between usb_ep_queue failing and the video stream being disabled, but we don't really care about that. Reverted. Thank you! - Avi.
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 98ba524c27f5..e5db1be14ca3 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -277,8 +277,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video, struct list_head *list = NULL; if (!video->is_enabled) { - uvc_video_free_request(req->context, video->ep); - return -ENODEV; + return -EINVAL; } if (queue_to_ep) { struct uvc_request *ureq = req->context; @@ -464,8 +463,15 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) * and this thread for isoc endpoints. */ ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk); - if (ret < 0) + if (ret < 0) { + /* + * Endpoint error, but the stream is still enabled. + * Put request back in req_free for it to be cleaned + * up later. + */ uvcg_queue_cancel(queue, 0); + list_add_tail(&to_queue->list, &video->req_free); + } } else { uvc_video_free_request(ureq, ep); }
There is a path that may lead to freed memory being referenced, and causing kernel panics. The kernel panic has the following stack trace: Workqueue: uvcgadget uvcg_video_pump.c51fb85fece46625450f86adbf92c56c.cfi_jt pstate: 60c00085 (nZCv daIf +PAN +UAO -TCO BTYPE=--) pc : __list_del_entry_valid+0xc0/0xd4 lr : __list_del_entry_valid+0xc0/0xd4 Call trace: __list_del_entry_valid+0xc0/0xd4 uvc_video_free_request+0x60/0x98 uvcg_video_pump+0x1cc/0x204 process_one_work+0x21c/0x4b8 worker_thread+0x29c/0x574 kthread+0x158/0x1b0 ret_from_fork+0x10/0x30 The root cause is that uvcg_video_usb_req_queue frees the uvc_request if is_enabled is false and returns an error status. video_pump also frees the associated request if uvcg_video_usb_req_queue returns an e rror status, leading to double free and accessing garbage memory. To fix the issue, this patch removes freeing logic from uvcg_video_usb_req_queue, and lets the callers to the function handle queueing errors as they see fit. Signed-off-by: Avichal Rakesh <arakesh@google.com> --- drivers/usb/gadget/function/uvc_video.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)