Re: [PATCH v15 03/23] KVM: selftests: Initialize the TDX VM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/kvm: uninitialized padding causes KVM_TDX_INIT_VM failure
- [Medium] selftests/kvm: data race on static tdx_cap in tdx_read_capabilities()
--

commit 3680cc9e7726e14977411713d50e23fbb8635ddc
Author: Sagi Shahar <sagis@google.com>

KVM: selftests: Initialize the TDX VM

Add tdx_init_vm() to handle the mandatory VM-level initialization
sequence required for Intel TDX. For TDX, the VM's CPUID configuration
must be "sealed" during KVM_TDX_INIT_VM before any vCPUs are created.
This commit masks the host-supported CPUID against the configurable
bits reported by KVM_TDX_CAPABILITIES.

> diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> new file mode 100644
> index 0000000000000..3a8900ff25404
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "processor.h"
> +#include "tdx/tdx_util.h"
> +
> +static const struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
> +{
> +	static struct kvm_tdx_capabilities *tdx_cap;
> +	int nr_cpuid_configs = 4;
> +	int rc = -1;
> +	int i;
> +
> +	if (tdx_cap)
> +		return tdx_cap;
> +
> +	do {
> +		nr_cpuid_configs *= 2;
> +
> +		tdx_cap = realloc(tdx_cap, sizeof(*tdx_cap) +
> +					   (sizeof(struct kvm_cpuid_entry2) * nr_cpuid_configs));

[Severity: Medium]
Could this realloc() cause a data race if multiple threads attempt to
create TDX VMs concurrently?

If tdx_read_capabilities() checks and reallocates the static pointer
tdx_cap without locking, concurrent calls might race on the realloc(),
potentially leading to memory corruption, a double free, or a
use-after-free during test execution.

[ ... ]
> +void tdx_init_vm(struct kvm_vm *vm)
> +{
> +	struct kvm_tdx_init_vm *init_vm;
> +	const struct kvm_cpuid2 *tmp;
> +	struct kvm_cpuid2 *cpuid;
> +
> +	tmp = kvm_get_supported_cpuid();
> +
> +	cpuid = allocate_kvm_cpuid2(tmp->nent);
> +	memcpy(cpuid, tmp, kvm_cpuid2_size(tmp->nent));
> +	tdx_filter_cpuid(vm, cpuid);
> +
> +	init_vm = calloc(1, sizeof(*init_vm) +
> +			 sizeof(init_vm->cpuid.entries[0]) * cpuid->nent);
> +	TEST_ASSERT(init_vm, "init_vm allocation failed");
> +
> +	memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));

[Severity: Medium]
Does this memcpy() copy uninitialized padding into the ioctl payload?

Since allocate_kvm_cpuid2() allocates struct kvm_cpuid2 using malloc(),
the padding field can be left uninitialized. The kernel's
KVM_GET_SUPPORTED_CPUID ioctl does not clear this padding, and it is
subsequently copied into init_vm->cpuid here.

Because the KVM_TDX_INIT_VM ioctl strictly validates that cpuid.padding
is zero, could this cause KVM_TDX_INIT_VM to fail with -EINVAL if the
padding contains garbage?

> +	free(cpuid);
> +
> +	init_vm->attributes = 0;
> +	init_vm->xfam = 0;
> +
> +	tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);
> +
> +	free(init_vm);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-tdx-selftests-v15-0-7c62a5d8a992@google.com?part=3