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

[sashiko-bot@kernel.org]

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