Re: [PATCH v6 1/5] KVM: s390: Fix UCONTROL inconsistencies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: s390: _crste_ucas() collides with crste.tok in _crste_hole()
--

commit a4c739d060e46a8362a876fd2edc730f00fb6503
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix UCONTROL inconsistencies

This commit updates how mapped-but-unallocated UCONTROL segment entries
are encoded in union crste so they do not set the tl field to 1 and
collide with the present bit, and adjusts UCAS walks to ignore holes.

> diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
> index 90389d47ba4e0..e6215c0b571f9 100644
> --- a/arch/s390/kvm/gmap/dat.h
> +++ b/arch/s390/kvm/gmap/dat.h
> @@ -109,6 +109,8 @@ union pte {
>  #define _REGION3_FR_MASK	(_REGION3_MASK >> PAGE_SHIFT)
>  #define _PAGES_PER_SEGMENT	_PAGE_ENTRIES
>  #define _PAGES_PER_REGION3	(_PAGES_PER_SEGMENT * _CRST_ENTRIES)
> +#define _UCAS_ENTRY_LENGTH	0
> +#define _UCAS_ENTRY_OFFSET	3
>  
>  /* Soft dirty, needed as macro for atomic operations on ptes */
>  #define _PAGE_SD 0x002
> @@ -421,6 +423,23 @@ static inline union crste _crste_fc0(kvm_pfn_t pfn, int tt)
>  	return res;
>  }
>  
> +static inline union crste _crste_ucas(kvm_pfn_t pfn)
> +{
> +	union crste res = { .val = PFN_PHYS(pfn) };
> +
> +	res.h.i = 1;
> +	res.h.tt = TABLE_TYPE_SEGMENT;
> +	/*
> +	 * Segment entries do not have a TF or TL field; reuse those bits to
> +	 * indicate that this is a special mappped-but-not-allocated UCAS
> +	 * entry. Setting TL to 0 ensures that the entry will always be
> +	 * considered not present.
> +	 */
> +	res.h.fc0.tl = _UCAS_ENTRY_LENGTH;
> +	res.h.fc0.tf = _UCAS_ENTRY_OFFSET;
> +	return res;
> +}

[Severity: High]
As reported by Sashiko on v5 [1], does _crste_ucas() collide with
crste.tok.type and crste.tok.par when checked by _crste_hole(),
_crste_none(), and dat_entry_walk()?

In union crste, tok.type occupies the top 16 bits (val >> 48) and tok.par
occupies the next 16 bits ((val >> 32) & 0xffff), while h.fc0.tf sits in
the unnamed 26-bit padding of struct tok and setting h.fc0.tl = 0 sets
tok.pr = 0:

arch/s390/kvm/gmap/dat.h:
	union {
		struct {
			unsigned long type :16; /* Token type */
			unsigned long par  :16; /* Token parameter */
			unsigned long      :26;
			unsigned long i    : 1; /* Must be 1 */
			unsigned long      : 1;
			unsigned long tt   : 2;
			unsigned long      : 1;
			unsigned long pr   : 1; /* Must be 0 */
		};

Because _crste_ucas(pfn) stores PFN_PHYS(pfn) in val, crste.tok.type is
(PFN_PHYS(pfn) >> 48) & 0xffff and crste.tok.par is
(PFN_PHYS(pfn) >> 32) & 0xffff.

However, _crste_hole() and _crste_none() do not check crste_is_ucas():

arch/s390/kvm/gmap/dat.h:
static inline bool _crste_hole(union crste crste)
{
	return crste.h.i && !crste.tok.pr && crste.tok.type != _DAT_TOKEN_NONE;
}

and dat_entry_walk() checks:

arch/s390/kvm/gmap/dat.c:dat_entry_walk() {
	...
	if (crste_hole(entry) && !ign_holes)
		return entry.tok.type == _DAT_TOKEN_PIC ? entry.tok.par : -EFAULT;
	...
	if (entry.pmd.h.i && !entry.pmd.h.fc) {
		if (!allocate)
			return any ? 0 : -ENOENT;
		rc = dat_split_ste(mc, &(*last)->pmd, gfn, asce, uses_skeys);
}

When a segment is mapped via KVM_S390_UCAS_MAP (kvm_vm_ioctl_ucas_map() ->
gmap_ucas_map() -> gmap_ucas_map_one()) before the parent segment is
allocated, gmap_ucas_map_one() installs _crste_ucas(p_gfn) into the child
vcpu->arch.gmap->asce. If another gmap function then walks
vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES:

1. When PFN_PHYS(p_gfn) < (1UL << 48) (i.e. host user address < 256 TiB),
entry.tok.type is 0 (_DAT_TOKEN_NONE), so crste_hole(entry) returns false
instead of failing with -EFAULT. If DAT_WALK_ALLOC is set (for example via
handle_sske() or handle_pfmf() -> dat_cond_set_storage_key(), or via
dat_set_storage_key() or dat_set_cmma_bits()), dat_entry_walk() calls
dat_split_ste() on the UCAS entry in vcpu->arch.gmap->asce. That allocates
a page table in the child gmap (which is leaked on gmap_dispose() because
GMAP_FLAG_OWNS_PAGETABLES is cleared on UCONTROL child gmaps) with zeroed
PGSTEs (PTVAL_VMADDR == 0), causing subsequent gmap_ucas_translate() calls
to translate the segment to GPA 0 in gmap->parent instead of the mapped
user_addr.

2. When (PFN_PHYS(p_gfn) >> 48) == _DAT_TOKEN_PIC (1) (i.e. 256 TiB <=
user_addr < 512 TiB), crste_hole(entry) is true and dat_entry_walk()
returns entry.tok.par ((PFN_PHYS(p_gfn) >> 32) & 0xffff) instead of
-EFAULT. If entry.tok.par is 0, dat_entry_walk() returns 0 with
*ptepp == NULL and *last pointing to the non-leaf UCAS entry, causing
dat_cond_set_storage_key() and dat_set_storage_key() to spin endlessly at
if (!crste_leaf(crste)) goto again; while holding
read_lock(&vcpu->kvm->mmu_lock). If entry.tok.par is non-zero, callers
such as handle_iske(), handle_rrbe(), handle_pfmf(), and
vcpu_check_access_key_gpa() treat those address bits as a guest Program
Interruption Code.

Should _crste_hole(), _crste_none(), and dat_entry_walk() explicitly
handle crste_is_ucas() so unallocated UCAS entries are always treated as
non-PIC holes?

[1] https://lore.kernel.org/r/20261029130044.125032-2-imbrenda@linux.ibm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105153705.208516-1-imbrenda@linux.ibm.com?part=1