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: <20240517191630.GC412700@ls.amr.corp.intel.com>
Date: Fri, 17 May 2024 12:16:30 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"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, May 17, 2024 at 06:16:26PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@...el.com> wrote:

> 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.

Thanks for looking at 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.

flush_remote_tlbs_range() is only for Hyper-V optimization.  In other cases,
x86_op.flush_remote_tlbs_range = NULL or the member isn't defined at compile
time.  So the remote tlb flush falls back to flushing whole range.  I don't
expect TDX in hyper-V guest.  I have to admit that the code looks superficially
broken and confusing.


> 3. A lot of tracepoints no longer have the "real" gfn

Anyway we'd like to sort out trace points and pr_err() eventually because we
already added new pferr flags.


> 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.

Yes, it's broken with kvm_mmu_max_gfn().


> 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?

Kai, do you have any thoughts?
-- 
Isaku Yamahata <isaku.yamahata@...el.com>