Re: [PATCH v4 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() entries are misclassified by _crste_hole()
--

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

KVM: s390: Fix UCONTROL inconsistencies

This patch updates mapped-but-unallocated UCONTROL segment entries to use a
new combination of flags (_UCAS_ENTRY_LENGTH and _UCAS_ENTRY_OFFSET) so they
no longer collide with the present bit, and adds DAT_WALK_IGN_HOLES to the
UCAS walk paths.

> 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() entries be misclassified by _crste_hole() and
dat_entry_walk() when walking a UCONTROL gmap without DAT_WALK_IGN_HOLES?

In _crste_ucas(), PFN_PHYS(pfn) is stored in bits 0..51 of res.val while
res.h.fc0.tl = 0 (clearing tok.pr) and res.h.fc0.tf = 3 (in bits 56..57).
However, _crste_hole() and _crste_none() only inspect crste.tok.type (bits
0..15 of res.val) and 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;
}

static inline bool _crste_none(union crste crste)
{
	return crste.h.i && !crste.tok.pr && crste.tok.type == _DAT_TOKEN_NONE;
}

And in dat_entry_walk():

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

Because crste.tok.type overlaps (PFN_PHYS(pfn) >> 48) & 0xffff and
crste.tok.par overlaps (PFN_PHYS(pfn) >> 32) & 0xffff, two problems can
occur when a caller walks vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES
(for example via KVM_S390_MEM_OP -> vcpu_check_access_key_gpa() ->
dat_get_storage_key(), or storage-key and CMMA handlers such as
handle_sske(), handle_pfmf(), or dat_perform_essa()):

1. When PFN_PHYS(pfn) < (1UL << 48) (256 TiB, which is true for all 3-level
   s390x user address spaces), crste.tok.type is 0 (_DAT_TOKEN_NONE).
   _crste_hole() returns false instead of treating the UCAS entry as a hole.
   If dat_entry_walk() was called with DAT_WALK_ALLOC (for instance via
   dat_set_storage_key(), dat_cond_set_storage_key(), or
   dat_set_cmma_bits()), it falls through to dat_split_ste(), overwriting
   the UCAS segment entry with a newly allocated page table that has
   PTVAL_VMADDR == 0, wasn't allocated in gmap->parent->asce, and is leaked
   when the child gmap is destroyed because GMAP_FLAG_OWNS_PAGETABLES is not
   set on vcpu->arch.gmap.

2. When (PFN_PHYS(pfn) >> 48) == _DAT_TOKEN_PIC (1, i.e. 256 TiB <=
   user_addr < 512 TiB under 5-level page tables), _crste_hole() returns
   true, but dat_entry_walk() sees entry.tok.type == _DAT_TOKEN_PIC and
   returns entry.tok.par (bits 16..31 of the user address) instead of
   -EFAULT. Callers interpret positive return values as a guest Program
   Interruption Code, or if bits 16..31 happen to be 0, dat_entry_walk()
   returns 0 with *ptepp == NULL, causing callers like dat_perform_essa()
   to dereference a NULL ptep.

Should _crste_hole() (and _crste_none()) account for crste_is_ucas(crste),
or should dat_entry_walk() explicitly check for crste_is_ucas(entry) before
inspecting entry.tok.type?

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