linux-kernel - Re: [PATCH v3 50/57] KVM: x86: Replace (almost) all guest CPUID feature queries with cpu

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: <Z2DALDUcfgAHJi79@google.com>
Date: Mon, 16 Dec 2024 16:05:00 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, 
	Jarkko Sakkinen <jarkko@...nel.org>, kvm@...r.kernel.org, linux-sgx@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Maxim Levitsky <mlevitsk@...hat.com>, 
	Hou Wenlong <houwenlong.hwl@...group.com>, Xiaoyao Li <xiaoyao.li@...el.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 v3 50/57] KVM: x86: Replace (almost) all guest CPUID
 feature queries with cpu_caps

On Fri, Dec 13, 2024, Chao Gao wrote:
> On Wed, Nov 27, 2024 at 05:34:17PM -0800, Sean Christopherson wrote:
> >diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >index d3c3e1327ca1..8d088a888a0d 100644
> >--- a/arch/x86/kvm/cpuid.c
> >+++ b/arch/x86/kvm/cpuid.c
> >@@ -416,7 +416,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > 	 * and can install smaller shadow pages if the host lacks 1GiB support.
> > 	 */
> > 	allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> >-				      guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> >+				      guest_cpu_cap_has(vcpu, X86_FEATURE_GBPAGES);
> > 	guest_cpu_cap_change(vcpu, X86_FEATURE_GBPAGES, allow_gbpages);
> > 
> > 	best = kvm_find_cpuid_entry(vcpu, 1);
> >@@ -441,7 +441,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > 
> > #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
> > 	vcpu->arch.cr4_guest_rsvd_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_) |
> >-					 __cr4_reserved_bits(guest_cpuid_has, vcpu);
> >+					 __cr4_reserved_bits(guest_cpu_cap_has, vcpu);
> 
> So, actually, __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_) can be dropped.
> Is there any reason to keep it? It makes perfect sense to just look up the
> guest cpu_caps given it already takes KVM caps into consideration.

Hmm, good point.  I agree that that keeping the __kvm_cpu_cap_has() checks is
unnecessary, though I'm tempted to turn it into a WARN.  E.g. to guard against
stuffing a feature into cpu_caps without thinking through the implications.

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index edef30359c19..3cbf384aeb7a 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -460,9 +460,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 
        kvm_pmu_refresh(vcpu);
 
+       vcpu->arch.cr4_guest_rsvd_bits = __cr4_reserved_bits(guest_cpu_cap_has, vcpu);
+       /*
+        * KVM's capabilities are incorporated into the vCPU's capabilities,
+        * and letting the guest to use a CR4-based feature that KVM doesn't
+        * support isn't allowed as KVM either needs to explicitly emulate the
+        * feature or set the CR4 bit in hardware.
+        */
 #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
-       vcpu->arch.cr4_guest_rsvd_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_) |
-                                        __cr4_reserved_bits(guest_cpu_cap_has, vcpu);
+       WARN_ON_ONCE(~vcpu->arch.cr4_guest_rsvd_bits &
+                    __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_));
 #undef __kvm_cpu_cap_has
 
        kvm_hv_set_cpuid(vcpu, kvm_cpuid_has_hyperv(vcpu));