Re: [PATCH] KVM: x86: TDX: Use validated CPUID entry count for TD init

[Binbin Wu <binbin.wu@linux.intel.com>]

On 7/8/2026 10:35 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Lock inversion and deadlock risk: `cpus_read_lock()` is acquired while holding `kvm->lock`, `vcpu->mutex`, and `kvm->slots_lock`, violating the KVM lock ordering hierarchy.
> --
> 
> --- Patch [1]: [PATCH] KVM: x86: TDX: Use validated CPUID entry count for TD init ---
> commit 60173e3dd2d8767ff9b1708338f63d8f3d6d8481
> Author: Binbin Wu <binbin.wu@linux.intel.com>
> 
> KVM: x86: TDX: Use validated CPUID entry count for TD init
> 
> This commit ensures KVM_TDX_INIT_VM uses the validated CPUID entry count 
> when parsing CPUID data. This prevents issues if userspace modifies the 
> input count concurrently during setup.
> 
>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>> index ffe9d0db58c59..b658b03e77508 100644
>> --- a/arch/x86/kvm/vmx/tdx.c
>> +++ b/arch/x86/kvm/vmx/tdx.c
>> @@ -2802,6 +2802,12 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
>>  	if (IS_ERR(init_vm))
>>  		return PTR_ERR(init_vm);
>>  
>> +	/*
>> +	 * Use the validated entry count, as user_data->cpuid.nent may have
>> +	 * changed.
>> +	 */
>> +	init_vm->cpuid.nent = nr_user_entries;
>> +
>>  	if (memchr_inv(init_vm->reserved, 0, sizeof(init_vm->reserved))) {
>>  		ret = -EINVAL;
>>  		goto out;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the KVM_TDX_INIT_VM
> initialization path have a lock inversion?
> 
> Looking at the call chain triggered by the KVM_TDX_INIT_VM ioctl, 
> tdx_vm_ioctl() first acquires the inner KVM locks via 
> tdx_acquire_vm_state_locks():
> 
> arch/x86/kvm/vmx/tdx.c:tdx_vm_ioctl() {
>     ...
>     CLASS(tdx_vm_state_guard, guard)(kvm);
>     ...
>     r = tdx_td_init(kvm, &tdx_cmd);
>     ...
> }
> 
> The tdx_vm_state_guard acquires kvm->lock, all vcpu->mutex locks, and 
> kvm->slots_lock. 
> 
> Then tdx_td_init() continues and calls __tdx_td_init(), where 
> cpus_read_lock() is acquired directly:
> 
> arch/x86/kvm/vmx/tdx.c:__tdx_td_init() {
>     ...
>     cpus_read_lock();
>     ...
> }
> 
> Since KVM locking rules require cpus_read_lock() to be the outermost lock,
> does acquiring it while holding kvm->lock and the other inner locks create
> a deadlock risk against CPU hotplug or other paths that acquire these locks
> in the correct order?
> 

The virt/kvm/locking.rst doesn't explicitly say cpus_read_lock() needs to be the
outermost lock, but
documentation:        cpus_read_lock() is taken outside kvm_lock
                      kvm->lock is taken outside kvm->slots_lock
​set_nx_huge_pages():  takes kvm_lock outside of kvm->slots_lock

So it's transitive that cpus_read_lock() should be taken outside of
kvm->slots_lock while KVM_TDX_INIT_VM takes cpus_read_lock() inside
kvm->slots_lock.

So there could be a deadlock case:
tdx_td_init()               set_nx_huge_pages()        __kvmclock_cpufreq_notifier
  lock(&kvm->slots_lock)                             
                              lock(kvm_lock)
                                                         lock(cpu_hotplug_lock)
                              lock(&kvm->slots_lock)
                                                         lock(kvm_lock)
  lock(cpu_hotplug_lock)

There was a discussion that is related to this issue:
https://lore.kernel.org/kvm/20250124191109.205955-2-pbonzini@redhat.com/
It seems the discussion is still open?

Back to this issue, does it make sense to take cpus_read_lock() before 
kvm->lock during KVM_TDX_INIT_VM as following:

---
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b658b03e7750..90fe7beadc06 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -484,7 +484,7 @@ static void smp_func_do_phymem_cache_wb(void *unused)
 	TDX_BUG_ON(err, TDH_PHYMEM_CACHE_WB, NULL);
 }
 
-void tdx_mmu_release_hkid(struct kvm *kvm)
+static void __tdx_mmu_release_hkid(struct kvm *kvm)
 {
 	bool packages_allocated, targets_allocated;
 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
@@ -494,12 +494,13 @@ void tdx_mmu_release_hkid(struct kvm *kvm)
 	int i;
 	u64 err;
 
+	lockdep_assert_cpus_held();
+
 	if (!is_hkid_assigned(kvm_tdx))
 		return;
 
 	packages_allocated = zalloc_cpumask_var(&packages, GFP_KERNEL);
 	targets_allocated = zalloc_cpumask_var(&targets, GFP_KERNEL);
-	cpus_read_lock();
 
 	kvm_for_each_vcpu(j, vcpu, kvm)
 		tdx_flush_vp_on_cpu(vcpu);
@@ -552,11 +553,16 @@ void tdx_mmu_release_hkid(struct kvm *kvm)
 
 out:
 	mutex_unlock(&tdx_lock);
-	cpus_read_unlock();
 	free_cpumask_var(targets);
 	free_cpumask_var(packages);
 }
 
+void tdx_mmu_release_hkid(struct kvm *kvm)
+{
+	guard(cpus_read_lock)();
+	__tdx_mmu_release_hkid(kvm);
+}
+
 static void tdx_reclaim_td_control_pages(struct kvm *kvm)
 {
 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
@@ -2490,7 +2496,7 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
 	if (!zalloc_cpumask_var(&packages, GFP_KERNEL))
 		goto free_tdcs;
 
-	cpus_read_lock();
+	lockdep_assert_cpus_held();
 
 	/*
 	 * Need at least one CPU of the package to be online in order to
@@ -2552,7 +2558,6 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
 		if (ret)
 			break;
 	}
-	cpus_read_unlock();
 	free_cpumask_var(packages);
 	if (ret) {
 		i = 0;
@@ -2608,13 +2613,12 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
 	if (!kvm_tdx->td.tdcs_pages)
 		kfree(tdcs_pages);
 
-	tdx_mmu_release_hkid(kvm);
+	__tdx_mmu_release_hkid(kvm);
 	tdx_reclaim_td_control_pages(kvm);
 
 	return ret;
 
 free_packages:
-	cpus_read_unlock();
 	free_cpumask_var(packages);
 
 free_tdcs:
@@ -2733,12 +2737,24 @@ static int tdx_read_cpuid(struct kvm_vcpu *vcpu, u32 leaf, u32 sub_leaf,
 	return -EIO;
 }
 
-typedef void *tdx_vm_state_guard_t;
+struct tdx_vm_state_guard {
+	struct kvm *kvm;
+	bool acquire_cpus_read_lock;
+};
 
-static tdx_vm_state_guard_t tdx_acquire_vm_state_locks(struct kvm *kvm)
+static struct tdx_vm_state_guard tdx_acquire_vm_state_locks(struct kvm *kvm,
+							    bool acquire_cpus_read_lock)
 {
+	struct tdx_vm_state_guard g = { .acquire_cpus_read_lock = acquire_cpus_read_lock };
 	int r;
 
+	/*
+	 * When needed, take cpu_hotplug_lock as the outermost lock, to avoid
+	 * lock inversion against the CPU hotplug path.
+	 */
+	if (acquire_cpus_read_lock)
+		cpus_read_lock();
+
 	mutex_lock(&kvm->lock);
 
 	if (kvm->created_vcpus != atomic_read(&kvm->online_vcpus)) {
@@ -2755,23 +2771,33 @@ static tdx_vm_state_guard_t tdx_acquire_vm_state_locks(struct kvm *kvm)
 	 * kvm->slots_lock!
 	 */
 	mutex_lock(&kvm->slots_lock);
-	return kvm;
+	g.kvm = kvm;
+	return g;
 
 out_err:
 	mutex_unlock(&kvm->lock);
-	return ERR_PTR(r);
+	if (acquire_cpus_read_lock)
+		cpus_read_unlock();
+	g.kvm = ERR_PTR(r);
+	return g;
 }
 
-static void tdx_release_vm_state_locks(struct kvm *kvm)
+static void tdx_release_vm_state_locks(struct tdx_vm_state_guard g)
 {
-	mutex_unlock(&kvm->slots_lock);
-	kvm_unlock_all_vcpus(kvm);
-	mutex_unlock(&kvm->lock);
+	if (IS_ERR(g.kvm))
+		return;
+
+	mutex_unlock(&g.kvm->slots_lock);
+	kvm_unlock_all_vcpus(g.kvm);
+	mutex_unlock(&g.kvm->lock);
+	if (g.acquire_cpus_read_lock)
+		cpus_read_unlock();
 }
 
-DEFINE_CLASS(tdx_vm_state_guard, tdx_vm_state_guard_t,
-	     if (!IS_ERR(_T)) tdx_release_vm_state_locks(_T),
-	     tdx_acquire_vm_state_locks(kvm), struct kvm *kvm);
+DEFINE_CLASS(tdx_vm_state_guard, struct tdx_vm_state_guard,
+	     tdx_release_vm_state_locks(_T),
+	     tdx_acquire_vm_state_locks(kvm, acquire_cpus_read_lock),
+	     struct kvm *kvm, bool acquire_cpus_read_lock);
 
 static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 {
@@ -2933,9 +2959,13 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
 	if (tdx_cmd.id == KVM_TDX_CAPABILITIES)
 		return tdx_get_capabilities(&tdx_cmd);
 
-	CLASS(tdx_vm_state_guard, guard)(kvm);
-	if (IS_ERR(guard))
-		return PTR_ERR(guard);
+	/*
+	 * KVM_TDX_INIT_VM programs the per-package encryption keys, which must
+	 * be done under cpus_read_lock().
+	 */
+	CLASS(tdx_vm_state_guard, guard)(kvm, tdx_cmd.id == KVM_TDX_INIT_VM);
+	if (IS_ERR(guard.kvm))
+		return PTR_ERR(guard.kvm);
 
 	switch (tdx_cmd.id) {
 	case KVM_TDX_INIT_VM:
@@ -3307,9 +3337,9 @@ int tdx_vcpu_unlocked_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
 	if (r)
 		return r;
 
-	CLASS(tdx_vm_state_guard, guard)(kvm);
-	if (IS_ERR(guard))
-		return PTR_ERR(guard);
+	CLASS(tdx_vm_state_guard, guard)(kvm, false);
+	if (IS_ERR(guard.kvm))
+		return PTR_ERR(guard.kvm);
 
 	if (!is_hkid_assigned(kvm_tdx) || kvm_tdx->state == TD_STATE_RUNNABLE)
 		return -EINVAL;