Re: [PATCH v2 07/10] 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 misclassified as holes by _crste_hole()
--

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

KVM: s390: Fix UCONTROL inconsistencies

This commit changes the encoding of mapped-but-unallocated UCONTROL segment
entries so that the tl field is set to 0 instead of 1, avoiding a collision
with the present bit, and updates the UCONTROL gmap helpers accordingly.

> diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
> index 319a8670fb10e..3493f033e2045 100644
> --- a/arch/s390/kvm/gmap/dat.h
> +++ b/arch/s390/kvm/gmap/dat.h
> @@ -105,6 +105,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
>  
>  #define CRDTE_NODAT		IPTE_NODAT
>  #define CRDTE_GUEST_ASCE	IPTE_GUEST_ASCE
> @@ -429,6 +431,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 entries created by _crste_ucas() be misclassified as memory holes by
_crste_hole() (or as empty entries by _crste_none())?

In union crste, tok.pr and s.fc1.pr both alias bit 63 (the least significant
bit of h.fc0.tl), while tok.type (bits 0..15) and tok.par (bits 16..31) alias
the upper 32 bits of PFN_PHYS(pfn).

Previously, mapped-but-unallocated UCAS segment entries had h.fc0.tl = 1, so
tok.pr was 1 and _crste_hole() evaluated to false. With _crste_ucas() setting
h.fc0.tl = _UCAS_ENTRY_LENGTH (0), tok.pr is now 0, while _crste_hole() and
_crste_none() do not check !crste_is_ucas(crste):

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

#define crste_hole(x) _crste_hole(_CRSTE(x))

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

When userspace maps a UCONTROL segment via KVM_S390_UCAS_MAP ->
gmap_ucas_map() -> gmap_ucas_map_one() -> _crste_ucas(p_gfn) with a host
userspace address >= (1UL << 48) (so pfn >= 1UL << 36, which is standard for
top-down mmap() on 4-level/5-level s390x systems), PFN_PHYS(pfn) has non-zero
bits in bits 0..15, making crste.tok.type != _DAT_TOKEN_NONE and causing
crste_hole() to return true.

While gmap_ucas_map_one(), gmap_ucas_translate_simple(), and
gmap_ucas_unmap_one() now pass DAT_WALK_IGN_HOLES, other walkers on
vcpu->arch.gmap->datt do not. For example, vcpu_check_access_key_gpa() and
handle_iske() call dat_get_storage_key(), and handle_rrbe() calls
dat_reset_reference_bit():

arch/s390/kvm/gmap/dat.c:dat_get_storage_key() {
    ...
	rc = dat_entry_walk(NULL, gfn, datt, DAT_WALK_ANY, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
	if (rc)
		return rc;

	if (!ptep) {
		union crste crste = READ_ONCE(*crstep);

		if (!crste_leaf(crste) && !crste.h.i)
			goto again;
		if (!crste.s.fc1.pr)
			return 0;
    ...
}

In dat_entry_walk(), at the segment level:

	if (crste_hole(entry) && !ign_holes)
		return entry.tok.type == _DAT_TOKEN_PIC ? entry.tok.par : -EFAULT;

Before dat_get_storage_key() or dat_reset_reference_bit() can reach the
!crste.s.fc1.pr check and return 0 for the unallocated segment, wouldn't
dat_entry_walk() abort with -EFAULT (or return bits 16..31 of the host
userspace address as a bogus guest program interruption code if bits 0..15
happen to match _DAT_TOKEN_PIC)?

Should _crste_hole() and _crste_none() also check !crste_is_ucas(crste)?

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