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

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

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In iscsit_setup_scsi_cmd(), the Extended CDB AHS path allocates the CDB
buffer using only the iSCSI AHS-declared length. The buffer is then
handed to target_cmd_init_cdb(), which re-derives the CDB length from
the SCSI opcode via scsi_command_size(). For a VARIABLE_LENGTH_CMD
(0x7f) the size is cdb[7] + 8, where both cdb[0] and cdb[7] come
verbatim from the initiator-controlled hdr->cdb. The AHS-declared length
and the opcode-declared length are never cross-checked.

An initiator can thus advertise a minimal AHS (ahslength = 1, which
passes the !ahslength and ahslength > hlength*4 - 3 checks added by
commit 2f3835771dff ("scsi: target: iscsi: reject invalid size Extended
CDB AHS") while setting cdb[0]=0x7f and cdb[7]=252. This yields a
16-byte kmalloc but scsi_command_size() returns 260, so
target_cmd_init_cdb() reads 260 bytes from a 16-byte heap object, a heap
out-of-bounds read. The leaked bytes are later parsed as the CDB and can
be indirectly observed by the initiator through sense data and
responses.

commit 2f3835771dff ("scsi: target: iscsi: reject invalid size Extended
CDB AHS") fixed the zero-length ahslength overflows and the AHS-buffer
overread, but did not cover the orthogonal "allocation size vs.
opcode-declared size" path. Reject the command when the opcode-declared
CDB size exceeds the AHS-provided length, before the kmalloc.

Fixes: 8f1f7d297bce ("scsi: target: iscsi: Add support for extended CDB AHS")
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
 drivers/target/iscsi/iscsi_target.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 62ada3a52210..e3e9254df960 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);
+		}
+
 		cdb = kmalloc(cdb_length, GFP_KERNEL);
 		if (cdb == NULL)
 			return iscsit_add_reject_cmd(cmd,
-- 
2.34.1

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

[sashiko-bot]

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 <caoguanghui@kylinos.cn>

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/20260806054820.75950-1-ghuicao@163.com?part=1

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

[ghuicao]

From: Cao Guanghui <caoguanghui@kylinos.cn>

In iscsit_setup_scsi_cmd(), the CDB is later re-parsed by
scsi_command_size() based on its SCSI opcode. For a VARIABLE_LENGTH_CMD
(0x7f) the returned size is cdb[7] + 8, where both cdb[0] and cdb[7]
come verbatim from the initiator-controlled PDU. The amount of CDB data
actually available is never cross-checked against this opcode-declared
length before the CDB is handed to target_cmd_init_cdb(), which does:

  memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb));

An initiator can set cdb[0]=0x7f and cdb[7]=252 so that
scsi_command_size() returns 260, while the available CDB space is only
16 bytes (the basic header, when no Extended CDB AHS is present) or the
AHS-provided length. target_cmd_init_cdb() then reads up to 244 bytes
past the end of the CDB buffer, a heap out-of-bounds read. The leaked
bytes are later parsed as the CDB and can be indirectly observed by the
initiator through sense data and responses.

Reject the command when the opcode-declared CDB size exceeds the
available CDB space, before the CDB is passed on:

- Without an Extended CDB AHS the CDB is limited to ISCSI_CDB_SIZE (16)
  bytes in the basic header, so reject when scsi_command_size() >
  ISCSI_CDB_SIZE.
- With an Extended CDB AHS the buffer is allocated from the AHS-declared
  length, so reject when scsi_command_size() > cdb_length.

commit 2f3835771dff ("scsi: target: iscsi: reject invalid size Extended
CDB AHS") fixed the zero-length ahslength overflows and the AHS-buffer
overread, but did not cover the orthogonal "available CDB space vs.
opcode-declared size" path.

Fixes: 8f1f7d297bce ("scsi: target: iscsi: Add support for extended CDB AHS")
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
Hi Martin,

Thanks for the review (via Sashiko). You are right that the original patch
only covered the Extended CDB AHS path and missed the standard command
path without an AHS, where the same out-of-bounds read is reachable. v2
adds the same bounds check for hdr->hlength == 0 and switches the two
paths to if/else for clarity.

v2:
  - Add the bounds check for the standard path without an AHS
    (hdr->hlength == 0) as well, fixing the same out-of-bounds read
    that was previously only covered for the Extended CDB AHS path.
  - Use if/else to make the two (mutually exclusive) paths explicit.

 drivers/target/iscsi/iscsi_target.c | 31 +++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 62ada3a52210..56f8fd461192 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1100,7 +1100,22 @@ int iscsit_setup_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
 
 	cdb = hdr->cdb;
 
-	if (hdr->hlength) {
+	if (!hdr->hlength) {
+		/*
+		 * Without an Extended CDB AHS the CDB is limited to the 16
+		 * bytes in the basic header. The CDB is later re-parsed by
+		 * scsi_command_size() based on its opcode, which may claim a
+		 * larger length (e.g. VARIABLE_LENGTH_CMD with cdb[7]=252).
+		 * Reject such a mismatch before handing the CDB to
+		 * target_cmd_init_cdb() to avoid an out-of-bounds read.
+		 */
+		if (scsi_command_size(hdr->cdb) > ISCSI_CDB_SIZE) {
+			pr_err("SCSI command size %u exceeds CDB size %u, protocol error.\n",
+			       scsi_command_size(hdr->cdb), ISCSI_CDB_SIZE);
+			return iscsit_add_reject_cmd(cmd,
+				ISCSI_REASON_PROTOCOL_ERROR, buf);
+		}
+	} else {
 		ecdb_ahdr = (struct iscsi_ecdb_ahdr *) (hdr + 1);
 		if (ecdb_ahdr->ahstype != ISCSI_AHSTYPE_CDB) {
 			pr_err("Additional Header Segment type %d not supported!\n",
@@ -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,
-- 
2.34.1