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 <[email protected]>
Signed-off-by: Rondreis <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Rondreis <[email protected]>
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
From: Frank Li <[email protected]>
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 <[email protected]>
> > Signed-off-by: Rondreis <[email protected]>
>
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