[PATCH v4] scsi: libiscsi_tcp: check that a read got the data the target claimed

[PATCH v4] scsi: libiscsi_tcp: check that a read got the data the target claimed

[Yehyeong Lee]

A target can finish a read without sending the data.  iscsi_tcp_data_in()
bounds each Data-In PDU against the command buffer but never adds them up,
so a completion that declares no underflow is believed and the command
ends with DID_OK.  The pages keep whatever they held.

Over a 1 MiB pread of a file that had never been read, 999424 bytes came
back as the contents of an unrelated file the same process had written
earlier.  pread() returned 1048576 and errno was 0.

Count the payload per task.  Require each Data-In to continue where the
last one ended, and at completion compare the total with the buffer
length, less the residual when an underflow is declared.  A read can end
on a Data-In or on a SCSI Response, so check both.  Writes are untouched.

Fixes: a081c13e39b5 ("[SCSI] iscsi_tcp: split module into lib and lld")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v4: also validate coverage when the status is CHECK_CONDITION with a sense
    key of RECOVERED_ERROR.  sd_done() counts RECOVERED_ERROR as the whole
    buffer transferred, so v3's "skip every non-GOOD status" left a target
    able to short a read and still have it reported as a full success
    (noted by Sashiko AI review).  Because the sense arrives in the data
    segment, the SCSI Response check moved to the point where that segment
    completes.
    The Data-In continuity check requires in-order Data-In and so does not
    interoperate with a target that negotiates DataPDUInOrder=No; that is
    deliberate, as making a safety check conditional on a peer-negotiated
    value would let the peer turn it off.
v3: only validate coverage for SAM_STAT_GOOD completions; a non-GOOD reply
    such as CHECK_CONDITION that omits the underflow flag must not be
    treated as a short read (noted by Sashiko AI review).
v2: take back_lock once on the SCSI Response path and make the counter
    uint32_t, both per Mike Christie's review.

On "was it built over another patch?" - no, v1 was generated against
v7.2-rc5 directly.  It stopped applying because c1dea15f819cd ("scsi:
libiscsi_tcp: Bound SCSI Response data segment to the connection
buffer") folded ISCSI_OP_SCSI_CMD_RSP into the shared response case,
which is the block v1's last hunk patched.  This version leaves that
fold alone and applies the check inside the shared case, so the data
segment bound stays in one place.

v1: https://lore.kernel.org/all/20260814121428.359541-1-yhlee@isslab.korea.ac.kr/ drivers/scsi/libiscsi_tcp.c | 128 +++++++++++++++++++++++++++++++++++-
 include/scsi/libiscsi_tcp.h |   1 +
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index d35f93451ee9b..599bfed79847b 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -397,6 +397,62 @@ void iscsi_tcp_hdr_recv_prep(struct iscsi_tcp_conn *tcp_conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_hdr_recv_prep);
 
+/**
+ * iscsi_tcp_check_data_in - verify a read received the data the target claimed
+ * @task: scsi command task
+ * @status: SCSI status of the PDU carrying the status
+ * @flags: flags of the PDU carrying the status
+ * @residual: residual count of that PDU
+ * @sense: sense data segment of that PDU, if it has one
+ * @senselen: length of @sense
+ *
+ * Check the payload only for a completion that reports the whole buffer as
+ * transferred.  That is SAM_STAT_GOOD, and also CHECK_CONDITION with a sense
+ * key of RECOVERED_ERROR, which sd_done() counts as fully transferred.  Any
+ * other status means the command failed and the data may legitimately be
+ * absent.
+ *
+ * ISCSI_FLAG_CMD_UNDERFLOW and ISCSI_FLAG_DATA_UNDERFLOW share a value, so the
+ * Data-In and SCSI Response paths both use this.
+ */
+static int iscsi_tcp_check_data_in(struct iscsi_task *task, u8 status,
+				   u32 flags, u32 residual,
+				   const void *sense, u32 senselen)
+{
+	struct iscsi_tcp_task *tcp_task = task->dd_data;
+	struct scsi_cmnd *sc = task->sc;
+	unsigned int expected;
+
+	if (!sc || sc->sc_data_direction == DMA_TO_DEVICE)
+		return 0;
+
+	if (status != SAM_STAT_GOOD) {
+		struct scsi_sense_hdr sshdr;
+		const u8 *s = sense;
+		u16 len;
+
+		if (status != SAM_STAT_CHECK_CONDITION || !s || senselen < 2)
+			return 0;
+		len = get_unaligned_be16(s);
+		if (len + 2U > senselen)
+			return 0;
+		if (!scsi_normalize_sense(s + 2, len, &sshdr) ||
+		    sshdr.sense_key != RECOVERED_ERROR)
+			return 0;
+	}
+
+	expected = sc->sdb.length;
+	if (flags & ISCSI_FLAG_DATA_UNDERFLOW) {
+		if (residual > expected)
+			return 0;
+		expected -= residual;
+	}
+	if (tcp_task->data_in_bytes == expected)
+		return 0;
+
+	return ISCSI_ERR_DATALEN;
+}
+
 /*
  * Handle incoming reply to any other type of command
  */
@@ -410,8 +466,30 @@ iscsi_tcp_data_recv_done(struct iscsi_tcp_conn *tcp_conn,
 	if (!iscsi_tcp_dgst_verify(tcp_conn, segment))
 		return ISCSI_ERR_DATA_DGST;
 
-	rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr,
-			conn->data, tcp_conn->in.datalen);
+	if ((tcp_conn->in.hdr->opcode & ISCSI_OPCODE_MASK) ==
+	    ISCSI_OP_SCSI_CMD_RSP) {
+		struct iscsi_scsi_rsp *rsp =
+			(struct iscsi_scsi_rsp *)tcp_conn->in.hdr;
+		struct iscsi_task *task;
+
+		spin_lock(&conn->session->back_lock);
+		task = iscsi_itt_to_ctask(conn, rsp->itt);
+		if (!task) {
+			spin_unlock(&conn->session->back_lock);
+			return ISCSI_ERR_BAD_ITT;
+		}
+		rc = iscsi_tcp_check_data_in(task, rsp->cmd_status, rsp->flags,
+					     be32_to_cpu(rsp->residual_count),
+					     conn->data, tcp_conn->in.datalen);
+		if (!rc)
+			rc = __iscsi_complete_pdu(conn, tcp_conn->in.hdr,
+						  conn->data,
+						  tcp_conn->in.datalen);
+		spin_unlock(&conn->session->back_lock);
+	} else {
+		rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr, conn->data,
+					tcp_conn->in.datalen);
+	}
 	if (rc)
 		return rc;
 
@@ -491,7 +569,7 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task)
 		iscsi_update_cmdsn(conn->session, (struct iscsi_nopin*)rhdr);
 
 	if (tcp_conn->in.datalen == 0)
-		return 0;
+		goto status;
 
 	if (tcp_task->exp_datasn != datasn) {
 		ISCSI_DBG_TCP(conn, "task->exp_datasn(%d) != rhdr->datasn(%d)"
@@ -509,7 +587,19 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task)
 		return ISCSI_ERR_DATA_OFFSET;
 	}
 
+	if (tcp_task->data_offset != tcp_task->data_in_bytes)
+		return ISCSI_ERR_DATA_OFFSET;
+
+	tcp_task->data_in_bytes += tcp_conn->in.datalen;
+
 	conn->datain_pdus_cnt++;
+
+status:
+	if (rhdr->flags & ISCSI_FLAG_DATA_STATUS)
+		return iscsi_tcp_check_data_in(task, rhdr->cmd_status,
+					       rhdr->flags,
+					       be32_to_cpu(rhdr->residual_count),
+					       NULL, 0);
 	return 0;
 }
 
@@ -783,6 +873,37 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 			break;
 		}
 
+		if (opcode == ISCSI_OP_SCSI_CMD_RSP) {
+			struct iscsi_scsi_rsp *rsp = (struct iscsi_scsi_rsp *)hdr;
+
+			spin_lock(&conn->session->back_lock);
+			task = iscsi_itt_to_ctask(conn, hdr->itt);
+			if (!task) {
+				spin_unlock(&conn->session->back_lock);
+				return ISCSI_ERR_BAD_ITT;
+			}
+			if (tcp_conn->in.datalen) {
+				/*
+				 * The sense is in the data segment, so the
+				 * check has to wait for it.
+				 */
+				spin_unlock(&conn->session->back_lock);
+				iscsi_tcp_data_recv_prep(tcp_conn);
+				return 0;
+			}
+			rc = iscsi_tcp_check_data_in(task, rsp->cmd_status,
+						     rsp->flags,
+						     be32_to_cpu(rsp->residual_count),
+						     NULL, 0);
+			if (rc) {
+				spin_unlock(&conn->session->back_lock);
+				return rc;
+			}
+			rc = __iscsi_complete_pdu(conn, hdr, NULL, 0);
+			spin_unlock(&conn->session->back_lock);
+			break;
+		}
+
 		/* If there's data coming in with the response,
 		 * receive it to the connection's buffer.
 		 */
@@ -995,6 +1116,7 @@ int iscsi_tcp_task_init(struct iscsi_task *task)
 
 	BUG_ON(kfifo_len(&tcp_task->r2tqueue));
 	tcp_task->exp_datasn = 0;
+	tcp_task->data_in_bytes = 0;
 
 	/* Prepare PDU, optionally w/ immediate data */
 	ISCSI_DBG_TCP(conn, "task deq [itt 0x%x imm %d unsol %d]\n",
diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
index ef53d4bea28a0..a6e63db75d674 100644
--- a/include/scsi/libiscsi_tcp.h
+++ b/include/scsi/libiscsi_tcp.h
@@ -66,6 +66,7 @@ struct iscsi_tcp_conn {
 
 struct iscsi_tcp_task {
 	uint32_t		exp_datasn;	/* expected target's R2TSN/DataSN */
+	uint32_t		data_in_bytes;	/* Data-In payload received */
 	int			data_offset;
 	struct iscsi_r2t_info	*r2t;		/* in progress solict R2T */
 	struct iscsi_pool	r2tpool;

base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

Re: [PATCH v4] scsi: libiscsi_tcp: check that a read got the data the target claimed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] iscsi: integer underflow bypasses payload validation via OVERFLOW flag
- [Critical] scsi: iscsi_tcp_check_data_in bypasses length validation on out-of-band RECOVERED_ERROR
- [High] iscsi: connection drop on target failure due to missing response field check
--

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

scsi: libiscsi_tcp: check that a read got the data the target claimed

A target can finish a read without sending the data.  iscsi_tcp_data_in()
bounds each Data-In PDU against the command buffer but never adds them up,
so a completion that declares no underflow is believed and the command
ends with DID_OK.  The pages keep whatever they held.
    
Over a 1 MiB pread of a file that had never been read, 999424 bytes came
back as the contents of an unrelated file the same process had written
earlier.  pread() returned 1048576 and errno was 0.

> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index d35f93451ee9b..599bfed79847b 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -397,6 +397,62 @@ void iscsi_tcp_hdr_recv_prep(struct iscsi_tcp_conn *tcp_conn)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_tcp_hdr_recv_prep);
>  
> +/**
> + * iscsi_tcp_check_data_in - verify a read received the data the target claimed
> + * @task: scsi command task
> + * @status: SCSI status of the PDU carrying the status
> + * @flags: flags of the PDU carrying the status
> + * @residual: residual count of that PDU
> + * @sense: sense data segment of that PDU, if it has one
> + * @senselen: length of @sense
> + *
> + * Check the payload only for a completion that reports the whole buffer as
> + * transferred.  That is SAM_STAT_GOOD, and also CHECK_CONDITION with a sense
> + * key of RECOVERED_ERROR, which sd_done() counts as fully transferred.  Any
> + * other status means the command failed and the data may legitimately be
> + * absent.
> + *
> + * ISCSI_FLAG_CMD_UNDERFLOW and ISCSI_FLAG_DATA_UNDERFLOW share a value, so the
> + * Data-In and SCSI Response paths both use this.
> + */
> +static int iscsi_tcp_check_data_in(struct iscsi_task *task, u8 status,
> +				   u32 flags, u32 residual,
> +				   const void *sense, u32 senselen)
> +{
> +	struct iscsi_tcp_task *tcp_task = task->dd_data;
> +	struct scsi_cmnd *sc = task->sc;
> +	unsigned int expected;
> +
> +	if (!sc || sc->sc_data_direction == DMA_TO_DEVICE)
> +		return 0;
> +
> +	if (status != SAM_STAT_GOOD) {
[Severity: High]
Does this code need to verify the iSCSI response field before checking status?

According to RFC 3720, if an iSCSI SCSI Response PDU has a non-zero response
field (indicating an iSCSI-level failure like Target Failure), the cmd_status
field is undefined and typically set to 0.

If response is non-zero and cmd_status is 0 (SAM_STAT_GOOD), this function
will enforce data_in_bytes == expected.

Because the command failed at the transport layer, no data was transferred
(data_in_bytes = 0). iscsi_tcp_check_data_in() will incorrectly return
ISCSI_ERR_DATALEN.

This error propagates up and causes the TCP connection to be entirely torn
down, instead of simply failing the individual SCSI command gracefully as the
pre-patch code did.

> +		struct scsi_sense_hdr sshdr;
> +		const u8 *s = sense;
> +		u16 len;
> +
> +		if (status != SAM_STAT_CHECK_CONDITION || !s || senselen < 2)
> +			return 0;
[Severity: Critical]
Does this bypass the length validation for out-of-band RECOVERED_ERROR sense
data?

If a target responds to a read command with a short transfer and
SAM_STAT_CHECK_CONDITION, but omits inline sense data (datalen = 0), this
code sees !s and returns 0, bypassing the new tcp_task->data_in_bytes ==
expected payload validation.

The SCSI midlayer error handler will then issue REQUEST SENSE. If the target
replies with RECOVERED_ERROR, sd_done() will force good_bytes =
scsi_bufflen(SCpnt) and the midlayer will successfully complete the command.

This can lead to silent data corruption and kernel memory leaks, which matches
the exact issue this patch aimed to fix.

> +		len = get_unaligned_be16(s);
> +		if (len + 2U > senselen)
> +			return 0;
> +		if (!scsi_normalize_sense(s + 2, len, &sshdr) ||
> +		    sshdr.sense_key != RECOVERED_ERROR)
> +			return 0;
> +	}
> +
> +	expected = sc->sdb.length;
> +	if (flags & ISCSI_FLAG_DATA_UNDERFLOW) {
> +		if (residual > expected)
> +			return 0;
[Severity: Critical]
Can this early return allow an integer underflow that bypasses the payload
validation?

If the target sets ISCSI_FLAG_DATA_UNDERFLOW, this unconditionally returns 0
if residual > expected, bypassing the new payload length check.

The code assumes the SCSI midlayer will safely reject out-of-bounds residuals.
However, in the preexisting iscsi_scsi_cmd_rsp() and iscsi_data_in_rsp(), the
bounds check res_count <= scsi_bufflen(sc) is bypassed using a short-circuit
operator if ISCSI_FLAG_CMD_OVERFLOW is also set (which shares the exact same
bit value 0x04 as ISCSI_FLAG_DATA_OVERFLOW):

drivers/scsi/libiscsi.c:iscsi_scsi_cmd_rsp() {
    ...
    if (res_count > 0 &&
        (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
         res_count <= scsi_bufflen(sc)))
            /* write side for bidi or uni-io set_resid */
            scsi_set_resid(sc, res_count);
    ...
}

If a target sends a PDU with BOTH the Underflow and Overflow flags set, along
with a residual_count larger than the buffer, scsi_set_resid(sc, residual)
is executed with the malicious count.

In scsi_finish_command(), the calculation good_bytes -= scsi_get_resid(cmd)
suffers an integer underflow, wrapping good_bytes to an enormous positive
value.

scsi_io_completion() then passes this value to blk_update_request(), which
caps it to the request size and completes the entire buffer as a success.
This completely bypasses the patch's protection.

> +		expected -= residual;
> +	}
> +	if (tcp_task->data_in_bytes == expected)
> +		return 0;
> +
> +	return ISCSI_ERR_DATALEN;
> +}
> +
>  /*
>   * Handle incoming reply to any other type of command
>   */
> @@ -410,8 +466,30 @@ iscsi_tcp_data_recv_done(struct iscsi_tcp_conn *tcp_conn,
>  	if (!iscsi_tcp_dgst_verify(tcp_conn, segment))
>  		return ISCSI_ERR_DATA_DGST;
>  
> -	rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr,
> -			conn->data, tcp_conn->in.datalen);
> +	if ((tcp_conn->in.hdr->opcode & ISCSI_OPCODE_MASK) ==
> +	    ISCSI_OP_SCSI_CMD_RSP) {
> +		struct iscsi_scsi_rsp *rsp =
> +			(struct iscsi_scsi_rsp *)tcp_conn->in.hdr;
> +		struct iscsi_task *task;
> +
> +		spin_lock(&conn->session->back_lock);
> +		task = iscsi_itt_to_ctask(conn, rsp->itt);
> +		if (!task) {
> +			spin_unlock(&conn->session->back_lock);
> +			return ISCSI_ERR_BAD_ITT;
> +		}
> +		rc = iscsi_tcp_check_data_in(task, rsp->cmd_status, rsp->flags,
> +					     be32_to_cpu(rsp->residual_count),
> +					     conn->data, tcp_conn->in.datalen);
[Severity: High]
Does this code need to pass or verify rsp->response before invoking the
payload length checks?

As mentioned above, if the iSCSI response field indicates a transport failure,
cmd_status is undefined. This caller passes rsp->cmd_status without verifying
the response field first.

[ ... ]
> @@ -783,6 +873,37 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
>  			break;
>  		}
>  
> +		if (opcode == ISCSI_OP_SCSI_CMD_RSP) {
> +			struct iscsi_scsi_rsp *rsp = (struct iscsi_scsi_rsp *)hdr;
> +
> +			spin_lock(&conn->session->back_lock);
> +			task = iscsi_itt_to_ctask(conn, hdr->itt);
> +			if (!task) {
> +				spin_unlock(&conn->session->back_lock);
> +				return ISCSI_ERR_BAD_ITT;
> +			}
> +			if (tcp_conn->in.datalen) {
> +				/*
> +				 * The sense is in the data segment, so the
> +				 * check has to wait for it.
> +				 */
> +				spin_unlock(&conn->session->back_lock);
> +				iscsi_tcp_data_recv_prep(tcp_conn);
> +				return 0;
> +			}
> +			rc = iscsi_tcp_check_data_in(task, rsp->cmd_status,
> +						     rsp->flags,
> +						     be32_to_cpu(rsp->residual_count),
> +						     NULL, 0);
[Severity: High]
Is it safe to invoke this validation using the potentially undefined
cmd_status field without verifying if rsp->response is
ISCSI_STATUS_CMD_COMPLETED?

Like the previous caller, this omits checking the response field, leading to
the same connection teardown issue on target failure.

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