linux-kernel - Re: [PATCH v19 032/130] KVM: TDX: Add helper functions to allocate/free TDX private host key id

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: <075322c9db65e2fa19d809357a98fe6067c80508.camel@intel.com>
Date: Wed, 13 Mar 2024 00:44:14 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Yamahata,
 Isaku" <isaku.yamahata@...el.com>
CC: "Zhang, Tina" <tina.zhang@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Yuan, Hang" <hang.yuan@...el.com>, "Huang, Kai"
	<kai.huang@...el.com>, "Chen, Bo2" <chen.bo@...el.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>
Subject: Re: [PATCH v19 032/130] KVM: TDX: Add helper functions to
 allocate/free TDX private host key id

On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Add helper functions to allocate/free TDX private host key id (HKID).
> 
> The memory controller encrypts TDX memory with the assigned TDX
> HKIDs.  The
> global TDX HKID is to encrypt the TDX module, its memory, and some
> dynamic
> data (TDR). 

I don't see any code about the global key id.

>  The private TDX HKID is assigned to guest TD to encrypt guest
> memory and the related data.  When VMM releases an encrypted page for
> reuse, the page needs a cache flush with the used HKID.

Not sure the cache part is pertinent to this patch. Sounds good for
some other patch.

>   VMM needs the
> global TDX HKID and the private TDX HKIDs to flush encrypted pages.

I think the commit log could have a bit more about what code is added.
What about adding something like this (some verbiage from Kai's setup
patch):

The memory controller encrypts TDX memory with the assigned TDX
HKIDs. Each TDX guest must be protected by its own unique TDX HKID.

The HW has a fixed set of these HKID keys. Out of those, some are set
aside for use by for other TDX components, but most are saved for guest
use. The code that does this partitioning, records the range chosen to
be available for guest use in the tdx_guest_keyid_start and
tdx_nr_guest_keyids variables.

Use this range of HKIDs reserved for guest use with the kernel's IDA
allocator library helper to create a mini TDX HKID allocator that can
be called when setting up a TD. This way it can have an exclusive HKID,
as is required. This allocator will be used in future changes.

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
> v19:
> - Removed stale comment in tdx_guest_keyid_alloc() by Binbin
> - Update sanity check in tdx_guest_keyid_free() by Binbin
> 
> v18:
> - Moved the functions to kvm tdx from arch/x86/virt/vmx/tdx/
> - Drop exporting symbols as the host tdx does.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
>  arch/x86/kvm/vmx/tdx.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index a7e096fd8361..cde971122c1e 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -11,6 +11,34 @@
>  #undef pr_fmt
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +/*
> + * Key id globally used by TDX module: TDX module maps TDR with this
> TDX global
> + * key id.  TDR includes key id assigned to the TD.  Then TDX module
> maps other
> + * TD-related pages with the assigned key id.  TDR requires this TDX
> global key
> + * id for cache flush unlike other TD-related pages.
> + */

The above comment is about tdx_global_keyid, which is unrelated to the
patch and code.

> +/* TDX KeyID pool */
> +static DEFINE_IDA(tdx_guest_keyid_pool);
> +
> +static int __used tdx_guest_keyid_alloc(void)
> +{
> +       if (WARN_ON_ONCE(!tdx_guest_keyid_start ||
> !tdx_nr_guest_keyids))
> +               return -EINVAL;

I think the idea of this warnings is to check if TDX failed to init? It
could check X86_FEATURE_TDX_HOST_PLATFORM or enable_tdx, but that seems
to be a weird thing to check in a low level function that is called in
the middle of in progress setup.

Don't know, I'd probably drop this warning.

> +
> +       return ida_alloc_range(&tdx_guest_keyid_pool,
> tdx_guest_keyid_start,
> +                              tdx_guest_keyid_start +
> tdx_nr_guest_keyids - 1,
> +                              GFP_KERNEL);
> +}
> +
> +static void __used tdx_guest_keyid_free(int keyid)
> +{
> +       if (WARN_ON_ONCE(keyid < tdx_guest_keyid_start ||
> +                        keyid > tdx_guest_keyid_start +
> tdx_nr_guest_keyids - 1))
> +               return;

This seems like a more useful warning, but still not sure it's that
risky. I guess the point is to check for returning garbage. Because a
double free would not be caught, but would be possible to using
idr_find(). I would think if we are worried we should do the full
check, but I'm not sure we can't just drop this. There are very limited
callers or things that change the checked configuration (1 of each).

> +
> +       ida_free(&tdx_guest_keyid_pool, keyid);
> +}
> +
>  static int __init tdx_module_setup(void)
>  {
>         int ret;