Re: [PATCH v2] KVM: TDX: Reject concurrent change to CPUID entry count

[sashiko-bot@kernel.org]

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