linux-kernel - Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

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: <43af672ad0dbdeefaa45f72fa7a4ae68bc5d0fb6.camel@intel.com>
Date: Fri, 30 Aug 2024 19:16:29 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "Hansen, Dave" <dave.hansen@...el.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>
CC: "Li, Xiaoyao" <xiaoyao.li@...el.com>, "tony.lindgren@...ux.intel.com"
	<tony.lindgren@...ux.intel.com>, "Huang, Kai" <kai.huang@...el.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

On Fri, 2024-08-30 at 11:45 -0700, Dave Hansen wrote:
> On 8/12/24 15:48, Rick Edgecombe wrote:
> > Each TDX guest has a root control structure called "Trust Domain
> > Root" (TDR).  Unlike the rest of the TDX guest, the TDR is protected
> > by the TDX global KeyID.  When tearing down the TDR, KVM will need to
> > pass the TDX global KeyID explicitly to the TDX module to flush cache
> > associated to the TDR.
> 
> What does that end up looking like?

The global key id callers looks like:
	err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(kvm_tdx->tdr_pa,
						     tdx_global_keyid));
	if (KVM_BUG_ON(err, kvm)) {
		pr_tdx_error(TDH_PHYMEM_PAGE_WBINVD, err);
		return;
	}

The TD keyid callers looks like:
	hpa_with_hkid = set_hkid_to_hpa(hpa, (u16)kvm_tdx->hkid);
	do {
		/*
		 * TDX_OPERAND_BUSY can happen on locking PAMT entry.  Because
		 * this page was removed above, other thread shouldn't be
		 * repeatedly operating on this page.  Just retry loop.
		 */
		err = tdh_phymem_page_wbinvd(hpa_with_hkid);
	} while (unlikely(err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX)));

> 
> In other words, should we export the global KeyID, or export a function
> to do the flush and then never actually expose the KeyID?

We could split it into two helpers if we wanted to remove the export of
tdx_global_keyid. One for global key id and one that only takes TD range key
ids. Adding more layers is a downside.

Separate from Dave's question, I wonder if we should open code set_hkid_to_hpa()
inside tdh_phymem_page_wbinvd(). The signature could change to
tdh_phymem_page_wbinvd(hpa_t pa, u16 hkid). set_hkid_to_hpa() is very
lightweight, so I don't think doing it outside the loop is much gain. It makes
the code cleaner.

> 
> > -static u32 tdx_global_keyid __ro_after_init;
> > -static u32 tdx_guest_keyid_start __ro_after_init;
> > -static u32 tdx_nr_guest_keyids __ro_after_init;
> > +u32 tdx_global_keyid __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_global_keyid);
> > +
> > +u32 tdx_guest_keyid_start __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_guest_keyid_start);
> > +
> > +u32 tdx_nr_guest_keyids __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_nr_guest_keyids);
> 
> I know the KVM folks aren't maniacs that will start writing to these or
> anything.

Yea. ro_after_init would stop most mischief as well.

> 
> But, in general, just exporting global variables isn't super nice.  If
> these are being used to set up the key allocator, I'd kinda just rather
> that the allocator be in core code and have its alloc/free functions
> exported.
> 

Makes sense. We could remove tdx_guest_keyid_start/tdx_nr_guest_keyids then in
any case. But if we want to remove tdx_global_keyid too, we could add a
tdh_phymem_page_wbinvd_global_keyid(void). I'm split on that one, but I'd lean
towards doing it.