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

[sashiko-bot@kernel.org]

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