usb: gadget: f_hid: don't call copy_from_user() under get_report_spinlock

f_hidg_get_report() already copies the incoming struct usb_hidg_report from userspace into the freshly allocated 'entry' before acquiring hidg->get_report_spinlock.

When a report with the same report_id is already present in hidg->report_list, the update path copies from userspace a second time, this time directly into ptr->report_data while the spinlock is held with interrupts disabled:

	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
	ptr = f_hidg_search_for_report(hidg, report_id);
	if (ptr) {
		if (copy_from_user(&ptr->report_data, buffer,
				sizeof(struct usb_hidg_report))) {

copy_from_user() may fault and sleep, so this is a sleeping function called from atomic context, reported by CONFIG_DEBUG_ATOMIC_SLEEP as "BUG: sleeping function called from invalid context". Userspace can reach it at will by issuing GADGET_HID_WRITE_GET_REPORT twice with the same report_id on /dev/hidgN.

The second copy is also redundant: the same user buffer has already been copied into entry->report_data a few lines above, and report_id was derived from that copy. Assign from the already copied data instead, which removes the fault from the critical section and makes the update atomic with respect to the list lookup.

---
Found by inspection while investigating the use-after-free fixed in patch 2/2, and build tested only; I do not have HID gadget hardware, but the path is reachable from /dev/hidgN with dummy_hcd.

 drivers/usb/gadget/function/f_hid.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

@@ -668,13 +668,7 @@ static int f_hidg_get_report(struct file
 
 	if (ptr) {
 		/* Report already exists in list - update it */
-		if (copy_from_user(&ptr->report_data, buffer,
-				sizeof(struct usb_hidg_report))) {
-			spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
-			ERROR(cdev, "copy_from_user error\n");
-			kfree(entry);
-			return -EINVAL;
-		}
+		ptr->report_data = entry->report_data;
 		kfree(entry);
 	} else {
 		/* Report does not exist in list - add it */

2.43.0

The /dev/hidgN character device stays open across function unbind: a process can open it, then remove the configfs gadget (echo "" > UDC, unlink the function from the config, rmdir the config directory), and keep using the still open file descriptor.

hidg_unbind() does not clear hidg->func.config, so the file operations continue to dereference the struct usb_configuration that configfs has already freed. f_hidg_get_report() does so unconditionally on entry:

	struct usb_composite_dev *cdev = hidg->func.config->cdev;

which gives a use-after-free read on the first ioctl() after the config directory is removed:

==================================================================

 BUG: KASAN: slab-use-after-free in f_hidg_get_report.isra.0+0x401/0x4a0
 Read of size 8 at addr ffff8881073d7950 by task init/172

 CPU: 2 UID: 0 PID: 172 Comm: init Not tainted 7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  f_hidg_get_report.isra.0+0x401/0x4a0
  f_hidg_ioctl+0xe1/0x110
  __x64_sys_ioctl+0x184/0x1d0
  do_syscall_64+0xda/0x4b0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 1:
  __kmalloc_cache_noprof+0x16a/0x380
  config_desc_make+0x1e6/0x590
  configfs_mkdir+0x4e9/0xe10
  vfs_mkdir+0x2ed/0x790
  __x64_sys_mkdir+0x6f/0xa0

 Freed by task 1:
  kfree+0x159/0x420
  config_item_cleanup+0x148/0x1e0
  config_item_put+0x90/0xb0
  configfs_rmdir+0x816/0xa50
  vfs_rmdir+0x2e6/0x810
  __x64_sys_rmdir+0x4b/0x70

 The buggy address belongs to the object at 0xffff8881073d7800
  which belongs to the cache kmalloc-1k of size 1024
 ==================================================================

A second splat follows from the ERROR() call in the same function.

Clear hidg->func.config in hidg_unbind() and check it in the paths that are reachable from an open file descriptor - f_hidg_read(), f_hidg_write() and f_hidg_get_report() - returning -ENODEV once the function is gone. f_hidg_req_complete() only uses the pointer to emit an error message, so guard that dereference as well.

While at it, drop the report_list entries in hidg_unbind(). They are allocated by f_hidg_get_report() and were only ever freed when the whole f_hidg was released, so reports queued before an unbind leaked.

---
No Fixes: tag: I could not identify a single commit that introduced the problem with confidence, so I have left it out rather than guess.

Reproduced on Linux 7.3.0-rc3 (5dd1818b15d9) with KASAN under QEMU using
dummy_hcd: set up a HID gadget through configfs, bind it to dummy_udc.0, open /dev/hidg0, unbind and rmdir the gadget, then call ioctl(fd, GADGET_HID_WRITE_GET_REPORT). With this patch applied the same sequence returns -ENODEV and produces no KASAN splat.

 drivers/usb/gadget/function/f_hid.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

@@ -426,6 +426,9 @@ static ssize_t f_hidg_read(struct file *
 {
 	struct f_hidg *hidg = file->private_data;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	if (hidg->use_out_ep)
 		return f_hidg_intout_read(file, buffer, count, ptr);
 	else

@@ -437,7 +440,7 @@ static void f_hidg_req_complete(struct u
 	struct f_hidg *hidg = (struct f_hidg *)ep->driver_data;
 	unsigned long flags;
 
-	if (req->status != 0) {
+	if (req->status != 0 && hidg->func.config && hidg->func.config->cdev) {
 		ERROR(hidg->func.config->cdev,
 			"End Point Request ERROR: %d\n", req->status);
 	}

@@ -456,6 +459,9 @@ static ssize_t f_hidg_write(struct file
 	unsigned long flags;
 	ssize_t status = -ENOMEM;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 	if (!hidg->req) {

@@ -644,12 +650,16 @@ static int f_hidg_get_report_id(struct f
 static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *buffer)
 {
 	struct f_hidg			*hidg = file->private_data;
-	struct usb_composite_dev	*cdev = hidg->func.config->cdev;
+	struct usb_composite_dev	*cdev;
 	unsigned long	flags;
 	struct report_entry *entry;
 	struct report_entry *ptr;
 	__u8 report_id;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+	cdev = hidg->func.config->cdev;
+
 	entry = kmalloc_obj(*entry);
 	if (!entry)
 		return -ENOMEM;

@@ -1582,10 +1592,19 @@ static void hidg_free(struct usb_functio
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
+	struct report_entry *entry, *tmp;
+	unsigned long flags;
 
 	cdev_device_del(hidg->cdev, &hidg->dev);
 	destroy_workqueue(hidg->workqueue);
+	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
+	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 	usb_free_all_descriptors(f);
+	hidg->func.config = NULL;
 }
 
 static struct usb_function *hidg_alloc(struct usb_function_instance *fi)

2.43.0

Please have your llm figure that out, worst case it's when it showed up, right?

thanks,

greg k-h

what commit id does this fix?

thanks,

greg k-h

f_hidg_get_report() already copies the incoming struct usb_hidg_report from userspace into the freshly allocated 'entry' before acquiring hidg->get_report_spinlock.

When a report with the same report_id is already present in hidg->report_list, the update path copies from userspace a second time, this time directly into ptr->report_data while the spinlock is held with interrupts disabled:

	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
	ptr = f_hidg_search_for_report(hidg, report_id);
	if (ptr) {
		if (copy_from_user(&ptr->report_data, buffer,
				sizeof(struct usb_hidg_report))) {

copy_from_user() may fault and sleep, so this is a sleeping function called from atomic context, reported by CONFIG_DEBUG_ATOMIC_SLEEP as "BUG: sleeping function called from invalid context". Userspace can reach it at will by issuing GADGET_HID_WRITE_GET_REPORT twice with the same report_id on /dev/hidgN.

The second copy is also redundant: the same user buffer has already been copied into entry->report_data a few lines above, and report_id was derived from that copy. Assign from the already copied data instead, which removes the fault from the critical section and makes the update atomic with respect to the list lookup.

---
v2: Add the Fixes: tag Greg asked for.

f_hidg_get_report(), the spinlock and the copy_from_user() call under it were all added together by a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL"), first released in v6.12, so that is where this starts. git blame on the removed lines agrees.

Found by inspection while investigating the use-after-free fixed in patch 2/2, and build tested only; I do not have HID gadget hardware, but the path is reachable from /dev/hidgN with dummy_hcd.

 drivers/usb/gadget/function/f_hid.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

@@ -668,13 +668,7 @@ static int f_hidg_get_report(struct file
 
 	if (ptr) {
 		/* Report already exists in list - update it */
-		if (copy_from_user(&ptr->report_data, buffer,
-				sizeof(struct usb_hidg_report))) {
-			spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
-			ERROR(cdev, "copy_from_user error\n");
-			kfree(entry);
-			return -EINVAL;
-		}
+		ptr->report_data = entry->report_data;
 		kfree(entry);
 	} else {
 		/* Report does not exist in list - add it */

2.43.0

The /dev/hidgN character device stays open across function unbind: a process can open it, then remove the configfs gadget (echo "" > UDC, unlink the function from the config, rmdir the config directory), and keep using the still open file descriptor.

hidg_unbind() does not clear hidg->func.config, so the file operations continue to dereference the struct usb_configuration that configfs has already freed. f_hidg_get_report() does so unconditionally on entry:

	struct usb_composite_dev *cdev = hidg->func.config->cdev;

which gives a use-after-free read on the first ioctl() after the config directory is removed:

==================================================================

 BUG: KASAN: slab-use-after-free in f_hidg_get_report.isra.0+0x401/0x4a0
 Read of size 8 at addr ffff8881073d7950 by task init/172

 CPU: 2 UID: 0 PID: 172 Comm: init Not tainted 7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <TASK>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  f_hidg_get_report.isra.0+0x401/0x4a0
  f_hidg_ioctl+0xe1/0x110
  __x64_sys_ioctl+0x184/0x1d0
  do_syscall_64+0xda/0x4b0
  entry_SYSCALL_64_after_hwframe+0x77/0x7f
  </TASK>

 Allocated by task 1:
  __kmalloc_cache_noprof+0x16a/0x380
  config_desc_make+0x1e6/0x590
  configfs_mkdir+0x4e9/0xe10
  vfs_mkdir+0x2ed/0x790
  __x64_sys_mkdir+0x6f/0xa0

 Freed by task 1:
  kfree+0x159/0x420
  config_item_cleanup+0x148/0x1e0
  config_item_put+0x90/0xb0
  configfs_rmdir+0x816/0xa50
  vfs_rmdir+0x2e6/0x810
  __x64_sys_rmdir+0x4b/0x70

 The buggy address belongs to the object at 0xffff8881073d7800
  which belongs to the cache kmalloc-1k of size 1024
 ==================================================================

A second splat follows from the ERROR() call in the same function.

Clear hidg->func.config in hidg_unbind() and check it in the paths that are reachable from an open file descriptor - f_hidg_read(), f_hidg_write() and f_hidg_get_report() - returning -ENODEV once the function is gone. f_hidg_req_complete() only uses the pointer to emit an error message, so guard that dereference as well.

While at it, drop the report_list entries in hidg_unbind(). They are allocated by f_hidg_get_report() and were only ever freed when the whole f_hidg was released, so reports queued before an unbind leaked.

---
v2: Add the Fixes: tag Greg asked for.

The unconditional dereference at function entry,

struct usb_composite_dev *cdev = hidg->func.config->cdev;

which is the one KASAN catches above, came in with a139c98f760e ("USB: gadget: f_hid: Add GET_REPORT via userspace IOCTL"), first released in v6.12, so I have used that.

For completeness: the exposure itself is older. hidg_unbind() has never cleared hidg->func.config, and the struct usb_configuration only became something userspace can free out from under an open /dev/hidgN with cb382536052f ("usb: gadget: f_hid: convert to new function interface with backward compatibility") in v3.19. Before a139c98f760e, though, the stale pointer was only reachable through the ERROR() calls on the write and completion error paths, and I have not reproduced a splat there. Tagging cb382536052f would also send this to stable trees where f_hidg_get_report() does not exist and the patch will not apply. Happy to re-tag if you prefer the older commit.

Reproduced on Linux 7.3.0-rc3 (5dd1818b15d9) with KASAN under QEMU using
dummy_hcd: set up a HID gadget through configfs, bind it to dummy_udc.0, open /dev/hidg0, unbind and rmdir the gadget, then call ioctl(fd, GADGET_HID_WRITE_GET_REPORT). With this patch applied the same sequence returns -ENODEV and produces no KASAN splat.

 drivers/usb/gadget/function/f_hid.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

@@ -426,6 +426,9 @@ static ssize_t f_hidg_read(struct file *
 {
 	struct f_hidg *hidg = file->private_data;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	if (hidg->use_out_ep)
 		return f_hidg_intout_read(file, buffer, count, ptr);
 	else

@@ -437,7 +440,7 @@ static void f_hidg_req_complete(struct u
 	struct f_hidg *hidg = (struct f_hidg *)ep->driver_data;
 	unsigned long flags;
 
-	if (req->status != 0) {
+	if (req->status != 0 && hidg->func.config && hidg->func.config->cdev) {
 		ERROR(hidg->func.config->cdev,
 			"End Point Request ERROR: %d\n", req->status);
 	}

@@ -456,6 +459,9 @@ static ssize_t f_hidg_write(struct file
 	unsigned long flags;
 	ssize_t status = -ENOMEM;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 	if (!hidg->req) {

@@ -644,12 +650,16 @@ static int f_hidg_get_report_id(struct f
 static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *buffer)
 {
 	struct f_hidg			*hidg = file->private_data;
-	struct usb_composite_dev	*cdev = hidg->func.config->cdev;
+	struct usb_composite_dev	*cdev;
 	unsigned long	flags;
 	struct report_entry *entry;
 	struct report_entry *ptr;
 	__u8 report_id;
 
+	if (!hidg->func.config || !hidg->func.config->cdev)
+		return -ENODEV;
+	cdev = hidg->func.config->cdev;
+
 	entry = kmalloc_obj(*entry);
 	if (!entry)
 		return -ENOMEM;

@@ -1582,10 +1592,19 @@ static void hidg_free(struct usb_functio
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
+	struct report_entry *entry, *tmp;
+	unsigned long flags;
 
 	cdev_device_del(hidg->cdev, &hidg->dev);
 	destroy_workqueue(hidg->workqueue);
+	spin_lock_irqsave(&hidg->get_report_spinlock, flags);
+	list_for_each_entry_safe(entry, tmp, &hidg->report_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
+	spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
 	usb_free_all_descriptors(f);
+	hidg->func.config = NULL;
 }
 
 static struct usb_function *hidg_alloc(struct usb_function_instance *fi)

2.43.0