linux-kernel - Re: [PATCH 3/3] KVM: VMX: Initialize TDX during KVM module load

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: <Z0fc3Z6YJB20uP3G@intel.com>
Date: Thu, 28 Nov 2024 11:00:45 +0800
From: Chao Gao <chao.gao@...el.com>
To: Paolo Bonzini <pbonzini@...hat.com>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<kai.huang@...el.com>
Subject: Re: [PATCH 3/3] KVM: VMX: Initialize TDX during KVM module load

>+static void __do_tdx_cleanup(void)
>+{
>+	/*
>+	 * Once TDX module is initialized, it cannot be disabled and
>+	 * re-initialized again w/o runtime update (which isn't
>+	 * supported by kernel).  Only need to remove the cpuhp here.
>+	 * The TDX host core code tracks TDX status and can handle
>+	 * 'multiple enabling' scenario.
>+	 */
>+	WARN_ON_ONCE(!tdx_cpuhp_state);
>+	cpuhp_remove_state_nocalls(tdx_cpuhp_state);

...

>+static int __init __do_tdx_bringup(void)
>+{
>+	int r;
>+
>+	/*
>+	 * TDX-specific cpuhp callback to call tdx_cpu_enable() on all
>+	 * online CPUs before calling tdx_enable(), and on any new
>+	 * going-online CPU to make sure it is ready for TDX guest.
>+	 */
>+	r = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
>+					 "kvm/cpu/tdx:online",
>+					 tdx_online_cpu, NULL);
>+	if (r < 0)
>+		return r;
>+
>+	tdx_cpuhp_state = r;
>+
>+	r = tdx_enable();
>+	if (r)
>+		__do_tdx_cleanup();

The self deadlock issue isn't addressed.

>+
>+	return r;
>+}
>+
>+static bool __init kvm_can_support_tdx(void)
>+{
>+	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);

...

>+int __init tdx_bringup(void)
>+{
>+	int r;
>+
>+	if (!enable_tdx)
>+		return 0;
>+
>+	if (!kvm_can_support_tdx()) {
>+		pr_err("tdx: no TDX private KeyIDs available\n");

The lack of X86_FEATURE_TDX_HOST_PLATFORM doesn't necessarily mean no TDX
private KeyIDs. In tdx_init(), X86_FEATURE_TDX_HOST_PLATFORM is not set if
hibernation is enabled.

>+		goto success_disable_tdx;
>+	}
>+
>+	if (!enable_virt_at_load) {
>+		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
>+		goto success_disable_tdx;
>+	}
>+
>+	/*
>+	 * Ideally KVM should probe whether TDX module has been loaded
>+	 * first and then try to bring it up.  But TDX needs to use SEAMCALL
>+	 * to probe whether the module is loaded (there is no CPUID or MSR
>+	 * for that), and making SEAMCALL requires enabling virtualization
>+	 * first, just like the rest steps of bringing up TDX module.
>+	 *
>+	 * So, for simplicity do everything in __tdx_bringup(); the first
>+	 * SEAMCALL will return -ENODEV when the module is not loaded.  The
>+	 * only complication is having to make sure that initialization
>+	 * SEAMCALLs don't return TDX_SEAMCALL_VMFAILINVALID in other
>+	 * cases.
>+	 */
>+	r = __tdx_bringup();
>+	if (r) {
>+		/*
>+		 * Disable TDX only but don't fail to load module if
>+		 * the TDX module could not be loaded.  No need to print
>+		 * message saying "module is not loaded" because it was
>+		 * printed when the first SEAMCALL failed.
>+		 */
>+		if (r == -ENODEV)
>+			goto success_disable_tdx;

Given that two error messages are added above, I think it is worth adding one
more here for consistency. And, reloading KVM will not trigger the "module is
not loaded" error which is printed once.

>+
>+		enable_tdx = 0;
>+	}
>+
>+	return r;
>+
>+success_disable_tdx:
>+	enable_tdx = 0;
>+	return 0;
>+}