Re: [PATCH v4] 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 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