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

[sashiko-bot@kernel.org]

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