Re: [PATCH v2 13/20] KVM: s390: arm64: Implement arm sysreg managing infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The s390-specific definition of `__vcpu_rmw_sys_reg` incorrectly swaps the register ID (`R`) and value (`V`) parameters compared to the shared ARM64 implementation.
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) race with vCPU preemption and illegal use of `smp_processor_id()` in a preemptible context causes a host `BUG_ON` panic.
- [High] Missing trap handler for `MIDR_EL1` causes guest crashes with an UNDEF exception on boot.
- [High] Guest-reachable `WARN(true)` in `kvm_handle_sys_reg` allows a malicious guest to spam the host kernel log.
--

commit 1e6a91a5668b6ea306fd97aa5d57b2154d91120f
Author: Steffen Eiden <seiden@linux.ibm.com>
KVM: s390: arm64: Implement arm sysreg managing infrastructure

This commit implements the infrastructure for the s390 host to manage arm64
guest system registers. It allows the host to read and write non-ID system
registers using new EASR/SASR instructions, while trapping and handling guest
accesses to ID registers.

> diff --git a/arch/s390/include/asm/kvm_host_arm64.h b/arch/s390/include/asm/kvm_host_arm64.h
> --- a/arch/s390/include/asm/kvm_host_arm64.h
> +++ b/arch/s390/include/asm/kvm_host_arm64.h
> @@ -221,4 +244,183 @@ static inline void kvm_arch_async_page_present_queued(struct kvm_vcpu *vcpu)
>  
>  #define kvm_supports_32bit_el0() false
>  
> +#define __vcpu_sys_reg(__vcpu, __reg) \
> +	vcpu_read_sys_reg(__vcpu, __reg)
> +
> +#define __vcpu_assign_sys_reg(__vcpu, __reg, __val) \
> +	vcpu_write_sys_reg(__vcpu, __val, __reg)
> +
> +#define __vcpu_rmw_sys_reg(C, V, OP, R)		\
> +({						\
> +	u64 __val = vcpu_read_sys_reg(C, R);	\
> +	__val OP V;				\
> +	vcpu_write_sys_reg(C, __val, R);	\
+})

[Severity: Critical]
Does this override transpose the register ID and value arguments compared to the
shared ARM64 implementation?

The shared ARM64 code expects this macro to receive (vcpu, reg, op, val).
This s390 override defines it as (C, V, OP, R). Any read-modify-write system
register operation using this macro (for instance, PMU updates) will attempt to
read from a register ID equal to the bitmask value, and use the actual register
ID as the bitmask, causing data corruption and host crashes.

> +/**
> + * _vcpu_read_sys_reg() - read a guest sysreg with easr
> + * - R - sysreg id; must be readable by easr; must be compile time constant
> + *
> + *   if SYSREGS_ON_CPU: proceed with flags = 0
> + *   otherwise:         proceed with either
> + *                         read:  flags = EASR_FLAG_SA
> + *                         write: flags = SASR_FLAG_INITIALIZED
> + *
> + */
> +#define _vcpu_read_sys_reg(C, R) \
> +	({	BUILD_BUG_ON(!__builtin_constant_p((R))); \
> +		BUG_ON(vcpu_is_loaded(C) && smp_processor_id() != (C)->cpu); \
> +		(vcpu_is_loaded(C)) \
> +			? __vcpu_read_sr((C), (R), 0) \
> +			: __vcpu_read_sr((C), (R), EASR_FLAG_SA); })
> +
> +/**
> + * _vcpu_write_sys_reg() - write a guest sysreg with sasr
> + * - R - sysreg id; must be readable by sasr; must be compile time constant
> +
> + *   if SYSREGS_ON_CPU: proceed with flags = 0
> + *   otherwise:         proceed with either
> + *                         read:  flags = EASR_FLAG_SA
> + *                         write: flags = SASR_FLAG_INITIALIZED
> + */
> +#define _vcpu_write_sys_reg(C, V, R) \
> +	({	BUILD_BUG_ON(!__builtin_constant_p((R))); \
> +		BUG_ON(vcpu_is_loaded(C) && smp_processor_id() != (C)->cpu); \
> +		(vcpu_is_loaded(C)) \
> +			? __vcpu_write_sr((C), (V), (R), 0) \
> +			: __vcpu_write_sr((C), (V), (R), SASR_FLAG_INITIALIZED); })

[Severity: Critical]
Is it safe to call smp_processor_id() in these macros?

These macros are invoked by kvm_handle_sys_reg(), which executes in a
preemptible context. Calling smp_processor_id() here will trigger a kernel
warning.

Furthermore, if the vCPU thread is preempted and migrated between evaluating
smp_processor_id() and checking (C)->cpu, this BUG_ON() will panic the host. A
guest could repeatedly trigger sysreg exits to intentionally cause a host denial
of service.

[ ... ]

> diff --git a/arch/s390/kvm/arm64/sys_regs.c b/arch/s390/kvm/arm64/sys_regs.c
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/sys_regs.c
> @@ -0,0 +1,750 @@

[ ... ]

> +static const struct sys_reg_desc sys_reg_descs[] = {
> +	/* Op0 = 2 */
> +	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> +	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_oslsr_el1, OSLSR_EL1,
> +	  OSLSR_EL1_OSLM_IMPLEMENTED, NULL, set_oslsr_el1 },
> +
> +	/* Op0 = 3 */
> +	/* Op1 = 0 */
> +	/* CRn = 0 */
> +	/* CRm = 0 */
> +
> +	{ SYS_DESC(SYS_MIDR_EL1), NULL, reset_midr, 0, GENMASK_ULL(31, 0),
> +	  get_id_reg, set_imp_id_reg },

[Severity: High]
Does this missing access handler cause guest boot failures?

The access handler here is set to NULL instead of access_imp_id_reg. When a
guest reads MIDR_EL1 (which Linux does early in boot), kvm_handle_sys_reg() uses
perform_access(). Since the handler is NULL, this will fall back to bad_trap(),
injecting an undefined instruction exception into the guest and printing a
WARN_ONCE on the host.

[ ... ]

> +int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
> +{
> +	const struct sys_reg_desc *desc = NULL;
> +	struct sys_reg_params params;
> +	unsigned long esr = kvm_vcpu_get_esr(vcpu);
> +	int Rt = kvm_vcpu_sys_get_rt(vcpu);
> +
> +	trace_kvm_handle_sys_reg(esr);
> +
> +	params = esr_sys64_to_params(esr);
> +	params.regval = vcpu_get_reg(vcpu, Rt);
> +
> +	/* System registers have Op0=={2,3}, as per DDI487 J.a C5.1.2 */
> +	if (params.Op0 == 2 || params.Op0 == 3)
> +		desc = find_reg(&params, sys_reg_descs, num_sys_reg_descs);
> +	else
> +		WARN(true, "system instruction handling not supported");

[Severity: High]
Can a malicious guest trigger this warning to flood the host kernel log?

If a guest intentionally executes a trapped cache maintenance instruction
(like DC CIVAC), it generates an exception with Op0 == 1. This hits the
unconditional WARN() here, violating KVM guidelines against guest-reachable
assertions and allowing a denial of service via log spam.

[ ... ]

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