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

[PATCH v2] 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>
---
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 | 69 ++++++++++++++++++++++++++++++++++++-
 include/scsi/libiscsi_tcp.h |  1 +
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index d35f93451ee9b..6d8ace85c51ca 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -467,6 +467,37 @@ void iscsi_tcp_cleanup_task(struct iscsi_task *task)
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_cleanup_task);
 
+/**
+ * iscsi_tcp_check_data_in - verify a read received the data the target claimed
+ * @task: scsi command task
+ * @flags: flags of the PDU carrying the status
+ * @residual: residual count of that PDU
+ *
+ * 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, u32 flags,
+				   u32 residual)
+{
+	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;
+
+	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;
+}
+
 /**
  * iscsi_tcp_data_in - SCSI Data-In Response processing
  * @conn: iscsi connection
@@ -491,7 +522,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 +540,17 @@ 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->flags,
+					       be32_to_cpu(rhdr->residual_count));
 	return 0;
 }
 
@@ -783,6 +824,31 @@ 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;
+			}
+			rc = iscsi_tcp_check_data_in(task, rsp->flags,
+						     be32_to_cpu(rsp->residual_count));
+			if (rc) {
+				spin_unlock(&conn->session->back_lock);
+				return rc;
+			}
+			if (tcp_conn->in.datalen) {
+				spin_unlock(&conn->session->back_lock);
+				iscsi_tcp_data_recv_prep(tcp_conn);
+				return 0;
+			}
+			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 +1061,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 v2] scsi: libiscsi_tcp: check that a read got the data the target claimed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iscsi: iscsi_tcp_check_data_in drops TCP connection on valid SCSI error responses
--

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

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

This commit adds per-task payload counting to verify that a read receives
the exact amount of data the target claimed. It compares the total received
bytes against the expected buffer length, returning an error if they do not
match.

> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index d35f93451ee9b..6d8ace85c51ca 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -467,6 +467,37 @@ void iscsi_tcp_cleanup_task(struct iscsi_task *task)

[ ... ]

> +static int iscsi_tcp_check_data_in(struct iscsi_task *task, u32 flags,
> +				   u32 residual)
> +{
> +	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;
> +
> +	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;
> +}

[Severity: High]
Will returning ISCSI_ERR_DATALEN here cause the connection to be dropped on
valid SCSI error responses?

For valid SCSI failures, such as SAM_STAT_CHECK_CONDITION, the target might
not start transferring Data-In PDUs and could optionally omit setting the
ISCSI_FLAG_CMD_UNDERFLOW flag. Under these circumstances, tcp_task->data_in_bytes
will be 0 while the expected length remains sc->sdb.length, resulting in an
immediate ISCSI_ERR_DATALEN error rather than allowing upper layers to
process the sense data.

[ ... ]

> @@ -783,6 +824,31 @@ 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;
> +			}
> +			rc = iscsi_tcp_check_data_in(task, rsp->flags,
> +						     be32_to_cpu(rsp->residual_count));
> +			if (rc) {
> +				spin_unlock(&conn->session->back_lock);
> +				return rc;
> +			}

[Severity: High]
Is it safe to perform this validation in iscsi_tcp_hdr_dissect() for every
ISCSI_OP_SCSI_CMD_RSP without first checking the command status?

Because cmd_status (e.g., SAM_STAT_GOOD versus SAM_STAT_CHECK_CONDITION) is
not verified before checking the data length, standard SCSI errors that
safely carry sense data but omit the underflow flag trigger an immediate TCP
connection teardown. Could this lead to endless session recovery loops when
processing normal error conditions?

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