USB mass storage gadget may fail during bind. USB multi gadget error path for failed bind is broken. USB mass storage gadget unbind leaks resources. Fix these three issues. Ahmad Fatoum (6): usb: gadget: implement and use system_partitions_get_null usb: gadget: don't register UMS with empty function usb: gadget: mass-storage: fix clean up of file descriptors usb: gadget: mass-storage: reference count allocations used in bthread usb: gadget: multi: fix broken handling of USB function bind error usb: gadget: multi: free UMS instance at multi_unbind time common/fastboot.c | 4 +- common/usbgadget.c | 6 +-- drivers/usb/gadget/f_mass_storage.c | 64 +++++++++++++++++++++++------ drivers/usb/gadget/multi.c | 24 ++++++++--- include/system-partitions.h | 8 ++++ include/usb/mass_storage.h | 1 + 6 files changed, 81 insertions(+), 26 deletions(-) -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
system_partitions_get() clones the system partitions file list and returns the copy. usb multi gadget code expects disabled gadgets to have a NULL file list, not an empty one, so fastboot and DFU handle this case. Add a new system_partitions_get_null helper that can be used instead. This will be used for USB mass storage gadget as well in a follow-up commit. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- common/fastboot.c | 4 +--- common/usbgadget.c | 4 +--- include/system-partitions.h | 8 ++++++++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/common/fastboot.c b/common/fastboot.c index 04a8573b4adb..f8ed40c86e00 100644 --- a/common/fastboot.c +++ b/common/fastboot.c @@ -920,9 +920,7 @@ struct file_list *get_fastboot_partitions(void) { if (fastboot_partitions && *fastboot_partitions) return file_list_parse_null(fastboot_partitions); - if (!system_partitions_empty()) - return system_partitions_get(); - return NULL; + return system_partitions_get_null(); } static int fastboot_globalvars_init(void) diff --git a/common/usbgadget.c b/common/usbgadget.c index e8c9f7d236d3..92e486199556 100644 --- a/common/usbgadget.c +++ b/common/usbgadget.c @@ -27,9 +27,7 @@ static inline struct file_list *get_dfu_function(void) { if (dfu_function && *dfu_function) return file_list_parse_null(dfu_function); - if (!system_partitions_empty()) - return system_partitions_get(); - return NULL; + return system_partitions_get_null(); } int usbgadget_register(const struct usbgadget_funcs *funcs) diff --git a/include/system-partitions.h b/include/system-partitions.h index 86de3612ccd8..e6d1a0f88bf3 100644 --- a/include/system-partitions.h +++ b/include/system-partitions.h @@ -2,6 +2,7 @@ #ifndef SYSTEM_PARTITIONS_H_ #define SYSTEM_PARTITIONS_H_ +#include <linux/types.h> #include <file-list.h> #ifdef CONFIG_SYSTEM_PARTITIONS @@ -37,4 +38,11 @@ static inline bool system_partitions_empty(void) #endif +static inline struct file_list *system_partitions_get_null(void) +{ + if (system_partitions_empty()) + return NULL; + return system_partitions_get(); +} + #endif -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
system_partitions_get clones the system partitions and passes it along. DFU and Fastboot use system partitions as a fallback and pass along a NULL file list if they are empty. This enables e.g. usbgadget -A '' to work: No files are expored, but fastboot OEM commands are possible. USB mass storage though does pass along an empty system partitions file list instead of NULL, which leads to bind failure, because UMS gadget refuses to bind with no LUNs. Detect this case. Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- common/usbgadget.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/usbgadget.c b/common/usbgadget.c index 92e486199556..2ec6d9226cca 100644 --- a/common/usbgadget.c +++ b/common/usbgadget.c @@ -52,7 +52,7 @@ int usbgadget_register(const struct usbgadget_funcs *funcs) opts->ums_opts.files = file_list_parse_null(funcs->ums_opts); if (IS_ENABLED(CONFIG_USB_GADGET_MASS_STORAGE) && file_list_empty(opts->ums_opts.files)) { file_list_free(opts->ums_opts.files); - opts->ums_opts.files = system_partitions_get(); + opts->ums_opts.files = system_partitions_get_null(); } } -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
When encountering an issue during bind time, we currently close only the last opened file descriptor, leaking others when more than one LUN was registered. Fix this. Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/usb/gadget/f_mass_storage.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 753042125d14..a49ac7803337 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -2419,7 +2419,7 @@ static int fsg_common_init(struct fsg_common *common, struct usb_gadget *gadget = cdev->gadget; struct file_list_entry *fentry; struct fsg_buffhd *bh; - int nluns, i, fd = -1, rc; + int nluns, i, rc; ums_count = 0; @@ -2436,17 +2436,20 @@ static int fsg_common_init(struct fsg_common *common, file_list_for_each_entry(ums_files, fentry) { unsigned flags = O_RDWR; struct stat st; + int fd; if (fentry->flags) { pr_err("flags not supported\n"); - return -ENOSYS; + rc = -ENOSYS; + goto close; } fd = open(fentry->filename, flags); if (fd < 0) { pr_err("open('%s') failed: %pe\n", fentry->filename, ERR_PTR(fd)); - return fd; + rc = fd; + goto close; } rc = fstat(fd, &st); @@ -2543,7 +2546,8 @@ error_release: common->state = FSG_STATE_TERMINATED; /* The thread is dead */ fsg_common_release(common); close: - close(fd); + for (i = 0; i < ums_count; i++) + close(ums[i].fd); return rc; } -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Since 997cca0f15dc ("bthread: replace blocking bthread_stop with nonblocking bthread_cancel"), the bthread may survive longer than the multigadget unbind. This didn't cause issues so far, because the multi gadget unbind didn't call usb_put_function[_instance] for mass-storage (but did so for other functions), so we just leaked the memory. In preparation for fixing the memory leak, we will need to straighten out the mass storage cleanup. We do so by reference counting the two shared structures: If bthread runs before usb_put_function[_instance], it will not free them yet (avoiding a double free) and if bthread runs after usb_put_function[_instance], it will still be able to access them (avoiding a use-after-free). A cleaner way would've been to wait for bthread completion, but we can't do that here, because gadget could be unbound in a poller and bthreads are only scheduled in command context. Fixes: 997cca0f15dc ("bthread: replace blocking bthread_stop with nonblocking bthread_cancel") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/usb/gadget/f_mass_storage.c | 52 ++++++++++++++++++++++++----- include/usb/mass_storage.h | 1 + 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index a49ac7803337..1c26c4d99681 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -263,8 +263,6 @@ static struct usb_gadget_strings fsg_stringtab = { struct bthread *thread_task; -struct kref {int x; }; - struct fsg_dev; static struct file_list *ums_files; @@ -282,6 +280,8 @@ struct fsg_common { struct fsg_buffhd *next_buffhd_to_drain; struct fsg_buffhd buffhds[FSG_NUM_BUFFERS]; + struct f_ums_opts *opts; + int cmnd_size; u8 cmnd[MAX_COMMAND_SIZE]; @@ -322,6 +322,20 @@ struct fsg_common { char inquiry_string[8 + 16 + 4 + 1]; }; +static struct f_ums_opts *f_ums_opts_get(struct f_ums_opts *opts) +{ + opts->refcnt++; + return opts; +} + +static void f_ums_opts_put(struct f_ums_opts *opts) +{ + if (--opts->refcnt == 0) { + kfree(opts->common); + kfree(opts); + } +} + struct fsg_config { unsigned nluns; struct fsg_lun_config { @@ -348,6 +362,8 @@ struct fsg_dev { struct usb_gadget *gadget; /* Copy of cdev->gadget */ struct fsg_common *common; + int refcnt; + u16 interface_number; unsigned int bulk_in_enabled:1; @@ -360,6 +376,17 @@ struct fsg_dev { struct usb_ep *bulk_out; }; +static struct fsg_dev *fsg_dev_get(struct fsg_dev *fsg) +{ + fsg->refcnt++; + return fsg; +} + +static void fsg_dev_put(struct fsg_dev *fsg) +{ + if (--fsg->refcnt == 0) + kfree(fsg); +} static inline int __fsg_is_set(struct fsg_common *common, const char *func, unsigned line) @@ -2337,12 +2364,14 @@ static void handle_exception(struct fsg_common *common) static void fsg_main_thread(void *fsg_) { - struct fsg_dev *fsg = fsg_; + struct fsg_dev *fsg = fsg_dev_get(fsg_); struct fsg_common *common = fsg->common; + struct f_ums_opts *opts = f_ums_opts_get(common->opts); struct fsg_buffhd *bh; unsigned i; int ret = 0; + /* The main loop */ while (common->state != FSG_STATE_TERMINATED) { if (exception_in_progress(common)) { @@ -2394,11 +2423,14 @@ static void fsg_main_thread(void *fsg_) ums_count = 0; ums_files = NULL; + + f_ums_opts_put(opts); + fsg_dev_put(fsg); } static void fsg_common_release(struct fsg_common *common); -static struct fsg_common *fsg_common_setup(void) +static struct fsg_common *fsg_common_setup(struct f_ums_opts *opts) { struct fsg_common *common; @@ -2409,6 +2441,7 @@ static struct fsg_common *fsg_common_setup(void) common->ops = NULL; common->private_data = NULL; + common->opts = opts; return common; } @@ -2659,7 +2692,7 @@ static void fsg_free(struct usb_function *f) fsg = container_of(f, struct fsg_dev, function); - kfree(fsg); + fsg_dev_put(fsg); } static struct usb_function *fsg_alloc(struct usb_function_instance *fi) @@ -2683,7 +2716,7 @@ static struct usb_function *fsg_alloc(struct usb_function_instance *fi) fsg->function.free_func = fsg_free; fsg->common = common; - common->fsg = fsg; + common->fsg = fsg_dev_get(fsg); return &fsg->function; } @@ -2692,8 +2725,7 @@ static void fsg_free_instance(struct usb_function_instance *fi) { struct f_ums_opts *opts = fsg_opts_from_func_inst(fi); - kfree(opts->common); - kfree(opts); + f_ums_opts_put(opts); } static struct usb_function_instance *fsg_alloc_inst(void) @@ -2706,12 +2738,14 @@ static struct usb_function_instance *fsg_alloc_inst(void) opts->func_inst.free_func_inst = fsg_free_instance; - opts->common = fsg_common_setup(); + opts->common = fsg_common_setup(opts); if (!opts->common) { free(opts); return ERR_PTR(-ENOMEM); } + f_ums_opts_get(opts); + return &opts->func_inst; } diff --git a/include/usb/mass_storage.h b/include/usb/mass_storage.h index 084b3c8e8f31..7be665ee4729 100644 --- a/include/usb/mass_storage.h +++ b/include/usb/mass_storage.h @@ -20,6 +20,7 @@ struct f_ums_opts { struct file_list *files; unsigned int num_sectors; int fd; + int refcnt; char name[16]; }; -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
If a function of a multi gadget fails, we run into multiple bugs: - All gadget are unbound, even those which weren't bound yet - We deallocate functions and function instances, but don't remove them from USB configuration, which leads to use-after-free when doing the composite unbind later on The correct course of action here is to undo the function instance allocation only, like Linux does. The rest will be cleaned up later at composite gadget unbind time. Fixes: bfb7aa1e1916 ("USB: gadget: Add a multi function gadget") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/usb/gadget/multi.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c index 0eb6d049d152..cd5b529d3eba 100644 --- a/drivers/usb/gadget/multi.c +++ b/drivers/usb/gadget/multi.c @@ -218,28 +218,28 @@ static int multi_bind(struct usb_composite_dev *cdev) printf("%s: creating Fastboot function\n", __func__); ret = multi_bind_fastboot(cdev); if (ret) - goto out; + return ret; } if (gadget_multi_opts->dfu_opts.files) { printf("%s: creating DFU function\n", __func__); ret = multi_bind_dfu(cdev); if (ret) - goto out; + goto unbind_fastboot; } if (gadget_multi_opts->ums_opts.files) { printf("%s: creating USB Mass Storage function\n", __func__); ret = multi_bind_ums(cdev); if (ret) - goto out; + goto unbind_dfu; } if (gadget_multi_opts->create_acm) { printf("%s: creating ACM function\n", __func__); ret = multi_bind_acm(cdev); if (ret) - goto out; + goto unbind_ums; } usb_ep_autoconfig_reset(cdev->gadget); @@ -247,8 +247,15 @@ static int multi_bind(struct usb_composite_dev *cdev) dev_info(&gadget->dev, DRIVER_DESC "\n"); return 0; -out: - multi_unbind(cdev); +unbind_ums: + if (gadget_multi_opts->ums_opts.files) + usb_put_function_instance(fi_ums); +unbind_dfu: + if (gadget_multi_opts->dfu_opts.files) + usb_put_function_instance(fi_dfu); +unbind_fastboot: + if (gadget_multi_opts->fastboot_opts.files) + usb_put_function_instance(fi_fastboot); return ret; } -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
We do this for all other gadgets, but not for UMS. Fix this. Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget") Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- drivers/usb/gadget/multi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c index cd5b529d3eba..102d8714f86d 100644 --- a/drivers/usb/gadget/multi.c +++ b/drivers/usb/gadget/multi.c @@ -169,6 +169,11 @@ static int multi_unbind(struct usb_composite_dev *cdev) usb_put_function_instance(fi_acm); } + if (gadget_multi_opts->ums_opts.files) { + usb_put_function(f_ums); + usb_put_function_instance(fi_ums); + } + if (gadget_multi_opts->dfu_opts.files) { usb_put_function(f_dfu); usb_put_function_instance(fi_dfu); -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Sat, Mar 19, 2022 at 12:02:40PM +0100, Ahmad Fatoum wrote: > USB mass storage gadget may fail during bind. USB multi gadget error > path for failed bind is broken. USB mass storage gadget unbind leaks > resources. Fix these three issues. > > Ahmad Fatoum (6): > usb: gadget: implement and use system_partitions_get_null > usb: gadget: don't register UMS with empty function > usb: gadget: mass-storage: fix clean up of file descriptors > usb: gadget: mass-storage: reference count allocations used in bthread > usb: gadget: multi: fix broken handling of USB function bind error > usb: gadget: multi: free UMS instance at multi_unbind time Applied, thanks Sascha > > common/fastboot.c | 4 +- > common/usbgadget.c | 6 +-- > drivers/usb/gadget/f_mass_storage.c | 64 +++++++++++++++++++++++------ > drivers/usb/gadget/multi.c | 24 ++++++++--- > include/system-partitions.h | 8 ++++ > include/usb/mass_storage.h | 1 + > 6 files changed, 81 insertions(+), 26 deletions(-) > > -- > 2.30.2 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox