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

[PATCH v2] 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
read is lockless - transport_deregister_session() clears the field under
se_tpg->session_lock, which target_complete() does not hold - so use
READ_ONCE() to keep the compiler from re-fetching se_sess->se_tpg after
the NULL test.  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>
---

v2:
- Use READ_ONCE() for the lockless read of se_sess->se_tpg.  The field is
  cleared concurrently by transport_deregister_session(), so nothing
  prevents the compiler from re-fetching it after the NULL test.  The
  store side is a plain aligned pointer write under se_tpg->session_lock
  and is left unannotated; happy to add WRITE_ONCE() there as well if
  preferred.
- Retested from scratch; every number below is from the v2 build.

Tested on 7.2.0-rc4 with soft-RoCE (rdma_rxe) + KASAN under iSCSI/iSER
connection churn.  Unpatched, 5 of 10 runs oops with the same RIP
(target_complete_cmd_with_sense.part.0+0x17d).  With this patch, 20
consecutive runs show no oops.

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 9 times
across 7 of those runs and was handled every time, with no oops.

Some runs end early with an initiator-side login timeout from the
soft-RoCE rig instead of completing the full 160-cycle workload: 4 of the
20 patched runs, and 2 of the 5 unpatched runs that did not oops (the
other 5 unpatched runs time out after the oops).  The rig behaves the
same way with and without the patch.

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.

 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..0b292e386691 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;
 	struct se_dev_attrib *da;
 	u8 compl_type;
 	int cpu;
-- 
2.43.0

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

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

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