linux-kernel - Re: [PATCH v2 18/49] KVM: x86: Account for max supported CPUID leaf when getting raw host CPUID

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c7025e6558d9344e10cda6ccc3614e1cdc1b43b.camel@redhat.com>
Date: Thu, 04 Jul 2024 21:17:59 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
 <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>, Oliver Upton
 <oliver.upton@...ux.dev>, Binbin Wu <binbin.wu@...ux.intel.com>, Yang
 Weijiang <weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 18/49] KVM: x86: Account for max supported CPUID leaf
 when getting raw host CPUID

On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> Explicitly zero out the feature word in kvm_cpu_caps if the word's
> associated CPUID function is greater than the max leaf supported by the
> CPU.  For such unsupported functions, Intel CPUs return the output from
> the last supported leaf, not all zeros.
> 
> Practically speaking, this is likely a benign bug, as KVM uses the raw
> host CPUID to mask the kernel's computed capabilities, and the kernel does
> perform max leaf checks when populating boot_cpu_data.  The only way KVM's
> goof could be problematic is if the kernel force-set a feature in a leaf
> that is completely unsupported, _and_ the max supported leaf happened to
> return a value with '1' the same bit position.  Which is theoretically
> possible, but extremely unlikely.  And even if that did happen, it's
> entirely possible that KVM would still provide the correct functionality;
> the kernel did set the capability after all.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a51e48663f53..77625a5477b1 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -571,18 +571,37 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid)
> +{
> +	struct kvm_cpuid_entry2 entry;
> +	u32 base;
> +
> +	/*
> +	 * KVM only supports features defined by Intel (0x0), AMD (0x80000000),
> +	 * and Centaur (0xc0000000).  WARN if a feature for new vendor base is
> +	 * defined, as this and other code would need to be updated.
> +	 */
> +	base = cpuid.function & 0xffff0000;
> +	if (WARN_ON_ONCE(base && base != 0x80000000 && base != 0xc0000000))
> +		return 0;
> +
> +	if (cpuid_eax(base) < cpuid.function)
> +		return 0;
> +
> +	cpuid_count(cpuid.function, cpuid.index,
> +		    &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
> +
> +	return *__cpuid_entry_get_reg(&entry, cpuid.reg);
> +}
> +
>  /* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */
>  static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf)
>  {
>  	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
> -	struct kvm_cpuid_entry2 entry;
>  
>  	reverse_cpuid_check(leaf);
>  
> -	cpuid_count(cpuid.function, cpuid.index,
> -		    &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
> -
> -	kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg);
> +	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);
>  }
>  
>  static __always_inline

Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
Best regards,
	Maxim Levitsky