[PATCH] scsi: target: iscsi: do not reject the command on a Data-Out overflow

[PATCH] scsi: target: iscsi: do not reject the command on a Data-Out overflow

[Yehyeong Lee]

__iscsit_check_dataout_hdr() answers a Data-Out whose offset plus length
exceeds the command's EDTL with iscsit_reject_cmd(), which queues the
Reject on the offending command and then drops its reference:

	cmd->i_state = ISTATE_SEND_REJECT;
	iscsit_add_cmd_to_response_queue(cmd, conn, cmd->i_state);
	/*
	 * Perform the kref_put now if se_cmd has already been setup by
	 * scsit_setup_scsi_cmd()
	 */
	if (do_put) {
		pr_debug("iscsi reject: calling target_put_sess_cmd >>>>>>\n");
		target_put_sess_cmd(&cmd->se_cmd);
	}

For a command that has already completed that put is the last reference.  A
WRITE that failed before it consumed its data - because the LUN does not
exist, or the LBA is out of range - stays on conn->conn_cmd_list until the
initiator's ExpStatSN acknowledges it in iscsit_ack_from_expstatsn(), which
Data-Out PDUs never reach.  So iscsit_release_cmd() runs on a command that
is still on that list and still queued for the tx thread, and the command's
tag goes back to se_sess->sess_tag_pool.  The tx thread then builds the
queued Reject from the released command and sets ISTATE_SENT_STATUS in it,
and the next PDU from the initiator frees it a second time.

Two PDUs on a session that may write are enough:

	WRITE(10), EDTL 512, to a LUN that is not mapped  -> CHECK CONDITION
	Data-Out, same ITT, Offset 512, DataSegmentLength 1, no payload

Declaring a DataSegmentLength that is not sent keeps the connection in
sync, so the session stays up and the second free arrives with the next
command; if the connection does drop first, it comes from
iscsit_release_commands_from_conn() instead.  On a KASAN + PROVE_LOCKING +
DEBUG_LIST guest, 5 boots out of 5:

  WARNING: drivers/target/iscsi/iscsi_target_util.c:658 at iscsit_release_cmd+0x2e0/0x390, CPU#0: iscsi_trx/140
  CPU: 0 UID: 0 PID: 140 Comm: iscsi_trx Not tainted 7.4.0-rc2-00563-g97d73c825122 #1 PREEMPT(lazy)
  Call Trace:
   <TASK>
   target_release_cmd_kref+0x3ea/0x9f0
   target_put_sess_cmd+0x48/0x60
   iscsit_add_reject_from_cmd+0x164/0x3c0
   iscsit_check_dataout_hdr+0x1c1/0x360
   iscsit_get_rx_pdu+0x5da/0x35d0
   iscsi_target_rx_thread+0x132/0x2f0
   kthread+0x2ce/0x3b0
   ret_from_fork+0x52e/0x780
   ret_from_fork_asm+0x1a/0x30
   </TASK>
  ---[ end trace 0000000000000000 ]---

  refcount_t: underflow; use-after-free.
  WARNING: lib/refcount.c:37 at refcount_warn_saturate+0xad/0xe0, CPU#0: iscsi_trx/140
  CPU: 0 UID: 0 PID: 140 Comm: iscsi_trx Tainted: G        W           7.4.0-rc2-00563-g97d73c825122 #1 PREEMPT(lazy)
  Call Trace:
   <TASK>
   transport_generic_free_cmd+0x296/0x2d0
   iscsit_free_cmd+0xa0/0x180
   iscsit_ack_from_expstatsn+0x31c/0x6c0
   iscsit_process_scsi_cmd+0xd4/0x300
   iscsit_get_rx_pdu+0x118c/0x35d0
   iscsi_target_rx_thread+0x132/0x2f0
   kthread+0x2ce/0x3b0
   ret_from_fork+0x52e/0x780
   ret_from_fork_asm+0x1a/0x30
   </TASK>
  ---[ end trace 0000000000000000 ]---

KASAN stays quiet because iscsit_release_cmd() ends in target_free_tag()
and commands are reused from the session tag pool instead of being freed to
the slab, so the second free lands on whichever command owns that tag.

Reject the PDU instead, with iscsit_add_reject(), which is what the
DataSegmentLength check in iscsit_check_dataout_hdr() a few lines below
already does for the same kind of protocol error: it builds the Reject from
a command of its own and leaves the offending command, and its reference,
where they are.

The put was added to this path by commit 3e1c81a95f0d77 ("iscsi-target:
Refactor RX PDU logic + export request PDU handling"), where it ran after
wait_for_completion(&cmd->reject_comp), that is after the tx thread had
sent the Reject; commit ba159914086f06 ("iscsi-target: Fix
iscsit_add_reject* usage for iser") removed that wait, so the release can
now also race the Reject that is still queued.

Fixes: 3e1c81a95f0d77 ("iscsi-target: Refactor RX PDU logic + export request PDU handling")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/target/iscsi/iscsi_target.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 124ff269b8e759..b101f1e7376d4e 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1485,7 +1485,12 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf,
 		pr_err("DataOut Offset: %u, Length %u greater than iSCSI Command EDTL %u, protocol error.\n",
 		       be32_to_cpu(hdr->offset), payload_length,
 		       cmd->se_cmd.data_length);
-		return iscsit_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_INVALID, buf);
+		/*
+		 * Reject the PDU, not the command: iscsit_reject_cmd() would
+		 * drop its last reference while it is on conn_cmd_list.
+		 */
+		return iscsit_add_reject(conn, ISCSI_REASON_BOOKMARK_INVALID,
+					 buf);
 	}
 
 	if (cmd->unsolicited_data) {
-- 
2.43.0

Re: [PATCH] scsi: target: iscsi: do not reject the command on a Data-Out overflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Critical] scsi: target: iscsi: NULL deref in isert_post_recv() via
  iscsit_add_reject()
--

commit b4583fae14731c9492a924f04662a5a7049c19bd
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: target: iscsi: do not reject the command on a Data-Out overflow

This commit changes __iscsit_check_dataout_hdr() to reject an overflowing
Data-Out PDU with iscsit_add_reject() instead of iscsit_reject_cmd() so
that the command's reference is not dropped while it remains on
conn_cmd_list.

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 03d618cd76721..884c9678b4ed9 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1485,7 +1485,12 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf,
>  		pr_err("DataOut Offset: %u, Length %u greater than iSCSI Command EDTL %u, protocol error.\n",
>  		       be32_to_cpu(hdr->offset), payload_length,
>  		       cmd->se_cmd.data_length);
> -		return iscsit_reject_cmd(cmd, ISCSI_REASON_BOOKMARK_INVALID, buf);
> +		/*
> +		 * Reject the PDU, not the command: iscsit_reject_cmd() would
> +		 * drop its last reference while it is on conn_cmd_list.
> +		 */
> +		return iscsit_add_reject(conn, ISCSI_REASON_BOOKMARK_INVALID,
> +					 buf);
>  	}
>  
>  	if (cmd->unsolicited_data) {

[Severity: Critical]
Does this cause a NULL pointer dereference in isert_post_recv() on iSER
connections when an unsolicited Data-Out PDU overflows the command's EDTL?

isert_handle_iscsi_dataout() calls iscsit_check_dataout_hdr() ->
__iscsit_check_dataout_hdr().  iscsit_add_reject() allocates a fresh
command with iscsit_allocate_cmd() rather than isert_allocate_cmd(),
leaving the transport-private struct isert_cmd zeroed with rx_desc,
iscsit_cmd, and conn all NULL, and returns -1 so
isert_handle_iscsi_dataout() returns without reposting the incoming
rx_desc.

When the TX thread processes the queued ISTATE_SEND_REJECT response via
isert_response_queue() -> isert_put_reject() -> isert_post_response(), it
passes that NULL isert_cmd->rx_desc to isert_post_recv():

drivers/infiniband/ulp/isert/ib_isert.c:isert_post_response() {
	ret = isert_post_recv(isert_conn, isert_cmd->rx_desc);
	...
}

drivers/infiniband/ulp/isert/ib_isert.c:isert_post_recv() {
	if (!rx_desc->in_use) {
	...
}

This dereferences rx_desc when it is NULL, leaks the original receive
descriptor, and leaves isert_cmd->iscsit_cmd and isert_cmd->conn NULL for
isert_send_done() and isert_do_control_comp().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114161931.941441-1-yhlee@isslab.korea.ac.kr?part=1