[PATCH 1/3] usb: gadget: f_tcm: take the TPG in tcm_bind() instead of tcm_alloc()

[PATCH 1/3] usb: gadget: f_tcm: take the TPG in tcm_bind() instead of tcm_alloc()

[Slawomir Stepien]

tcm_alloc() runs when a TCM function is symlinked into a gadget
configuration. It searches tpg_instances[] for the function instance
and copies the TPG pointer into fu->tpg at that point, under
tpg_instances_lock. This has two problems.

The function and the TPG can be set up in either order. If the
function is linked into a configuration before the TPG directory is
created, tpg_instances[i].tpg is still NULL and fu->tpg stays NULL.
Creating and enabling the TPG afterwards sets can_attach, so
tcm_bind() succeeds, and the first USB request that reaches the
target code dereferences the NULL fu->tpg.

Removing a TPG also leaves can_attach set for configfs instances, so a
later bind is not refused even though the TPG it would use is gone.

Store the TPG in struct f_tcm_opts instead: set it in usbg_make_tpg()
and clear it in usbg_drop_tpg(), under opts->dep_lock. In tcm_bind(),
refuse to bind unless both can_attach and opts->tpg are set, and copy
opts->tpg into fu->tpg there. Also clear can_attach in usbg_drop_tpg()
for configfs instances. The legacy g_target driver sets can_attach once
at module load, and its attach callback does not set it again, so
leave it alone in that case or g_target could never bind again after a
TPG is recreated.

With fu->tpg no longer set in tcm_alloc(), the tpg_instances[] lookup
there serves no purpose: f->fi already identifies the instance for the
item's whole lifetime. Drop it, and with it the tpg_instances_lock
acquisition. config_usb_cfg_link() calls tcm_alloc() with gi->lock
held, so this also removes the gi->lock -> tpg_instances_lock edge
from the following lockdep report:

  WARNING: possible circular locking dependency detected
  task is trying to acquire lock:
   (tpg_instances_lock){+.+.}-{4:4}, at: tcm_alloc
  but task is already holding lock:
   (&gi->lock){+.+.}-{4:4}, at: config_usb_cfg_link

  the existing dependency chain (in reverse order) is:
  -> #2 (&gi->lock):
         config_usb_cfg_link
         configfs_symlink
  -> #1 (&sb->s_type->i_mutex_key#24):
         configfs_depend_item_unlocked
         usbg_make_tpg
         target_fabric_make_tpg
         configfs_mkdir
  -> #0 (tpg_instances_lock):
         tcm_alloc
         usb_get_function
         config_usb_cfg_link
         configfs_symlink

Fixes: 4bb8548df632 ("usb: gadget: f_tcm: add configfs support")
Reported-by: syzbot+2799146a53c356f8017d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2799146a53c356f8017d
Link: https://syzkaller.appspot.com/ai_job?id=51e1f189-6d91-4321-a6ba-07d4eb3cd8cd
Assisted-by: Gemini:gemini-4-argon Gemini:gemini-3.8-flash syzbot
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/usb/gadget/function/f_tcm.c | 29 +++++++++++------------------
 drivers/usb/gadget/function/u_tcm.h |  4 ++++
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9e6d4f39900a..cc75d7e9792a 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1720,6 +1720,9 @@ static struct se_portal_group *usbg_make_tpg(struct se_wwn *wwn,
 
 	tpg_instances[i].tpg = tpg;
 	tpg->fi = tpg_instances[i].func_inst;
+	mutex_lock(&opts->dep_lock);
+	opts->tpg = tpg;
+	mutex_unlock(&opts->dep_lock);
 	mutex_unlock(&tpg_instances_lock);
 	return &tpg->se_tpg;
 
@@ -1760,11 +1763,14 @@ static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 		opts = container_of(tpg_instances[i].func_inst,
 			struct f_tcm_opts, func_inst);
 		mutex_lock(&opts->dep_lock);
-		if (opts->has_dep)
+		opts->tpg = NULL;
+		if (opts->has_dep) {
 			module_put(opts->dependent);
-		else
+		} else {
+			opts->can_attach = false;
 			configfs_undepend_item_unlocked(
 				&opts->func_inst.group.cg_item);
+		}
 		mutex_unlock(&opts->dep_lock);
 	}
 	mutex_unlock(&tpg_instances_lock);
@@ -2300,10 +2306,11 @@ static int tcm_bind(struct usb_configuration *c, struct usb_function *f)
 	opts = container_of(f->fi, struct f_tcm_opts, func_inst);
 
 	mutex_lock(&opts->dep_lock);
-	if (!opts->can_attach) {
+	if (!opts->can_attach || !opts->tpg) {
 		mutex_unlock(&opts->dep_lock);
 		return -ENODEV;
 	}
+	fu->tpg = opts->tpg;
 	mutex_unlock(&opts->dep_lock);
 	us = usb_gstrings_attach(c->cdev, tcm_strings,
 		ARRAY_SIZE(tcm_us_strings));
@@ -2729,22 +2736,10 @@ static void tcm_unbind(struct usb_configuration *c, struct usb_function *f)
 static struct usb_function *tcm_alloc(struct usb_function_instance *fi)
 {
 	struct f_uas *fu;
-	unsigned i;
-
-	mutex_lock(&tpg_instances_lock);
-	for (i = 0; i < TPG_INSTANCES; ++i)
-		if (tpg_instances[i].func_inst == fi)
-			break;
-	if (i == TPG_INSTANCES) {
-		mutex_unlock(&tpg_instances_lock);
-		return ERR_PTR(-ENODEV);
-	}
 
 	fu = kzalloc_obj(*fu);
-	if (!fu) {
-		mutex_unlock(&tpg_instances_lock);
+	if (!fu)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	fu->function.name = "Target Function";
 	fu->function.bind = tcm_bind;
@@ -2754,12 +2749,10 @@ static struct usb_function *tcm_alloc(struct usb_function_instance *fi)
 	fu->function.setup = tcm_setup;
 	fu->function.disable = tcm_disable;
 	fu->function.free_func = tcm_free;
-	fu->tpg = tpg_instances[i].tpg;
 	INIT_WORK(&fu->delayed_set_alt, tcm_delayed_set_alt);
 	spin_lock_init(&fu->delayed_set_alt_lock);
 
 	hash_init(fu->stream_hash);
-	mutex_unlock(&tpg_instances_lock);
 
 	return &fu->function;
 }
diff --git a/drivers/usb/gadget/function/u_tcm.h b/drivers/usb/gadget/function/u_tcm.h
index 2cd15d9a1c0d..fa38d55c2a17 100644
--- a/drivers/usb/gadget/function/u_tcm.h
+++ b/drivers/usb/gadget/function/u_tcm.h
@@ -15,6 +15,8 @@
 
 #include <linux/usb/composite.h>
 
+struct usbg_tpg;
+
 /**
  * @dependent: optional dependent module. Meant for legacy gadget.
  * If non-null its refcount will be increased when a tpg is created and
@@ -23,10 +25,12 @@
  * @ready: true if the dependent module information is set.
  * @can_attach: true a function can be bound to gadget
  * @has_dep: true if there is a dependent module
+ * @tpg: target portal group associated with this function instance.
  *
  */
 struct f_tcm_opts {
 	struct usb_function_instance	func_inst;
+	struct usbg_tpg			*tpg;
 	struct module			*dependent;
 	struct mutex			dep_lock;
 	bool				ready;
-- 
2.56.0

[PATCH 2/3] usb: gadget: f_tcm: detach gadget when dropping an enabled TPG

[Slawomir Stepien]

Removing a TPG directory via configfs rmdir ends up in usbg_drop_tpg()
through target_fabric_tpg_release(). The target core does not disable
the TPG first, so if the TPG is still enabled and the gadget is bound to
a UDC, usbg_drop_tpg() destroys the TPG workqueue and frees the TPG
while the bound function still references it through fu->tpg.
Subsequent USB traffic (tcm_setup(), bot_submit_command(),
usbg_submit_command(), ...) then dereferences freed memory and queues
work onto a destroyed workqueue.

Disable the TPG through usbg_enable_tpg() before tearing it down, so the
gadget is unbound in the same way as when writing 0 to the enable
attribute. Only do so when the gadget is connected: for the legacy
g_target driver, detaching a never-attached TPG would call
usb_composite_unregister() on a driver that was never registered.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/usb/gadget/function/f_tcm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index cc75d7e9792a..bcf11f4405ae 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1742,6 +1742,7 @@ static struct se_portal_group *usbg_make_tpg(struct se_wwn *wwn,
 }
 
 static int tcm_usbg_drop_nexus(struct usbg_tpg *);
+static int usbg_enable_tpg(struct se_portal_group *se_tpg, bool enable);
 
 static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 {
@@ -1750,6 +1751,9 @@ static void usbg_drop_tpg(struct se_portal_group *se_tpg)
 	unsigned i;
 	struct f_tcm_opts *opts;
 
+	if (tpg->gadget_connect)
+		usbg_enable_tpg(se_tpg, false);
+
 	tcm_usbg_drop_nexus(tpg);
 	core_tpg_deregister(se_tpg);
 	destroy_workqueue(tpg->workqueue);
-- 
2.56.0

Re: [PATCH 2/3] usb: gadget: f_tcm: detach gadget when dropping an enabled TPG

[sashiko-bot]

> Removing a TPG directory via configfs rmdir ends up in usbg_drop_tpg()
> through target_fabric_tpg_release(). The target core does not disable
> the TPG first, so if the TPG is still enabled and the gadget is bound to
> a UDC, usbg_drop_tpg() destroys the TPG workqueue and frees the TPG
> while the bound function still references it through fu->tpg.
> Subsequent USB traffic (tcm_setup(), bot_submit_command(),
> usbg_submit_command(), ...) then dereferences freed memory and queues
> work onto a destroyed workqueue.
> 
> Disable the TPG through usbg_enable_tpg() before tearing it down, so the
> gadget is unbound in the same way as when writing 0 to the enable
> attribute. Only do so when the gadget is connected: for the legacy
> g_target driver, detaching a never-attached TPG would call
> usb_composite_unregister() on a driver that was never registered.
> 
> Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022134926.1125473-1-sst@poczta.fm?part=2

[PATCH 3/3] usb: gadget: f_tcm: don't hold dep_lock across unregister_gadget_item()

[Slawomir Stepien]

tcm_unregister_callback() runs when a TPG is disabled and unbinds the
configfs gadget via unregister_gadget_item() while holding
opts->dep_lock. unregister_gadget_item() takes gi->lock.

The opposite order is taken when a UDC is assigned to the gadget:
gadget_dev_desc_UDC_store() holds gi->lock across
usb_gadget_register_driver(), which binds the configuration and ends
up in tcm_bind(), which takes opts->dep_lock:

  CPU0: echo 0 > .../tpgt_N/enable    CPU1: echo <udc> > .../UDC
  tcm_unregister_callback()           gadget_dev_desc_UDC_store()
    mutex_lock(&opts->dep_lock)         mutex_lock(&gi->lock)
                                        ...
                                          tcm_bind()
                                            mutex_lock(&opts->dep_lock)
    unregister_gadget_item()
      mutex_lock(&gi->lock)

Disabling a TPG while a UDC is being assigned can therefore deadlock.

Here dep_lock only needs to cover the can_attach update, so clear
can_attach under the lock and call unregister_gadget_item() after
dropping it. Once can_attach is false, any later tcm_bind() fails with
-ENODEV, and a bind that already passed the check is undone by the
unregister that follows. The function item, and with it the gadget
item reached via ci_parent, stays alive because the TPG still holds a
configfs dependency on it.

Fixes: 4bb8548df632 ("usb: gadget: f_tcm: add configfs support")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/usb/gadget/function/f_tcm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index bcf11f4405ae..d4bb7ad3f1ae 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -2648,10 +2648,11 @@ static void tcm_unregister_callback(struct usb_function_instance *f)
 	struct f_tcm_opts *opts = container_of(f, struct f_tcm_opts, func_inst);
 
 	mutex_lock(&opts->dep_lock);
-	unregister_gadget_item(opts->
-		func_inst.group.cg_item.ci_parent->ci_parent);
 	opts->can_attach = false;
 	mutex_unlock(&opts->dep_lock);
+
+	/* Takes gi->lock; UDC_store holds gi->lock while tcm_bind() takes dep_lock. */
+	unregister_gadget_item(opts->func_inst.group.cg_item.ci_parent->ci_parent);
 }
 
 static int usbg_attach(struct usbg_tpg *tpg)
-- 
2.56.0

Re: [PATCH 3/3] usb: gadget: f_tcm: don't hold dep_lock across unregister_gadget_item()

[sashiko-bot]

> tcm_unregister_callback() runs when a TPG is disabled and unbinds the
> configfs gadget via unregister_gadget_item() while holding
> opts->dep_lock. unregister_gadget_item() takes gi->lock.
> 
> The opposite order is taken when a UDC is assigned to the gadget:
> gadget_dev_desc_UDC_store() holds gi->lock across
> usb_gadget_register_driver(), which binds the configuration and ends
> up in tcm_bind(), which takes opts->dep_lock:
> 
>   CPU0: echo 0 > .../tpgt_N/enable    CPU1: echo <udc> > .../UDC
>   tcm_unregister_callback()           gadget_dev_desc_UDC_store()
>     mutex_lock(&opts->dep_lock)         mutex_lock(&gi->lock)
>                                         ...
>                                           tcm_bind()
>                                             mutex_lock(&opts->dep_lock)
> [ ... ]
> configfs dependency on it.
> 
> Fixes: 4bb8548df632 ("usb: gadget: f_tcm: add configfs support")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022134926.1125473-1-sst@poczta.fm?part=3

Re: [PATCH 1/3] usb: gadget: f_tcm: take the TPG in tcm_bind() instead of tcm_alloc()

[sashiko-bot]

> tcm_alloc() runs when a TCM function is symlinked into a gadget
> configuration. It searches tpg_instances[] for the function instance
> and copies the TPG pointer into fu->tpg at that point, under
> tpg_instances_lock. This has two problems.
> 
> The function and the TPG can be set up in either order. If the
> function is linked into a configuration before the TPG directory is
> created, tpg_instances[i].tpg is still NULL and fu->tpg stays NULL.
> Creating and enabling the TPG afterwards sets can_attach, so
> tcm_bind() succeeds, and the first USB request that reaches the
> target code dereferences the NULL fu->tpg.
> 
> Removing a TPG also leaves can_attach set for configfs instances, so a
> later bind is not refused even though the TPG it would use is gone.
> 
> [ ... ]
> Reported-by: syzbot+2799146a53c356f8017d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2799146a53c356f8017d
> Link: https://syzkaller.appspot.com/ai_job?id=51e1f189-6d91-4321-a6ba-07d4eb3cd8cd
> Assisted-by: Gemini:gemini-4-argon Gemini:gemini-3.8-flash syzbot
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022134926.1125473-1-sst@poczta.fm?part=1