Re: [PATCH v3 1/4] 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_hole() and _crste_none()
--

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

KVM: s390: Fix UCONTROL inconsistencies

This commit changes the flag encoding used for mapped but unallocated
UCONTROL vCPU segment entries so that they do not set the present bit,
and updates the UCAS gmap helpers 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]
With _UCAS_ENTRY_LENGTH set to 0, _crste_ucas() leaves bit 63 (res.tok.pr)
cleared while setting res.h.i = 1 and placing PFN_PHYS(pfn) across bits
0..51, which overlaps res.tok.type (bits 0..15) and res.tok.par (bits
16..31).

In arch/s390/kvm/gmap/dat.h, _crste_hole() and _crste_none() check:

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 a UCONTROL segment is mapped via KVM_S390_UCAS_MAP to a host address
with non-zero bits in 63:48 (i.e. PFN_PHYS(pfn) >= (1UL << 48), which is
normal for 5-level s390x mmap addresses), crste_hole() evaluates to true
for the UCAS entry.

While this patch passes DAT_WALK_IGN_HOLES in the gmap_ucas_*() helpers,
other callers walk vcpu->arch.gmap->asce without DAT_WALK_IGN_HOLES, such
as:

- vcpu_check_access_key_gpa() (via KVM_S390_MEM_OP) and handle_iske() ->
  dat_get_storage_key() -> dat_entry_walk(..., DAT_WALK_ANY, ...)
- handle_rrbe() -> dat_reset_reference_bit() ->
  dat_entry_walk(..., DAT_WALK_ANY, ...)

In dat_entry_walk(), hitting the segment entry triggers:

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

Instead of returning 0 at the !crste.s.fc1.pr check in
dat_get_storage_key() or dat_reset_reference_bit(), won't this fail with
-EFAULT (or return the upper address bits as a bogus program interruption
code if entry.tok.type happens to equal _DAT_TOKEN_PIC)?

Should _crste_hole() and _crste_none() exclude crste_is_ucas(crste) (or
check !crste.h.fc0.tf)?

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