Re: [PATCH v3 7/8] KVM: selftests: Test the SNP APIC-ID-list GHCB request

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: 32-bit nr_vcpus passed to 64-bit va_arg in run_apic_id_test()
--

commit 3cc5f05e1197383f380887a6674ec843f8015520
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: selftests: Test the SNP APIC-ID-list GHCB request

This commit adds a KVM selftest exercising the GET_AP_APIC_IDS NAE event from
an SNP guest, testing valid requests, missing RAX validity, zero-sized
requests, invalid GPAs, and the 1024-vCPU descriptor boundary.

> diff --git a/tools/testing/selftests/kvm/x86/sev_snp_apic_id_test.c b/tools/testing/selftests/kvm/x86/sev_snp_apic_id_test.c
> new file mode 100644
> index 0000000000000..8ad9c15091c2d
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/sev_snp_apic_id_test.c

[ ... ]

> +static void guest_code(void *ghcb, gpa_t ghcb_gpa, void *list,
> +		       gpa_t list_gpa, struct apic_id_results *results,
> +		       u64 expected_vcpus)
> +{

[ ... ]

> +	do_get_apic_ids(ghcb, list_gpa, 2, true);
> +	results->valid_info1 = ghcb_get_field(ghcb, GHCB_SAVE_SW_EXIT_INFO_1_OFFSET);
> +	results->valid_info2 = ghcb_get_field(ghcb, GHCB_SAVE_SW_EXIT_INFO_2_OFFSET);
> +	results->nr_entries = desc->nr_entries;
> +	results->first_apic_id = desc->apic_ids[0];
> +	results->last_apic_id = desc->apic_ids[expected_vcpus - 1];
> +
> +terminate:
> +	wrmsr(MSR_AMD64_SEV_ES_GHCB, GHCB_MSR_TERM_REQ);
> +	vmgexit();
> +}
> +
> +static void run_apic_id_test(unsigned int nr_vcpus)
> +{
> +	struct apic_id_results *results;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	gva_t ghcb_gva, list_gva, results_gva;
> +	gpa_t ghcb_gpa, list_gpa;
> +	unsigned int i;
> +
> +	kvm_set_files_rlimit(nr_vcpus);
> +	vm = vm_sev_create_with_one_vcpu(KVM_X86_SNP_VM, guest_code, &vcpu);
> +	for (i = 1; i < nr_vcpus; i++)
> +		__vm_vcpu_add(vm, i);
> +
> +	ghcb_gva = vm_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR,
> +				   MEM_REGION_TEST_DATA);
> +	list_gva = vm_alloc_shared(vm, 2 * PAGE_SIZE, KVM_UTIL_MIN_VADDR,
> +				   MEM_REGION_TEST_DATA);
> +	results_gva = vm_alloc_shared(vm, PAGE_SIZE, KVM_UTIL_MIN_VADDR,
> +				      MEM_REGION_TEST_DATA);
> +	ghcb_gpa = addr_gva2gpa(vm, ghcb_gva);
> +	list_gpa = addr_gva2gpa(vm, list_gva);
> +	results = addr_gva2hva(vm, results_gva);
> +
> +	vcpu_args_set(vcpu, 6, ghcb_gva, ghcb_gpa, list_gva, list_gpa,
> +		      results_gva, nr_vcpus);

[Severity: Medium]
In run_apic_id_test(), nr_vcpus is declared as unsigned int (32-bit) and
passed without a cast as the 6th variadic argument to vcpu_args_set().

In C, default argument promotions do not widen unsigned int to 64 bits, while
tools/testing/selftests/kvm/lib/x86/processor.c:vcpu_args_set() extracts all
variadic arguments as 64-bit u64 values:

    if (num >= 6)
        regs.r9 = va_arg(ap, u64);

Because the first six arguments to vcpu_args_set() consume all six integer
argument registers on x86-64, nr_vcpus is passed on the stack. A 32-bit store
to that stack slot leaves the upper 32 bits uninitialized, and va_arg(ap, u64)
loads all 64 bits into regs.r9, which guest_code() receives as u64
expected_vcpus and uses to index desc->apic_ids[expected_vcpus - 1].

Can this result in reading uninitialized upper bits in expected_vcpus and
indexing out of bounds in desc->apic_ids[]? Should nr_vcpus be cast to u64 (or
declared as u64) when passed to vcpu_args_set()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102114137.1198982-1-joro@8bytes.org?part=7