linux-kernel - Re: [PATCH V2] KVM: X86: fix tlb_flush

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, 8 Jun 2021 19:36:10 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>
Cc:     Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org,
        Lai Jiangshan <laijs@...ux.alibaba.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        kvm@...r.kernel.org
Subject: Re: [PATCH V2] KVM: X86: fix tlb_flush_guest()

On 08/06/21 02:03, Sean Christopherson wrote:
> On Tue, Jun 08, 2021, Maxim Levitsky wrote:
>> So this patch *does* fix the windows boot without TDP!
> 
> Woot!
> 
>> Tested-by: Maxim Levitsky <mlevitsk@...hat.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
> 
> Lai,
> 
> I have a reworded version of your patch sitting in a branch that leverages this
> path to fix similar bugs and do additional cleanup.  Any objection to me gathering
> Maxim's tags and posting the version below?  I'm more than happy to hold off if
> you'd prefer to send your own version, but I don't want to send my own series
> without this fix as doing so would introduce bugs.
> 
> Thanks!
> 
> Author: Lai Jiangshan <laijs@...ux.alibaba.com>
> Date:   Tue Jun 1 01:22:56 2021 +0800
> 
>      KVM: x86: Unload MMU on guest TLB flush if TDP disabled to force MMU sync
>      
>      When using shadow paging, unload the guest MMU when emulating a guest TLB
>      flush to all roots are synchronized.  From the guest's perspective,
>      flushing the TLB ensures any and all modifications to its PTEs will be
>      recognized by the CPU.
>      
>      Note, unloading the MMU is overkill, but is done to mirror KVM's existing
>      handling of INVPCID(all) and ensure the bug is squashed.  Future cleanup
>      can be done to more precisely synchronize roots when servicing a guest
>      TLB flush.
>      
>      If TDP is enabled, synchronizing the MMU is unnecessary even if nested
>      TDP is in play, as a "legacy" TLB flush from L1 does not invalidate L1's
>      TDP mappgins.  For EPT, an explicit INVEPT is required to invalidate
>      guest-physical mappings.  For NPT, guest mappings are always tagged with
>      an ASID and thus can only be invalidated via the VMCB's ASID control.
>      
>      This bug has existed since the introduction of KVM_VCPU_FLUSH_TLB, but
>      was only recently exposed after Linux guests stopped flushing the local
>      CPU's TLB prior to flushing remote TLBs (see commit 4ce94eabac16,
>      "x86/mm/tlb: Flush remote and local TLBs concurrently").
>      
>      Tested-by: Maxim Levitsky <mlevitsk@...hat.com>
>      Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
>      Fixes: f38a7b75267f ("KVM: X86: support paravirtualized help for TLB shootdowns")
>      Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
>      [sean: massaged comment and changelog]
>      Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1cd6d4685932..3b02528d5ee8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3072,6 +3072,18 @@ static void kvm_vcpu_flush_tlb_all(struct kvm_vcpu *vcpu)
>   static void kvm_vcpu_flush_tlb_guest(struct kvm_vcpu *vcpu)
>   {
>          ++vcpu->stat.tlb_flush;
> +
> +       if (!tdp_enabled) {
> +               /*
> +                * Unload the entire MMU to force a sync of the shadow page
> +                * tables.  A TLB flush on behalf of the guest is equivalent
> +                * to INVPCID(all), toggling CR4.PGE, etc...  Note, loading the
> +                * MMU will also do an actual TLB flush.
> +                */

I slightly prefer it in the opposite order (first why, then how):

                /*
                 * A TLB flush on behalf of the guest is equivalent to
                 * INVPCID(all), toggling CR4.PGE, etc., which requires
                 * a forced sync of the shadow page tables.  Unload the
                 * entire MMU here and the subsequent load will sync the
                 * shadow page tables, and also flush the TLB.
                 */

Queued, thanks all!  It's great that this fixes an actual bug.

Paolo