linux-kernel - Re: [PATCH 1/2] KVM: X86: Move ignore

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: <20200626181820.GG6583@linux.intel.com>
Date:   Fri, 26 Jun 2020 11:18:20 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 1/2] KVM: X86: Move ignore_msrs handling upper the stack

On Fri, Jun 26, 2020 at 02:07:32PM -0400, Peter Xu wrote:
> On Thu, Jun 25, 2020 at 09:25:40AM -0700, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 5eb618dbf211..64322446e590 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> >                 *ebx = entry->ebx;
> >                 *ecx = entry->ecx;
> >                 *edx = entry->edx;
> > -               if (function == 7 && index == 0) {
> > +               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
> >                         u64 data;
> > -                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
> > +                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
> >                             (data & TSX_CTRL_CPUID_CLEAR))
> >                                 *ebx &= ~(F(RTM) | F(HLE));
> >                 }
> > 
> > 
> > On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array
> > regardless of whether or not it is being advertised to userspace (this is
> > a bug in its own right).  Using the host_initiated variant means KVM will
> > incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly
> > clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL
> > without advertising it to the guest.
> 
> Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities &
> ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want
> to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr().  From an
emulation perspective, this really is a "guest" access to the MSR, in the
sense that it the virtual CPU is in the guest domain, i.e. not a god-like
entity that gets to break the rules of emulation.

The other thing to consider is that SVM doesn't have any knowledge of any
of this, so arguably the "ignored msr" crud should get logged for SVM as
it's effectively a userspace bug if they've configured the VM to have RTM
or HLE on a system that can't possibly support either.