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

On 06/06/2026 02:54, 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
> iport_ptr as a raw pointer into that mapped buffer (the ISID 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.
> 
> Copy the ISID into a caller-owned stack buffer while the mapping is still
> live and repoint iport_ptr at it, mirroring the existing initiator_str
> handling. strscpy_pad() NUL-terminates and zero-fills the tail so the fixed
> 8-byte get_unaligned_be64() read stays in-bounds and deterministic even for
> an ISID shorter than 8 bytes. The NULL (device-format / non-iSCSI) case is
> preserved by copying only when iport_ptr is non-NULL.
> 
> Fixes: 4949314c7283 ("target: Allow control CDBs with data > 1 page")
> Cc: [email protected]
> Signed-off-by: Bryam Vargas <[email protected]>
> ---
> Everything below the --- is dropped by git am.
> 
> 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 -- the bug predates the git history reachable
> here; 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, so the alias stays valid) -- confirming the
>      multi-page vmap()/vunmap() path is what makes iport_ptr dangle.
>    - With this patch (same 8192-byte request): no report, command completes.
> 
>   drivers/target/target_core_pr.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 11790f2c5d80..b102f5f67793 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3160,6 +3160,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
>   	unsigned char *buf;
>   	unsigned char initiator_str[TRANSPORT_IQN_LEN];
>   	char *iport_ptr = NULL, i_buf[PR_REG_ISID_ID_LEN] = { };
> +	char isid_buf[PR_REG_ISID_LEN] = { };
>   	u32 tid_len, tmp_tid_len;
>   	int new_reg = 0, type, scope, matching_iname;
>   	sense_reason_t ret;
> @@ -3293,6 +3294,22 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
>   		goto out;
>   	}
> 
> +	/*
> +	 * For an iSCSI TransportID, iport_ptr aliases directly into the data
> +	 * buffer mapped above.  When that buffer spans more than one page it is
> +	 * a vmap() region that transport_kunmap_data_sg() is about to vunmap(),
> +	 * tearing down the kernel mapping and leaving iport_ptr dangling for
> +	 * every consumer below.  Copy the ISID into caller-owned storage now,
> +	 * while the mapping is still live.  strscpy_pad() NUL-terminates and
> +	 * zero-fills the tail so the later 8-byte get_unaligned_be64() read in
> +	 * __core_scsi3_do_alloc_registration() stays in-bounds and deterministic
> +	 * even for an ISID shorter than 8 bytes.
> +	 */
> +	if (iport_ptr) {
> +		strscpy_pad(isid_buf, iport_ptr, sizeof(isid_buf));
> +		iport_ptr = isid_buf;

It's not so nice to re-assign the pointer like this or have it even 
pointing at a local array.

Is it really messy for iscsi_parse_pr_out_transport_id() to do something 
like kstrdup and then the caller has the job of later free'ing it?

Thanks,
John