KVM: TDX: Reject concurrent change to CPUID entry count

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Acquiring cpus_read_lock() while holding kvm->lock and kvm->slots_lock violates the KVM lock ordering hierarchy and creates an AB-BA deadlock risk with CPU hotplug.
- [High] Failing to charge the misc_cg in __tdx_td_init() jumps to a cleanup label that incorrectly uncharges and decrements a global counter.
- [High] Kernel heap buffer overflow when setting up TD params if the TDX module reports a large number of CPUID configurations.
--

--- Patch [1]: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count --- commit 43b81457d393443ba395e2515a2d545aac295cf0
Author: Binbin Wu <[email protected]>

KVM: TDX: Reject concurrent change to CPUID entry count

Reject KVM_TDX_INIT_VM if userspace changes cpuid.nent between the initial read and the subsequent copy of the initialization data.

Quoted 7 lines

arch/x86/kvm/vmx/tdx.c +0 -0

index 2d7a4d52ccfb4..c2349c54d45b5 100644
@@ -2738,7 +2738,11 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 		goto out;
 	}

arch/x86/kvm/vmx/tdx.c static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd) · line 2748

Quoted 10 lines

arch/x86/kvm/vmx/tdx.c +5 -1

 
-	if (init_vm->cpuid.padding) {
+	/*
+	 * Reject the request if userspace changes cpuid.nent between the
+	 * initial read and the subsequent copy.
+	 */
+	if (init_vm->cpuid.padding || init_vm->cpuid.nent != nr_user_entries) {
 		ret = -EINVAL;
 		goto out;
 	}

[Severity: High]
This is a pre-existing issue, but I noticed a potential lock ordering problem in the initialization path called shortly after this check.

In __tdx_td_init(), the code acquires the cpu read lock:

arch/x86/kvm/vmx/tdx.c:__tdx_td_init() {
    ...
    cpus_read_lock();
    ...
}

However, tdx_td_init() is called while the vm state locks are already held higher in the stack:

arch/x86/kvm/vmx/tdx.c:tdx_acquire_vm_state_locks() {
    ...
    mutex_lock(&kvm->lock);
    ...
}

Taking cpus_read_lock() while holding kvm->lock creates a deadlock risk against other subsystems, like cpu hotplug, that hold cpu_hotplug_lock via cpus_write_lock() and subsequently attempt to acquire kvm->lock.

Can this lead to a system-wide deadlock during cpu hotplug operations?

[Severity: High]
This is another pre-existing issue in the same __tdx_td_init() path.

If misc_cg_try_charge() fails, it jumps to the free_hkid label:

arch/x86/kvm/vmx/tdx.c:__tdx_td_init() {
    ...
    ret = misc_cg_try_charge(MISC_CG_RES_TDX, kvm_tdx->misc_cg, 1);
    if (ret)
        goto free_hkid;
    ...
}

The free_hkid cleanup path executes tdx_hkid_free(), which unconditionally decrements the configured hkid counter and uncharges the cgroup:

arch/x86/kvm/vmx/tdx.c:tdx_hkid_free() {
    ...
    atomic_dec(&nr_configured_hkid);
    misc_cg_uncharge(MISC_CG_RES_TDX, kvm_tdx->misc_cg, 1);
    ...
}

Since the initial charge failed and the code jumped past the atomic_inc() for nr_configured_hkid, does this error path underflow the global atomic counter and corrupt the cgroup accounting?

[Severity: High]
This isn't a bug introduced by this patch, but there is a potential out of bounds write later in this function when setting up the td params.

In setup_tdparams_cpuids(), the loop is bounded by num_cpuid_config:

arch/x86/kvm/vmx/tdx.c:setup_tdparams_cpuids() {
    ...
    for (i = 0; i < td_conf->num_cpuid_config; i++) {
        ...
        value = &td_params->cpuid_values[i];
        value->eax = entry->eax;
        ...
    }
}

The td_params structure is strictly sized to 1024 bytes, which only leaves room for 48 elements in the cpuid_values array.

Since tdx_global_metadata.c permits td_conf->num_cpuid_config to be up to 128, will this loop write past the end of the td_params kernel heap allocation if the module reports a large number of cpuid configurations?

Reject KVM_TDX_INIT_VM if userspace changes cpuid.nent between the initial read and the subsequent copy of the initialization data.

tdx_td_init() first reads user_data->cpuid.nent to size the flexible kvm_tdx_init_vm copy. The copied structure also contains cpuid.nent, and that field can differ from the value used to size the allocation if userspace modifies the input concurrently. setup_tdparams_cpuids() later passes init_vm->cpuid.nent to kvm_find_cpuid_entry2(), which uses it as the array bound for the copied entries.

Require the copied count to match the value used to size the allocation so that CPUID parsing cannot access beyond the entries actually copied.

Fixes 0bd0a4a1428b ("KVM: TDX: Replace kmalloc + copy_from_user with memdup_user in tdx_td_init()")

Reported-by Sashiko:gemini-3.1-pro-preview

Cc <[email protected]>

Signed-off-by Binbin Wu <[email protected]>

---
v2:
- Reject the request if mismatch instead overwriting the value. (Thorsten, Rick)
- Lump the check into the existing sanity check on the cpuid field. (Sean)
- "KVM: x86: TDX:" -> "KVM: TDX:" in the shortlog.

v1:
- https://lore.kernel.org/kvm/[email protected]/

---
 arch/x86/kvm/vmx/tdx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/tdx.c +5 -1

index 6ff1469e91cc..d1af0a752e97 100644
@@ -2797,7 +2797,11 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 		goto out;
 	}
 
-	if (init_vm->cpuid.padding) {
+	/*
+	 * Reject the request if userspace changes cpuid.nent between the
+	 * initial read and the subsequent copy.
+	 */
+	if (init_vm->cpuid.padding || init_vm->cpuid.nent != nr_user_entries) {
 		ret = -EINVAL;
 		goto out;
 	}

base-commit: f1e5ada5ab62dbe32350bc161771c9afc6a896de

Signature · 1 line

2.46.0

Reject KVM_TDX_INIT_VM if userspace changes cpuid.nent between the initial read and the subsequent copy of the initialization data.

tdx_td_init() first reads user_data->cpuid.nent to size the flexible kvm_tdx_init_vm copy. The copied structure also contains cpuid.nent, and that field can differ from the value used to size the allocation if userspace modifies the input concurrently. setup_tdparams_cpuids() later passes init_vm->cpuid.nent to kvm_find_cpuid_entry2(), which uses it as the array bound for the copied entries.

Require the copied count to match the value used to size the allocation so that CPUID parsing cannot access beyond the entries actually copied.

Fixes 0bd0a4a1428b ("KVM: TDX: Replace kmalloc + copy_from_user with memdup_user in tdx_td_init()")

Reported-by Sashiko:gemini-3.1-pro-preview

Cc <[email protected]>

Signed-off-by Binbin Wu <[email protected]>

---
v2:
- Reject the request if mismatch instead overwriting the value. (Thorsten, Rick)
- Lump the check into the existing sanity check on the cpuid field. (Sean)
- "KVM: x86: TDX:" -> "KVM: TDX:" in the shortlog.

v1:
- https://lore.kernel.org/kvm/[email protected]/

---
  arch/x86/kvm/vmx/tdx.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/tdx.c +5 -1

index 6ff1469e91cc..d1af0a752e97 100644
@@ -2797,7 +2797,11 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
  		goto out;
  	}
 
-	if (init_vm->cpuid.padding) {
+	/*
+	 * Reject the request if userspace changes cpuid.nent between the
+	 * initial read and the subsequent copy.
+	 */
+	if (init_vm->cpuid.padding || init_vm->cpuid.nent != nr_user_entries) {
  		ret = -EINVAL;
  		goto out;
  	}

base-commit: f1e5ada5ab62dbe32350bc161771c9afc6a896de

Reject KVM_TDX_INIT_VM if userspace changes cpuid.nent between the initial read and the subsequent copy of the initialization data.

tdx_td_init() first reads user_data->cpuid.nent to size the flexible kvm_tdx_init_vm copy. The copied structure also contains cpuid.nent, and that field can differ from the value used to size the allocation if userspace modifies the input concurrently. setup_tdparams_cpuids() later passes init_vm->cpuid.nent to kvm_find_cpuid_entry2(), which uses it as the array bound for the copied entries.

[...]