linux-kernel - Re: [PATCH 00/15] KVM: x86/mmu: TLB fixes and related cleanups

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

Message-ID: <5da71719-1783-0f98-072d-49139354b80f@redhat.com>
Date:   Thu, 10 Jun 2021 18:10:39 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...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, Junaid Shahid <junaids@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        Lai Jiangshan <laijs@...ux.alibaba.com>
Subject: Re: [PATCH 00/15] KVM: x86/mmu: TLB fixes and related cleanups

On 10/06/21 01:42, Sean Christopherson wrote:
> Fixes for two (very) theoretical TLB flushing bugs (patches 1 and 4),
> and clean ups on top to (hopefully) consolidate and simplifiy the TLB
> flushing logic.
> 
> The basic gist of the TLB flush and MMU sync code shuffling  is to stop
> relying on the logic in __kvm_mmu_new_pgd() (but keep it for forced
> flushing), and instead handle the flush+sync logic in the caller
> independent from whether or not the "fast" switch occurs.
> 
> I spent a fair bit of time trying to shove the necessary logic down into
> __kvm_mmu_new_pgd(), but it always ended up a complete mess because the
> requirements and contextual information is always different.  The rules
> for MOV CR3 are different from nVMX transitions (and those vary based on
> EPT+VPID), and nSVM will be different still (once it adds proper TLB
> handling).  In particular, I like that nVMX no longer has special code
> for synchronizing the MMU when using shadowing paging and instead relies
> on the common rules for TLB flushing.
> 
> Note, this series (indirectly) relies heavily on commit b53e84eed08b
> ("KVM: x86: Unload MMU on guest TLB flush if TDP disabled to force MMU
> sync"), as it uses KVM_REQ_TLB_FLUSH_GUEST (was KVM_REQ_HV_TLB_FLUSH)
> to do the TLB flush _and_ the MMU sync in non-PV code.
> 
> Tested all combinations for i386, EPT, NPT, and shadow paging. I think...
> 
> The EPTP switching and INVPCID single-context changes in particular lack
> meaningful coverage in kvm-unit-tests+Linux.  Long term it's on my todo
> list to remedy that, but realistically I doubt I'll get it done anytime
> soon.
> 
> To test EPTP switching, I hacked L1 to set up a duplicate top-level EPT
> table, copy the "real" table to the duplicate table on EPT violation,
> populate VMFUNC.EPTP_LIST with the two EPTPs, expose  VMFUNC.EPTP_SWITCH
> to L2.  I then hacked L2 to do an EPTP switch to a random (valid) EPTP
> index on every task switch.
> 
> To test INVPCID single-context I modified L1 to iterate over all possible
> PCIDs using INVPCID single-context in native_flush_tlb_global().  I also
> verified that the guest crashed if it didn't do any INVPCID at all
> (interestingly, the guest made it through boot without the flushes when
> EPT was enabled, which implies the missing MMU sync on INVPCID was the
> source of the crash, not a stale TLB entry).
> 
> Sean Christopherson (15):
>    KVM: nVMX: Sync all PGDs on nested transition with shadow paging
>    KVM: nVMX: Ensure 64-bit shift when checking VMFUNC bitmap
>    KVM: nVMX: Don't clobber nested MMU's A/D status on EPTP switch
>    KVM: x86: Invalidate all PGDs for the current PCID on MOV CR3 w/ flush
>    KVM: x86: Uncondtionally skip MMU sync/TLB flush in MOV CR3's PGD
>      switch
>    KVM: nSVM: Move TLB flushing logic (or lack thereof) to dedicated
>      helper
>    KVM: x86: Drop skip MMU sync and TLB flush params from "new PGD"
>      helpers
>    KVM: nVMX: Consolidate VM-Enter/VM-Exit TLB flush and MMU sync logic
>    KVM: nVMX: Free only guest_mode (L2) roots on INVVPID w/o EPT
>    KVM: x86: Use KVM_REQ_TLB_FLUSH_GUEST to handle INVPCID(ALL) emulation
>    KVM: nVMX: Use fast PGD switch when emulating VMFUNC[EPTP_SWITCH]
>    KVM: x86: Defer MMU sync on PCID invalidation
>    KVM: x86: Drop pointless @reset_roots from kvm_init_mmu()
>    KVM: nVMX: WARN if subtly-impossible VMFUNC conditions occur
>    KVM: nVMX: Drop redundant checks on vmcs12 in EPTP switching emulation
> 
>   arch/x86/include/asm/kvm_host.h |   6 +-
>   arch/x86/kvm/hyperv.c           |   2 +-
>   arch/x86/kvm/mmu.h              |   2 +-
>   arch/x86/kvm/mmu/mmu.c          |  57 ++++++++-----
>   arch/x86/kvm/svm/nested.c       |  40 ++++++---
>   arch/x86/kvm/vmx/nested.c       | 139 ++++++++++++--------------------
>   arch/x86/kvm/x86.c              |  75 ++++++++++-------
>   7 files changed, 169 insertions(+), 152 deletions(-)
> 

I tried this a couple times but was blocked on what is essentially your 
first patch, so thanks!  Patches queued for 5.14.

Paolo