Re: [PATCH v3] 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 2 potential issue(s) to consider:
- [High] iscsi: libiscsi_tcp: bypass of short-read validation via CHECK_CONDITION
- [Medium] iscsi: libiscsi_tcp: strict offset enforcement breaks out-of-order Data-In sequences
--

commit c98a9b76c4a9e3b246d783ea606f33c9c6d70d07
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.

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

[ ... ]

> +static int iscsi_tcp_check_data_in(struct iscsi_task *task, u8 status,
> +				   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;
> +
> +	/*
> +	 * Only a successful completion promises the whole buffer.  Any other
> +	 * status is handled by the normal status and sense path.
> +	 */
> +	if (status != SAM_STAT_GOOD)
> +		return 0;

[Severity: High]
Does this early return allow a malicious target to bypass the new validation?

If a target returns a short payload followed by a SCSI Command Response
containing SAM_STAT_CHECK_CONDITION and a RECOVERED_ERROR sense key, the length
validation is skipped here.

The SCSI midlayer (such as sd_done() in drivers/scsi/sd.c) explicitly handles
RECOVERED_ERROR by setting:

	good_bytes = scsi_bufflen(SCpnt);

treating the entire buffer as successfully read despite the short payload.

Could this leave the short-read vulnerability fully exploitable through this
error path?

> +
> +	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;
> +}

[ ... ]

> @@ -509,7 +548,18 @@ 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;

[Severity: Medium]
Does this strict monotonic offset check break iSCSI protocol compliance?

The iSCSI protocol (RFC 7143) explicitly permits targets to send Data-In
sequences out of order if negotiated (DataSequenceInOrder=No). Within such
sequences, DataSN strictly increments, but the byte offsets can jump
arbitrarily.

Prior to this patch, out-of-order sequences were correctly handled by relying
on iscsi_segment_seek_sg() to seek the scatterlist and write data at the exact
tcp_task->data_offset specified by the PDU header.

Could mandating data_offset == data_in_bytes trigger a fatal protocol error
on perfectly valid out-of-order sequences, terminating the connection?

> +
> +	tcp_task->data_in_bytes += tcp_conn->in.datalen;
> +
>  	conn->datain_pdus_cnt++;

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