linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: Assign a unique name for each configfs driver
@ 2022-09-07 11:22 Rondreis
  2022-09-07 11:57 ` Greg KH
  0 siblings, 1 reply; 3+ messages in thread
From: Rondreis @ 2022-09-07 11:22 UTC (permalink / raw)
  To: balbi, stern; +Cc: gregkh, linux-usb, linux-kernel, Rondreis

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.

Reported-and-tested-by: Rondreis <linhaoguo86@gmail.com>
Signed-off-by: Rondreis <linhaoguo86@gmail.com>
---
 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
+
 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);
+	if (gi->driver_id_number >= 0)
+		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;
 	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;
 
 	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);
+	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);
 }
 
 static void gadgets_drop(struct config_group *group, struct config_item *item)
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] usb: gadget: Assign a unique name for each configfs driver
  2022-09-07 11:22 [PATCH] usb: gadget: Assign a unique name for each configfs driver Rondreis
@ 2022-09-07 11:57 ` Greg KH
  2022-12-08 16:53   ` [PATCH] usb: gadget: Assign a unique name for each configfs Frank Li
  0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2022-09-07 11:57 UTC (permalink / raw)
  To: Rondreis; +Cc: balbi, stern, linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] usb: gadget: Assign a unique name for each configfs
  2022-09-07 11:57 ` Greg KH
@ 2022-12-08 16:53   ` Frank Li
  0 siblings, 0 replies; 3+ messages in thread
From: Frank Li @ 2022-12-08 16:53 UTC (permalink / raw)
  To: gregkh, Rondreis; +Cc: balbi, linux-kernel, linux-usb, stern, imx

From: Frank Li <Frank.li@nxp.com>

On Wed, Sep 07, 2022 at 01:57:07PM +0200, Greg KH wrote:
> 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>
> 

I met the same issue, do you plan continue this patch after fix greg's comments.
If you have not time, I can continue it. 

Frank Li

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-12-08 16:54 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-07 11:22 [PATCH] usb: gadget: Assign a unique name for each configfs driver Rondreis
2022-09-07 11:57 ` Greg KH
2022-12-08 16:53   ` [PATCH] usb: gadget: Assign a unique name for each configfs Frank Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).