[PATCH] scsi: target: Verify the iSCSI ISID is NUL-terminated

[PATCH] scsi: target: Verify the iSCSI ISID is NUL-terminated

[Yuanhe Shu]

iscsi_parse_pr_out_transport_id() returns the ISID of a FORMAT CODE 01b
TransportID as a pointer into the PERSISTENT RESERVE OUT parameter list.
The lower-case conversion loop only scans the ISID and never terminates
it, and while spc4r37 Table 508 requires the field to be null-terminated,
a non-conforming initiator can omit the NUL.  When the ISID ends at the
end of the received buffer, the pointer is not a C string.

The registration code still consumes it as one: strcmp(isid,
pr_reg->pr_reg_isid) in __core_scsi3_locate_pr_reg(), and snprintf("%s")
and strcmp() in __core_scsi3_do_alloc_registration() and in the REGISTER
AND MOVE path.

When the parameter list spans more than one page, transport_kmap_data_sg()
maps it with vmap(), and the over-read walks off the end of the mapping
into the vmalloc guard page and oopses.  Reproduced with a PR OUT
REGISTER + SPEC_I_PT parameter list of exactly 8192 bytes whose final
TransportID carries a 12-byte ISID without a terminating NUL:

  BUG: unable to handle page fault for address: ffffc9000138c000
  RIP: 0010:strcmp+0xc/0x30
  Call Trace:
   __core_scsi3_locate_pr_reg+0xd3/0x120 [target_core_mod]
   core_scsi3_decode_spec_i_port+0x8fc/0x13a0 [target_core_mod]
   core_scsi3_emulate_pro_register+0x73c/0x900 [target_core_mod]
   ...
   iscsi_target_rx_thread+0xef/0x1d0 [iscsi_target_mod]

With a single-page parameter list the buffer is a kmap() direct mapping,
so the over-read continues into the physically adjacent page instead of
faulting.  snprintf("%s") then copies whatever follows the ISID in
memory into pr_reg_isid, and PR IN READ FULL STATUS reports the
registration back to any initiator, turning the over-read into an
info leak of kernel heap contents.  Reproduced with a 4096-byte
parameter list whose last TransportID ends at the end of the buffer
with a 4-byte ISID and no NUL, interleaved with I/O to populate
adjacent pages: the READ FULL STATUS response returned the ISID as
"0xleak" followed by raw bytes that were not part of the parameter
list.  The snprintf() into pr_reg_isid[PR_REG_ISID_LEN] caps the echoed
ISID at 15 characters, so at most 11 bytes past the buffer are leaked
per command, but the PR OUT command is not rate limited.

Any initiator that can send PR OUT to an iSCSI-exported LUN can trigger
either variant, including a guest via vhost-scsi.

Require the TransportID to contain the terminating NUL before the ISID
pointer is returned, so neither reproducer reaches the registration
consumers.  REGISTER AND MOVE fails with TCM_INVALID_PARAMETER_LIST;
the SPEC_I_PT scan skips the entry.

Fixes: d04a179085c2 ("scsi: target: Bound PR-OUT TransportID parsing to the received buffer")
Reported-by: kitta <kitta@linux.alibaba.com>
Cc: stable@vger.kernel.org
Cc: Bryam Vargas <hexlabsecurity@proton.me>
Cc: John Garry <john.garry@linux.dev>
Assisted-by: LLM
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---
 drivers/target/target_core_fabric_lib.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 2853b95b2c59..e15f5118b209 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -371,6 +371,17 @@ static bool iscsi_parse_pr_out_transport_id(
 			pr_err("Missing ISID in iSCSI Initiator port TransportID\n");
 			return false;
 		}
+		/*
+		 * The conversion loop below only scans the ISID without
+		 * ever terminating it, but the registration code reads it
+		 * as a C string.  With no NUL inside the TransportID,
+		 * the reads run past the descriptor, and past the
+		 * mapped buffer when the descriptor ends the buffer.
+		 */
+		if (!memchr(p, '\0', buf + tid_len - p)) {
+			pr_err("Missing NUL terminator for ISID in iSCSI Initiator port TransportID\n");
+			return false;
+		}
 		*port_nexus_ptr = p;
 		/*
 		 * Go ahead and do the lower case conversion of the received
-- 
2.43.7

Re: [PATCH] scsi: target: Verify the iSCSI ISID is NUL-terminated

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: target: 8-byte OOB read in __core_scsi3_do_alloc_registration()
--

commit 9e538bce0beb997676ced65cf68b1194c6fc594a
Author: Yuanhe Shu <xiangzao@linux.alibaba.com>

scsi: target: Verify the iSCSI ISID is NUL-terminated

This patch verifies that the iSCSI ISID in a FORMAT CODE 01b TransportID
contains a NUL terminator within the descriptor before returning a pointer
into the PERSISTENT RESERVE OUT parameter list buffer. This aims to prevent
string operations in the registration code from reading past the end of the
mapped buffer.

> diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
> index 2853b95b2c597..e15f5118b2099 100644
> --- a/drivers/target/target_core_fabric_lib.c
> +++ b/drivers/target/target_core_fabric_lib.c
> @@ -371,6 +371,17 @@ static bool iscsi_parse_pr_out_transport_id(
>  			pr_err("Missing ISID in iSCSI Initiator port TransportID\n");
>  			return false;
>  		}
> +		/*
> +		 * The conversion loop below only scans the ISID without
> +		 * ever terminating it, but the registration code reads it
> +		 * as a C string.  With no NUL inside the TransportID,
> +		 * the reads run past the descriptor, and past the
> +		 * mapped buffer when the descriptor ends the buffer.
> +		 */
> +		if (!memchr(p, '\0', buf + tid_len - p)) {
> +			pr_err("Missing NUL terminator for ISID in iSCSI Initiator port TransportID\n");
> +			return false;
> +		}
>  		*port_nexus_ptr = p;

[Severity: High]
Does checking only for a NUL byte within buf + tid_len - p still leave an
out-of-bounds read when the remaining length is between 1 and 7 bytes?

Both the SPEC_I_PT path and the REGISTER AND MOVE path pass *port_nexus_ptr
(as iport_ptr / isid) into __core_scsi3_do_alloc_registration():

  core_scsi3_decode_spec_i_port()
    -> __core_scsi3_alloc_registration(..., iport_ptr, ...)
      -> __core_scsi3_do_alloc_registration(..., isid, ...)

  core_scsi3_emulate_pro_register_and_move()
    -> core_scsi3_alloc_registration(..., iport_ptr, ...)
      -> __core_scsi3_alloc_registration(..., isid, ...)
        -> __core_scsi3_do_alloc_registration(..., isid, ...)

In drivers/target/target_core_pr.c:__core_scsi3_do_alloc_registration(),
before copying isid with snprintf(), the code unconditionally reads 8 bytes
from isid:

	if (isid != NULL) {
		pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
		snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);
		pr_reg->isid_present_at_reg = 1;
	}

If a FORMAT CODE 01b TransportID at the end of the parameter list buffer
has 1 <= buf + tid_len - p < 8 bytes after the ",i,0x" separator and
includes a '\0' byte, memchr(p, '\0', buf + tid_len - p) succeeds, but
get_unaligned_be64(isid) still reads up to 7 bytes past buf + tid_len (and
into the trailing vmalloc guard page on multi-page mappings).

Should get_unaligned_be64() read from the zero-initialized
pr_reg->pr_reg_isid buffer after snprintf(), or should
iscsi_parse_pr_out_transport_id() require the full ISID length?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026044632.1738901-1-xiangzao@linux.alibaba.com?part=1

[PATCH v2] scsi: target: Verify the iSCSI ISID is NUL-terminated

[Yuanhe Shu]

iscsi_parse_pr_out_transport_id() returns the ISID of a FORMAT CODE 01b
TransportID as a pointer into the PERSISTENT RESERVE OUT parameter list,
lowercased in place and never NUL-terminated.  spc4r37 Table 508
requires the field to be null-terminated, but a non-conforming
initiator can omit the NUL; when the ISID ends at the end of the
received buffer, the pointer is not a C string.

The registration code still consumes it as one: strcmp() in
__core_scsi3_locate_pr_reg() and in the REGISTER AND MOVE path, and
snprintf("%s") in __core_scsi3_do_alloc_registration(), which also
reads its first 8 bytes unconditionally with get_unaligned_be64() -
a NUL check does not bound that read.

When the parameter list spans more than one page, transport_kmap_data_sg()
maps it with vmap(), and the over-read walks off the end of the mapping
into the vmalloc guard page and oopses.  Reproduced with a PR OUT
REGISTER + SPEC_I_PT parameter list of exactly 8192 bytes whose final
TransportID carries a 12-byte ISID without a terminating NUL:

  BUG: unable to handle page fault for address: ffffc9000138c000
  RIP: 0010:strcmp+0xc/0x30
  Call Trace:
   __core_scsi3_locate_pr_reg+0xd3/0x120 [target_core_mod]
   core_scsi3_decode_spec_i_port+0x8fc/0x13a0 [target_core_mod]
   core_scsi3_emulate_pro_register+0x73c/0x900 [target_core_mod]
   ...
   iscsi_target_rx_thread+0xef/0x1d0 [iscsi_target_mod]

With a single-page parameter list the buffer is a kmap() direct mapping,
so the over-read continues into the physically adjacent page instead of
faulting.  snprintf("%s") then copies whatever follows the ISID in
memory into pr_reg_isid, and PR IN READ FULL STATUS reports the
registration back to any initiator, turning the over-read into an
info leak of kernel heap contents.  Reproduced with a 4096-byte
parameter list whose last TransportID ends at the end of the buffer
with a 4-byte ISID and no NUL, interleaved with I/O to populate
adjacent pages: the READ FULL STATUS response returned the ISID as
"0xleak" followed by raw bytes that were not part of the parameter
list.  The snprintf() into pr_reg_isid[PR_REG_ISID_LEN] caps the echoed
ISID at 15 characters, so at most 11 bytes past the buffer are leaked
per command, but the PR OUT command is not rate limited.

Any initiator that can send PR OUT to an iSCSI-exported LUN can trigger
either variant, including a guest via vhost-scsi.

Require 8 bytes to remain after the ",i,0x" separator, and a NUL
inside the rest of the descriptor, before the ISID pointer is
returned.  REGISTER AND MOVE fails with TCM_INVALID_PARAMETER_LIST;
on SPEC_I_PT no target port claims the malformed TransportID, and the
command fails the same way.

Fixes: d04a179085c2 ("scsi: target: Bound PR-OUT TransportID parsing to the received buffer")
Reported-by: kitta <kitta@linux.alibaba.com>
Reported-by: sashiko AI review <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/sashiko-outbox-184760@kernel.org/
Cc: stable@vger.kernel.org
Cc: Bryam Vargas <hexlabsecurity@proton.me>
Cc: John Garry <john.garry@linux.dev>
Assisted-by: LLM
Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
---

Changes since v1:

- Bound the 8-byte get_unaligned_be64() read in
  __core_scsi3_do_alloc_registration() as well (sashiko review).
- Corrected the SPEC_I_PT outcome: the command fails, the scan does
  not skip the entry.

 drivers/target/target_core_fabric_lib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 2853b95b2c59..ad024009df56 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -371,6 +371,16 @@ static bool iscsi_parse_pr_out_transport_id(
 			pr_err("Missing ISID in iSCSI Initiator port TransportID\n");
 			return false;
 		}
+		/*
+		 * The registration code reads the ISID as a C string,
+		 * and its first 8 bytes with get_unaligned_be64(), so
+		 * require 8 bytes after the separator and a NUL inside
+		 * the rest of the descriptor.
+		 */
+		if (buf + tid_len - p < 8 || !memchr(p, '\0', buf + tid_len - p)) {
+			pr_err("Incomplete or unterminated ISID in iSCSI Initiator port TransportID\n");
+			return false;
+		}
 		*port_nexus_ptr = p;
 		/*
 		 * Go ahead and do the lower case conversion of the received
-- 
2.43.7

Re: [PATCH] scsi: target: Verify the iSCSI ISID is NUL-terminated

[Yuanhe Shu]

Sashiko is right.  get_unaligned_be64() reads the first 8 bytes
unconditionally, and the NUL check alone does not bound that when
fewer than 8 bytes remain in the descriptor.

v2 requires 8 bytes to remain after the separator, in addition to
the NUL, so every consumer stays inside the descriptor.

v2: https://lore.kernel.org/r/20261026124138.2708365-1-xiangzao@linux.alibaba.com/

-- 
Yuanhe