linux-kernel - Re: [PATCH v2 7/7] KVM: WARN if is_unsync_root() is called on a root without a shadow page

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: <CANgfPd_zdQAu7m1M_g0wy0wsUpyHDtbE+tUZOKQN59y0ABpvPw@mail.gmail.com>
Date:   Mon, 28 Feb 2022 14:33:55 -0800
From:   Ben Gardon <bgardon@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [PATCH v2 7/7] KVM: WARN if is_unsync_root() is called on a root
 without a shadow page

On Fri, Feb 25, 2022 at 10:23 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> WARN and bail if is_unsync_root() is passed a root for which there is no
> shadow page, i.e. is passed the physical address of one of the special
> roots, which do not have an associated shadow page.  The current usage
> squeaks by without bug reports because neither kvm_mmu_sync_roots() nor
> kvm_mmu_sync_prev_roots() calls the helper with pae_root or pml4_root,
> and 5-level AMD CPUs are not generally available, i.e. no one can coerce
> KVM into calling is_unsync_root() on pml5_root.
>
> Note, this doesn't fix the mess with 5-level nNPT, it just (hopefully)
> prevents KVM from crashing.
>
> Cc: Lai Jiangshan <jiangshanlai@...il.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 825996408465..3e7c8ad5bed9 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3634,6 +3634,14 @@ static bool is_unsync_root(hpa_t root)
>          */
>         smp_rmb();
>         sp = to_shadow_page(root);
> +
> +       /*
> +        * PAE roots (somewhat arbitrarily) aren't backed by shadow pages, the
> +        * PDPTEs for a given PAE root need to be synchronized individually.
> +        */
> +       if (WARN_ON_ONCE(!sp))
> +               return false;
> +

I was trying to figure out if this should be returning true or false,
but neither really seems correct. Since we never expect this to fire,
perhaps it doesn't matter and it's easier to just return false so the
callers don't need to be changed. If this did fire in a production
scenario, I'd want it to terminate the VM too.

>         if (sp->unsync || sp->unsync_children)
>                 return true;
>
> --
> 2.35.1.574.g5d30c73bfb-goog
>