linux-kernel - Re: [PATCH 48/61] KVM: x86: Do host CPUID at load time to mask KVM cpu caps

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-next>] [day] [month] [year] [list]

Message-ID: <20200224233119.GS29865@linux.intel.com>
Date:   Mon, 24 Feb 2020 15:31:19 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 48/61] KVM: x86: Do host CPUID at load time to mask KVM
 cpu caps

On Mon, Feb 24, 2020 at 11:46:09PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@...el.com> writes:
> 
> > Mask kvm_cpu_caps based on host CPUID in preparation for overriding the
> > CPUID results during KVM_GET_SUPPORTED_CPUID instead of doing the
> > masking at runtime.
> >
> > Note, masking may or may not be necessary, e.g. the kernel rarely, if
> > ever, sets real CPUID bits that are not supported by hardware.  But, the
> > code is cheap and only runs once at load, so an abundance of caution is
> > warranted.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index ab2a34337588..4416f2422321 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -272,8 +272,22 @@ static __always_inline void cpuid_entry_mask(struct kvm_cpuid_entry2 *entry,
> >  
> >  static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask)
> >  {
> > +	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
> > +	struct kvm_cpuid_entry2 entry;
> > +
> >  	reverse_cpuid_check(leaf);
> >  	kvm_cpu_caps[leaf] &= mask;
> > +
> > +#ifdef CONFIG_KVM_CPUID_AUDIT
> > +	/* Entry needs to be fully populated when auditing is enabled. */
> > +	entry.function = cpuid.function;
> > +	entry.index = cpuid.index;
> > +#endif
> > +
> > +	cpuid_count(cpuid.function, cpuid.index,
> > +		    &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
> > +
> > +	kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, &cpuid);
> >  }
> >  
> >  void kvm_set_cpu_caps(void)
> 
> If we don't really believe that masking will actually mask anything,
> maybe we should move it under '#ifdef CONFIG_KVM_CPUID_AUDIT'? And/or 
> add a WARN_ON()?

I'm not opposed to trying that, but I'd definitely want to do it as a
separate patch, or maybe even let it stew separately in kvm/queue for a
few cycles.

> The patch itself looks good, so:
> Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> 
> -- 
> Vitaly
>