Skip to contentSkip to conversation

usb: gadget: f_tcm: detach the gadget in usbg_drop_tpg() to fix a use-after-free

PATCH Superseded Hui Peng 1 participant 2 messages Sep 19 USB

lore
Share & tools
lore

Hui Peng

author
#0 lore raw

PATCH 1/1 usb: gadget: f_tcm: detach the gadget in usbg_drop_tpg() to fix a use-after-free

A target portal group can be removed with rmdir() on its configfs directory while it is still enabled and still attached to a USB gadget function. usbg_drop_tpg() frees the struct usbg_tpg but never calls usbg_detach(), and it leaves opts->can_attach set, so the function instance keeps a dangling tpg pointer and a subsequent bind to a UDC happily attaches to freed memory.

Enumerating the gadget then dereferences the freed tpg from the UAS/BOT completion path, in softirq context:

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

 BUG: KASAN: slab-use-after-free in usbg_submit_command.isra.0+0xbe7/0xd50
 Read of size 8 at addr ffff888107493478 by task swapper/3/0

 CPU: 3 UID: 0 PID: 0 Comm: swapper/3 Tainted: G    B D    7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <IRQ>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  usbg_submit_command.isra.0+0xbe7/0xd50
  uasp_cmd_complete+0x83/0xf0
  dummy_timer+0x1337/0x2d60
  __hrtimer_run_queues+0x2a3/0x640
  hrtimer_run_softirq+0x1b1/0x3d0
  handle_softirqs+0x188/0x4e0
  __irq_exit_rcu+0x62/0x150
  sysvec_apic_timer_interrupt+0x6b/0x80
  </IRQ>

 Allocated by task 1:
  __kmalloc_cache_noprof+0x16a/0x380
  usbg_make_tpg+0x243/0x580
  target_fabric_make_tpg+0xb1/0x7f0
  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
 ==================================================================

Detach the gadget in usbg_drop_tpg() if the tpg is still connected, and clear opts->can_attach for every function instance bound to this tpg so that a later usbg_attach() cannot pick up the freed object.

The forward declarations of usbg_attach() and usbg_detach() are moved above usbg_drop_tpg() since it now calls usbg_detach().

Assisted-by LLM

---
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: create a tcm function and a target tpgt_1, write 1 to its "enable" attribute, link the function into a gadget config, rmdir tpgt_1 without disabling it first, then bind the gadget to dummy_udc.0. With this patch applied the same sequence produces no KASAN splat.

 drivers/usb/gadget/function/f_tcm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/usb/gadget/function/f_tcm.c +9 -3
@@ -1756,6 +1756,9 @@ unlock_inst:
 
 static int tcm_usbg_drop_nexus(struct usbg_tpg *);
 
+static int usbg_attach(struct usbg_tpg *);
+static void usbg_detach(struct usbg_tpg *);
+
 static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 {
 	struct usbg_tpg *tpg = container_of(se_tpg,
@@ -1763,6 +1766,11 @@ static void usbg_drop_tpg(struct se_port
 	unsigned i;
 	struct f_tcm_opts *opts;
 
+	if (tpg->gadget_connect) {
+		usbg_detach(tpg);
+		tpg->gadget_connect = false;
+	}
+
 	tcm_usbg_drop_nexus(tpg);
 	core_tpg_deregister(se_tpg);
 	destroy_workqueue(tpg->workqueue);
@@ -1776,6 +1784,7 @@ static void usbg_drop_tpg(struct se_port
 		opts = container_of(tpg_instances[i].func_inst,
 			struct f_tcm_opts, func_inst);
 		mutex_lock(&opts->dep_lock);
+		opts->can_attach = false;
 		if (opts->has_dep)
 			module_put(opts->dependent);
 		else
@@ -1832,9 +1841,6 @@ static struct configfs_attribute *usbg_w
 	NULL,
 };
 
-static int usbg_attach(struct usbg_tpg *);
-static void usbg_detach(struct usbg_tpg *);
-
 static int usbg_enable_tpg(struct se_portal_group *se_tpg, bool enable)
 {
 	struct usbg_tpg  *tpg = container_of(se_tpg, struct usbg_tpg, se_tpg);
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v2 1/1 usb: gadget: f_tcm: detach the gadget in usbg_drop_tpg() to fix a use-after-free

A target portal group can be removed with rmdir() on its configfs directory while it is still enabled and still attached to a USB gadget function. usbg_drop_tpg() frees the struct usbg_tpg but never calls usbg_detach(), and it leaves opts->can_attach set, so the function instance keeps a dangling tpg pointer and a subsequent bind to a UDC happily attaches to freed memory.

Enumerating the gadget then dereferences the freed tpg from the UAS/BOT completion path, in softirq context:

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

 BUG: KASAN: slab-use-after-free in usbg_submit_command.isra.0+0xbe7/0xd50
 Read of size 8 at addr ffff888107493478 by task swapper/3/0

 CPU: 3 UID: 0 PID: 0 Comm: swapper/3 Tainted: G    B D    7.3.0-rc3-g5dd1818b15d9 #1 PREEMPT(lazy)
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 Call Trace:
  <IRQ>
  dump_stack_lvl+0x70/0xa0
  print_report+0x153/0x4c6
  kasan_report+0xf1/0x120
  usbg_submit_command.isra.0+0xbe7/0xd50
  uasp_cmd_complete+0x83/0xf0
  dummy_timer+0x1337/0x2d60
  __hrtimer_run_queues+0x2a3/0x640
  hrtimer_run_softirq+0x1b1/0x3d0
  handle_softirqs+0x188/0x4e0
  __irq_exit_rcu+0x62/0x150
  sysvec_apic_timer_interrupt+0x6b/0x80
  </IRQ>

 Allocated by task 1:
  __kmalloc_cache_noprof+0x16a/0x380
  usbg_make_tpg+0x243/0x580
  target_fabric_make_tpg+0xb1/0x7f0
  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
 ==================================================================

Detach the gadget in usbg_drop_tpg() if the tpg is still connected, and clear opts->can_attach for every function instance bound to this tpg so that a later usbg_attach() cannot pick up the freed object.

The forward declarations of usbg_attach() and usbg_detach() are moved above usbg_drop_tpg() since it now calls usbg_detach().

Fixes dc8c46a5ae77 ("usb: gadget: f_tcm: convert to new function interface with backward compatibility")
Assisted-by LLM

---
v2: Add the Fixes: tag that v1 said it could not determine.

dc8c46a5ae77 introduced opts->can_attach, the !opts->can_attach gate in tcm_bind(), usbg_attach()/usbg_detach(), the tpg_instances[] table and fu->tpg, and the version of usbg_drop_tpg() that tears all of that down without ever detaching - i.e. every line this patch touches. Note git blame points at 08a1cb0f65fd ("usb: gadget: tcm: factor out f_tcm") for the top of usbg_drop_tpg(), but that commit is a pure file split out of legacy/tcm_usb_gadget.c, so it is not the tag.

There is a defensible alternative: 4bb8548df632 ("usb: gadget: f_tcm:
add configfs support") added the callbacks that set can_attach for a pure-configfs gadget, which is what my reproducer uses. I went with dc8c46a5ae77 because the legacy gadget already set can_attach unconditionally, so the hole was reachable without 4bb8548df632 - and in any case both landed in v4.5-rc1, five patches apart in the same series, so the backport target is identical. Happy to switch if you prefer the other.

For completeness, the underlying "free the tpg without detaching" shape is as old as c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT", v3.5-rc1), but f_tcm.c does not exist before v4.5-rc1 and opts->can_attach - which this patch has to clear - did not either, so dc8c46a5ae77 is the oldest commit this patch is actually applicable to.

Two things worth a reviewer's attention: tpg->gadget_connect is read here without the target-core configfs locking that usbg_enable_tpg() writes it under, and the opts->can_attach = false lands inside the if (i < TPG_INSTANCES) arm added by e877b729c649 ("usb: gadget: f_tcm:
out of bound access in usbg_drop_tpg"). If you would rather refuse the rmdir than auto-detach, note that fabric_drop_tpg returns void, so it cannot be failed from here.

Reproduced on Linux 7.3.0-rc3 (5dd1818b15d9) with KASAN under QEMU using
dummy_hcd: create a tcm function and a target tpgt_1, write 1 to its "enable" attribute, link the function into a gadget config, rmdir tpgt_1 without disabling it first, then bind the gadget to dummy_udc.0. With this patch applied the same sequence produces no KASAN splat.

 drivers/usb/gadget/function/f_tcm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/usb/gadget/function/f_tcm.c +9 -3
@@ -1756,6 +1756,9 @@ unlock_inst:
 
 static int tcm_usbg_drop_nexus(struct usbg_tpg *);
 
+static int usbg_attach(struct usbg_tpg *);
+static void usbg_detach(struct usbg_tpg *);
+
 static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 {
 	struct usbg_tpg *tpg = container_of(se_tpg,
@@ -1763,6 +1766,11 @@ static void usbg_drop_tpg(struct se_port
 	unsigned i;
 	struct f_tcm_opts *opts;
 
+	if (tpg->gadget_connect) {
+		usbg_detach(tpg);
+		tpg->gadget_connect = false;
+	}
+
 	tcm_usbg_drop_nexus(tpg);
 	core_tpg_deregister(se_tpg);
 	destroy_workqueue(tpg->workqueue);
@@ -1776,6 +1784,7 @@ static void usbg_drop_tpg(struct se_port
 		opts = container_of(tpg_instances[i].func_inst,
 			struct f_tcm_opts, func_inst);
 		mutex_lock(&opts->dep_lock);
+		opts->can_attach = false;
 		if (opts->has_dep)
 			module_put(opts->dependent);
 		else
@@ -1832,9 +1841,6 @@ static struct configfs_attribute *usbg_w
 	NULL,
 };
 
-static int usbg_attach(struct usbg_tpg *);
-static void usbg_detach(struct usbg_tpg *);
-
 static int usbg_enable_tpg(struct se_portal_group *se_tpg, bool enable)
 {
 	struct usbg_tpg  *tpg = container_of(se_tpg, struct usbg_tpg, se_tpg);
Signature · 1 line
2.43.0