Re: [PATCH v18 01/23] KVM: arm64: protected VM: Handle set_one_reg CNTVCT_EL0/CNTPCT_EL0

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

On 15/09/2026 17:46, Marc Zyngier wrote:
> On Tue, 15 Sep 2026 17:01:19 +0100,
> Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Protected VMs doesn't allow setting offsets for virtual and phyiscal
>> counters, as the offset is always fixed to 0. The VM ioclt is filtered
>> 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 pKVM init time. The
>> userspace cannot use the KVM_ARM_SET_COUNTER_OFFSET, as it is blocked for a
>> protected VM.
>>
>> A userspace writing to the SYS_CNT*CT would observe success, without
>> any real effect. This was chosen over preventing the writes to these
>> registers and returning -EPERM.
>>
>> With that, we always have a valid vm_offset pointer, remove the checks for
>> vm_offset == NULL.
>>
>> Reported by Sashiko here
>> 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@krenel.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   arch/arm64/kvm/arch_timer.c    | 15 +++++----------
>>   arch/arm64/kvm/arm.c           | 15 +++++++++++++++
>>   arch/arm64/kvm/hyp/nvhe/pkvm.c | 26 +++++++++++++-------------
>>   include/kvm/arm_arch_timer.h   |  3 +--
>>   4 files changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>> index 6ac3321f4c575..dda020da4c9c7 100644
>> --- a/arch/arm64/kvm/arch_timer.c
>> +++ b/arch/arm64/kvm/arch_timer.c
>> @@ -1079,14 +1079,10 @@ static void timer_context_init(struct kvm_vcpu *vcpu, int timerid)
>>   
>>   	ctxt->timer_id = timerid;
>>   
>> -	if (!kvm_vm_is_protected(vcpu->kvm)) {
>> -		if (timerid == TIMER_VTIMER)
>> -			ctxt->offset.vm_offset = &kvm->arch.timer_data.voffset;
>> -		else
>> -			ctxt->offset.vm_offset = &kvm->arch.timer_data.poffset;
>> -	} else {
>> -		ctxt->offset.vm_offset = NULL;
>> -	}
>> +	if (timerid == TIMER_VTIMER)
>> +		ctxt->offset.vm_offset = &kvm->arch.timer_data.voffset;
>> +	else
>> +		ctxt->offset.vm_offset = &kvm->arch.timer_data.poffset;
>>   
>>   	hrtimer_setup(&ctxt->hrtimer, kvm_hrtimer_expire, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD);
>>   
>> @@ -1110,8 +1106,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);
>>   	}
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 8b080804bc90b..7c88508cac8a1 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -214,6 +214,20 @@ static int kvm_arm_default_max_vcpus(void)
>>   	return vgic_present ? kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
>>   }
>>   
>> +/*
>> + * Fix the counter offset to 0 for Protected VMs and mark the
>> + * offset flag. The user can't set the offset via KVM_ARM_SET_COUNTER_OFFSET.
>> + */
>> +static void kvm_arch_fix_timer_offsets(struct kvm *kvm)
>> +{
>> +	if (!kvm_vm_is_protected(kvm))
>> +		return;
>> +
>> +	/* Fix the counter offset to 0 and mark the offset initialised */
>> +	kvm->arch.timer_data.poffset = kvm->arch.timer_data.voffset = 0;
>> +	set_bit(KVM_ARCH_FLAG_VM_COUNTER_OFFSET, &kvm->arch.flags);
>> +}
>> +
>>   /**
>>    * kvm_arch_init_vm - initializes a VM data structure
>>    * @kvm:	pointer to the KVM struct
>> @@ -267,6 +281,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>   
>>   	kvm_vgic_early_init(kvm);
>>   
>> +	kvm_arch_fix_timer_offsets(kvm);
>>   	kvm_timer_init_vm(kvm);
> 
> This should all be moved to the timer code.
> 
>>   
>>   	/* The maximum number of VCPUs is limited by the host's GIC model */
>> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
>> index 459bd9eb7e4bc..e7b38eff63bd1 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
>> @@ -528,19 +528,19 @@ static int init_pkvm_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu,
>>   	hyp_vcpu->vcpu.arch.cflags = READ_ONCE(host_vcpu->arch.cflags);
>>   	hyp_vcpu->vcpu.arch.mp_state.mp_state = KVM_MP_STATE_STOPPED;
>>   
>> -	if (!pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
>> -		/*
>> -		 * Timer offsets are pointing to the untrusted KVM copy,
>> -		 * which is pinned in __pkvm_init_vm() for the VM life time.
>> -		 * It is worth noting that hyp_vm->host_kvm points to an EL2
>> -		 * linear map address and timer_get_offset() will use
>> -		 * kern_hyp_va() which is safe as it is idempotent.
>> -		 */
>> -		vcpu_vtimer(&hyp_vcpu->vcpu)->offset.vm_offset =
>> -			&hyp_vm->host_kvm->arch.timer_data.voffset;
>> -		vcpu_ptimer(&hyp_vcpu->vcpu)->offset.vm_offset =
>> -			&hyp_vm->host_kvm->arch.timer_data.poffset;
>> -	}
>> +	/*
>> +	 * Timer offsets are pointing to the untrusted KVM copy,
>> +	 * which is pinned in __pkvm_init_vm() for the VM life time.
>> +	 * It is worth noting that hyp_vm->host_kvm points to an EL2
>> +	 * linear map address and timer_get_offset() will use
>> +	 * kern_hyp_va() which is safe as it is idempotent.
>> +	 * Also for protected VMs the offset is fixed to 0 and is prevented
>> +	 * from changing.
>> +	 */
>> +	vcpu_vtimer(&hyp_vcpu->vcpu)->offset.vm_offset =
>> +		&hyp_vm->host_kvm->arch.timer_data.voffset;
>> +	vcpu_ptimer(&hyp_vcpu->vcpu)->offset.vm_offset =
>> +		&hyp_vm->host_kvm->arch.timer_data.poffset;
> 
> I don't think this is right. Protected guests have no offset, and this
> needs to be ensured by the hypervisor. Here, the host can change the
> offset any time it wants, and that's not acceptable.

Ah, you're right. :facepalm:

> 
>>   
>>   	ret = pkvm_vcpu_init_sysregs(hyp_vcpu);
>>   	if (ret)
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index bc6f2fdd7ad33..4f0aa3bb69f45 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -176,8 +176,7 @@ static inline bool has_cntpoff(void)
>>   		if (__ctxt) {						\
>>   			struct arch_timer_offset *ato = &__ctxt->offset;\
>>   									\
>> -			if (ato->vm_offset)				\
>> -				off += *KERN_HYP_VA(ato->vm_offset);	\
>> +			off += *KERN_HYP_VA(ato->vm_offset);		\
>>   			if (ato->vcpu_offset)				\
>>   				off += *KERN_HYP_VA(ato->vcpu_offset);	\
>>   		}							\
> 
> And as you drop the previous hunk, this also needs to be restored to
> its original state.

Ack.

Suzuki


> 
> 	M.
>