Re: [PATCH v5 1/7] 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 de19569965dfbfabc785467bbb1e64c29e58b5d9
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix UCONTROL inconsistencies

This commit changes the flag encoding used for mapped but unallocated UCONTROL
segment table entries so that they do not set the present bit via fc0.tl = 1,
and updates the UCAS walk and translation helpers in gmap accordingly.

> diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
> index 90389d47ba4e0..4665c1112bb91 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,17 @@ 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;
> +	res.h.fc0.tl = _UCAS_ENTRY_LENGTH;
> +	res.h.fc0.tf = _UCAS_ENTRY_OFFSET;
> +	return res;
> +}

[Severity: High]
Can _crste_ucas() cause _crste_hole() and dat_entry_walk() to misinterpret the
upper 32 bits of PFN_PHYS(pfn) as crste.tok.type and crste.tok.par?

In union crste, setting res.h.fc0.tl = 0 leaves bit 63 (res.tok.pr) as 0 while
res.h.i is 1, and res.h.fc0.tf sits in the unnamed padding of struct tok.
Meanwhile, PFN_PHYS(pfn) occupies bits 0..51, overlapping res.tok.type (bits
0..15) and res.tok.par (bits 16..31).

Because _crste_hole() and _crste_none() do not check crste_is_ucas():

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

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);
	...
}

Waiters or walks on vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES will
behave differently depending on the upper bits of the mapped userspace address
PFN_PHYS(p_gfn):

1. When PFN_PHYS(p_gfn) < (1UL << 48) (any standard 3-level or 4-level
userspace address below 256 TiB), entry.tok.type is 0 (_DAT_TOKEN_NONE), so
crste_hole(entry) is false instead of returning -EFAULT. If a walk with
DAT_WALK_ALLOC is triggered (such as handle_sske() or handle_pfmf() ->
dat_cond_set_storage_key() -> dat_entry_walk()), dat_entry_walk() calls
dat_split_ste() on the UCAS entry. That allocates a struct page_table in the
child gmap (which leaks on unmap/dispose because GMAP_FLAG_OWNS_PAGETABLES is
not set on UCONTROL vCPU gmaps) and zeroes its PGSTEs, leaving PTVAL_VMADDR as
0 and causing subsequent gmap_ucas_translate() calls to translate the guest
segment to host address 0.

2. When 0x0001000000000000UL <= PFN_PHYS(p_gfn) < 0x0002000000000000UL (on
5-level page tables), entry.tok.type is 1 (_DAT_TOKEN_PIC). When entry.tok.par
is non-zero, dat_entry_walk() returns those address bits as a positive program
interruption code instead of -EFAULT.

3. When entry.tok.type is _DAT_TOKEN_PIC and entry.tok.par is 0 (for example,
PFN_PHYS(p_gfn) == 0x0001000000000000UL), dat_entry_walk() returns 0 while
leaving *ptepp as NULL. In handle_essa() -> __do_essa() -> dat_perform_essa(),
this leads to a NULL pointer dereference in pgste_get_lock(ptep):

arch/s390/kvm/gmap/dat.c:dat_perform_essa() {
	...
	if (dat_entry_walk(NULL, gfn, asce, 0, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep)) {
		*state = (union essa_state) { .exception = 1 };
		return -1;
	}

	pgste = pgste_get_lock(ptep);
}

And in dat_cond_set_storage_key(), returning 0 with ptep == NULL on a non-leaf
UCAS crste causes an infinite loop at goto again under read_lock(&kvm->mmu_lock).

Should _crste_hole() and _crste_none() (or dat_entry_walk()) explicitly account
for crste_is_ucas() so that unallocated UCAS entries are consistently treated
as non-PIC holes when DAT_WALK_IGN_HOLES is not set?

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