Re: [PATCH 45/60] kvm: x86: Update state for all plane VCPUs after CPUID update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `kvm_check_cpuid()` is called solely on the target `vcpu` outside the `vcpu_for_each_plane` loop, failing to expand the FPU state buffer (`guest_fpu`) for secondary vCPUs.
- [High] `kvm_hv_vcpu_init()` is incorrectly called on the target `vcpu` instead of the loop variable `v` within the `vcpu_for_each_plane` loop, leaving sibling vCPUs' Hyper-V state uninitialized.
- [High] Bypassing KVM_RUN protection on CPUID updates leads to plane-wide state corruption and `KVM_BUG_ON` crashes because `kvm_can_set_cpuid_and_feature_msrs` is only evaluated on the target `vcpu`.
--

commit 456fecfe1475933e67e33e9b01b14d9e0c1dc102
Author: Joerg Roedel <joerg.roedel@amd.com>

Make sure to update CPUID dependent state for all VCPUs of a given
plane when CPUID state is updated.

This commit modifies KVM to update CPUID-dependent state across all vCPUs
in a plane when a CPUID update occurs. It iterates over sibling vCPUs to
apply Hyper-V initialization, Xen CPUID updates, and general post-CPUID
state synchronization.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 27e2f7e25038..fab075bb6fdc 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -513,6 +513,8 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  {
>  	struct kvm_vcpu_common *common = vcpu->common;
>  	u32 vcpu_caps[NR_KVM_CPU_CAPS];
> +	struct kvm_vcpu *v;
> +	unsigned i;
>  	int r;
>  
>  	/*

[Severity: High]
Does this plane-wide update bypass the run protection for sibling vCPUs?

The immutability gate kvm_can_set_cpuid_and_feature_msrs() is checked only
against the target vcpu before this newly added plane-wide update.

Could userspace execute the first vCPU, and then successfully call
KVM_SET_CPUID on an un-run second vCPU?

This would apply the CPUID change to the entire plane and invoke:
kvm_vcpu_after_set_cpuid()
  kvm_mmu_after_set_cpuid()

on the already-run first vCPU, which then triggers a KVM_BUG_ON().

[ ... ]

> @@ -562,9 +564,11 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  
>  #ifdef CONFIG_KVM_HYPERV
>  	if (kvm_cpuid_has_hyperv(vcpu)) {
> -		r = kvm_hv_vcpu_init(vcpu);
> -		if (r)
> -			goto err;
> +		vcpu_for_each_plane(common, i, v) {
> +			r = kvm_hv_vcpu_init(vcpu);

[Severity: High]
Does this loop initialize the target vcpu rather than the iterator v?

This leaves v->arch.hyperv unallocated for secondary vCPUs.

When kvm_vcpu_after_set_cpuid() later invokes kvm_hv_set_cpuid(), can it
trigger a WARN_ON_ONCE if an unprivileged host-userspace process triggers
this path?

> +			if (r)
> +				goto err;
> +		}
>  	}
>  #endif
>  
>  	r = kvm_check_cpuid(vcpu);
>  	if (r)
>  		goto err;

[Severity: Critical]
Is kvm_check_cpuid() expected to be called on all siblings?

Because kvm_check_cpuid() handles dynamic reallocation of the guest_fpu
buffer for XFD features like AMX, calling it only on the target vcpu means
sibling vCPUs miss the expansion.

Could this lead to a host kernel heap out-of-bounds write when the hardware
xsave operation stores expanded FPU state into the unexpanded buffer of a
sibling?

> +	vcpu_for_each_plane(vcpu->common, i, v) {
>  #ifdef CONFIG_KVM_XEN
> -	vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE);
> +		v->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE);
>  #endif
> -	kvm_vcpu_after_set_cpuid(vcpu);
> +		kvm_vcpu_after_set_cpuid(v);
> +	}
>  
>  success:
>  	kvfree(e2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=45