* Re: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count
2026-07-10 3:53 [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count Binbin Wu
@ 2026-07-10 4:01 ` sashiko-bot
2026-07-10 8:57 ` Thorsten Blum
2026-07-10 9:18 ` Xiaoyao Li
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-10 4:01 UTC (permalink / raw)
To: Binbin Wu; +Cc: kvm
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 <binbin.wu@linux.intel.com>
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.
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 2d7a4d52ccfb4..c2349c54d45b5 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2738,7 +2738,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;
> }
[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?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710035324.3170534-1-binbin.wu@linux.intel.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count
2026-07-10 3:53 [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count Binbin Wu
2026-07-10 4:01 ` sashiko-bot
@ 2026-07-10 8:57 ` Thorsten Blum
2026-07-10 9:18 ` Xiaoyao Li
2 siblings, 0 replies; 4+ messages in thread
From: Thorsten Blum @ 2026-07-10 8:57 UTC (permalink / raw)
To: Binbin Wu
Cc: linux-kernel, kvm, stable, seanjc, pbonzini, kas,
rick.p.edgecombe
On Fri, Jul 10, 2026 at 11:53:23AM +0800, Binbin Wu wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
LGTM, and thanks for fixing this!
Reviewed-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> 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/20260708022937.2465796-1-binbin.wu@linux.intel.com/
> ---
> arch/x86/kvm/vmx/tdx.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 6ff1469e91cc..d1af0a752e97 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -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
> --
> 2.46.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count
2026-07-10 3:53 [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count Binbin Wu
2026-07-10 4:01 ` sashiko-bot
2026-07-10 8:57 ` Thorsten Blum
@ 2026-07-10 9:18 ` Xiaoyao Li
2 siblings, 0 replies; 4+ messages in thread
From: Xiaoyao Li @ 2026-07-10 9:18 UTC (permalink / raw)
To: Binbin Wu, linux-kernel, kvm
Cc: stable, seanjc, pbonzini, kas, rick.p.edgecombe, thorsten.blum
On 7/10/2026 11:53 AM, Binbin Wu wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> 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/20260708022937.2465796-1-binbin.wu@linux.intel.com/
> ---
> arch/x86/kvm/vmx/tdx.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 6ff1469e91cc..d1af0a752e97 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -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
^ permalink raw reply [flat|nested] 4+ messages in thread