[PATCH] scsi: target: fix demo-mode dynamic ACL dev-entry UAF vs LUN teardown

[PATCH] scsi: target: fix demo-mode dynamic ACL dev-entry UAF vs LUN teardown

[Yu Zhang]

A demo-mode (generate_node_acls=1) login that allocates a dynamic
NodeACL populates that ACL's per-LUN dev entries in
core_tpg_add_node_to_devs() *before* the ACL is linked onto
tpg->acl_node_list by target_add_node_acl():

	core_tpg_check_initiator_node_acl():
		...
		core_tpg_add_node_to_devs(acl, tpg, NULL); /* builds deves */
		target_add_node_acl(acl);                  /* only now listed */

The LUN teardown sweep, core_clear_lun_from_tpg(), enumerates only
tpg->acl_node_list:

	list_for_each_entry(nacl, &tpg->acl_node_list, acl_list) {
		mutex_lock(&nacl->lun_entry_mutex);
		hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link) {
			if (lun != deve->se_lun)
				continue;
			core_disable_device_list_for_node(lun, deve, nacl, tpg);
		}
		...
	}

so a dev entry whose ACL is still in the window above is invisible to
the sweep and is never disabled.  It is left on the
se_lun->lun_deve_list after core_tpg_remove_lun() frees the se_lun.
When the same unpacked_lun is later re-added and the se_lun is
reallocated at the same address, the stale dev entry is rediscovered by
target_nacl_find_deve() (which matches on mapped_lun) and
core_enable_device_list_for_node() takes the replace branch:

	spin_lock(&lun->lun_deve_lock);
	list_del(&orig->lun_link);	/* corrupts lun_deve_list */

corrupting the freed/reused list.  With list hardening this fires as:

  Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
  RIP: __list_del_entry_valid_or_report+0x105/0x140
  Call Trace:
   core_enable_device_list_for_node+0x5ce/0x880
   core_tpg_add_node_to_devs+0x169/0x190
   core_dev_add_lun+0x146/0x190
   target_fabric_port_link+0xe9/0x1b0
   configfs_symlink+0x381/0xae0
   __x64_sys_symlink+0x55/0x80
  list_del corruption. prev->next should be ffff888005d7ba80,
    but was ffff88800249a580. (prev=ffff888006e23820)

The demo-mode login path that drives the ACL/dev-entry creation is
reachable by an unauthenticated initiator (the work runs during
first-stage login negotiation, before authentication); the colliding
LUN add/remove is a local configfs operation.

Commit 49cb77e297dc ("target: Avoid mappedlun symlink creation during
lun shutdown") introduced se_lun->lun_shutdown and guarded the
*explicit* ACL MappedLUN path (target_fabric_mappedlun_link()), but the
demo-mode automatic path, core_tpg_add_node_to_devs() ->
core_enable_device_list_for_node(), was never given the equivalent
guard, and the publish-after-populate ordering in
core_tpg_check_initiator_node_acl() was left unchanged.

Fix both halves, which are each necessary:

 1) Publish the dynamic NodeACL onto acl_node_list *before* creating
    its dev entries, so the teardown sweep can observe them.  On its
    own this still races: the sweep may pass the ACL before the dev
    entry is added.

 2) Refuse to attach a dev entry to an se_lun that is shutting down,
    checked under nacl->lun_entry_mutex -- the same lock the sweep
    takes.  Because core_tpg_remove_lun() sets lun_shutdown before the
    sweep, once the sweep has passed a given ACL any later enable for
    that LUN observes lun_shutdown and skips it, so no dev entry can be
    left dangling on the lun_deve_list of an se_lun about to be freed.

core_tpg_add_node_to_devs() already ignores the return value of
core_enable_device_list_for_node(), so a shutting-down LUN is simply
skipped.

Note: moving target_add_node_acl() earlier makes a concurrent lookup
for the same InitiatorName observe the ACL slightly before its demo
MappedLUNs are populated; this transient was already possible between
the two original calls and is harmless, but flagging it for review.
The precise lock chosen to publish/observe lun_shutdown is open to
maintainer preference.

Fixes: 49cb77e297dc ("target: Avoid mappedlun symlink creation during lun shutdown")
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
Tested-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/target/target_core_device.c | 13 +++++++++++++
 drivers/target/target_core_tpg.c    | 10 +++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 1111111..2222222 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -349,6 +349,19 @@
 	new->attach_count++;
 
 	mutex_lock(&nacl->lun_entry_mutex);
+	/*
+	 * Do not attach a new dev entry to an se_lun that is being torn down.
+	 * lun_shutdown is set by core_tpg_remove_lun() before its
+	 * core_clear_lun_from_tpg() sweep; checking it here under the same
+	 * nacl->lun_entry_mutex the sweep takes guarantees that once the sweep
+	 * has passed this ACL no dev entry can be left dangling on the
+	 * se_lun->lun_deve_list that is about to be freed.
+	 */
+	if (lun->lun_shutdown) {
+		mutex_unlock(&nacl->lun_entry_mutex);
+		ret = -ENODEV;
+		goto free_stats;
+	}
 	orig = target_nacl_find_deve(nacl, mapped_lun);
 	if (orig && orig->se_lun) {
 		struct se_lun *orig_lun = orig->se_lun;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 3333333..4444444 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -267,6 +267,15 @@
 	acl->dynamic_node_acl = 1;
 
 	/*
+	 * Publish the dynamic NodeACL on tpg->acl_node_list before creating
+	 * its demo-mode MappedLUN dev entries below, so that a concurrent LUN
+	 * teardown sweep (core_clear_lun_from_tpg(), which walks acl_node_list)
+	 * can observe and disable those dev entries.  Otherwise a dev entry can
+	 * be left on an se_lun->lun_deve_list that is about to be freed.
+	 */
+	target_add_node_acl(acl);
+
+	/*
 	 * Here we only create demo-mode MappedLUNs from the active
 	 * TPG LUNs if the fabric is not explicitly asking for
 	 * tpg_check_demo_mode_login_only() == 1.
@@ -275,7 +284,6 @@
 	    (tpg->se_tpg_tfo->tpg_check_demo_mode_login_only(tpg) != 1))
 		core_tpg_add_node_to_devs(acl, tpg, NULL);
 
-	target_add_node_acl(acl);
 	return acl;
 }
 EXPORT_SYMBOL(core_tpg_check_initiator_node_acl);
-- 
2.43.0

Re: [PATCH] scsi: target: fix demo-mode dynamic ACL dev-entry UAF vs LUN teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: target: TOCTOU race in core_enable_device_list_for_node causes UAF
--

commit fe6f881325b9fa3f70c9e7a80a2d042d342f070d
Author: Yu Zhang <yuz08559@gmail.com>
scsi: target: fix demo-mode dynamic ACL dev-entry UAF vs LUN teardown

This commit addresses a use-after-free triggered by a race between a demo-mode
login creating dynamic node acls and a concurrent lun teardown. It attempts to
fix the issue by publishing the node acl before populating its dev entries, and
refusing to attach new dev entries to a shutting down se_lun.

> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 9db2201aa553f..5eef87aa3b674 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -349,6 +349,19 @@ int core_enable_device_list_for_node(
>  	new->attach_count++;
>  
>  	mutex_lock(&nacl->lun_entry_mutex);
> +	/*
> +	 * Do not attach a new dev entry to an se_lun that is being torn down.
> +	 * lun_shutdown is set by core_tpg_remove_lun() before its
> +	 * core_clear_lun_from_tpg() sweep; checking it here under the same
> +	 * nacl->lun_entry_mutex the sweep takes guarantees that once the sweep
> +	 * has passed this ACL no dev entry can be left dangling on the
> +	 * se_lun->lun_deve_list that is about to be freed.
> +	 */
> +	if (lun->lun_shutdown) {
> +		mutex_unlock(&nacl->lun_entry_mutex);
> +		ret = -ENODEV;
> +		goto free_stats;
> +	}

[Severity: High]
Can a time-of-check to time-of-use race condition regression occur here?

Because the target_core_tpg.c portion of this patch publishes the dynamic
node acl before creating its dev entries, a concurrent lun teardown sweep can
now observe the dev entry while it is being added in
core_enable_device_list_for_node().

If the teardown sweep sets lun->lun_shutdown = true right after we check it
here, the sweep can run and wait on nacl->lun_entry_mutex.

When core_enable_device_list_for_node() later drops that mutex:

drivers/target/target_core_device.c:core_enable_device_list_for_node() {
    ...
    hlist_add_head_rcu(&new->link, &nacl->lun_entry_hlist);
    mutex_unlock(&nacl->lun_entry_mutex);

    spin_lock(&lun->lun_deve_lock);
    list_add_tail(&new->lun_link, &lun->lun_deve_list);
    spin_unlock(&lun->lun_deve_lock);
    ...
}

The teardown sweep takes the mutex and calls
core_disable_device_list_for_node(), which performs:

    list_del(&orig->lun_link);

Because new->lun_link is merely INIT_LIST_HEAD'ed at this point, list_del()
silently succeeds and sets list poisoning rather than preventing the
concurrent thread from linking it.

Then, the original thread resumes, takes lun->lun_deve_lock, and calls
list_add_tail(&new->lun_link, &lun->lun_deve_list), overwriting the poison
and successfully linking the doomed entry.

When the rcu grace period expires, the entry is freed, leaving a dangling
pointer in lun->lun_deve_list. Iterating lun->lun_deve_list subsequently can
trigger a use-after-free.

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