Re: [PATCH 1/2] KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Numeric range iteration over sparse `enum vcpu_sysreg` values.
- [High] TOCTOU race allows userspace to modify VM ID registers after initialization has begun, causing inconsistent KVM state.
--

--- Patch [1]: [PATCH 1/2] KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs ---
commit 2366ff54838e2cdbf8037630978a72215c68a9b6
Author: Mark Brown <broonie@kernel.org>

KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs
    
In commit d82d09d5ba4b ("KVM: arm64: Don't skip per-vcpu NV
initialisation") the NV register sanitisation was moved earlier in
kvm_finalize_sys_regs() so that it runs for each vCPU rather than only
once per guest. This means that for the first vCPU it runs prior to vGIC
finalization, but the vGIC finalization updates the ID registers which
the NV initialization uses so we may end up with a mismatch. For
example, HFGRTR_EL2.ICC_IGRPENn_EL1 depends on GICv3 being enabled in
ID_AA64PFR0_EL1.GIC so may be mistakenly marked or not marked as RES0.

Split the initialization which runs once per guest into a separate
function and run that before the per-vCPU initialisation for NV,
renaming the per-vCPU function to make it clear that it does per-vCPU
setup.

Fixes: d82d09d5ba4b ("KVM: arm64: Don't skip per-vcpu NV initialisation")

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 50adfff75be82..2b75e1d5ca8db 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -931,7 +931,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  			return ret;
>  	}
>  
> -	ret = kvm_finalize_sys_regs(vcpu);
> +	ret = kvm_vcpu_finalize_sys_regs(vcpu);
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does dropping the config_lock here leave a
window where userspace can still modify VM ID registers?

Since kvm_vcpu_finalize_sys_regs() takes and then drops
kvm->arch.config_lock, and KVM_ARCH_FLAG_HAS_RAN_ONCE is not set until
much later in kvm_arch_vcpu_run_pid_change(), a concurrent secondary
vCPU thread could invoke KVM_SET_ONE_REG to modify an ID register.

The ioctl handler set_id_reg() in sys_regs.c permits the write because
kvm_vm_has_ran_once(kvm) is still false:

	if (kvm_vm_has_ran_once(vcpu->kvm)) {
		if (val != read_id_reg(vcpu, rd))
			ret = -EBUSY;

Could this lead to inconsistent internal feature configuration between
sysreg_masks set in kvm_init_nv_sysregs and traps computed in
kvm_calculate_traps?

[ ... ]
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5d5c579d45790..958d7ef78785f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
[ ... ]
> @@ -5825,6 +5814,29 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
> +/*
> + * Because this can be called once per CPU, changes must be idempotent.
> + */
> +int kvm_vcpu_finalize_sys_regs(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	int ret;
> +
> +	guard(mutex)(&kvm->arch.config_lock);
> +
> +	ret = kvm_vm_finalize_sys_regs(kvm);
> +	if (ret)
> +		return ret;
> +
> +	if (vcpu_has_nv(vcpu)) {
> +		ret = kvm_init_nv_sysregs(vcpu);

[Severity: High]
This isn't a bug introduced by this patch, but does the function called here
iterate over the sparse enum vcpu_sysreg using a numeric range?

Looking at kvm_init_nv_sysregs() in arch/arm64/kvm/nested.c:

	for (enum vcpu_sysreg sr = __SANITISED_REG_START__; sr < NR_SYS_REGS; sr++)
		__vcpu_rmw_sys_reg(vcpu, sr, |=, 0);

Iterating over a numeric range of enum vcpu_sysreg values traverses gaps
in the enumeration space, as VNCR-mapped entries are sparsely numbered by
their byte offsets. Does this unconditionally traverse sparse gaps and
silently touch unintended offsets, leading to state desynchronization?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-kvm-arm64-idreg-final-v1-0-3de2a5616dc6@kernel.org?part=1