usb: gadget: f_tcm: keep port count until LUN teardown completes
Subsystems (4)
PATCH 1/1 usb: gadget: f_tcm: keep port count until LUN teardown completes
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.
--- 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(-)
drivers/target/target_core_fabric_configfs.c +8 -0
@@ -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)
drivers/usb/gadget/function/f_tcm.c +1 -1
@@ -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,
include/target/target_core_fabric.h +2 -0
@@ -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 *);
Signature · 1 line
2.43.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 7 lines
drivers/target/target_core_fabric_configfs.c +0
-0@@ -690,6 +690,14 @@ static void target_fabric_port_unlink( }
drivers/target/target_core_fabric_configfs.c static void target_fabric_port_unlink( · line 701
Quoted 10 lines
drivers/target/target_core_fabric_configfs.c +8
-0core_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?
drivers/usb/gadget/function/f_tcm.c static const struct target_core_fabric_ops usbg_ops = { · line 2028
Quoted 11 lines
drivers/usb/gadget/function/f_tcm.c +1
-1@@ -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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1