Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer

[John Garry]

On 09/06/2026 01:59, Bryam Vargas wrote:
> core_scsi3_emulate_pro_register_and_move() maps the PERSISTENT RESERVE OUT
> parameter list with transport_kmap_data_sg() and parses the destination
> TransportID with target_parse_pr_out_transport_id(). For an iSCSI
> TransportID (FORMAT CODE 01b) iscsi_parse_pr_out_transport_id() returns
> the ISID as a raw pointer into that mapped buffer (the bytes following the
> ",i,0x" separator).
>
> The function then unmaps the buffer with transport_kunmap_data_sg() before
> dereferencing iport_ptr in strcmp(), __core_scsi3_locate_pr_reg() and
> core_scsi3_alloc_registration() (the last reads 8 bytes via
> get_unaligned_be64() and copies the string with snprintf()). When the
> parameter list spans more than one page (PARAMETER LIST LENGTH > 4096),
> transport_kmap_data_sg() uses vmap() and transport_kunmap_data_sg() does
> vunmap(), so the kernel virtual address backing iport_ptr is torn down on
> all architectures and every subsequent dereference is a use-after-free of
> the unmapped region.
>
> initiator_str does not have this problem because the parser strscpy()s it
> into a caller-owned buffer; iport_ptr is the only output left as a borrowed
> alias. core_scsi3_decode_spec_i_port() consumes the same alias safely
> because it unmaps only after all uses.
>
> Move ownership of the ISID string into the parser: after lowercasing the
> ISID in-place, allocate a private copy with kzalloc(PR_REG_ISID_LEN) and
> return that instead of the raw buffer pointer. kzalloc() zero-fills the
> allocation to PR_REG_ISID_LEN bytes so the fixed 8-byte
> get_unaligned_be64() read in __core_scsi3_do_alloc_registration() stays
> in-bounds and returns deterministic results even for an ISID shorter than
> 8 characters (plain kstrdup() would give an allocation shorter than 8 bytes
> for a malformed short ISID, turning the be64 read into a heap out-of-bounds).
>
> Both callers now own the returned pointer and must kfree() it when done.
> core_scsi3_decode_spec_i_port() frees before each inner-loop parse
> iteration (so that a failed ACL match that continues the search does not
> leak the previous parse's allocation) and at the error and success exit
> paths. core_scsi3_emulate_pro_register_and_move() frees at both the
> success return and the error label.
>
> Fixes: 4949314c7283 ("target: Allow control CDBs with data > 1 page")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> ---
> v2 (review of v1 by John Garry):
>   - Move the ISID copy into iscsi_parse_pr_out_transport_id() so the parser
>     returns an owned allocation via *port_nexus_ptr; callers kfree() it.
>     v1's stack-buffer approach (isid_buf[] + iport_ptr = isid_buf) is gone.
>   - Use kzalloc(PR_REG_ISID_LEN) + strscpy_pad() rather than plain kstrdup():
>     __core_scsi3_do_alloc_registration() reads the ISID with a fixed 8-byte
>     get_unaligned_be64(isid); a malformed ISID shorter than 8 chars would
>     give a kstrdup allocation smaller than 8 bytes, causing a heap OOB on
>     that read.  kzalloc zero-fills to PR_REG_ISID_LEN (16) bytes.
>   - core_scsi3_decode_spec_i_port(): add kfree(iport_ptr) before each
>     inner-loop reset (line ~1574), at out_unmap:, and before return 0.
>
> Class / impact: CWE-416 use-after-free (use-after-vunmap) in the LIO SCSI
> target. Triggerable by an authenticated iSCSI initiator that is a current
> Persistent Reservation registrant on the LUN: it sends PERSISTENT RESERVE
> OUT / REGISTER AND MOVE with an iSCSI (FORMAT CODE 01b) TransportID and a
> PARAMETER LIST LENGTH > 4096 so the parameter list spans >1 page and is
> mapped with vmap(). After transport_kunmap_data_sg() vunmap()s that region,
> the retained iport_ptr is dereferenced -> kernel read of an unmapped
> vmalloc address (oops / DoS; memory-safety corruption confirmed by KASAN).
> Primarily a remotely-reachable authenticated denial of service.
>
> Affected: all maintained trees -- it became a destructive dangling
> dereference with 4949314c7283 (v3.3, 2012), which introduced the
> multi-page vmap() path. Verified present at mainline v7.1-rc6 and
> stable v6.12.92.
>
> Reproducer (authenticated iSCSI initiator, current PR reservation holder):
>    1. PERSISTENT RESERVE OUT / REGISTER a key from the iSCSI nexus.
>    2. PERSISTENT RESERVE OUT / REGISTER AND MOVE, FORMAT CODE 01b TransportID
>       (IQN + ",i,0x" + 12-char ISID), RELATIVE TARGET PORT IDENTIFIER of an
>       existing target port, with PARAMETER LIST LENGTH = 8192 (two pages ->
>       vmap()/vunmap()), the inner ADDITIONAL LENGTH set so tid_len + 24 ==
>       data_length, the remainder zero padding.
>
> A/B verification (CONFIG_KASAN_VMALLOC=y, kasan.fault=report, x86-64,
> 6.12.90; reproduced with both a 64-bit and a 32-bit initiator):
>    - Without this patch (8192-byte, two-page request):
>        BUG: KASAN: vmalloc-out-of-bounds in strcmp+0xa7/0xb0
>          strcmp
>          core_scsi3_emulate_pro_register_and_move [target_core]
>          ? remove_vm_area
>          target_scsi3_emulate_pr_out [target_core]
>          __target_execute_cmd / iscsit_execute_cmd / iscsi_target_rx_thread
>        The buggy address belongs to a vmalloc virtual mapping
>        BUG: unable to handle page fault for address ... (PTE 0)
>    - Control (56/128-byte, single-page request): no report (kunmap is a
>      no-op on 64-bit !HIGHMEM).
>    - With this patch (same 8192-byte request): no report, command completes.
>
> drivers/target/target_core_fabric_lib.c | 16 ++++++++++++++++
>   drivers/target/target_core_pr.c         |  5 +++++
>   2 files changed, 21 insertions(+)
> diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
> index 87c5d26a5089..b5ad45f072bd 100644
> --- a/drivers/target/target_core_fabric_lib.c
> +++ b/drivers/target/target_core_fabric_lib.c
> @@ -19,6 +19,7 @@
>   #include <linux/hex.h>
>   #include <linux/kernel.h>
>   #include <linux/string.h>
> +#include <linux/slab.h>
>   #include <linux/ctype.h>
>   #include <linux/spinlock.h>
>   #include <linux/export.h>
> @@ -367,6 +368,21 @@ static bool iscsi_parse_pr_out_transport_id(
>   			*p = tolower(*p);
>   			p++;
>   		}
> +		/*
> +		 * The loop above advanced p past the ISID; *port_nexus_ptr still
> +		 * holds the ISID start.  Replace the borrowed buffer alias with an
> +		 * owned heap copy so callers can safely use the ISID past the
> +		 * buffer lifetime (e.g. after transport_kunmap_data_sg() in
> +		 * register_and_move).  kzalloc() zero-fills to PR_REG_ISID_LEN
> +		 * bytes so the 8-byte get_unaligned_be64() read in
> +		 * __core_scsi3_do_alloc_registration() stays in-bounds even for
> +		 * an ISID shorter than 8 characters.
> +		 */
> +		p = *port_nexus_ptr;
> +		*port_nexus_ptr = kzalloc(PR_REG_ISID_LEN, GFP_KERNEL);
> +		if (!*port_nexus_ptr)
> +			return false;
> +		strscpy_pad(*port_nexus_ptr, p, PR_REG_ISID_LEN);
>   	} else>   		*port_nexus_ptr = NULL;
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 11790f2c5d80..d7340c4fbd07 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1571,6 +1571,7 @@ core_scsi3_decode_spec_i_port(
>   				continue;
>   			dest_rtpi = tmp_lun->lun_tpg->tpg_rtpi;
>
> +			kfree(iport_ptr);
>   			iport_ptr = NULL;
>   			tid_found = target_parse_pr_out_transport_id(tmp_tpg,
>   					ptr, &tid_len, &iport_ptr, i_str);
> @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
>   		core_scsi3_tpg_undepend_item(dest_tpg);
>   	}
>
> +	kfree(iport_ptr);
>   	return 0;
>   out_unmap:
>   	transport_kunmap_data_sg(cmd);
> +	kfree(iport_ptr);

sorry for suggesting this change, but this is worse than what you had 
originally, as we have so many paths to call kfree() [which means more 
bugs possible]

it's hard to make good suggestions for this target code as the functions 
are so large and complex.

Is there any reason why we can't just keep the transport_kmap_data_sg() 
in place for (much) longer, i.e. until at the out: label? We already 
handle the the unmap properly there. I do notice that there would be 
regions which we keep spinlocks held when this mapping is in place, but 
I am not sure if that makes an difference

>   out:
>   	/*
>   	 * For the failure case, release everything from tid_dest_list
> @@ -3532,10 +3535,12 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
>   	core_scsi3_update_and_write_aptpl(cmd->se_dev, aptpl);
>
>   	core_scsi3_put_pr_reg(dest_pr_reg);
> +	kfree(iport_ptr);
>   	return 0;
>   out:
>   	if (buf)
>   		transport_kunmap_data_sg(cmd);
> +	kfree(iport_ptr);
>   	if (dest_se_deve)
>   		core_scsi3_lunacl_undepend_item(dest_se_deve);
>   	if (dest_node_acl)
>
> base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66