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]

Date:   Tue, 30 Jun 2020 08:47:26 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Peter Xu <peterx@...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 Sat, Jun 27, 2020 at 04:24:34PM +0200, Paolo Bonzini wrote:
> On 26/06/20 20:18, Sean Christopherson wrote:
> >> 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.
> 
> But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are
> two choices:
> 
> 1) check ARCH_CAPABILITIES first
> 
> 2) blindly access it and default to 0.
> 
> Both are fine, because we know MSR_IA32_TSX_CTRL has no
> reserved/must-be-one bits.  Calling __kvm_get_msr and checking for an
> invalid MSR through the return value is not breaking the rules of
> emulation, it is "faking" a #GP handler.

"guest" was the wrong choice of word.  My point was that, IMO, emulation
should never set host_initiated=true.

To me, accessing MSRs with host_initiated is the equivalent of loading a
ucode patch, i.e. it's super duper special stuff that deliberately turns
off all safeguards and can change the fundamental behavior of the (virtual)
CPU.

Emulation on the other handle should either be subject to all the normal
rules or have dedicated, intelligent handling for breaking the normal rules,
e.g. nested usage of vmx_set_efer(), vmx_set_cr0, vmx_set_cr4, etc...

> So I think Peter's patch is fine, but (possibly on top as a third patch)
> __must_check should be added to MSR getters and setters.  Also one
> possibility is to return -EINVAL for invalid MSRs.
> 
> Paolo
>