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

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

[Yehyeong Lee]

Commit a47fa41381a0 ("scsi: target: Fix NULL dereference on XCOPY
completion") guarded target_complete() against a NULL se_tpg_wwn, which the
EXTENDED COPY worker hits because its global xcopy_pt_tpg carries no
se_tpg_wwn.  target_complete() dereferences
cmd->se_sess->se_tpg->se_tpg_wwn; that fix handled the innermost pointer.

The same statement can fault one level earlier: se_sess->se_tpg itself may
be NULL.  After a session is deregistered, transport_deregister_session()
clears se_sess->se_tpg, yet the isert receive path can still submit and
complete a command that references that session, so the backend completion
runs target_complete() with se_tpg == NULL and dereferences it:

  Oops: general protection fault, probably for non-canonical address ...
  KASAN: null-ptr-deref in range [0x0000000000000080-0x0000000000000087]
  Workqueue: ib-comp-wq ib_cq_poll_work
  RIP: 0010:target_complete_cmd_with_sense.part.0+0x17d/0xc90
  Call Trace:
   fd_execute_rw
   __target_execute_cmd
   iscsit_execute_cmd
   iscsit_sequence_cmd
   isert_recv_done
   __ib_process_cq
   ib_cq_poll_work

The exact window in which an isert completion outlives the session
deregistration has not been pinned down; the guard defends the dereference
regardless of how the command reaches this point.

Load se_tpg first and fall back to a NULL wwn when it is absent.  The
existing "if (!wwn)" path already queues the completion on cmd->cpuid,
so no new branch is needed; this extends that guard by one pointer.

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.

Reproduced with soft-RoCE (rdma_rxe) and KASAN under iSCSI connection
churn; no kernel-side test hooks were needed.

Fixes: 39ae3edda325 ("scsi: target: core: Make completion affinity configurable")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Tested on 7.2.0-rc4 with soft-RoCE (rdma_rxe) + KASAN under iSCSI/iSER
connection churn.  Without this patch the target crashes in 2 of 3 runs;
with it, 10 consecutive runs are clean and complete the full workload
(160 of 160 cycles, matching the unpatched baseline).

To confirm that the guard - and not an incidental timing change - is what
prevents the crash, a build carrying an additional print inside the new
NULL branch was run 10 more times: a NULL se_tpg was observed 13 times
across 7 of those runs and was handled every time, with no oops and no
change in I/O counts.

 drivers/target/target_core_transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index fad03a15c969..ee5781d89433 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;
 	struct se_dev_attrib *da;
 	u8 compl_type;
 	int cpu;

base-commit: 248951ddc14de84de3910f9b13f51491a8cd91df
-- 
2.43.0

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

[sashiko-bot]

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 <yhlee@isslab.korea.ac.kr>

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/20260725084939.694403-1-yhlee@isslab.korea.ac.kr?part=1