Re: [PATCH] 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:

New issues:
- [Medium] Missing `READ_ONCE()` on lockless read of `cmd->se_sess->se_tpg` allows compiler TOCTOU race.

Pre-existing issues:
- [Critical] Incomplete fix leaves identical NULL dereferences and intentional BUG_ON() crashes unhandled in the submission and PR paths, allowing remote Denial of Service.
--

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

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

Commit a47fa41381a0 guarded target_complete() against a NULL se_tpg_wwn.
This patch adds an additional check to prevent a NULL pointer dereference
when se_sess->se_tpg itself is NULL after a session is deregistered.

> 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.

[Severity: Critical]
This is a pre-existing issue, but does deferring these fixes leave the
kernel exposed to crashes from remote initiators?

If an initiator sends a SCSI command or TMR concurrently with session
teardown, the RX path could hit the same race.

For example, could this trigger the BUG_ON() in target_init_cmd() if
transport_deregister_session() clears se_tpg concurrently?

drivers/target/target_core_transport.c:target_init_cmd() {
    ...
    se_tpg = se_sess->se_tpg;
    BUG_ON(!se_tpg);
    ...
}

Could it also crash during TMR submission?

drivers/target/target_core_transport.c:target_submit_tmr() {
    ...
    se_tpg = se_sess->se_tpg;
    BUG_ON(!se_tpg);
    ...
}

Would it be safer to handle all se_tpg teardown races in a single patch?

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index fad03a15c969e..ee5781d894339 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 = cmd->se_sess->se_tpg;
> +	struct se_wwn *wwn = se_tpg ? se_tpg->se_tpg_wwn : NULL;

[Severity: Medium]
Is a READ_ONCE() needed here when reading cmd->se_sess->se_tpg?

Since transport_deregister_session() can concurrently set se_sess->se_tpg to
NULL locklessly, could the compiler refetch the pointer after the NULL check
and still cause a NULL pointer dereference?

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