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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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.

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)
>