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

From: Bryam Vargas <[email protected]>

core_scsi3_decode_spec_i_port() and core_scsi3_emulate_register_and_move()
hand the raw PERSISTENT RESERVE OUT parameter buffer to
target_parse_pr_out_transport_id() without telling it how many bytes are
valid.  For an iSCSI TransportID (FORMAT CODE 01b),
iscsi_parse_pr_out_transport_id() locates the ",i,0x" ISID separator with
an unbounded strstr() (and on the error path prints the name with a
further unbounded "%s").  An initiator can submit a TransportID whose
iSCSI name contains neither a ",i,0x" substring nor a NUL terminator,
filling the parameter list to its end, so the scan runs off the end of the
buffer.

When the parameter list spans more than one page the buffer is a
multi-page vmap (transport_kmap_data_sg()), so the over-read walks into
the trailing vmalloc guard page and oopses (KASAN: vmalloc-out-of-bounds
in strstr).  It is reachable by any fabric that delivers a PR OUT to a
device exported through an iSCSI TPG, including a guest via vhost-scsi.

Pass the number of received bytes down to the parser and validate the
iSCSI TransportID's own self-described length (ADDITIONAL LENGTH + 4)
once, up front: reject it if it is below the spc4r17 minimum or larger
than the received buffer, then bound the separator search, the ISID walk
and the name copy by that length.  This is the length check the callers
already perform after the parse (core_scsi3_decode_spec_i_port() compares
tid_len against tpdl, core_scsi3_emulate_register_and_move() validates it
against data_length), moved ahead of the scan.  Also drop the unbounded
"%s" of the unterminated name.

Add per-format explicit name-length checks before copying into i_str,
rather than silently truncating with min_t: for FORMAT CODE 00b reject
if the descriptor body (tid_len - 4 bytes) cannot fit in
i_str[TRANSPORT_IQN_LEN]; for FORMAT CODE 01b reject if the name portion
(from &buf[4] up to the separator) cannot fit.  Both checks make the
bounds intent explicit at each format branch.

While here, also reject a FORMAT CODE 01b TransportID whose ",i,0x"
separator sits at the very end of the descriptor: that leaves an empty
ISID and points the returned port nexus pointer at buf + tid_len, one past
the descriptor, which the registration code (__core_scsi3_locate_pr_reg(),
__core_scsi3_alloc_registration()) then dereferences as the ISID string --
the same over-read of the parameter buffer for a malformed descriptor.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Cc: [email protected]
Signed-off-by: Bryam Vargas <[email protected]>
Reviewed-by: John Garry <[email protected]>
Reviewed-by: David Disseldorp <[email protected]>
---
This fix was developed and reviewed under embargo on [email protected]
with the SCSI target maintainers and reviewers (John Garry, David
Disseldorp); the 7-day embargo has elapsed, so it is reposted here on the
public lists for pickup.  No functional change since that review -- the
Reviewed-by tags are carried in the patch.

Impact / reproducer (CONFIG_KASAN_VMALLOC, A/B-proven):

A PERSISTENT RESERVE OUT with SPEC_I_PT=1 and a multi-page (>= 2 page)
parameter list carrying a single FORMAT CODE 01b iSCSI TransportID whose
iSCSI name is all 'A's -- no ",i,0x" substring and no NUL terminator --
makes the unbounded strstr() walk off the multi-page vmap into the
trailing vmalloc guard page:

  BUG: KASAN: vmalloc-out-of-bounds in strstr
   target_parse_pr_out_transport_id [target_core_mod]
   core_scsi3_decode_spec_i_port [target_core_mod]
   core_scsi3_emulate_pro_register [target_core_mod]
   target_scsi3_emulate_pr_out [target_core_mod]
   __target_execute_cmd [target_core_mod]
   iscsit_get_rx_pdu [iscsi_target_mod]

It is reachable by any fabric that delivers a PR OUT to a device exported
through an iSCSI TPG (iSCSI native, tcm_loop, and guest->host via
vhost-scsi were all reproduced).  With the patch the same input is
rejected cleanly (CHECK CONDITION, INVALID FIELD IN PARAMETER LIST) and a
well-formed TransportID still parses; no KASAN on any arm.

Changelog:

v6: first public posting (v1..v5 were embargoed on [email protected]);
    dropped the redundant Reported-by (it equalled the Signed-off-by);
    collected Reviewed-by from John Garry and David Disseldorp; reflowed
    one log message onto a single line to clear a checkpatch warning.  No
    functional change.
v5: cite spc4r17 7.5.4.6; move the FORMAT CODE 01b name-length check to the
    separator so a legal 212..223-byte name is no longer rejected.
v4: split the shared min_t() name copy into explicit per-format
    name-length rejects (00b on tid_len - 4, 01b on the name portion).
v3: keep the name copy bounded by TRANSPORT_IQN_LEN (not an up-front
    tid_len cap, which would reject legal long iSCSI names); reject an
    empty ISID (",i,0x" ending the descriptor).
v2: validate the self-described TransportID length up front and bound the
    separator search, the ISID walk and the name copy by it; drop the
    unbounded "%s" print of the unterminated name.
---
 drivers/target/target_core_fabric_lib.c | 89 +++++++++++++++++++++++++--------
 drivers/target/target_core_internal.h   |  3 +-
 drivers/target/target_core_pr.c         |  4 +-
 3 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 87c5d26a5089..2853b95b2c59 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -290,13 +290,24 @@ static void sbp_parse_pr_out_transport_id(char *buf, char *i_str)
 static bool iscsi_parse_pr_out_transport_id(
 	struct se_portal_group *se_tpg,
 	char *buf,
+	u32 buf_len,
 	u32 *out_tid_len,
 	char **port_nexus_ptr,
 	char *i_str)
 {
 	char *p;
+	u32 tid_len;
 	int i;
-	u8 format_code = (buf[0] & 0xc0);
+	u8 format_code;
+
+	/*
+	 * The 4-byte iSCSI TransportID header (FORMAT CODE + 2-byte ADDITIONAL
+	 * LENGTH) must be present before any of it can be parsed.
+	 */
+	if (buf_len < 4)
+		return false;
+
+	format_code = buf[0] & 0xc0;
 	/*
 	 * Check for FORMAT CODE 00b or 01b from spc4r17, section 7.5.4.6:
 	 *
@@ -316,15 +327,17 @@ static bool iscsi_parse_pr_out_transport_id(
 		return false;
 	}
 	/*
-	 * If the caller wants the TransportID Length, we set that value for the
-	 * entire iSCSI Tarnsport ID now.
+	 * Reconstruct the self-described TransportID length from the ADDITIONAL
+	 * LENGTH field plus the 4-byte header.  Reject it if it is below the
+	 * spc4r17 section 7.5.4.6 minimum (ADDITIONAL LENGTH shall be at least
+	 * 20) or if it runs past the bytes actually received, so that every
+	 * access below stays inside the TransportID.
 	 */
-	if (out_tid_len) {
-		/* The shift works thanks to integer promotion rules */
-		*out_tid_len = get_unaligned_be16(&buf[2]);
-		/* Add four bytes for iSCSI Transport ID header */
-		*out_tid_len += 4;
-	}
+	tid_len = get_unaligned_be16(&buf[2]) + 4;
+	if (tid_len < 24 || tid_len > buf_len)
+		return false;
+	if (out_tid_len)
+		*out_tid_len = tid_len;
 
 	/*
 	 * Check for ',i,0x' separator between iSCSI Name and iSCSI Initiator
@@ -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 */
 		p += 5; /* Skip over ",i,0x" separator */
 
+		/*
+		 * The ISID must follow the separator.  A ",i,0x" sitting at the
+		 * very end of the TransportID leaves no ISID and would point the
+		 * port nexus at buf + tid_len, i.e. past the descriptor, which
+		 * the registration code then reads as the ISID string.
+		 */
+		if (p >= buf + tid_len) {
+			pr_err("Missing ISID in iSCSI Initiator port TransportID\n");
+			return false;
+		}
 		*port_nexus_ptr = p;
 		/*
 		 * Go ahead and do the lower case conversion of the received
@@ -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++) {
 			/*
 			 * The first ISCSI INITIATOR SESSION ID field byte
 			 * containing an ASCII null character terminates the
@@ -367,10 +396,22 @@ static bool iscsi_parse_pr_out_transport_id(
 			*p = tolower(*p);
 			p++;
 		}
-	} 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) {
+			pr_err("iSCSI Initiator port name too long in TransportID\n");
+			return false;
+		}
+		strscpy(i_str, &buf[4], tid_len - 4);
+	}
 	return true;
 }
 
@@ -420,8 +461,16 @@ int target_get_pr_transport_id(struct se_node_acl *nacl,
 }
 
 bool target_parse_pr_out_transport_id(struct se_portal_group *tpg,
-		char *buf, u32 *out_tid_len, char **port_nexus_ptr, char *i_str)
+		char *buf, u32 buf_len, u32 *out_tid_len,
+		char **port_nexus_ptr, char *i_str)
 {
+	/*
+	 * The fixed-length SAS/SRP/FCP/SBP TransportIDs are 24 bytes; the iSCSI
+	 * format is variable and bounds itself against buf_len below.
+	 */
+	if (tpg->proto_id != SCSI_PROTOCOL_ISCSI && buf_len < 24)
+		return false;
+
 	switch (tpg->proto_id) {
 	case SCSI_PROTOCOL_SAS:
 		/*
@@ -440,8 +489,8 @@ bool target_parse_pr_out_transport_id(struct se_portal_group *tpg,
 		sbp_parse_pr_out_transport_id(buf, i_str);
 		break;
 	case SCSI_PROTOCOL_ISCSI:
-		return iscsi_parse_pr_out_transport_id(tpg, buf, out_tid_len,
-					port_nexus_ptr, i_str);
+		return iscsi_parse_pr_out_transport_id(tpg, buf, buf_len,
+					out_tid_len, port_nexus_ptr, i_str);
 	default:
 		pr_err("Unknown proto_id: 0x%02x\n", tpg->proto_id);
 		return false;
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 763e6d26e187..f0886ea29034 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -104,7 +104,8 @@ int	target_get_pr_transport_id(struct se_node_acl *nacl,
 		struct t10_pr_registration *pr_reg, int *format_code,
 		unsigned char *buf);
 bool target_parse_pr_out_transport_id(struct se_portal_group *tpg,
-		char *buf, u32 *out_tid_len, char **port_nexus_ptr, char *i_str);
+		char *buf, u32 buf_len, u32 *out_tid_len,
+		char **port_nexus_ptr, char *i_str);
 
 /* target_core_hba.c */
 struct se_hba *core_alloc_hba(const char *, u32, u32);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 11790f2c5d80..0b19997c2edd 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);
 			if (!tid_found)
 				continue;
 			/*
@@ -3285,7 +3285,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 		goto out;
 	}
 	tid_found = target_parse_pr_out_transport_id(dest_se_tpg,
-			&buf[24], &tmp_tid_len, &iport_ptr, initiator_str);
+			&buf[24], tid_len, &tmp_tid_len, &iport_ptr, initiator_str);
 	if (!tid_found) {
 		pr_err("SPC-3 PR REGISTER_AND_MOVE: Unable to locate"
 			" initiator_str from Transport ID\n");

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260611-b4-disp-9f20739e-c62c19cb2dcf

Best regards,
-- 
Bryam Vargas <[email protected]>