[PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes

[PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes

[Shuangpeng Bai]

tcm_usbg_drop_nexus() permits session removal once tpg_port_count
reaches zero. However, usbg_port_unlink() currently decrements that
count from the fabric_pre_unlink() callback, before core_dev_del_lun()
waits for active se_lun references to drain.

If removal of the last LUN races a nexus removal, the latter can observe
a zero port count and call target_remove_session(). This frees
sess_cmd_map while an in-flight struct usbg_cmd, including its work item,
can still be accessed.

Overlapping the last-LUN unlink with nexus removal reproduces this
lifetime violation as a DEBUG_OBJECTS "free active" warning for
usbg_cmd_work, followed by a target-core BUG/Oops.

The generic target-core unlink path has no callback after
core_dev_del_lun() completes. Add an optional fabric_post_unlink()
callback and use it for the f_tcm port count. The count now remains
nonzero until core_dev_del_lun() has finished draining active LUN
references, preventing nexus removal from freeing the session during
command completion.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Cc: stable@vger.kernel.org
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 drivers/target/target_core_fabric_configfs.c | 8 ++++++++
 drivers/usb/gadget/function/f_tcm.c          | 2 +-
 include/target/target_core_fabric.h          | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 166dbf4c4061..ab8f81650710 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -690,6 +690,14 @@ static void target_fabric_port_unlink(
 	}
 
 	core_dev_del_lun(se_tpg, lun);
+
+	if (tf->tf_ops->fabric_post_unlink) {
+		/*
+		 * Allow fabrics to release state that must remain valid until
+		 * core_dev_del_lun() has drained all active LUN references.
+		 */
+		tf->tf_ops->fabric_post_unlink(se_tpg, lun);
+	}
 }
 
 static void target_fabric_port_release(struct config_item *item)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index b3fa5a17fd2d..98414e7611c0 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -2024,7 +2024,7 @@ static const struct target_core_fabric_ops usbg_ops = {
 	.fabric_enable_tpg		= usbg_enable_tpg,
 	.fabric_drop_tpg		= usbg_drop_tpg,
 	.fabric_post_link		= usbg_port_link,
-	.fabric_pre_unlink		= usbg_port_unlink,
+	.fabric_post_unlink		= usbg_port_unlink,
 	.fabric_init_nodeacl		= usbg_init_nodeacl,
 
 	.tfc_wwn_attrs			= usbg_wwn_attrs,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index e9039e73d058..390ace5bb252 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -95,6 +95,8 @@ struct target_core_fabric_ops {
 				struct se_lun *);
 	void (*fabric_pre_unlink)(struct se_portal_group *,
 				struct se_lun *);
+	void (*fabric_post_unlink)(struct se_portal_group *se_tpg,
+				   struct se_lun *lun);
 	struct se_tpg_np *(*fabric_make_np)(struct se_portal_group *,
 				struct config_group *, const char *);
 	void (*fabric_drop_np)(struct se_tpg_np *);
-- 
2.43.0

Re: [PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch moves the `tpg_port_count` decrement to `fabric_post_unlink` for `f_tcm` to fix a nexus removal UAF race, but fails to apply this fix to sister drivers `vhost_scsi` and `xen-scsiback`.
- [High] Lockless access to `tpg->tpg_nexus` in the I/O path leads to Use-After-Free and NULL pointer dereferences when racing with `tcm_usbg_drop_nexus`.
- [High] The patch's fix for 'free active' and `sess_cmd_map` UAF is incomplete because it ignores pending work items when `tpg_port_count == 0`.
- [High] TOCTOU race between `target_fabric_port_link` and `tcm_usbg_drop_nexus` allows the nexus to be destroyed while a LUN is actively linked.
--

--- Patch [1]: [PATCH] usb: gadget: f_tcm: keep port count until LUN teardown completes ---
commit 052b5e8542d90321dbacef963d1804574c8248ec
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

usb: gadget: f_tcm: keep port count until LUN teardown completes

This patch introduces an optional fabric_post_unlink callback to the
target core. It uses this callback in f_tcm to defer decrementing the
port count until after core_dev_del_lun has drained active LUN
references, fixing a use-after-free race during nexus removal.

Cc: stable@vger.kernel.org

> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> index 166dbf4c40616..ab8f81650710e 100644
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -690,6 +690,14 @@ static void target_fabric_port_unlink(
>  	}
>  
>  	core_dev_del_lun(se_tpg, lun);
> +
> +	if (tf->tf_ops->fabric_post_unlink) {
> +		/*
> +		 * Allow fabrics to release state that must remain valid until
> +		 * core_dev_del_lun() has drained all active LUN references.
> +		 */
> +		tf->tf_ops->fabric_post_unlink(se_tpg, lun);
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the separation of LUN
attachment and port count increment create a time-of-check to time-of-use
race?

In target_fabric_port_link():

drivers/target/target_core_fabric_configfs.c:target_fabric_port_link() {
    ...
    ret = core_dev_add_lun(se_tpg, dev, lun);
    ...
    if (tf->tf_ops->fabric_post_link)
        tf->tf_ops->fabric_post_link(se_tpg, lun);
}

Since tpg_port_count is incremented inside fabric_post_link() for f_tcm, is
there a window after core_dev_add_lun() attaches the LUN where
tcm_usbg_drop_nexus() could observe tpg_port_count == 0?

If it executes concurrently during this window, would it bypass the safety
check and remove the session while a LUN has just been fully linked?

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index b3fa5a17fd2db..98414e7611c08 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -2024,7 +2024,7 @@ static const struct target_core_fabric_ops usbg_ops = {
>  	.fabric_enable_tpg		= usbg_enable_tpg,
>  	.fabric_drop_tpg		= usbg_drop_tpg,
>  	.fabric_post_link		= usbg_port_link,
> -	.fabric_pre_unlink		= usbg_port_unlink,
> +	.fabric_post_unlink		= usbg_port_unlink,
>  	.fabric_init_nodeacl		= usbg_init_nodeacl,

[Severity: High]
This isn't a bug introduced by this patch, but does moving this decrement to
fabric_post_unlink for f_tcm leave vhost_scsi and xen-scsiback vulnerable to
the same race?

Looking at vhost_scsi_ops in drivers/vhost/scsi.c:

    .fabric_pre_unlink = vhost_scsi_port_unlink,

And scsiback_ops in drivers/xen/xen-scsiback.c:

    .fabric_pre_unlink = scsiback_port_unlink,

If a nexus removal is triggered concurrently with the removal of the last
LUN on either of these fabrics, won't the early port count decrement in
fabric_pre_unlink allow them to bypass the EBUSY check? It appears they could
proceed to call target_remove_session() before core_dev_del_lun() has finished
waiting for active commands to drain, potentially causing a similar
use-after-free.

[Severity: High]
This is a pre-existing issue, but can lockless reads of tpg->tpg_nexus
in the I/O path race with nexus removal?

In tcm_usbg_drop_nexus(), we free the nexus and clear the pointer:

drivers/usb/gadget/function/f_tcm.c:tcm_usbg_drop_nexus() {
    ...
    tpg->tpg_nexus = NULL;
    kfree(tv_nexus);
    ...
}

Meanwhile, the USB completion callbacks like usbg_submit_command() and
usbg_submit_tmr() read it without holding tpg_mutex or the RCU read lock:

drivers/usb/gadget/function/f_tcm.c:usbg_submit_command() {
    ...
    tv_nexus = tpg->tpg_nexus;
    ...
}

Could this result in a NULL pointer dereference or use-after-free if the
nexus is dropped via configfs while a command is actively being processed?

[Severity: High]
This is a pre-existing issue, but does this fix cover the case where
tpg_port_count is 0 but work is still pending?

If no explicit LUNs are configured, tpg_port_count is 0. If a command is
queued to usbg_cmd_work concurrently with the admin dropping the nexus:

drivers/usb/gadget/function/f_tcm.c:tcm_usbg_drop_nexus() {
    ...
    if (tpg->tpg_port_count != 0) {
        ...
    }
    ...
    target_remove_session(se_sess);
}

Could this flow proceed to remove the session and free sess_cmd_map without
waiting for usbg_cmd_work to complete?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807060733.3186624-1-shuangpeng.kernel@gmail.com?part=1