Re: [PATCH v6] scsi: target: bound PR-OUT TransportID parsing to the received buffer

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch incorrectly rejects valid, maximum-length (224-byte) FORMAT CODE 00b iSCSI TransportIDs due to an off-by-one error.

Pre-existing issues:
- [High] The parser does not enforce that the ISID string in a FORMAT CODE 01b TransportID is NUL-terminated or bounded to 8 bytes, leading to out-of-bounds reads and potential Information Leaks.
- [High] In-place modification of the TransportID buffer breaks `SPEC_I_PT` PR OUT registrations if the matching iSCSI Target Portal Group (TPG) is not the first one evaluated.
- [High] Leak of configfs dependency and `pr_kref` for `local_pr_reg->pr_reg_deve` in `core_scsi3_decode_spec_i_port`.
--

commit 00c1537650e798bd12f371445b4780a799d66bab
Author: Bryam Vargas <hexlabsecurity@proton.me>

scsi: target: bound PR-OUT TransportID parsing to the received buffer

This commit fixes out-of-bounds reads when parsing an iSCSI TransportID
without a separator or NUL terminator by adding length validation. It checks
the lengths of the TransportID elements before scanning or copying data.

> diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
> index 87c5d26a50895..2853b95b2c597 100644
> --- a/drivers/target/target_core_fabric_lib.c
> +++ b/drivers/target/target_core_fabric_lib.c
[ ... ]
> @@ -332,16 +345,32 @@ static bool iscsi_parse_pr_out_transport_id(
>  	 * format.
>  	 */
>  	if (format_code == 0x40) {
> -		p = strstr(&buf[4], ",i,0x");
> +		p = strnstr(&buf[4], ",i,0x", tid_len - 4);
>  		if (!p) {
> -			pr_err("Unable to locate \",i,0x\" separator"
> -				" for Initiator port identifier: %s\n",
> -				&buf[4]);
> +			pr_err("Unable to locate \",i,0x\" separator in iSCSI TransportID\n");
> +			return false;
> +		}
> +		/*
> +		 * The iSCSI name runs from &buf[4] up to the separator; reject it
> +		 * if it cannot fit in i_str[TRANSPORT_IQN_LEN].
> +		 */
> +		if (p - &buf[4] >= TRANSPORT_IQN_LEN) {
> +			pr_err("iSCSI Initiator port name too long in TransportID\n");
>  			return false;
>  		}
>  		*p = '\0'; /* Terminate iSCSI Name */

[Severity: High]
This is a pre-existing issue, but does mutating the shared request buffer
in-place break SPEC_I_PT PR OUT registrations?

In core_scsi3_decode_spec_i_port(), the code loops over multiple exported
TPGs and calls target_parse_pr_out_transport_id() on the same payload
buffer.

If the node ACL is not found on the first matching TPG, the loop continues
and attempts to parse the exact same buffer again for the next TPG.

Because the separator is overwritten with a '\0' here during the first
pass, strnstr() fails to find ",i,0x" on the second pass, returning false
and causing the valid registration to fail.

[ ... ]
> @@ -349,7 +378,7 @@ static bool iscsi_parse_pr_out_transport_id(
>  		 * for comparison against the running iSCSI session's ISID from
>  		 * iscsi_target.c:lio_sess_get_initiator_sid()
>  		 */
> -		for (i = 0; i < 12; i++) {
> +		for (i = 0; i < 12 && p < buf + tid_len; i++) {

[Severity: High]
This isn't a bug introduced by this patch, but does the parser enforce that
the ISID string is NUL-terminated or bounded to 8 bytes?

If an attacker provides an unterminated ISID string that extends exactly
to the end of the mapped command buffer page,
iscsi_parse_pr_out_transport_id() correctly bounds the lowercasing loop but
returns the port_nexus_ptr pointing to the unterminated string.

Callers then read past the buffer boundary. For example,
__core_scsi3_do_alloc_registration() calls get_unaligned_be64(isid) and
snprintf("%s", isid) reading past the buffer.

Could this read into the adjacent vmalloc guard page causing a KASAN crash?

[ ... ]
> -	} else
> +		strscpy(i_str, &buf[4], TRANSPORT_IQN_LEN);
> +	} else {
>  		*port_nexus_ptr = NULL;
> -
> -	strscpy(i_str, &buf[4], TRANSPORT_IQN_LEN);
> +		/*
> +		 * FORMAT CODE 00b: the name occupies buf[4..tid_len-1].  The
> +		 * declared length tid_len - 4 must fit in i_str[TRANSPORT_IQN_LEN].
> +		 * (For 01b the same tid_len bound would be over-restrictive: the
> +		 * descriptor also carries the separator and ISID, so a legal
> +		 * <=223-byte name gives tid_len up to 244.)
> +		 */
> +		if (tid_len - 4 >= TRANSPORT_IQN_LEN) {

[Severity: Medium]
Does this incorrectly reject valid, maximum-length (224-byte) FORMAT CODE
00b iSCSI TransportIDs?

The length tid_len - 4 represents the entire length of the iSCSI NAME field
including its mandatory NUL terminator.

An iSCSI name of 223 characters plus a NUL terminator equals exactly 224
bytes, which SPC-4 explicitly allows.

Because strscpy() operates safely when the source perfectly matches the
destination array size, should this check use ">" instead of ">="?

[ ... ]
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 11790f2c5d80f..0b19997c2edde 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1573,7 +1573,7 @@ core_scsi3_decode_spec_i_port(
>  
>  			iport_ptr = NULL;
>  			tid_found = target_parse_pr_out_transport_id(tmp_tpg,
> -					ptr, &tid_len, &iport_ptr, i_str);
> +					ptr, tpdl, &tid_len, &iport_ptr, i_str);

[Severity: High]
This is a pre-existing issue, but does core_scsi3_decode_spec_i_port()
leak a configfs dependency and pr_kref for local_pr_reg->pr_reg_deve?

Earlier in this function, local_pr_reg is created and
core_scsi3_lunacl_depend_item(local_pr_reg->pr_reg_deve) is called,
acquiring a configfs dependency.

The registration is added to tid_dest_list via a newly allocated tidh_new,
but tidh_new->dest_se_deve is left as NULL.

On the success path, the configfs dependency is not released. On the error
path, the cleanup loop bypasses core_scsi3_lunacl_undepend_item() because
tidh->dest_se_deve is NULL for local_pr_reg.

Could this prevent administrative tear down of the LUN via configfs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-b4-disp-9f20739e-v6-1-f6630e2aae44@proton.me?part=1