Re: [RFC PATCH 22/27] KVM: x86: Verify userspace CPUID inputs in paranoid mode

[Binbin Wu <binbin.wu@linux.intel.com>]

Below are responses to issues reported by sashiko. [1]
[1] https://sashiko.dev/#/patchset/20260417073610.3246316-1-binbin.wu%40linux.intel.com?part=22

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 08f5bc1d26b10..2027230a1f426 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
[ ... ]
> +static int cpuid_check_and_set_vcpu_caps(struct kvm_vcpu *vcpu,
> +					 struct kvm_cpuid_entry2 *entry)
> +{
[ ... ]
> +		/*
> +		 * A vCPU has a feature if it's supported by KVM and is enabled
> +		 * in guest CPUID.  Note, this includes features that are
> +		 * supported by KVM but aren't advertised to userspace!
> +		 */
> +		vcpu->arch.cpu_caps[leaf] = supported;
> +		vcpu->arch.cpu_caps[leaf] &= cpuid_get_reg_unsafe(entry, reg);
> +	}
> +
> +	return 0;
> +}
> +
> +int kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> +{
[ ... ]
> +	/*
> +	 * If CPUID paranoid mode is enabled, KVM rejects userspace's guest
> +	 * CPUID definition if it contains any bits that aren't supported or
> +	 * unknown by KVM.  Otherwise, reset guest capabilities to userspace's
> +	 * guest CPUID definition, i.e. honor userspace's definition for
> +	 * features that don't require KVM or hardware management/support (or
> +	 * that KVM simply doesn't care about).
> +	 */
> +	for (int i = 0; i < vcpu->arch.cpuid_nent; i++) {
> +		r = cpuid_check_and_set_vcpu_caps(vcpu, &vcpu->arch.cpuid_entries[i]);

Issue #1:
"KVM_SET_CPUID2 allows userspace to pass an array of CPUID entries which may
include duplicates. Because this loop iterates sequentially over the entries,
if duplicates are provided, won't vcpu->arch.cpu_caps reflect the
capabilities of the last matching entry in the array?
KVM's guest CPUID emulation uses kvm_find_cpuid_entry(), which searches the
array sequentially and returns the first matching entry. Could this cause a
state mismatch where KVM enforces access controls and emulates behaviors using
the capabilities defined in the last entry, but the guest software makes
decisions based on the first entry?"

Indeed.

I am not sure whether CPUID paranoid mode should reject userspace's input
if there are duplicated entries.
There could be two options:
- Detect whether a vcpu cap has been set, if there is a duplicated entry,
  reject the whole input or ignore the later one.
- Split the cpuid paranoid check out.


> +		/*
> +		 * No need to worry about the changes having been made if any
> +		 * check fails, all the changes will be reverted when returning
> +		 * an error on the set CPUID patch.
> +		 */
> +		if (r)
> +			return r;

Issue #2:
"The comment mentions all changes will be reverted on error, but is
vcpu->arch.xen.cpuid fully restored if we fail here?
When kvm_set_cpuid() updates vcpu->arch.xen.cpuid with the Xen base and limit
from the new entries, it happens before calling kvm_vcpu_after_set_cpuid().
If this paranoid check fails and returns -EINVAL, the error recovery in
kvm_set_cpuid() restores the old CPUID entries and cpu_caps, but it appears
to miss vcpu->arch.xen.cpuid.
Could this leave vcpu->arch.xen.cpuid out-of-sync with the restored CPUID
array, potentially allowing a malicious VMM to expose Xen-specific emulations
at a ghost CPUID leaf?"

It's the same issue as below:
https://lore.kernel.org/kvm/1931c7d3-b21e-4704-9012-4a16597bbebf@linux.intel.com/

>  	}
>  
>  	kvm_update_cpuid_runtime(vcpu);