On Wed, Sep 07, 2022 at 07:22:10PM +0800, Rondreis wrote:
> When fuzzing the kernel, I couldn't use configfs to attach more than one
> gadget. When attaching the second gadget with a different UDC it always
> failed and the kernel message said:
>
> Error: Driver 'configfs-gadget' is already registered, aborting...
> UDC core: g1: driver registration failed: -16
>
> The problem is that when creating multiple gadgets with configfs and
> binding them to different UDCs, the UDC drivers have the same name
> "configfs-gadget". Because of the addition of the "gadget" bus,
> naming conflicts will occur when more than one UDC drivers
> registered to the bus.
>
> It's not an isolated case, this patch refers to the commit f2d8c2606825
> ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
> Each configfs-gadget driver will be assigned a unique name
> "configfs-gadget.N", with a different value of N for each driver instance.
Please wrap your changelog text at 72 columns like the documentation
asks for.
> Reported-and-tested-by: Rondreis <linhaoguo86@xxxxxxxxx>
> Signed-off-by: Rondreis <linhaoguo86@xxxxxxxxx>
You can't reported and test your own patch :)
Also, I need a full name here, what you use to sign legal documents.
And, what commit does this "fix"? This should have worked before the
gadget bus happened, so it is a regression and needs a valid "Fixes:"
tag, right?
> ---
> drivers/usb/gadget/configfs.c | 39 ++++++++++++++++++++++++++++++++---
> 1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3a6b4926193e..7e7ff94dbaab 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -4,12 +4,18 @@
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/nls.h>
> +#include <linux/idr.h>
> #include <linux/usb/composite.h>
> #include <linux/usb/gadget_configfs.h>
> #include "configfs.h"
> #include "u_f.h"
> #include "u_os_desc.h"
>
> +#define DRIVER_NAME "configfs-gadget"
> +
> +static DEFINE_IDA(driver_id_numbers);
> +#define DRIVER_DRIVER_NAME_LENGTH_MAX 32
Why this number?
> +
> int check_user_usb_string(const char *name,
> struct usb_gadget_strings *stringtab_dev)
> {
> @@ -46,6 +52,7 @@ struct gadget_info {
>
> struct usb_composite_driver composite;
> struct usb_composite_dev cdev;
> + int driver_id_number;
> bool use_os_desc;
> char b_vendor_code;
> char qw_sign[OS_STRING_QW_SIGN_LEN];
> @@ -252,6 +259,11 @@ static int unregister_gadget(struct gadget_info *gi)
> return ret;
> kfree(gi->composite.gadget_driver.udc_name);
> gi->composite.gadget_driver.udc_name = NULL;
> +
> + kfree(gi->composite.gadget_driver.driver.name);
Are you sure the driver name is safe to free here? You don't own the
lifecycle of this structure, so this feels very risky.
> + if (gi->driver_id_number >= 0)
How would that ever be negative?
> + ida_free(&driver_id_numbers, gi->driver_id_number);
> +
> return 0;
> }
>
> @@ -1571,7 +1583,6 @@ static const struct usb_gadget_driver configfs_driver_template = {
> .max_speed = USB_SPEED_SUPER_PLUS,
> .driver = {
> .owner = THIS_MODULE,
> - .name = "configfs-gadget",
> },
> .match_existing_only = 1,
> };
> @@ -1580,6 +1591,8 @@ static struct config_group *gadgets_make(
> struct config_group *group,
> const char *name)
> {
> + int ret = 0;
> + char *driver_driver_name;
Why not just "driver_name"?
> struct gadget_info *gi;
>
> gi = kzalloc(sizeof(*gi), GFP_KERNEL);
> @@ -1609,6 +1622,7 @@ static struct config_group *gadgets_make(
> gi->composite.suspend = NULL;
> gi->composite.resume = NULL;
> gi->composite.max_speed = USB_SPEED_SUPER_PLUS;
> + gi->driver_id_number = -1;
What is this magic "-1"?
>
> spin_lock_init(&gi->spinlock);
> mutex_init(&gi->lock);
> @@ -1622,16 +1636,35 @@ static struct config_group *gadgets_make(
>
> gi->composite.gadget_driver = configfs_driver_template;
>
> + ret = ida_alloc(&driver_id_numbers, GFP_KERNEL);
> + if (ret < 0)
> + goto err;
> + gi->driver_id_number = ret;
> +
> + driver_driver_name = kmalloc(DRIVER_DRIVER_NAME_LENGTH_MAX, GFP_KERNEL);
> + if (!driver_driver_name) {
> + ret = -ENOMEM;
> + goto err_free_driver_id_number;
> + }
> + snprintf(driver_driver_name, DRIVER_DRIVER_NAME_LENGTH_MAX,
> + DRIVER_NAME ".%d", gi->driver_id_number);
What happens if this fails? And please use the recommended function for
this string operation, which isn't snprintf().
> + gi->composite.gadget_driver.driver.name = driver_driver_name;
> +
> gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
> gi->composite.name = gi->composite.gadget_driver.function;
>
> - if (!gi->composite.gadget_driver.function)
> + if (!gi->composite.gadget_driver.function) {
> + ret = -ENOMEM;
> goto err;
> + }
>
> return &gi->group;
> +
> +err_free_driver_id_number:
> + ida_free(&driver_id_numbers, gi->driver_id_number);
> err:
> kfree(gi);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(ret);
You leaked the memory you allocated for "driver_driver_name" :(
thanks,
greg k-h
