patch "usb: gadget: Fix non-unique driver names in raw-gadget driver" added to usb-linus
Subsystems (3)
This is a note to let you know that I've just added the patch titled
usb: gadget: Fix non-unique driver names in raw-gadget driver
to my usb git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-linus branch.
The patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The patch will hopefully also be merged in Linus's tree for the next -rc kernel release.
If you have any questions about this process, please let me know.
From f2d8c2606825317b77db1f9ba0fc26ef26160b30 Mon Sep 17 00:00:00 2001
From: Alan Stern <[email protected]>
Date: Mon, 13 Jun 2022 10:17:03 -0400
Subject: usb: gadget: Fix non-unique driver names in raw-gadget driver
In a report for a separate bug (which has already been fixed by commit 5f0b5f4d50fa "usb: gadget: fix race when gadget driver register via ioctl") in the raw-gadget driver, the syzbot console log included error messages caused by attempted registration of a new driver with the same name as an existing driver:
Quoted 3 lines
kobject_add_internalfailed for raw-gadget with-EEXIST, don't try to register things with the same name in the same directory. UDC core: USB Raw Gadget: driver registration failed: -17
misc raw-gadget: fail,usb_gadget_register_driverreturned -17
These errors arise because raw_gadget.c registers a separate UDC driver for each of the UDC instances it creates, but these drivers all have the same name: "raw-gadget". Until recently this wasn't a problem, but when the "gadget" bus was added and UDC drivers were registered on this bus, it became possible for name conflicts to cause the registrations to fail. The reason is simply that the bus code in the driver core uses the driver name as a sysfs directory name (e.g., /sys/bus/gadget/drivers/raw-gadget/), and you can't create two directories with the same pathname.
To fix this problem, the driver names used by raw-gadget are made distinct by appending a unique ID number: "raw-gadget.N", with a different value of N for each driver instance. And to avoid the proliferation of error handling code in the raw_ioctl_init() routine, the error return paths are refactored into the common pattern (goto statements leading to cleanup code at the end of the routine).
--- drivers/usb/gadget/legacy/raw_gadget.c | 62 +++++++++++++++++++------- 1 file changed, 46 insertions(+), 16 deletions(-)
drivers/usb/gadget/legacy/raw_gadget.c +46 -16
@@ -11,6 +11,7 @@#include <linux/ctype.h> #include <linux/debugfs.h> #include <linux/delay.h> +#include <linux/idr.h> #include <linux/kref.h> #include <linux/miscdevice.h> #include <linux/module.h>
@@ -36,6 +37,9 @@ MODULE_LICENSE("GPL");/*----------------------------------------------------------------------*/ +static DEFINE_IDA (driver_id_numbers); +#define DRIVER_DRIVER_NAME_LENGTH_MAX 32 +#define RAW_EVENT_QUEUE_SIZE 16struct raw_event_queue {
@@ -161,6 +165,9 @@ struct raw_dev {/* Reference to misc device: */ struct device *dev; +/* Make driver names unique */ +int driver_id_number; +/* Protected by lock: */ enum dev_state state;bool gadget_registered;
@@ -189,6 +196,7 @@ static struct raw_dev *dev_new(void)spin_lock_init (&dev->lock);init_completion (&dev->ep0_done);raw_event_queue_init (&dev->queue); + dev->driver_id_number =-1 ;return dev; }
@@ -199,6 +207,9 @@ static void dev_free(struct kref *kref)kfree (dev->udc_name);kfree (dev->driver.udc_name); +kfree (dev->driver.driver.name); +if (dev->driver_id_number >=0 ) +ida_free (&driver_id_numbers, dev->driver_id_number);if (dev->req) {if (dev->ep0_urb_queued)usb_ep_dequeue (dev->gadget->ep0, dev->req);
@@ -422,6 +433,7 @@ static int raw_ioctl_init(struct raw_dev *dev, unsigned long value)struct usb_raw_init arg;char *udc_driver_name;char *udc_device_name; +char *driver_driver_name;unsigned long flags;if (copy_from_user (&arg, (void __user *)value,sizeof (arg)))
@@ -440,36 +452,44 @@ static int raw_ioctl_init(struct raw_dev *dev, unsigned long value)return -EINVAL ; } + ret =ida_alloc (&driver_id_numbers,GFP_KERNEL ); +if (ret <0 ) +return ret; + dev->driver_id_number = ret; + + driver_driver_name =kmalloc (DRIVER_DRIVER_NAME_LENGTH_MAX ,GFP_KERNEL ); +if (!driver_driver_name) { + ret = -ENOMEM ; + goto out_free_driver_id_number; + } +snprintf (driver_driver_name,DRIVER_DRIVER_NAME_LENGTH_MAX , +DRIVER_NAME ".%d" , dev->driver_id_number); + udc_driver_name =kmalloc (UDC_NAME_LENGTH_MAX ,GFP_KERNEL ); -if (!udc_driver_name) -return -ENOMEM ; +if (!udc_driver_name) { + ret = -ENOMEM ; + goto out_free_driver_driver_name; + } ret =strscpy (udc_driver_name, &arg.driver_name[0 ],UDC_NAME_LENGTH_MAX ); -if (ret <0 ) { -kfree (udc_driver_name); -return ret; - } +if (ret <0 ) + goto out_free_udc_driver_name; ret =0 ; udc_device_name =kmalloc (UDC_NAME_LENGTH_MAX ,GFP_KERNEL );if (!udc_device_name) { -kfree (udc_driver_name); -return -ENOMEM ; + ret = -ENOMEM ; + goto out_free_udc_driver_name; } ret =strscpy (udc_device_name, &arg.device_name[0 ],UDC_NAME_LENGTH_MAX ); -if (ret <0 ) { -kfree (udc_driver_name); -kfree (udc_device_name); -return ret; - } +if (ret <0 ) + goto out_free_udc_device_name; ret =0 ;spin_lock_irqsave (&dev->lock, flags);if (dev->state !=STATE_DEV_OPENED ) {dev_dbg (dev->dev,"fail, device is not opened\n" ); -kfree (udc_driver_name); -kfree (udc_device_name); ret = -EINVAL ; goto out_unlock; }
@@ -484,14 +504,24 @@ static int raw_ioctl_init(struct raw_dev *dev, unsigned long value) dev->driver.suspend = gadget_suspend; dev->driver.resume = gadget_resume; dev->driver.reset = gadget_reset; - dev->driver.driver.name =DRIVER_NAME ; + dev->driver.driver.name = driver_driver_name; dev->driver.udc_name = udc_device_name; dev->driver.match_existing_only =1 ; dev->state =STATE_DEV_INITIALIZED ; +spin_unlock_irqrestore (&dev->lock, flags); +return ret; out_unlock:spin_unlock_irqrestore (&dev->lock, flags); +out_free_udc_device_name: +kfree (udc_device_name); +out_free_udc_driver_name: +kfree (udc_driver_name); +out_free_driver_driver_name: +kfree (driver_driver_name); +out_free_driver_id_number: +ida_free (&driver_id_numbers, dev->driver_id_number);return ret; }
Signature · 1 line
2.36.1
PATCH 1/1 USB: gadget: Fix double-free bug in raw_gadget driver
Re-reading a recently merged fix to the raw_gadget driver showed that it inadvertently introduced a double-free bug in a failure pathway. If raw_ioctl_init() encounters an error after the driver ID number has been allocated, it deallocates the ID number before returning. But when dev_free() runs later on, it will then try to deallocate the ID number a second time.
Closely related to this issue is another error in the recent fix: The ID number is stored in the raw_dev structure before the code checks to see whether the structure has already been initialized, in which case the new ID number would overwrite the earlier value.
The solution to both bugs is to keep the new ID number in a local variable, and store it in the raw_dev structure only after the check for prior initialization. No errors can occur after that point, so the double-free will never happen.
---
I just noticed this while scanning the email message that f2d8c2606825 had been merged. I'm marking this fix for -stable because the commit it fixes is also marked that way.
[as1982]
drivers/usb/gadget/legacy/raw_gadget.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/usb/gadget/legacy/raw_gadget.c +7 -6
@@ -430,6 +430,7 @@ out_put:static int raw_ioctl_init (struct raw_dev *dev,unsigned long value) {int ret =0 ; +int driver_id_number;struct usb_raw_init arg;char *udc_driver_name;char *udc_device_name;
@@ -452,10 +453,9 @@ static int raw_ioctl_init(struct raw_devreturn -EINVAL ; } - ret =ida_alloc (&driver_id_numbers,GFP_KERNEL ); -if (ret <0 ) -return ret; - dev->driver_id_number = ret; + driver_id_number =ida_alloc (&driver_id_numbers,GFP_KERNEL ); +if (driver_id_number <0 ) +return driver_id_number; driver_driver_name =kmalloc (DRIVER_DRIVER_NAME_LENGTH_MAX ,GFP_KERNEL );if (!driver_driver_name) {
@@ -463,7 +463,7 @@ static int raw_ioctl_init(struct raw_dev goto out_free_driver_id_number; }snprintf (driver_driver_name,DRIVER_DRIVER_NAME_LENGTH_MAX , -DRIVER_NAME ".%d" , dev->driver_id_number); +DRIVER_NAME ".%d" , driver_id_number); udc_driver_name =kmalloc (UDC_NAME_LENGTH_MAX ,GFP_KERNEL );if (!udc_driver_name) {
@@ -507,6 +507,7 @@ static int raw_ioctl_init(struct raw_dev dev->driver.driver.name = driver_driver_name; dev->driver.udc_name = udc_device_name; dev->driver.match_existing_only =1 ; + dev->driver_id_number = driver_id_number; dev->state =STATE_DEV_INITIALIZED ;spin_unlock_irqrestore (&dev->lock, flags);
@@ -521,7 +522,7 @@ out_free_udc_driver_name: out_free_driver_driver_name:kfree (driver_driver_name); out_free_driver_id_number: -ida_free (&driver_id_numbers, dev->driver_id_number); +ida_free (&driver_id_numbers, driver_id_number);return ret; }