Re: [PATCH v2] scsi: target: iscsi: Reject CDB size exceeding available buffer in iscsit_setup_scsi_cmd

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] FORTIFY_SOURCE kernel panic due to out-of-bounds memcpy from fixed-size AHS ecdb array.
--

commit c7194730eff1d4be537c227966c11dc80b91b92b
Author: Cao Guanghui <[email protected]>

scsi: target: iscsi: Reject CDB size exceeding available buffer in iscsit_setup_scsi_cmd

This commit fixes a heap out-of-bounds read in iscsit_setup_scsi_cmd()
caused by an initiator claiming a larger SCSI command size than the available
CDB space. It adds bounds checks to reject the command if the opcode-declared
CDB size exceeds either the basic header limit or the AHS-provided length.

Fixes: 8f1f7d297bce ("scsi: target: iscsi: Add support for extended CDB AHS")

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 62ada3a52210e..56f8fd461192d 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c

[ ... ]

> @@ -1124,6 +1139,20 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
>  
>  		cdb_length = ahslength - 1 + ISCSI_CDB_SIZE;
>  
> +		/*
> +		 * The CDB buffer is later re-parsed by scsi_command_size()
> +		 * based on its opcode, which may claim a length larger than
> +		 * the AHS provided. Reject such a mismatch before allocating
> +		 * to avoid an out-of-bounds read of the CDB buffer in
> +		 * target_cmd_init_cdb().
> +		 */
> +		if (scsi_command_size(hdr->cdb) > cdb_length) {
> +			pr_err("Extended CDB AHS: SCSI command size %u exceeds AHS-provided CDB length %u, protocol error.\n",
> +			       scsi_command_size(hdr->cdb), cdb_length);
> +			return iscsit_add_reject_cmd(cmd,
> +				ISCSI_REASON_PROTOCOL_ERROR, buf);
> +		}
> +
>  		cdb = kmalloc(cdb_length, GFP_KERNEL);
>  		if (cdb == NULL)
>  			return iscsit_add_reject_cmd(cmd,
>  				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
>  		memcpy(cdb, hdr->cdb, ISCSI_CDB_SIZE);
>  		memcpy(cdb + ISCSI_CDB_SIZE, ecdb_ahdr->ecdb, cdb_length - ISCSI_CDB_SIZE);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does this memcpy() risk triggering a
FORTIFY_SOURCE kernel panic?

If a remote attacker sends an iSCSI SCSI CMD PDU with an Extended CDB AHS
where ahslength is large (e.g., 1017), the target accepts it because it is
bounded by the total AHS buffer length.

However, when copying the AHS payload to the newly allocated CDB buffer,
memcpy() reads cdb_length - ISCSI_CDB_SIZE (up to 1016) bytes from
ecdb_ahdr->ecdb.

Since ecdb_ahdr->ecdb is declared as a fixed-size array of 244 bytes
(SCSI_MAX_VARLEN_CDB_SIZE - ISCSI_CDB_SIZE) in include/scsi/iscsi_proto.h:

struct iscsi_ecdb_ahdr {
	...
	/* 4-byte aligned extended CDB spillover */
	uint8_t ecdb[SCSI_MAX_VARLEN_CDB_SIZE - ISCSI_CDB_SIZE];
};

reading more than 244 bytes with CONFIG_FORTIFY_SOURCE=y will detect an
out-of-bounds read and panic the kernel, causing a system-wide Denial of
Service.

Should we restrict ahslength (or cdb_length) to not exceed the maximum allowed
Extended CDB length of 244 bytes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1