linux-kernel - Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

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: <d7b5a1e327d6a91e8c2596996df3ff100992dc6c.camel@intel.com>
Date: Fri, 17 May 2024 18:16:26 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Huang, Kai" <kai.huang@...el.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...ux.intel.com"
	<isaku.yamahata@...ux.intel.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"Zhao, Yan Y" <yan.y.zhao@...el.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU

On Fri, 2024-05-17 at 02:03 -0700, Isaku Yamahata wrote:
> 
> On top of your patch, I created the following patch to remove
> kvm_gfn_for_root().
> Although I haven't tested it yet, I think the following shows my idea.
> 
> - Add gfn_shared_mask to struct tdp_iter.
> - Use iter.gfn_shared_mask to determine the starting sptep in the root.
> - Remove kvm_gfn_for_root()

I investigated it.

After this, gfn_t's never have shared bit. It's a simple rule. The MMU mostly
thinks it's operating on a shared root that is mapped at the normal GFN. Only
the iterator knows that the shared PTEs are actually in a different location.

There are some negative side effects:
1. The struct kvm_mmu_page's gfn doesn't match it's actual mapping anymore.
2. As a result of above, the code that flushes TLBs for a specific GFN will be
confused. It won't functionally matter for TDX, just look buggy to see flushing
code called with the wrong gfn.
3. A lot of tracepoints no longer have the "real" gfn
4. mmio spte doesn't have the shared bit, as previous (no effect)
5. Some zapping code (__tdp_mmu_zap_root(), tdp_mmu_zap_leafs()) intends to
actually operating on the raw_gfn. It wants to iterate the whole EPT, so it goes
from 0 to tdp_mmu_max_gfn_exclusive(). So now for mirrored it does, but for
shared it only covers the shared range. Basically kvm_mmu_max_gfn() is wrong if
we pretend shared GFNs are just strangely mapped normal GFNs. Maybe we could
just fix this up to report based on GPAW for TDX? Feels wrong.

On the positive effects side:
1. There is code that passes sp->gfn into things that it shouldn't (if it has
shared bits) like memslot lookups.
2. Also code that passes iter.gfn into things it shouldn't like
kvm_mmu_max_mapping_level().

These places are not called by TDX, but if you know that gfn's might include
shared bits, then that code looks buggy.

I think the solution in the diff is more elegant then before, because it hides
what is really going on with the shared root. That is both good and bad. Can we
accept the downsides?