Re: [PATCH v2] scsi: target: Fix NULL se_tpg dereference in target_complete()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch masks a KASAN NULL pointer dereference in `target_complete()` but leaves a severe Use-After-Free vulnerability. The original NULL dereference was a symptom of a teardown race where commands outlive the session because `iscsit_setup_scsi_cmd()` and `iscsit_handle_task_mgt_cmd()` ignore the `-ESHUTDOWN` error returned by `target_get_sess_cmd()`. This bypasses `cmd_cnt` tracking entirely, allowing `target_wait_for_cmds()` to complete early. Consequently, `transport_free_session()` frees `se_sess->sess_cmd_map` (which contains the memory for `cmd`) and `se_sess->cmd_cnt`. By replacing the NULL dereference with a `READ_ONCE()` check, the patch allows `target_complete()` to proceed and execute `INIT_WORK(&cmd->work, ...)` and `queue_work_on()` on a freed `cmd` structure, corrupting memory. Furthermore, `target_release_cmd_kref()` will eventually call `percpu_ref_put()` on the freed `cmd_cnt`.
- [High] The commit deliberately leaves identical, known NULL pointer dereferences and BUG_ON crashes unpatched in other active command paths, creating a reliable remote Denial-of-Service vector. While the patch mitigates the NULL dereference in `target_complete()`, the commit message explicitly acknowledges that `target_submit()`, `target_init_cmd()`, and `target_submit_tmr()` are exposed to the exact same teardown race where `se_sess->se_tpg` is cleared. `target_submit()` dereferences `se_tpg->se_tpg_tfo` locklessly without a NULL check, while `target_init_cmd()` and `target_submit_tmr()` contain `BUG_ON(!se_tpg)` assertions. `transport_lookup_cmd_lun()` also accesses `se_sess->se_tpg->tpg_virt_lun0` locklessly.
--

--- Patch [1]: [PATCH v2] scsi: target: Fix NULL se_tpg dereference in target_complete() ---
commit 3e9112f38426c64acec2836a61cb50a5d659490b
Author: Yehyeong Lee <[email protected]>

scsi: target: Fix NULL se_tpg dereference in target_complete()

This patch attempts to fix a NULL pointer dereference in target_complete()
by using READ_ONCE to load se_sess->se_tpg and gracefully falling back to a
NULL wwn if it is absent. The issue occurs when an EXTENDED COPY worker or
isert completion outlives session deregistration, resulting in an oops when
accessing cmd->se_sess->se_tpg->se_tpg_wwn.

> target_submit() reads the same chain (se_sess->se_tpg->se_tpg_tfo) and is
> exposed to the identical NULL.  That path has not been reproduced here and
> is left for a separate change.
>
[ ... ]
>
> The BUG_ON(!se_tpg) in target_init_cmd() and target_submit_tmr() can be
> reached by the same teardown race on fabrics that use those helpers.
> iscsit does not - it calls __target_init_cmd() directly and has its own
> TMF path - so those paths could not be reproduced or tested here, and
> converting a deliberate assertion into graceful failure looked like a
> decision for the maintainers rather than something to fold into this fix.

[Severity: High]
This is a pre-existing issue, but does leaving these paths unfixed
leave the kernel vulnerable to remote denial-of-service during session
teardown?

If a misbehaving initiator continues submitting commands during teardown,
or if fabric drivers process commands concurrently, paths like
target_submit() will dereference se_tpg locklessly:

drivers/target/target_core_transport.c:target_submit() {
    ...
    const struct target_core_fabric_ops *tfo = se_cmd->se_sess->se_tpg->se_tpg_tfo;
    ...
}

Similarly, transport_lookup_cmd_lun() accesses se_tpg without a NULL check:

drivers/target/target_core_device.c:transport_lookup_cmd_lun() {
    ...
    se_lun = se_sess->se_tpg->tpg_virt_lun0;
    ...
}

Can a remote initiator trigger these NULL pointer dereferences or hit the
BUG_ON(!se_tpg) in target_init_cmd() and target_submit_tmr() during this race?

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index fad03a15c969e..0b292e3866911 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -904,7 +904,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd)
>  
>  static void target_complete(struct se_cmd *cmd, int success)
>  {
> -	struct se_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn;
> +	struct se_portal_group *se_tpg = READ_ONCE(cmd->se_sess->se_tpg);
> +	struct se_wwn *wwn = se_tpg ? se_tpg->se_tpg_wwn : NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but does bypassing the NULL
dereference here expose a use-after-free on the cmd structure?

The original NULL dereference appears to be a symptom of a teardown race
where commands outlive the session. If iscsit_setup_scsi_cmd() and
iscsit_handle_task_mgt_cmd() ignore the -ESHUTDOWN error returned by
target_get_sess_cmd() during teardown:

drivers/target/iscsi/iscsi_target.c:iscsit_setup_scsi_cmd() {
    ...
    target_get_sess_cmd(&cmd->se_cmd, true);
    ...
}

This bypasses cmd_cnt tracking, allowing target_wait_for_cmds() to
complete early. Consequently, transport_free_session() frees
se_sess->sess_cmd_map (which contains the cmd memory) and cmd_cnt:

drivers/target/target_core_transport.c:transport_free_session() {
    ...
    kvfree(se_sess->sess_cmd_map);
    if (se_sess->cmd_cnt)
        target_free_cmd_counter(se_sess->cmd_cnt);
    ...
}

By allowing target_complete() to proceed past the se_tpg check, does this
execute INIT_WORK(&cmd->work, ...) and queue_work_on() on a freed cmd
structure further down in target_complete()?

Additionally, target_release_cmd_kref() will eventually call
percpu_ref_put() on the freed cmd_cnt. Can remote initiators reach
this memory corruption during connection churn?

>  	struct se_dev_attrib *da;
>  	u8 compl_type;
>  	int cpu;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1