Re: [PATCH v2 10/28] KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the split-state preservation for `CNTHCTL_EL2` on FEAT_NV2p1 systems permanently corrupts the guest's in-memory ECV bit and leaks host-specific trap overrides to userspace.
--

--- Patch [10]: [PATCH v2 10/28] KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present ---
commit e272e43dc6ce85d3337902d78430aad6bf5b83fc
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: Relax CNTHCTL_EL2 handling when FEAT_NV2p1 is present
    
With NV2p1, it is no longer necessary to use the split approach
where bits of CNTHCTL_EL2 cannot be accessed via CNTKCTL_EL1,
and we can treat the CNTKCTL_EL1 accessor as if it was "normal".

> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4155fe89b58a1..db60facad9f3c 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c

[ ... ]

> @@ -876,8 +876,14 @@ static void timer_set_traps(struct kvm_vcpu *vcpu, struct timer_map *map)
>  	assign_clear_set_bit(tvt02, CNTHCTL_EL1NVVCT, clr, set);
>  	assign_clear_set_bit(tpt02, CNTHCTL_EL1NVPCT, clr, set);
>  
> -	/* This only happens on VHE, so use the CNTHCTL_EL2 accessor. */
> -	sysreg_clear_set(cnthctl_el2, clr, set);
> +	/*
> +	 * This only happens on VHE, so use the CNTHCTL_EL2 accessor, unless
> +	 * we are sure CNTKCTL_EL1 is completely stateful with FEAT_NV2p1.
> +	 */
> +	if (!cpus_have_final_cap(ARM64_HAS_NV2P1))
> +		sysreg_clear_set(cnthctl_el2, clr, set);
> +	else
> +		sysreg_clear_set(cntkctl_el1, clr, set);
>  }

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index 6f0f046e4ca4e..0c4ef1ce32ae7 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c

[ ... ]

> @@ -69,11 +69,18 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
>  		 * The EL1 view of CNTKCTL_EL1 has a bunch of RES0 bits where
>  		 * the interesting CNTHCTL_EL2 bits live. So preserve these
>  		 * bits when reading back the guest-visible value.
> +		 *
> +		 * While NV2p1 fixes some of that, it makes CNTHCTL_EL2.ECV
> +		 * even more broken than it already was with NV2.
>  		 */
>  		val = read_sysreg_el1(SYS_CNTKCTL);
> -		val &= CNTKCTL_VALID_BITS;
> -		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, &=, ~CNTKCTL_VALID_BITS);
> -		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, |=, val);
> +		if (!cpus_have_final_cap(ARM64_HAS_NV2P1)) {
> +			val &= CNTKCTL_VALID_BITS;
> +			__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, &=, ~CNTKCTL_VALID_BITS);
> +			__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, |=, val);
> +		} else {
> +			__vcpu_assign_sys_reg(vcpu, CNTHCTL_EL2, val);
> +		}

[Severity: High]
Does removing the split-state preservation for CNTHCTL_EL2 unconditionally
overwrite the guest's in-memory register state?

Since timer_set_traps() applies host-specific overrides to CNTKCTL_EL1 on the
host side, calling __vcpu_assign_sys_reg() here appears to blindly copy those
host-specific traps directly into the guest's architectural state on exit.

Furthermore, as the comment above notes that NV2p1 makes the ECV bit even more
broken, won't this assignment corrupt the guest's ECV state as well?

>  	}
>  
>  	__vcpu_assign_sys_reg(vcpu, SP_EL2,	 read_sysreg(sp_el1));

[ ... ]

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1dfc1f88bec82..9439c5b2b1fe8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c

[ ... ]

> @@ -322,8 +322,10 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg)
>  		switch (reg) {
>  		case CNTHCTL_EL2:
>  			val = read_sysreg_el1(SYS_CNTKCTL);
> -			val &= CNTKCTL_VALID_BITS;
> -			val |= __vcpu_sys_reg(vcpu, reg) & ~CNTKCTL_VALID_BITS;
> +			if (!cpus_have_final_cap(ARM64_HAS_NV2P1)) {
> +				val &= CNTKCTL_VALID_BITS;
> +				val |= __vcpu_sys_reg(vcpu, reg) & ~CNTKCTL_VALID_BITS;
> +			}
>  			return val;

[Severity: High]
Does this directly return the host-polluted physical register to userspace
during KVM_GET_ONE_REG?

Because the host-specific overrides from timer_set_traps() are present in
CNTKCTL_EL1, skipping the bitmasking on ARM64_HAS_NV2P1 systems seems to leak
these hypervisor-specific configurations to userspace. This could break live
migration by restoring an altered execution environment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714091641.1970822-1-maz@kernel.org?part=10