* 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-13 2:16 ` Binbin Wu
2026-07-10 8:57 ` Thorsten Blum
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ 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] 6+ messages in thread* Re: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count
2026-07-10 4:01 ` sashiko-bot
@ 2026-07-13 2:16 ` Binbin Wu
0 siblings, 0 replies; 6+ messages in thread
From: Binbin Wu @ 2026-07-13 2:16 UTC (permalink / raw)
To: sashiko-reviews; +Cc: kvm, Sean Christopherson
On 7/10/2026 12:01 PM, sashiko-bot@kernel.org wrote:
> 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?
This is considered as a set_nx_huge_pages() problem, not a TDX
problem per the discussion.
https://lore.kernel.org/kvm/ak5L2csITyi1tGp0@google.com/
>
> [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?
nr_configured_hkid has been removed on kvm-x86's next branch by Sean's patches
https://lore.kernel.org/all/20260214012702.2368778-14-seanjc@google.com/
https://lore.kernel.org/all/20260214012702.2368778-15-seanjc@google.com/
But the issue raised for cgroup accounting is valid.
Will cook a patch to fix it.
>
> [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?
>
The td_params structure is fixed to 1024byte, which is defined by the TDX spec
and the value of td_conf->num_cpuid_config is read from the TDX module.
So, this should not happen unless there is a bug in the TDX module.
Not sure if KVM should do a preventive check.
^ permalink raw reply [flat|nested] 6+ 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
2026-07-14 18:41 ` Sean Christopherson
3 siblings, 0 replies; 6+ 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] 6+ 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
2026-07-14 18:41 ` Sean Christopherson
3 siblings, 0 replies; 6+ 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] 6+ 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
` (2 preceding siblings ...)
2026-07-10 9:18 ` Xiaoyao Li
@ 2026-07-14 18:41 ` Sean Christopherson
3 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2026-07-14 18:41 UTC (permalink / raw)
To: Sean Christopherson, linux-kernel, kvm, Binbin Wu
Cc: stable, pbonzini, kas, rick.p.edgecombe, thorsten.blum
On Fri, 10 Jul 2026 11:53:23 +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.
>
> [...]
Applied to kvm-x86 fixes, thanks!
[1/1] KVM: TDX: Reject concurrent change to CPUID entry count
https://github.com/kvm-x86/linux/commit/cfbebb55e512
--
https://github.com/kvm-x86/linux/tree/next
^ permalink raw reply [flat|nested] 6+ messages in thread