[PATCH] scsi: target: tcm_loop: fix tl_nexus use-after-free on concurrent nexus drop

[PATCH] scsi: target: tcm_loop: fix tl_nexus use-after-free on concurrent nexus drop

[Yu Zhang]

tcm_loop keeps the per-TPG I_T nexus in tl_tpg->tl_nexus with no lock,
refcount or RCU protection.  The configfs control path that creates and
destroys it is not serialized: configfs runs concurrent ->store() calls
in parallel (configfs_write_iter() only holds a per-open-file
buffer->mutex, and flush_write_buffer() takes frag_sem for read), so two
writers to the nexus attribute, or a writer racing the transport_status
or nexus show attribute, all touch tl_nexus concurrently.

A nexus created without linking a LUN has tl_tpg_port_count == 0, so
tcm_loop_drop_nexus() ("echo NULL > .../nexus") passes its port-count
guard and frees the nexus (target_remove_session() + kfree()) with no
lock held.  Two concurrent drops therefore both read the same tl_nexus,
both pass the guard, and both free the same se_session -> use-after-free
/ double free.  The sibling fabric drivers vhost_scsi and xen-scsiback
serialize the equivalent nexus create/drop/show with a per-TPG
tv_tpg_mutex; tcm_loop has no such lock.

KASAN, two concurrent "echo NULL > .../loopback/<wwn>/tpgt_1/nexus"
(nexus created, no LUN linked), reproduces in seconds:

  BUG: KASAN: slab-use-after-free in transport_free_session+0x12f/0x3f0
  Call Trace:
   transport_free_session+0x12f/0x3f0
   tcm_loop_drop_nexus+0x85/0xf0
   tcm_loop_tpg_nexus_store+0x392/0x580
   configfs_write_iter+0x1a7/0x250
  Allocated by task 3008:  ... tcm_loop_tpg_nexus_store <- configfs_write_iter
  Freed by task 3010:      ... tcm_loop_drop_nexus <- configfs_write_iter

Add a per-TPG tl_nexus_mutex and take it around every control-path
access to tl_nexus: tcm_loop_make_nexus(), tcm_loop_drop_nexus(),
tcm_loop_tpg_nexus_show() and the transport_status "offline" path that
dereferences tl_nexus->se_sess.  tcm_loop_drop_nexus() clears tl_nexus
under the mutex and only releases/frees the session after dropping it,
so a racing control-path caller either observes the live nexus under the
lock or sees NULL and bails -- it can no longer free it a second time.

The I/O submission path (tcm_loop_target_queue_cmd(), tcm_loop_issue_tmr())
reads tl_nexus without the mutex and is left unchanged: it can run in
atomic context, and it is already serialized against drop by
tl_tpg_port_count plus SCSI-midlayer draining (a live scsi_device keeps
tl_tpg_port_count > 0, which makes tcm_loop_drop_nexus() return -EPERM).

Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
Tested-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/target/loopback/tcm_loop.c | 44 +++++++++++++++++++++++-------
 drivers/target/loopback/tcm_loop.h |  3 +++
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 1111111..2222222 100644
@@ -672,14 +672,18 @@
 	struct tcm_loop_nexus *tl_nexus;
 	int ret;
 
+	mutex_lock(&tl_tpg->tl_nexus_mutex);
 	if (tl_tpg->tl_nexus) {
 		pr_debug("tl_tpg->tl_nexus already exists\n");
-		return -EEXIST;
+		ret = -EEXIST;
+		goto out;
 	}
 
 	tl_nexus = kzalloc_obj(*tl_nexus);
-	if (!tl_nexus)
-		return -ENOMEM;
+	if (!tl_nexus) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	tl_nexus->se_sess = target_setup_session(&tl_tpg->tl_se_tpg, 0, 0,
 					TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS,
@@ -687,12 +691,15 @@
 	if (IS_ERR(tl_nexus->se_sess)) {
 		ret = PTR_ERR(tl_nexus->se_sess);
 		kfree(tl_nexus);
-		return ret;
+		goto out;
 	}
 
 	pr_debug("TCM_Loop_ConfigFS: Established I_T Nexus to emulated %s Initiator Port: %s\n",
 		 tcm_loop_dump_proto_id(tl_hba), name);
-	return 0;
+	ret = 0;
+out:
+	mutex_unlock(&tl_tpg->tl_nexus_mutex);
+	return ret;
 }
 
 static int tcm_loop_drop_nexus(
@@ -701,17 +708,23 @@
 	struct se_session *se_sess;
 	struct tcm_loop_nexus *tl_nexus;
 
+	mutex_lock(&tpg->tl_nexus_mutex);
 	tl_nexus = tpg->tl_nexus;
-	if (!tl_nexus)
+	if (!tl_nexus) {
+		mutex_unlock(&tpg->tl_nexus_mutex);
 		return -ENODEV;
+	}
 
 	se_sess = tl_nexus->se_sess;
-	if (!se_sess)
+	if (!se_sess) {
+		mutex_unlock(&tpg->tl_nexus_mutex);
 		return -ENODEV;
+	}
 
 	if (atomic_read(&tpg->tl_tpg_port_count)) {
 		pr_err("Unable to remove TCM_Loop I_T Nexus with active TPG port count: %d\n",
 		       atomic_read(&tpg->tl_tpg_port_count));
+		mutex_unlock(&tpg->tl_nexus_mutex);
 		return -EPERM;
 	}
 
@@ -719,10 +732,14 @@
 		 tcm_loop_dump_proto_id(tpg->tl_hba),
 		 tl_nexus->se_sess->se_node_acl->initiatorname);
 	/*
-	 * Release the SCSI I_T Nexus to the emulated Target Port
+	 * Clear tl_nexus under tl_nexus_mutex so a concurrent drop/show/
+	 * transport_status store cannot observe or free it a second time,
+	 * then release and free the session after dropping the lock.
 	 */
-	target_remove_session(se_sess);
 	tpg->tl_nexus = NULL;
+	mutex_unlock(&tpg->tl_nexus_mutex);
+
+	target_remove_session(se_sess);
 	kfree(tl_nexus);
 	return 0;
 }
@@ -737,12 +754,16 @@
 	struct tcm_loop_nexus *tl_nexus;
 	ssize_t ret;
 
+	mutex_lock(&tl_tpg->tl_nexus_mutex);
 	tl_nexus = tl_tpg->tl_nexus;
-	if (!tl_nexus)
+	if (!tl_nexus) {
+		mutex_unlock(&tl_tpg->tl_nexus_mutex);
 		return -ENODEV;
+	}
 
 	ret = snprintf(page, PAGE_SIZE, "%s\n",
 		tl_nexus->se_sess->se_node_acl->initiatorname);
+	mutex_unlock(&tl_tpg->tl_nexus_mutex);
 
 	return ret;
 }
@@ -861,11 +882,13 @@
 	}
 	if (!strncmp(page, "offline", 7)) {
 		tl_tpg->tl_transport_status = TCM_TRANSPORT_OFFLINE;
+		mutex_lock(&tl_tpg->tl_nexus_mutex);
 		if (tl_tpg->tl_nexus) {
 			struct se_session *tl_sess = tl_tpg->tl_nexus->se_sess;
 
 			core_allocate_nexus_loss_ua(tl_sess->se_node_acl);
 		}
+		mutex_unlock(&tl_tpg->tl_nexus_mutex);
 		return count;
 	}
 	return -EINVAL;
@@ -923,6 +946,7 @@
 	tl_tpg = &tl_hba->tl_hba_tpgs[tpgt];
 	tl_tpg->tl_hba = tl_hba;
 	tl_tpg->tl_tpgt = tpgt;
+	mutex_init(&tl_tpg->tl_nexus_mutex);
 	/*
 	 * Register the tl_tpg as a emulated TCM Target Endpoint
 	 */
diff --git a/drivers/target/loopback/tcm_loop.h b/drivers/target/loopback/tcm_loop.h
index 3333333..4444444 100644
@@ -2,6 +2,7 @@
 #include <linux/types.h>
 #include <linux/device.h>
 #include <target/target_core_base.h> /* struct se_cmd */
+#include <linux/mutex.h>
 
 #define TCM_LOOP_VERSION		"v2.1-rc2"
 #define TL_WWN_ADDR_LEN			256
@@ -39,6 +40,8 @@
 	struct se_portal_group tl_se_tpg;
 	struct tcm_loop_hba *tl_hba;
 	struct tcm_loop_nexus *tl_nexus;
+	/* Serializes tl_nexus accesses on the configfs control path */
+	struct mutex tl_nexus_mutex;
 };
 
 struct tcm_loop_hba {
-- 
2.43.0

Re: [PATCH] scsi: target: tcm_loop: fix tl_nexus use-after-free on concurrent nexus drop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tcm_loop: UAF in I/O path due to port link and nexus drop TOCTOU race
--

commit f5acf49b795a91429ba7f28b4f3ce2e9fca1884a
Author: Yu Zhang <yuz08559@gmail.com>

    scsi: target: tcm_loop: fix tl_nexus use-after-free on concurrent nexus drop
    
    tcm_loop keeps the per-TPG I_T nexus in tl_tpg->tl_nexus with no lock,
    refcount or RCU protection.  The configfs control path that creates and
    destroys it is not serialized: configfs runs concurrent ->store() calls
    in parallel (configfs_write_iter() only holds a per-open-file
    buffer->mutex, and flush_write_buffer() takes frag_sem for read), so two
    writers to the nexus attribute, or a writer racing the transport_status
    or nexus show attribute, all touch tl_nexus concurrently.

[ ... ]

    The I/O submission path (tcm_loop_target_queue_cmd(), tcm_loop_issue_tmr())
    reads tl_nexus without the mutex and is left unchanged: it can run in
    atomic context, and it is already serialized against drop by
    tl_tpg_port_count plus SCSI-midlayer draining (a live scsi_device keeps
    tl_tpg_port_count > 0, which makes tcm_loop_drop_nexus() return -EPERM).
    
    Cc: stable@vger.kernel.org
    Signed-off-by: Yu Zhang <yuz08559@gmail.com>
    Tested-by: Yu Zhang <yuz08559@gmail.com>

> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index d29830b951f75..980e9e65b788c 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c

[ ... ]

> @@ -701,17 +708,23 @@ static int tcm_loop_drop_nexus(
>  	struct se_session *se_sess;
>  	struct tcm_loop_nexus *tl_nexus;
>  
> +	mutex_lock(&tpg->tl_nexus_mutex);
>  	tl_nexus = tpg->tl_nexus;
> -	if (!tl_nexus)
> +	if (!tl_nexus) {
> +		mutex_unlock(&tpg->tl_nexus_mutex);
>  		return -ENODEV;
> +	}
>  
>  	se_sess = tl_nexus->se_sess;
> -	if (!se_sess)
> +	if (!se_sess) {
> +		mutex_unlock(&tpg->tl_nexus_mutex);
>  		return -ENODEV;
> +	}
>  
>  	if (atomic_read(&tpg->tl_tpg_port_count)) {
>  		pr_err("Unable to remove TCM_Loop I_T Nexus with active TPG port count: %d\n",
>  		       atomic_read(&tpg->tl_tpg_port_count));
> +		mutex_unlock(&tpg->tl_nexus_mutex);
>  		return -EPERM;
>  	}
>  

[Severity: High]
Does this port count check prevent a concurrent port link from immediately
incrementing the count and issuing I/O before the nexus is freed?

While this check is under tl_nexus_mutex, tcm_loop_port_link() increments
the port count and calls scsi_add_device() without holding tl_nexus_mutex:

drivers/target/loopback/tcm_loop.c:tcm_loop_port_link() {
    ...
    atomic_inc_mb(&tl_tpg->tl_tpg_port_count);
    /*
     * Add Linux/SCSI struct scsi_device by HCTL
     */
    scsi_add_device(tl_hba->sh, 0, tl_tpg->tl_tpgt, lun->unpacked_lun);
    ...
}

If tcm_loop_drop_nexus() sees a count of 0 and proceeds to clear and free
tl_nexus, could a concurrent configfs symlink creation (tcm_loop_port_link)
transition the count to 1 and queue INQUIRY commands via scsi_add_device()?

Since scsi_add_device() synchronously queues commands via
tcm_loop_target_queue_cmd(), which locklessly reads tl_tpg->tl_nexus, could
the I/O path dereference the newly freed tl_nexus->se_sess if it races with
tcm_loop_drop_nexus() clearing and freeing it?

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