* [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request
@ 2023-02-06 22:52 Xin Zhao
2023-02-07 0:34 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: Xin Zhao @ 2023-02-06 22:52 UTC (permalink / raw)
To: gregkh, jakobkoschel, rdunlap, ira.weiny, linux-usb, linux-kernel
Cc: Xin Zhao
DummyHCD assume when dummy_free_request is called, the request
is already detached from request queues. It is correct in most
cases.
But when DummyHCD is detached from gadget configfs with pending
requests and some requests are still in pending queue,
dummy_free_request would free them directly.
Later on, dummy_udc_stop would iterate pending queue to release
the requests again.
Stacktrace for dummy_free_reqeust
```
kfree(const void * x) (slub.c:4200)
dummy_free_request(struct usb_ep * _ep, struct usb_request * _req) (dummy_hcd.c:691)
usb_ep_free_request(struct usb_ep * ep, struct usb_request * req) (core.c:201)
functionfs_unbind(struct ffs_data * ffs) (f_fs.c:1894)
ffs_func_unbind(struct usb_function * f) (f_fs.c:3614)
purge_configs_funcs(struct gadget_info * gi) (configfs.c:1303)
configfs_composite_unbind(struct usb_gadget * gadget) (configfs.c:1528)
usb_gadget_remove_driver(struct usb_udc * udc) (core.c:1436)
usb_gadget_unregister_driver(struct usb_gadget_driver * driver) (core.c:1585)
unregister_gadget(struct gadget_info * gi) (configfs.c:281)
gadget_dev_desc_UDC_store(struct config_item * item) (configfs.c:308)
flush_write_buffer(struct file * file, struct configfs_buffer * buffer, size_t count) (file.c:251)
```
Stacktrace of use-after-free
```
list_del_init(struct list_head * entry) (list.h:204)
nuke(struct dummy * dum, struct dummy_ep * ep) (dummy_hcd.c:344)
stop_activity(struct dummy * dum) (dummy_hcd.c:366)
dummy_udc_stop(struct usb_gadget * g) (dummy_hcd.c:1032)
usb_gadget_udc_stop(struct usb_udc * udc) (core.c:1141)
usb_gadget_remove_driver(struct usb_udc * udc) (core.c:1437)
usb_gadget_unregister_driver(struct usb_gadget_driver * driver) (core.c:1585)
unregister_gadget(struct gadget_info * gi) (configfs.c:281)
gadget_dev_desc_UDC_store(struct config_item * item) (configfs.c:308)
flush_write_buffer(struct file * file, struct configfs_buffer * buffer, size_t count) (file.c:251)
configfs_write_file(struct file * file)
```
Signed-off-by: Xin Zhao <xnzhao@google.com>
---
drivers/usb/gadget/udc/dummy_hcd.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index 899ac9f9c279..afead69d7487 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -679,7 +679,11 @@ static void dummy_free_request(struct usb_ep *_ep, struct usb_request *_req)
}
req = usb_request_to_dummy_request(_req);
- WARN_ON(!list_empty(&req->queue));
+ if (!list_empty(&req->queue)) {
+ WARN_ON(1);
+ return;
+ }
+
kfree(req);
}
--
2.39.1.519.gcb327c4b5f-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request
2023-02-06 22:52 [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request Xin Zhao
@ 2023-02-07 0:34 ` Alan Stern
2023-02-07 11:59 ` John Keeping
0 siblings, 1 reply; 4+ messages in thread
From: Alan Stern @ 2023-02-07 0:34 UTC (permalink / raw)
To: Xin Zhao
Cc: gregkh, jakobkoschel, rdunlap, ira.weiny, linux-usb, linux-kernel
On Mon, Feb 06, 2023 at 10:52:58PM +0000, Xin Zhao wrote:
> DummyHCD assume when dummy_free_request is called, the request
> is already detached from request queues. It is correct in most
> cases.
> But when DummyHCD is detached from gadget configfs with pending
> requests and some requests are still in pending queue,
> dummy_free_request would free them directly.
> Later on, dummy_udc_stop would iterate pending queue to release
> the requests again.
>
> Stacktrace for dummy_free_reqeust
> ```
> kfree(const void * x) (slub.c:4200)
> dummy_free_request(struct usb_ep * _ep, struct usb_request * _req) (dummy_hcd.c:691)
> usb_ep_free_request(struct usb_ep * ep, struct usb_request * req) (core.c:201)
> functionfs_unbind(struct ffs_data * ffs) (f_fs.c:1894)
That's the bug right there. The kerneldoc for usb_ep_free_request()
says "Caller guarantees the request is not queued". So it looks like
the real solution is to fix functionfs_unbind().
> ffs_func_unbind(struct usb_function * f) (f_fs.c:3614)
> purge_configs_funcs(struct gadget_info * gi) (configfs.c:1303)
> configfs_composite_unbind(struct usb_gadget * gadget) (configfs.c:1528)
> usb_gadget_remove_driver(struct usb_udc * udc) (core.c:1436)
> usb_gadget_unregister_driver(struct usb_gadget_driver * driver) (core.c:1585)
> unregister_gadget(struct gadget_info * gi) (configfs.c:281)
> gadget_dev_desc_UDC_store(struct config_item * item) (configfs.c:308)
> flush_write_buffer(struct file * file, struct configfs_buffer * buffer, size_t count) (file.c:251)
> ```
>
> Stacktrace of use-after-free
> ```
> list_del_init(struct list_head * entry) (list.h:204)
> nuke(struct dummy * dum, struct dummy_ep * ep) (dummy_hcd.c:344)
> stop_activity(struct dummy * dum) (dummy_hcd.c:366)
> dummy_udc_stop(struct usb_gadget * g) (dummy_hcd.c:1032)
> usb_gadget_udc_stop(struct usb_udc * udc) (core.c:1141)
> usb_gadget_remove_driver(struct usb_udc * udc) (core.c:1437)
> usb_gadget_unregister_driver(struct usb_gadget_driver * driver) (core.c:1585)
> unregister_gadget(struct gadget_info * gi) (configfs.c:281)
> gadget_dev_desc_UDC_store(struct config_item * item) (configfs.c:308)
> flush_write_buffer(struct file * file, struct configfs_buffer * buffer, size_t count) (file.c:251)
> configfs_write_file(struct file * file)
> ```
>
> Signed-off-by: Xin Zhao <xnzhao@google.com>
> ---
> drivers/usb/gadget/udc/dummy_hcd.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index 899ac9f9c279..afead69d7487 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -679,7 +679,11 @@ static void dummy_free_request(struct usb_ep *_ep, struct usb_request *_req)
> }
>
> req = usb_request_to_dummy_request(_req);
> - WARN_ON(!list_empty(&req->queue));
> + if (!list_empty(&req->queue)) {
> + WARN_ON(1);
> + return;
> + }
Once the bug in functionfs_unbind() is fixed, this change won't be
necessary.
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request
2023-02-07 0:34 ` Alan Stern
@ 2023-02-07 11:59 ` John Keeping
2023-02-07 21:58 ` Xin Zhao
0 siblings, 1 reply; 4+ messages in thread
From: John Keeping @ 2023-02-07 11:59 UTC (permalink / raw)
To: Alan Stern
Cc: Xin Zhao, gregkh, jakobkoschel, rdunlap, ira.weiny, linux-usb,
linux-kernel
On Mon, Feb 06, 2023 at 07:34:40PM -0500, Alan Stern wrote:
> On Mon, Feb 06, 2023 at 10:52:58PM +0000, Xin Zhao wrote:
> > DummyHCD assume when dummy_free_request is called, the request
> > is already detached from request queues. It is correct in most
> > cases.
> > But when DummyHCD is detached from gadget configfs with pending
> > requests and some requests are still in pending queue,
> > dummy_free_request would free them directly.
> > Later on, dummy_udc_stop would iterate pending queue to release
> > the requests again.
> >
> > Stacktrace for dummy_free_reqeust
> > ```
> > kfree(const void * x) (slub.c:4200)
> > dummy_free_request(struct usb_ep * _ep, struct usb_request * _req) (dummy_hcd.c:691)
> > usb_ep_free_request(struct usb_ep * ep, struct usb_request * req) (core.c:201)
> > functionfs_unbind(struct ffs_data * ffs) (f_fs.c:1894)
>
> That's the bug right there. The kerneldoc for usb_ep_free_request()
> says "Caller guarantees the request is not queued". So it looks like
> the real solution is to fix functionfs_unbind().
This is commit ce405d561b02 ("usb: gadget: f_fs: Ensure ep0req is
dequeued before free_request") IIUC.
Xin, are you able to test a version with that commit?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request
2023-02-07 11:59 ` John Keeping
@ 2023-02-07 21:58 ` Xin Zhao
0 siblings, 0 replies; 4+ messages in thread
From: Xin Zhao @ 2023-02-07 21:58 UTC (permalink / raw)
To: John Keeping
Cc: Alan Stern, gregkh, jakobkoschel, rdunlap, ira.weiny, linux-usb,
linux-kernel
On Tue, Feb 07, 2023 at 11:59:25AM +0000, John Keeping wrote:
> On Mon, Feb 06, 2023 at 07:34:40PM -0500, Alan Stern wrote:
> > On Mon, Feb 06, 2023 at 10:52:58PM +0000, Xin Zhao wrote:
> > > DummyHCD assume when dummy_free_request is called, the request
> > > is already detached from request queues. It is correct in most
> > > cases.
> > > But when DummyHCD is detached from gadget configfs with pending
> > > requests and some requests are still in pending queue,
> > > dummy_free_request would free them directly.
> > > Later on, dummy_udc_stop would iterate pending queue to release
> > > the requests again.
> > >
> > > Stacktrace for dummy_free_reqeust
> > > ```
> > > kfree(const void * x) (slub.c:4200)
> > > dummy_free_request(struct usb_ep * _ep, struct usb_request * _req) (dummy_hcd.c:691)
> > > usb_ep_free_request(struct usb_ep * ep, struct usb_request * req) (core.c:201)
> > > functionfs_unbind(struct ffs_data * ffs) (f_fs.c:1894)
> >
> > That's the bug right there. The kerneldoc for usb_ep_free_request()
> > says "Caller guarantees the request is not queued". So it looks like
> > the real solution is to fix functionfs_unbind().
>
> This is commit ce405d561b02 ("usb: gadget: f_fs: Ensure ep0req is
> dequeued before free_request") IIUC.
>
> Xin, are you able to test a version with that commit?
Yes John, the commit may fix the issue. Thanks for bring it up.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-07 21:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-06 22:52 [PATCH] usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request Xin Zhao
2023-02-07 0:34 ` Alan Stern
2023-02-07 11:59 ` John Keeping
2023-02-07 21:58 ` Xin Zhao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox