linux-kernel - Re: [PATCH v2 11/15] KVM: x86/tdp_mmu: Reflect tearing down mirror page tables

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]

Date: Fri, 14 Jun 2024 23:11:35 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Huang, Kai" <kai.huang@...el.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...ux.intel.com"
	<isaku.yamahata@...ux.intel.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Zhao, Yan Y" <yan.y.zhao@...el.com>, "Aktas,
 Erdem" <erdemaktas@...gle.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>
Subject: Re: [PATCH v2 11/15] KVM: x86/tdp_mmu: Reflect tearing down mirror
 page tables

On Wed, 2024-06-12 at 11:39 -0700, Isaku Yamahata wrote:
> > TDX of course has the limitation around the ordering of the zapping S-EPT.
> > So I
> > read the comment to be referring to how the implementation avoids zapping
> > any
> > non-leaf PTEs during TD runtime.
> > 
> > But I'm going to have to circle back here after investigating a bit more.
> > Isaku,
> > any comments on this comment and conditional?
> 
> It's for large page page merge/split.  At this point, it seems only confusing.
> We need only leaf zapping.  Maybe reflect_removed_leaf_spte() or something.
> Later we can come back when large page support.

Yes, I don't see the need for the is_shadow_present_pte() in this function. The
leaf check is needed, but actually it could be done well enough in TDX code for
now (for as long as we have no huge pages).

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0155b8330e15..e54dd2355005 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1721,6 +1721,10 @@ static int tdx_sept_remove_private_spte(struct kvm *kvm,
gfn_t gfn,
 {
        int ret;

+       /* Only 4k pages currently, nothing to do for other levels */
+       if (level != PG_LEVEL_4K)
+               return 0;
+
        ret = tdx_sept_zap_private_spte(kvm, gfn, level);
        if (ret)
                return ret;

The benefits would be that we have less mirror logic in core MMU code that
actually is about TDX specifics unrelated to the mirror concept. The downsides
are returning success there feels kind of wrong. I'm going to move forward with
dropping the is_shadow_present_pte() check and adding a comment (see below), but
I thought the alternative was worth mentioning because I was tempted.

/*
 * External (TDX) SPTEs are limited to PG_LEVEL_4K, and external
 * PTs are removed in a special order, involving free_external_spt().
 * But remove_external_spte() will be called on non-leaf PTEs via
 * __tdp_mmu_zap_root(), so avoid the error the former would return
 * in this case.
 */