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

[sashiko-bot@kernel.org]

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