Re: [RFC PATCH v7 20/28] KVM: arm64: Allow guest SPE physical timestamps only if kernel allows it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Inverted logic for `perf_allow_kernel()` grants physical timestamps to unprivileged users and denies them to privileged users.
- [High] Capability check `perf_allow_kernel()` is evaluated at `KVM_RUN` time (TOCTOU) instead of VM/VCPU configuration time.
- [Medium] Silently-on guest-visible feature (SPE physical timestamps) breaks live migration and violates subsystem guidelines.
- [Medium] Concurrent first `KVM_RUN` invocations on multiple vCPUs cause a data race on the VM-wide `kvm_spe->guest_pmscr_el2`.
--

commit 8640bfaf0b943972e8c97350b961b39b15830f1b
Author: Alexandru Elisei <alexandru.elisei@arm.com>

KVM: arm64: Allow guest SPE physical timestamps only if kernel allows it

The SPE driver allows userspace to use physical timestamps for records only
if the kernel allows it, do the same for a virtual machine.

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/vhe/spe-sr.c b/arch/arm64/kvm/hyp/vhe/spe-sr.c
> index 978620ad99fa5..3c32a1338cc8d 100644
> --- a/arch/arm64/kvm/hyp/vhe/spe-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/spe-sr.c

[ ... ]

> @@ -120,10 +121,10 @@ void __kvm_spe_restore_guest_state_vhe(struct kvm_vcpu *vcpu, struct kvm_cpu_con
>  		write_sysreg_s(pmbsr, SYS_PMBSR_EL1);
>  	}
>  
> -	__kvm_spe_restore_sampling_regs(vcpu->kvm, guest_ctxt);
> +	__kvm_spe_restore_sampling_regs(kvm, guest_ctxt);
>  
>  	write_sysreg_el1(ctxt_sys_reg(guest_ctxt, PMSCR_EL1), SYS_PMSCR);
> -	write_sysreg_el2(0, SYS_PMSCR);
> +	write_sysreg_el2(kvm->arch.kvm_spe.guest_pmscr_el2, SYS_PMSCR);

[Severity: Medium]
Can this read of kvm->arch.kvm_spe.guest_pmscr_el2 race with a concurrent
write from another vCPU?

If multiple vCPUs are starting their first KVM_RUN, one vCPU might complete
kvm_arch_vcpu_run_pid_change(), proceed to enter the guest, and execute
__kvm_spe_restore_guest_state_vhe(), reading guest_pmscr_el2.

Because kvm_spe_vcpu_first_run_init() performs unlocked writes to this
shared VM-wide variable, another vCPU could be modifying it at the same
time. Should this use READ_ONCE() and WRITE_ONCE() to prevent LKMM
violations and KCSAN data race splats?

>  }
>  NOKPROBE_SYMBOL(__kvm_spe_restore_guest_state_vhe);
>  
> diff --git a/arch/arm64/kvm/spe.c b/arch/arm64/kvm/spe.c
> index af1e19cb8e9ae..68b751bdb384b 100644
> --- a/arch/arm64/kvm/spe.c
> +++ b/arch/arm64/kvm/spe.c

[ ... ]

> @@ -97,6 +99,12 @@ int kvm_spe_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	if (!kvm_spe->pmblimitr_el1_res0_mask)
>  		kvm_spe_compute_pmblimitr_el1_res0_mask(kvm);
>  
> +	if (kvm_vm_has_ran_once(kvm))
> +		return 0;
> +
> +	if (perf_allow_kernel())
> +		kvm_spe->guest_pmscr_el2 = PMSCR_EL2_PCT_PHYS;

[Severity: High]
Does this logic invert the intended permission check?

The perf_allow_kernel() function returns 0 on success (access granted) and
a negative error code (like -EACCES) on failure. By evaluating to true when
access is denied, it seems we might be granting physical timestamps to
unprivileged users and incorrectly denying them to privileged users.

[Severity: High]
Is kvm_spe_vcpu_first_run_init() the right place to check these
capabilities?

Checking perf_allow_kernel() here means it is evaluated during the KVM_RUN
ioctl, rather than during VM configuration. VMMs typically configure VMs
with full privileges but drop privileges before executing KVM_RUN for
sandboxing. Checking at KVM_RUN time will incorrectly deny physical
timestamps to properly sandboxed VMMs.

Furthermore, an unprivileged process could configure a VM and pass the vCPU
file descriptor to a privileged process, improperly gaining access to
physical timestamps using the privileged thread's capabilities.

[Severity: Medium]
Could silently enabling SPE physical timestamps based on the host kernel
break live migration?

If a VM is migrated from a host where perf_allow_kernel() grants access to
one where it denies it, the guest would silently downgrade to virtual
timestamps, causing data corruption in the profiling trace.

The KVM guidelines require that new behavior the guest can observe must be
off by default and explicitly enabled through standard capability
negotiation (like a KVM_CAP_* or ID register flag). Should this require an
explicit opt-in from userspace instead?

[Severity: Medium]
Can this write to kvm_spe->guest_pmscr_el2 happen concurrently across
multiple vCPUs?

The kvm_vm_has_ran_once(kvm) flag is not set until the end of
kvm_arch_vcpu_run_pid_change(), which happens after
kvm_spe_vcpu_first_run_init() completes. If multiple vCPUs start their
first KVM_RUN concurrently, they will all see kvm_vm_has_ran_once(kvm) as
false and write to this shared variable without synchronization.

> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903160623.315525-1-alexandru.elisei@arm.com?part=20