lkml.org 
[lkml]   [2023]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v10 02/16] x86/virt/tdx: Detect TDX during kernel boot
From
On 16.03.23 23:37, Huang, Kai wrote:
> On Thu, 2023-03-16 at 13:48 +0100, David Hildenbrand wrote:
>> 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.
>>

Sorry for the late reply!

>> 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 "trust" in this context means security, but not functionality. BIOS needs
> to do the right thing in order to make things work correctly in terms of
> functionality.
>
> For physical memory hotplug or CPU hotplug, we don't have patch to _explicitly_
> distinguish them (from logical memory hotplug and logical cpu online/offline),
> but actually they are kinda also handled: For memory hotplug, and hot-added
> memory is rejected to go online (because they cannot be in TDX's convertible
> memory ranges). For CPU hotplug, we have a function to do per-cpu
> initialization (tdx_cpu_enable() in patch 5), and it will return error for hot-
> added physical cpu.

Make sense, thanks!

>
>>
>>>
>>> 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 ...
>
> No it doesn't happen in practice, because the BIOS is sane enough.
>
> But since the public spec doesn't explicitly say it is guaranteed this doesn't
> happen when TDX is enabled, I just added this sanity check.

Okay!

>
>>
>>>
>>> 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@intel.com>
>>> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.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/ ?
>
> Sure.
>
>>
>> Also, is "protected VMs" the right term to use here? "Encrypted VMs",
>> "Confidential VMs" ... ?
>
> "Encrypted VM" perhaps is not a good choice, because there are more things than
> encryption. I am also OK with "Confidential VMs", but "protected VMs" is also
> used in the KVM series (not upstreamed yet), and also used by s390 by looking at
> the git log.
>
> So both "protected VM" and "confidential VM" work for me.
>
> Not sure anyone else wants to comment?

I'm fine as long as it's used consistently. "Protected VM" would have
been the one out of the 3 alternatives that I have heard least frequently.

>
>>
> [...]
>
>>> +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.
>
> My thinking was by reserving the global keyid and taking it out first, I can
> check the remaining keyids for TDX guests easily:
>
>
> + if (!nr_tdx_keyids) {
> + pr_info("initialization failed: too few private KeyIDs
> available.\n");
> + goto no_tdx;
> + }
>
> Otherwise need to do:
>
> if (nr_tdx_keyids < 2) {
> ...
> }
>
> Also, in the later patch to handle memory hotplug we will add an additional step
> to register_memory_notifier() which can also fail, so I just introduced
> clear_tdx() here.
>
> But nothing is big deal, and yes we can set the global keyid at last and remove
> clear_tdx().

Good, that simplifies things, thanks!

--
Thanks,

David / dhildenb

\
 
 \ /
  Last update: 2023-03-27 01:15    [W:0.105 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site