Re: [PATCH] scsi: target: iscsi: Reject Extended CDB AHS when CDB size exceeds AHS length

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

Pre-existing issues:
- [High] The patch addresses the heap out-of-bounds read for Extended CDB AHS commands (`hdr->hlength > 0`), but fails to implement a similar bounds check when the command has no AHS (`hdr->hlength == 0`), leaving the exact same vulnerability open for standard iSCSI commands.
--

--- Patch [1]: [PATCH] scsi: target: iscsi: Reject Extended CDB AHS when CDB size exceeds AHS length ---
commit 5a3d59b1c504777c7edf6a6c77fbad41b0da5d11
Author: Cao Guanghui <[email protected]>

scsi: target: iscsi: Reject Extended CDB AHS when CDB size exceeds AHS length

This commit adds a check to reject iSCSI commands when the SCSI opcode-declared
size exceeds the AHS-provided length. This prevents an out-of-bounds read
during Extended CDB AHS parsing in target_cmd_init_cdb().

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 62ada3a52210e..e3e9254df9602 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1124,6 +1124,19 @@ 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);
> +		}
> +

[Severity: High]
This is a pre-existing issue, but does the same out-of-bounds read occur when
an initiator sends a command without an AHS (hdr->hlength == 0)?

If an initiator sends an iSCSI command PDU without an AHS and sets the CDB to a
VARIABLE_LENGTH_CMD (cdb[0] = 0x7F) with an additional length of 252
(cdb[7] = 252), the newly added bounds check is bypassed because it is inside
the if (hdr->hlength) block.

In this case, iscsit_setup_scsi_cmd() will pass the raw 16-byte hdr->cdb
pointer directly to target_cmd_init_cdb(). The target_cmd_init_cdb() function
will then call scsi_command_size(cdb) (which evaluates to 260) and read 244
bytes past the end of the cdb array (and 228 bytes past the end of the buf
allocation).

Could a similar bounds check be added for the standard path without AHS,
verifying that scsi_command_size(hdr->cdb) <= ISCSI_CDB_SIZE?

>  		cdb = kmalloc(cdb_length, GFP_KERNEL);
>  		if (cdb == NULL)
>  			return iscsit_add_reject_cmd(cmd,

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