Re: [PATCH] KVM: arm64: Fix hvhe and broken CNTVOFF_EL2

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Virtual timer traps are inadvertently disabled for non-protected guests under pKVM on hardware with broken CNTVOFF_EL2, breaking their configured timer offsets.

Pre-existing issues:
- [High] `__timer_enable_traps()` uses the global `is_protected_kvm_enabled()` check and an unsynced `poffset`, unconditionally disabling physical timer traps for non-protected VMs under pKVM.
--

--- Patch [1]: [PATCH] KVM: arm64: Fix hvhe and broken CNTVOFF_EL2 ---
commit d5e405a74fba9d73744f97970471012a4473ce3c
Author: Mostafa Saleh <[email protected]>
Subject: KVM: arm64: Fix hvhe and broken CNTVOFF_EL2

The commit fixes a guest kernel panic when launching a protected VM with
protected hvhe mode on hardware with a broken CNTVOFF_EL2. It achieves
this by conditionally enabling virtual timer traps only when the guest
actually has a non-zero virtual timer offset.

> diff --git a/arch/arm64/kvm/hyp/nvhe/timer-sr.c b/arch/arm64/kvm/hyp/nvhe/timer-sr.c
> index ff176f4ce7deb..98b6e37ee8fa4 100644
> --- a/arch/arm64/kvm/hyp/nvhe/timer-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/timer-sr.c
[ ... ]

[Severity: High]
This is a pre-existing issue, but does the physical timer trap logic earlier
in __timer_enable_traps() improperly grant all guests under pKVM access to
the physical timer?

Looking at the physical timer access check in this function:

	if (is_protected_kvm_enabled() ||
	    !kern_hyp_va(vcpu->kvm)->arch.timer_data.poffset)
		set |= CNTHCTL_EL1PCTEN;

The is_protected_kvm_enabled() static key reflects host pKVM support and
applies to all guests. Additionally, the fallback poffset check evaluates to
true under pKVM because the EL2-private hyp_vm->kvm is never populated with
the host's poffset during init_pkvm_hyp_vm().

Can this cause non-protected VMs on older hardware without CNTPOFF_EL2 to
bypass physical timer offset emulation and read the raw physical timer
directly?

> @@ -63,7 +64,7 @@ void __timer_enable_traps(struct kvm_vcpu *vcpu)
>  	 * Trap the virtual counter/timer if we have a broken cntvoff
>  	 * implementation.
>  	 */
> -	if (has_broken_cntvoff())
> +	if (has_broken_cntvoff() && hyp_timer_get_offset(vcpu_vtimer(vcpu)))

[Severity: High]
Could this condition inadvertently disable virtual timer traps for
non-protected guests under pKVM?

The new check evaluates hyp_timer_get_offset(vcpu_vtimer(vcpu)). However,
under pKVM (nVHE), the EL2-private pkvm_hyp_vcpu structure's timer_cpu state
is never synchronized from the host vCPU during init_pkvm_hyp_vcpu() or
flush_hyp_vcpu().

Since ctxt->offset.vm_offset remains NULL, hyp_timer_get_offset() will
unconditionally return 0. Will this allow any non-protected guest with a
configured virtual timer offset to bypass the trap and incorrectly observe an
offset of 0?

>  		set |= CNTHCTL_EL1TVT | CNTHCTL_EL1TVCT;
>  
>  	sysreg_clear_set(cnthctl_el2, clr, set);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1