linux-kernel - Re: [PATCH v1 1/3] x86/tdx: Check for TDX partitioning during early TDX init

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: <0799b692-4b26-4e00-9cec-fdc4c929ea58@linux.microsoft.com>
Date:   Wed, 22 Nov 2023 18:19:20 +0100
From:   Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-hyperv@...r.kernel.org, stefan.bader@...onical.com,
        tim.gardner@...onical.com, roxana.nicolescu@...onical.com,
        cascardo@...onical.com, kys@...rosoft.com, haiyangz@...rosoft.com,
        wei.liu@...nel.org, sashal@...nel.org, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Michael Kelley <mhkelley58@...il.com>,
        Nikolay Borisov <nik.borisov@...e.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tom Lendacky <thomas.lendacky@....com>, x86@...nel.org,
        Dexuan Cui <decui@...rosoft.com>
Subject: Re: [PATCH v1 1/3] x86/tdx: Check for TDX partitioning during early
 TDX init

On 22/11/2023 18:01, Jeremi Piotrowski wrote:
> Check for additional CPUID bits to identify TDX guests running with Trust
> Domain (TD) partitioning enabled. TD partitioning is like nested virtualization
> inside the Trust Domain so there is a L1 TD VM(M) and there can be L2 TD VM(s).
> 
> In this arrangement we are not guaranteed that the TDX_CPUID_LEAF_ID is visible
> to Linux running as an L2 TD VM. This is because a majority of TDX facilities
> are controlled by the L1 VMM and the L2 TDX guest needs to use TD partitioning
> aware mechanisms for what's left. So currently such guests do not have
> X86_FEATURE_TDX_GUEST set.
> 
> We want the kernel to have X86_FEATURE_TDX_GUEST set for all TDX guests so we
> need to check these additional CPUID bits, but we skip further initialization
> in the function as we aren't guaranteed access to TDX module calls.
> 
> Cc: <stable@...r.kernel.org> # v6.5+
> Signed-off-by: Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
> ---
>  arch/x86/coco/tdx/tdx.c    | 29 ++++++++++++++++++++++++++---
>  arch/x86/include/asm/tdx.h |  3 +++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..c7bbbaaf654d 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -8,6 +8,7 @@
>  #include <linux/export.h>
>  #include <linux/io.h>
>  #include <asm/coco.h>
> +#include <asm/hyperv-tlfs.h>
>  #include <asm/tdx.h>
>  #include <asm/vmx.h>
>  #include <asm/insn.h>
> @@ -37,6 +38,8 @@
>  
>  #define TDREPORT_SUBTYPE_0	0
>  
> +bool tdx_partitioning_active;
> +
>  /* Called from __tdx_hypercall() for unrecoverable failure */
>  noinstr void __tdx_hypercall_failed(void)
>  {
> @@ -757,19 +760,38 @@ static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>  	return true;
>  }
>  
> +
> +static bool early_is_hv_tdx_partitioning(void)
> +{
> +	u32 eax, ebx, ecx, edx;
> +	cpuid(HYPERV_CPUID_ISOLATION_CONFIG, &eax, &ebx, &ecx, &edx);
> +	return eax & HV_PARAVISOR_PRESENT &&
> +	       (ebx & HV_ISOLATION_TYPE) == HV_ISOLATION_TYPE_TDX;
> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	u64 cc_mask;
>  	u32 eax, sig[3];
>  
>  	cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2],  &sig[1]);
> -
> -	if (memcmp(TDX_IDENT, sig, sizeof(sig)))
> -		return;
> +	if (memcmp(TDX_IDENT, sig, sizeof(sig))) {
> +		tdx_partitioning_active = early_is_hv_tdx_partitioning();
> +		if (!tdx_partitioning_active)
> +			return;
> +	}

Hi Borislav,

Just wanted to run another option by you. Instead of checking the CPUID here we
could accomplish the same result by doing _this_ in the hyperv cc init:

diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 8c6bf07f7d2b..705794642d34 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -595,6 +595,8 @@ void __init hv_vtom_init(void)
 #endif
 
 	case HV_ISOLATION_TYPE_TDX:
+		setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
+		tdx_partitioning_active = true;
 		cc_vendor = CC_VENDOR_INTEL;
 		break;
 

Which approach do you prefer?

Thanks,
Jeremi

>  
>  	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
>  
>  	cc_vendor = CC_VENDOR_INTEL;
> +
> +	/*
> +	 * Need to defer cc_mask and page visibility callback initializations
> +	 * to a TD-partitioning aware implementation.
> +	 */
> +	if (tdx_partitioning_active)
> +		goto exit;
> +
>  	tdx_parse_tdinfo(&cc_mask);
>  	cc_set_mask(cc_mask);
>  
> @@ -820,5 +842,6 @@ void __init tdx_early_init(void)
>  	 */
>  	x86_cpuinit.parallel_bringup = false;
>  
> +exit:
>  	pr_info("Guest detected\n");
>  }
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 603e6d1e9d4a..fe22f8675859 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -52,6 +52,7 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>  
>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>  
> +extern bool tdx_partitioning_active;
>  #else
>  
>  static inline void tdx_early_init(void) { };
> @@ -71,6 +72,8 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
>  {
>  	return -ENODEV;
>  }
> +
> +#define tdx_partitioning_active false
>  #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASM_X86_TDX_H */