linux-kernel - Re: [PATCH v2 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: <ZzrdL5iSu7/DNoBG@intel.com>
Date: Mon, 18 Nov 2024 14:22:39 +0800
From: Chao Gao <chao.gao@...el.com>
To: Kai Huang <kai.huang@...el.com>
CC: <pbonzini@...hat.com>, <seanjc@...gle.com>, <kvm@...r.kernel.org>,
	<rick.p.edgecombe@...el.com>, <isaku.yamahata@...el.com>,
	<reinette.chatre@...el.com>, <binbin.wu@...ux.intel.com>,
	<xiaoyao.li@...el.com>, <yan.y.zhao@...el.com>, <adrian.hunter@...el.com>,
	<tony.lindgren@...el.com>, <kristen@...ux.intel.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] KVM: VMX: Initialize TDX during KVM module load

>+static int tdx_online_cpu(unsigned int cpu)
>+{
>+	unsigned long flags;
>+	int r;
>+
>+	/* Sanity check CPU is already in post-VMXON */
>+	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
>+
>+	local_irq_save(flags);
>+	r = tdx_cpu_enable();
>+	local_irq_restore(flags);

The comment above tdx_cpu_enable() is outdated because now it may be called
from CPU hotplug rather than IPI function calls only.

Can we relax the assertion lockdep_assert_irqs_disabled() in tdx_cpu_enable()?
looks the requirement is just the enabling work won't be migrated and done to
another CPU.

>+
>+	return r;
>+}
>+
>+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);

...

>+	tdx_cpuhp_state = 0;
>+}
>+
>+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();

this calls cpuhp_remove_state_nocalls(), which acquires cpu locks again,
causing a potential deadlock IIUC.

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

I think "static __init bool" is the preferred order. see

https://www.kernel.org/doc/html/latest/process/coding-style.html#function-prototypes

>+{
>+	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);
>+}
>+
>+static int __init __tdx_bringup(void)
>+{
>+	int r;
>+
>+	/*
>+	 * Enabling TDX requires enabling hardware virtualization first,
>+	 * as making SEAMCALLs requires CPU being in post-VMXON state.
>+	 */
>+	r = kvm_enable_virtualization();
>+	if (r)
>+		return r;
>+
>+	cpus_read_lock();
>+	r = __do_tdx_bringup();
>+	cpus_read_unlock();
>+
>+	if (r)
>+		goto tdx_bringup_err;
>+
>+	/*
>+	 * Leave hardware virtualization enabled after TDX is enabled
>+	 * successfully.  TDX CPU hotplug depends on this.
>+	 */

Shouldn't we make enable_tdx dependent on enable_virt_at_load? Otherwise, if
someone sets enable_tdx=1 and enable_virt_at_load=0, they will get hardware
virtualization enabled at load time while enable_virt_at_load still shows 0.
This behavior is a bit confusing to me.

I think a check against enable_virt_at_load in kvm_can_support_tdx() will work.

The call of kvm_enable_virtualization() here effectively moves
kvm_init_virtualization() out of kvm_init() when enable_tdx=1. I wonder if it
makes more sense to refactor out kvm_init_virtualization() for non-TDX cases
as well, i.e.,

  vmx_init();
  kvm_init_virtualization();
  tdx_init();
  kvm_init();

I'm not sure if this was ever discussed. To me, this approach is better because
TDX code needn't handle virtualization enabling stuff. It can simply check that
enable_virt_at_load=1, assume virtualization is enabled and needn't disable
virtualization on errors.

A bonus is that on non-TDX-capable systems, hardware virtualization won't be
toggled twice at KVM load time for no good reason.

>+	return 0;
>+tdx_bringup_err:
>+	kvm_disable_virtualization();
>+	return r;
>+}