Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Sep 2022 13:57:07 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Rondreis <linhaoguo86@gmail.com>
Cc:     balbi@kernel.org, stern@rowland.harvard.edu,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: gadget: Assign a unique name for each configfs
 driver
Message-ID: <YxiHE9s4NDZOILpe@kroah.com>
References: <20220907112210.11949-1-linhaoguo86@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220907112210.11949-1-linhaoguo86@gmail.com>
Precedence: bulk

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@gmail.com>
> Signed-off-by: Rondreis <linhaoguo86@gmail.com>

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