Re: [PATCH v2 12/20] KVM: s390: arm64: Implement feature sanitisation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Hardware Feature Mis-mapping: ARM64_HAS_ECV_CNTPOFF maps to PAN instead of ECV
- [High] Hardware Feature Mis-mapping: ARM64_HAS_HCR_NV1 fails to account for CPUs that remove NV1
- [High] Incomplete Feature Check: ARM64_HAS_RASV1P1_EXTN omits the primary fallback check
- [High] Feature Leakage: MASK_RESERVED uses a blacklist approach, leaking unknown features to the guest
- [Low] Undefined reference to __qaaf_qmp causing potential linker errors
- [Low] Integer overflow in kvm_sae_max_vcpus due to u16 return type
--

commit ea4286d0301a7cdfe0535f60a7c1ca1b65c64103
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Implement feature sanitisation

Implement feature detection and sanitization for arm64 KVM guests on s390.
Queries hardware capabilities using the QAAF instruction and provides accessor
functions for capability mapping.

> diff --git a/arch/s390/kvm/arm64/feature.c b/arch/s390/kvm/arm64/feature.c
> new file mode 100644
> index 0000000000000..39c8ae7e951cf
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/feature.c
> @@ -0,0 +1,165 @@
[ ... ]
> +#define MASK_RESERVED(_qaafp, _id)				\
> +	({							\
> +		u64 *_reg = &((_qaafp)->regs[QAAF_REG_##_id]);	\
> +		*_reg = (*_reg & ~_id##_RES0) | _id##_RES1;	\
> +	})

[Severity: High]
Does this blacklist approach inadvertently leak unknown features to the guest?

By clearing only the explicitly unarchitected RES0 bits, any valid CPU
features that are not recognized or handled by KVM will be passed through
unconditionally. This goes against the KVM invariants that new guest-visible
features should default off, which could allow a guest to use unsupported
features and lead to hardware traps or hypervisor state corruption.

[ ... ]
> +bool cpus_have_final_cap(unsigned int num)
> +{
> +	u64 reg_val;
> +
> +	switch (num) {
[ ... ]
> +	case ARM64_HAS_RASV1P1_EXTN:
> +		reg_val = read_sanitised_ftr_reg(SYS_ID_AA64PFR1_EL1);
> +		return SYS_FIELD_GET(ID_AA64PFR1_EL1, RAS_frac, reg_val) ==
> +		       ID_AA64PFR1_EL1_RAS_frac_RASv1p1;

[Severity: High]
Is this missing the primary check for the RAS extension?

The check looks exclusively at the fractional extension field RAS_frac in
ID_AA64PFR1_EL1, but ignores the primary ID_AA64PFR0_EL1.RAS field. On
modern CPUs that correctly advertise V1P1 support in ID_AA64PFR0_EL1, this
will return false and break feature discovery.

> +
> +	case ARM64_HAS_HCR_NV1:
> +		reg_val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR2_EL1);
> +		return SYS_FIELD_GET(ID_AA64MMFR2_EL1, NV, reg_val) >=
> +		       ID_AA64MMFR2_EL1_NV_IMP;

[Severity: High]
Will this misidentify NV2-only systems as having NV1 support?

This check verifies if Nested Virtualization is implemented but ignores
the ID_AA64MMFR4_EL1.E2H0 field. On newer hosts that implement NV2 but
explicitly remove NV1 (such as the Apple M2 series), this function will
return true and attempt to use unsupported NV1 features.

[ ... ]
> +	case ARM64_HAS_ECV_CNTPOFF:
> +		reg_val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> +		return SYS_FIELD_GET(ID_AA64MMFR1_EL1, PAN, reg_val) >=
> +		       ID_AA64MMFR1_EL1_PAN_IMP;

[Severity: High]
Does this check query the wrong register and field for Enhanced Counter
Virtualization (ECV)?

It is querying ID_AA64MMFR1_EL1.PAN instead of ID_AA64MMFR0_EL1.ECV. Because
PAN is widely supported on modern ARM64 CPUs, KVM will incorrectly assume
ECV is present, potentially causing undefined instruction exceptions or
timer misbehavior if those features are used.

> diff --git a/arch/s390/kvm/arm64/feature.h b/arch/s390/kvm/arm64/feature.h
> new file mode 100644
> index 0000000000000..9c902df1f26c6
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/feature.h
> @@ -0,0 +1,87 @@
[ ... ]
> +static inline u32 kvm_sae_supported_sd_formats(void)
> +{
> +	extern struct qaaf_qmc_block __qaaf_qmp;
> +
> +	return __qaaf_qmp.ssdf;
> +}

[Severity: Low]
Is this referencing an undefined symbol?

The symbol __qaaf_qmp does not exist anywhere in the kernel (the actual
structure seems to be exposed via aef_qmc()). While this inline function
is currently dead code, calling it will cause a linker error.

[ ... ]
> +static inline u16 kvm_sae_max_vcpus(void)
> +{
> +	extern struct qaaf_qmc_block __qaaf_qmp;
> +
> +	/* QAAF QMP reports the max id not the max num */
> +	return __qaaf_qmp.maxncpu + 1;
> +}

[Severity: Low]
Can this result in an integer overflow?

The maxncpu field can reach 0xFFFF. Returning maxncpu + 1 cast to a u16
will overflow the 16-bit integer boundary and wrap around to 0. (Also, as
noted above, __qaaf_qmp is an unresolved symbol).

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