| Message ID | 20230206225258.2302954-1-xnzhao@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: dummyhcd: Fix use-after-free in dummy_free_request | expand |
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
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.
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); }
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(-)