| 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
| ||
|
Message-ID: <90f6a15c-0dec-4a19-7a21-b18b73932a21@redhat.com>
Date: Thu, 16 Mar 2023 13:48:54 +0100
From: David Hildenbrand <david@...hat.com>
To: Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: linux-mm@...ck.org, dave.hansen@...el.com, peterz@...radead.org,
tglx@...utronix.de, seanjc@...gle.com, pbonzini@...hat.com,
dan.j.williams@...el.com, rafael.j.wysocki@...el.com,
kirill.shutemov@...ux.intel.com, ying.huang@...el.com,
reinette.chatre@...el.com, len.brown@...el.com,
tony.luck@...el.com, ak@...ux.intel.com, isaku.yamahata@...el.com,
chao.gao@...el.com, sathyanarayanan.kuppuswamy@...ux.intel.com,
bagasdotme@...il.com, sagis@...gle.com, imammedo@...hat.com
Subject: Re: [PATCH v10 02/16] x86/virt/tdx: Detect TDX during kernel boot
On 06.03.23 15:13, Kai Huang wrote:
> Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
> host and certain physical attacks. A CPU-attested software module
> called 'the TDX module' runs inside a new isolated memory range as a
> trusted hypervisor to manage and run protected VMs.
>
> Pre-TDX Intel hardware has support for a memory encryption architecture
> called MKTME. The memory encryption hardware underpinning MKTME is also
> used for Intel TDX. TDX ends up "stealing" some of the physical address
> space from the MKTME architecture for crypto-protection to VMs. The
> BIOS is responsible for partitioning the "KeyID" space between legacy
> MKTME and TDX. The KeyIDs reserved for TDX are called 'TDX private
> KeyIDs' or 'TDX KeyIDs' for short.
>
> TDX doesn't trust the BIOS. During machine boot, TDX verifies the TDX
> private KeyIDs are consistently and correctly programmed by the BIOS
> across all CPU packages before it enables TDX on any CPU core. A valid
> TDX private KeyID range on BSP indicates TDX has been enabled by the
> BIOS, otherwise the BIOS is buggy.
So we don't trust the BIOS, but trust the BIOS that it won't hot-remove
physical memory or hotplug physical CPUS (if I understood the cover
letter correctly)? :)
>
> The TDX module is expected to be loaded by the BIOS when it enables TDX,
> but the kernel needs to properly initialize it before it can be used to
> create and run any TDX guests. The TDX module will be initialized by
> the KVM subsystem when KVM wants to use TDX.
>
> Add a new early_initcall(tdx_init) to detect the TDX by detecting TDX
> private KeyIDs. Also add a function to report whether TDX is enabled by
> the BIOS. Similar to AMD SME, kexec() will use it to determine whether
> cache flush is needed.
>
> The TDX module itself requires one TDX KeyID as the 'TDX global KeyID'
> to protect its metadata. Each TDX guest also needs a TDX KeyID for its
> own protection. Just use the first TDX KeyID as the global KeyID and
> leave the rest for TDX guests. If no TDX KeyID is left for TDX guests,
> disable TDX as initializing the TDX module alone is useless.
Does that really happen in practice that we care about that at all?
Seems weird and rather like a broken firmware or sth like that ...
>
> To start to support TDX, create a new arch/x86/virt/vmx/tdx/tdx.c for
> TDX host kernel support. Add a new Kconfig option CONFIG_INTEL_TDX_HOST
> to opt-in TDX host kernel support (to distinguish with TDX guest kernel
> support). So far only KVM uses TDX. Make the new config option depend
> on KVM_INTEL.
>
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
[...]
> ---
> arch/x86/Kconfig | 12 ++++
> arch/x86/Makefile | 2 +
> arch/x86/include/asm/msr-index.h | 3 +
> arch/x86/include/asm/tdx.h | 7 +++
> arch/x86/virt/Makefile | 2 +
> arch/x86/virt/vmx/Makefile | 2 +
> arch/x86/virt/vmx/tdx/Makefile | 2 +
> arch/x86/virt/vmx/tdx/tdx.c | 105 +++++++++++++++++++++++++++++++
> 8 files changed, 135 insertions(+)
> create mode 100644 arch/x86/virt/Makefile
> create mode 100644 arch/x86/virt/vmx/Makefile
> create mode 100644 arch/x86/virt/vmx/tdx/Makefile
> create mode 100644 arch/x86/virt/vmx/tdx/tdx.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3604074a878b..fc010973a6ff 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1952,6 +1952,18 @@ config X86_SGX
>
> If unsure, say N.
>
> +config INTEL_TDX_HOST
> + bool "Intel Trust Domain Extensions (TDX) host support"
> + depends on CPU_SUP_INTEL
> + depends on X86_64
> + depends on KVM_INTEL
> + help
> + Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
> + host and certain physical attacks. This option enables necessary TDX
> + support in host kernel to run protected VMs.
s/in host/in the host/ ?
Also, is "protected VMs" the right term to use here? "Encrypted VMs",
"Confidential VMs" ... ?
> +
> + If unsure, say N.
> +
> config EFI
> bool "EFI runtime service support"
> depends on ACPI
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 9cf07322875a..972b5a64ce38 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -252,6 +252,8 @@ archheaders:
>
> libs-y += arch/x86/lib/
>
> +core-y += arch/x86/virt/
> +
> # drivers-y are linked after core-y
> drivers-$(CONFIG_MATH_EMULATION) += arch/x86/math-emu/
> drivers-$(CONFIG_PCI) += arch/x86/pci/
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 37ff47552bcb..952374ddb167 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -512,6 +512,9 @@
> #define MSR_RELOAD_PMC0 0x000014c1
> #define MSR_RELOAD_FIXED_CTR0 0x00001309
>
> +/* KeyID partitioning between MKTME and TDX */
> +#define MSR_IA32_MKTME_KEYID_PARTITIONING 0x00000087
> +
> /*
> * AMD64 MSRs. Not complete. See the architecture manual for a more
> * complete list.
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 25fd6070dc0b..4dfe2e794411 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -94,5 +94,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> return -ENODEV;
> }
> #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
> +
> +#ifdef CONFIG_INTEL_TDX_HOST
> +bool platform_tdx_enabled(void);
> +#else /* !CONFIG_INTEL_TDX_HOST */
> +static inline bool platform_tdx_enabled(void) { return false; }
> +#endif /* CONFIG_INTEL_TDX_HOST */
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/virt/Makefile b/arch/x86/virt/Makefile
> new file mode 100644
> index 000000000000..1e36502cd738
> --- /dev/null
> +++ b/arch/x86/virt/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-y += vmx/
> diff --git a/arch/x86/virt/vmx/Makefile b/arch/x86/virt/vmx/Makefile
> new file mode 100644
> index 000000000000..feebda21d793
> --- /dev/null
> +++ b/arch/x86/virt/vmx/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_INTEL_TDX_HOST) += tdx/
> diff --git a/arch/x86/virt/vmx/tdx/Makefile b/arch/x86/virt/vmx/tdx/Makefile
> new file mode 100644
> index 000000000000..93ca8b73e1f1
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-y += tdx.o
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> new file mode 100644
> index 000000000000..a600b5d0879d
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2023 Intel Corporation.
> + *
> + * Intel Trusted Domain Extensions (TDX) support
> + */
> +
> +#define pr_fmt(fmt) "tdx: " fmt
> +
> +#include <linux/types.h>
> +#include <linux/cache.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/printk.h>
> +#include <asm/msr-index.h>
> +#include <asm/msr.h>
> +#include <asm/tdx.h>
> +
> +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;
> +
> +/*
> + * Use tdx_global_keyid to indicate that TDX is uninitialized.
> + * This is used in TDX initialization error paths to take it from
> + * initialized -> uninitialized.
> + */
> +static void __init clear_tdx(void)
> +{
> + tdx_global_keyid = 0;
> +}
Why not set "tdx_global_keyid" last, such that you don't have to clear
when anything goes wrong before that? Seems more straight forward.
> +
> +static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
> + u32 *nr_tdx_keyids)
> +{
> + u32 _nr_mktme_keyids, _tdx_keyid_start, _nr_tdx_keyids;
> + int ret;
> +
> + /*
> + * IA32_MKTME_KEYID_PARTIONING:
> + * Bit [31:0]: Number of MKTME KeyIDs.
> + * Bit [63:32]: Number of TDX private KeyIDs.
> + */
> + ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &_nr_mktme_keyids,
> + &_nr_tdx_keyids);
> + if (ret)
> + return -ENODEV;
> +
> + if (!_nr_tdx_keyids)
> + return -ENODEV;
> +
> + /* TDX KeyIDs start after the last MKTME KeyID. */
> + _tdx_keyid_start = _nr_mktme_keyids + 1;
> +
> + *tdx_keyid_start = _tdx_keyid_start;
> + *nr_tdx_keyids = _nr_tdx_keyids;
> +
> + return 0;
> +}
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists