linux-kernel - Re: [PATCH v3 5/8] KVM: x86/mmu: Set disallowed_nx_huge

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: <YvHT0dA0BGgCQ8L+@yzhao56-desk.sh.intel.com>
Date:   Tue, 9 Aug 2022 11:26:09 +0800
From:   Yan Zhao <yan.y.zhao@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        David Matlack <dmatlack@...gle.com>,
        "Mingwei Zhang" <mizhang@...gle.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [PATCH v3 5/8] KVM: x86/mmu: Set disallowed_nx_huge_page in TDP
 MMU before setting SPTE

On Fri, Aug 05, 2022 at 11:05:10PM +0000, Sean Christopherson wrote:
> Set nx_huge_page_disallowed in TDP MMU shadow pages before making the SP
> visible to other readers, i.e. before setting its SPTE.  This will allow
> KVM to query the flag when determining if a shadow page can be replaced
> by a NX huge page without violating the rules of the mitigation.
> 
> Note, the shadow/legacy MMU holds mmu_lock for write, so it's impossible
> for another CPU to see a shadow page without an up-to-date
> nx_huge_page_disallowed, i.e. only the TDP MMU needs the complicated
> dance.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> Reviewed-by: David Matlack <dmatlack@...gle.com>
> ---
>  arch/x86/kvm/mmu/mmu.c          | 28 +++++++++++++-------
>  arch/x86/kvm/mmu/mmu_internal.h |  5 ++--
>  arch/x86/kvm/mmu/tdp_mmu.c      | 46 +++++++++++++++++++++++----------
>  3 files changed, 53 insertions(+), 26 deletions(-)
>
<snip>

> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 0e94182c87be..34994ca3d45b 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -392,8 +392,19 @@ static void tdp_mmu_unlink_sp(struct kvm *kvm, struct kvm_mmu_page *sp,
>  		lockdep_assert_held_write(&kvm->mmu_lock);
>  
>  	list_del(&sp->link);
> -	if (sp->nx_huge_page_disallowed)
> -		unaccount_nx_huge_page(kvm, sp);
> +
> +	/*
> +	 * Ensure nx_huge_page_disallowed is read after observing the present
> +	 * shadow page.  A different vCPU may have _just_ finished installing
> +	 * the shadow page if mmu_lock is held for read.  Pairs with the
> +	 * smp_wmb() in kvm_tdp_mmu_map().
> +	 */
> +	smp_rmb();
hi Sean,

I understand this smp_rmb() is intended to prevent the reading of
p->nx_huge_page_disallowed from happening before it's set to true in
kvm_tdp_mmu_map(). Is this understanding right?

If it's true, then do we also need the smp_rmb() for read of sp->gfn in
handle_removed_pt()? (or maybe for other fields in sp in other places?)

Thanks
Yan

> +
> +	if (sp->nx_huge_page_disallowed) {
> +		sp->nx_huge_page_disallowed = false;
> +		untrack_possible_nx_huge_page(kvm, sp);
> +	}
>  
>  	if (shared)
>  		spin_unlock(&kvm->arch.tdp_mmu_pages_lock);