Re: [PATCH v19 01/20] KVM: arm64: protected VM: Handle user writes to CNTVCT_EL0/CNTPCT_EL0

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Tue, 22 Sep 2026 23:04:42 +0100
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:

> On 22/09/2026 20:25, Jonathan Cameron wrote:
> > On Sun, 20 Sep 2026 22:28:26 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > 
> > Hi Suzuki,
> >   
> >> Protected VMs doesn't allow setting offsets for virtual and phyiscal  
> > 
> > physical
> >   
> >> counters, as the offset is always fixed to 0. The VM ioclt is filtered  
> > 
> > ioctl
> >   
> >> out based on the cap. However we don't prevent the userspace from trying
> >> to write to the CNTVCT/CNTPCT registers. This would lead to KVM triggering
> >> a WARN() in timer_set_offset() as the vm_offset pointer is set to NULL.
> >>
> >> Fix this by always "fixing" the timer offsets to 0 and marking that the
> >> timer offset is set in the kvm->arch.flags at KVM init time for protected
> >> VMs. A userspace writing to the CNT*CT_EL0 would observe success, without
> >> any real effect. This was chosen over preventing the writes to these
> >> registers and returning -EPERM.  
> > 
> > Why?  I don't mind the decision but telling us what was chosen is something
> > we can see in the code - patch description should give us the stuff we
> > can't see.
> > 
> > One comment on the comment below.  
> >>
> >> Reported by Sashiko
> >>
> >> Link: https://lore.kernel.org/all/20260908164641.416911F00A3A@smtp.kernel.org
> >> Fixes: f7d05ee84a6a ("KVM: arm64: Prevent host from managing timer offsets for protected VMs")
> >> Suggested-by: Marc Zyngier <maz@kernel.org>
> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> ---
> >> Changes since v18:
> >>   - Retain NULL vm_offset for protected VMs to avoid host tampering with the
> >>     offset.
> >>   - Moved the flag setting into kvm_timer_init_vm(), where it should have been
> >>     in the first place
> >> ---
> >>   arch/arm64/kvm/arch_timer.c | 12 ++++++++++--
> >>   1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >> index 6ac3321f4c575..226cd5a495c8b 100644
> >> --- a/arch/arm64/kvm/arch_timer.c
> >> +++ b/arch/arm64/kvm/arch_timer.c
> >> @@ -1110,8 +1110,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
> >>   		timer_context_init(vcpu, i);
> >>   
> >>   	/* Synchronize offsets across timers of a VM if not already provided */
> >> -	if (!vcpu_is_protected(vcpu) &&
> >> -	    !test_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &vcpu->kvm->arch.flags)) {
> >> +	if (!test_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &vcpu->kvm->arch.flags)) {
> >>   		timer_set_offset(vcpu_vtimer(vcpu), kvm_phys_timer_read());
> >>   		timer_set_offset(vcpu_ptimer(vcpu), 0);
> >>   	}
> >> @@ -1133,6 +1132,15 @@ void kvm_timer_init_vm(struct kvm *kvm)
> >>   	 */
> >>   	for (int i = 0; i < NR_KVM_TIMERS; i++)
> >>   		kvm->arch.timer_data.ppi[i] = get_vgic_ppi(kvm, default_ppi[i]);
> >> +
> >> +	/*
> >> +	 * Protected VMs don't allow any offset being set from userspace,
> >> +	 * either set via writes to the counters or using the dedicated  
> > This sentence confused me.  Second clause isn't obviously the ways that
> > are being blocked. Maybe shorten to:
> > 
> > 	Protected VMs don't allow the offset to be set from userspace,
> > 	whether via writes to the counters or the dedicated ioctl.  
> 
> LLM suggests:
> 
> 
>     * Protected VMs don't allow userspace to set counter offsets,
>     * either via counter register writes or the dedicated ioctl.

Hmm.  I'd not combine a negative with either.  Still good enough I think!
Our Booker prize may be a way off yet :)

> 
> Have updated the comment.
> 
> Cheers
> Suzuki
> 
> >   
> >> +	 * ioctl. Pretend the offset has already been set and rely on the
> >> +	 * default offset being 0.
> >> +	 */
> >> +	if (kvm_vm_is_protected(kvm))
> >> +		set_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &kvm->arch.flags);
> >>   }
> >>   
> >>   void kvm_timer_cpu_up(void)  
> >   
>