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

[Lisa Wang <wyihan@google.com>]

> > +
> > +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.

In KVM selftests, VM creation is performed on the main thread during test
setup, matching other cached helpers like kvm_get_supported_cpuid(). I
don't think we should care about potential race here.

> [ ... ]
> > +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?

This might not happen in practice, as this allocation happens at the
beginning of VM creation. It would only happen if a previously freed
chunk with non-zero data is reused by malloc(), polluting cpuid->padding.

Still, we can set cpuid->padding = 0 here, or change malloc() to
calloc() in allocate_kvm_cpuid2().