Re: [PATCH 5/5] KVM: selftests: Test protection of the ucall MMIO region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] KVM: selftests: new standalone reserved_slot_test for single memslot check
- [Medium] KVM: selftests: reserved_slot_test omitted from TEST_GEN_PROGS_loongarch
- [Medium] KVM: selftests: reserved slot 1 in __vm_create() conflicts with vgic_v5
- [Low] KVM: selftests: open-coded kvm_munmap() and exit reason checks in test
--

commit 73bef34a870afd5da66eced7325a3e568ddcd08a
Author: Tharit Tangkijwanichakul <tharitt97@gmail.com>

KVM: selftests: Test protection of the ucall MMIO region

This commit adds a new selftest to verify that creating a memslot overlapping
the ucall MMIO region fails with EEXIST and that ucalls continue to work
before and after the attempt.

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 1f4bc2a1322d8..ff4a5e9733e9f 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -68,6 +68,7 @@ TEST_GEN_PROGS_COMMON += set_memory_region_test
>  TEST_GEN_PROGS_COMMON += memslot_modification_stress_test
>  TEST_GEN_PROGS_COMMON += memslot_perf_test
>  TEST_GEN_PROGS_COMMON += vm_types_test
> +TEST_GEN_PROGS_COMMON += reserved_slot_test
>  
>  # Compiled test targets
>  TEST_GEN_PROGS_x86 = $(TEST_GEN_PROGS_COMMON)

[Severity: Medium]
Should reserved_slot_test also be added to TEST_GEN_PROGS_loongarch?

In tools/testing/selftests/kvm/Makefile.kvm, TEST_GEN_PROGS_COMMON is
included by x86, arm64, s390, and riscv, whereas TEST_GEN_PROGS_loongarch
lists its targets explicitly without including $(TEST_GEN_PROGS_COMMON).

Because main() requires UCALL_EXIT_REASON == KVM_EXIT_MMIO, and across
tools/testing/selftests/kvm/include/*/ucall.h only arm64 and loongarch define
UCALL_EXIT_REASON as KVM_EXIT_MMIO, adding this target only to
TEST_GEN_PROGS_COMMON leaves it unbuilt on LoongArch while building it on
x86, s390, and riscv where it unconditionally skips.

> diff --git a/tools/testing/selftests/kvm/reserved_slot_test.c b/tools/testing/selftests/kvm/reserved_slot_test.c
> new file mode 100644
> index 0000000000000..4a908017ca73d
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/reserved_slot_test.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "test_util.h"
> +
> +#define CLOBBERED_SLOT		TEST_MEM_SLOT_BASE

[Severity: Medium]
In the earlier commits of this series ("KVM: selftests: Introduce a shared
base for test memslots" and "KVM: selftests: Reserve memslot 1 for the ucall
MMIO address"), TEST_MEM_SLOT_BASE was bumped to 2 and __vm_create() in
tools/testing/selftests/kvm/lib/kvm_util.c was updated to reserve
KVM_UCALL_MEMSLOT (slot 1) when KVM_CAP_READONLY_MEM is supported:

	if (vm_check_cap(vm, KVM_CAP_READONLY_MEM))
		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
					    mmio_gpa, KVM_UCALL_MEMSLOT,
					    vm_adjust_num_guest_pages(vm->mode, 1),
					    KVM_MEM_READONLY);

However, tools/testing/selftests/kvm/arm64/vgic_v5.c still defines
VGIC_V5_LPI_MEMSLOT as 1 instead of TEST_MEM_SLOT_BASE:

#define VGIC_V5_LPI_MEMSLOT	1

When vgic_v5_lpi_ist_vm_create() calls __vm_create() followed by
vm_userspace_mem_region_add() with VGIC_V5_LPI_MEMSLOT, won't vm_mem_add()
find slot 1 already present in vm->regions.slot_hash and abort with
TEST_FAIL() on arm64?

> +
> +static void guest_code(void)
> +{
> +	GUEST_SYNC(1);
> +	GUEST_SYNC(2);
> +}
> +
> +static void test_slot1_protected(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_run *run;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	u32 nr_pages;
> +	size_t slot_size;
> +	void *slot_mem;
> +	int ret;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	nr_pages = vm_adjust_num_guest_pages(vm->mode, 1);
> +	slot_size = (size_t)nr_pages * vm->page_size;
> +
> +	run = vcpu->run;
> +	vcpu_run(vcpu);
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_MMIO,
> +		    "Unexpected exit reason = %u", run->exit_reason);
> +	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_SYNC && uc.args[1] == 1,
> +		    "Expected the first sync before clobbering the ucall GPA");
> +
> +	slot_mem = kvm_mmap(slot_size, PROT_READ | PROT_WRITE,
> +			    MAP_PRIVATE | MAP_ANONYMOUS, -1);
> +	ret = __vm_set_user_memory_region2(vm, CLOBBERED_SLOT, 0,
> +					   vm->ucall_mmio_addr, slot_size, slot_mem, 0, 0);
> +	TEST_ASSERT(ret == -1 && errno == EEXIST,
> +		    "Expected the ucall GPA overlap to be rejected: ret=%d errno=%d",
> +		    ret, errno);

[Severity: Medium]
Could this check be added as a subtest in
tools/testing/selftests/kvm/set_memory_region_test.c instead of introducing a
separate reserved_slot_test binary?

set_memory_region_test.c already exercises KVM_SET_USER_MEMORY_REGION and
KVM_SET_USER_MEMORY_REGION2 error handling and overlapping memslot rejection,
whereas having a separate binary for this single check adds build and test
overhead for a target that immediately skips on most architectures in
TEST_GEN_PROGS_COMMON.

> +
> +	vcpu_run(vcpu);
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_MMIO,
> +		    "Unexpected exit reason = %u", run->exit_reason);
> +	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_SYNC && uc.args[1] == 2,
> +		    "Expected the second sync after attempting to clobber the ucall GPA");
> +
> +	kvm_vm_free(vm);
> +	TEST_ASSERT(!munmap(slot_mem, slot_size), "munmap() failed, errno=%d", errno);
> +}

[Severity: Low]
This isn't a bug, but could test_slot1_protected() use
TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_MMIO) from include/test_util.h for
the two exit reason checks, and kvm_munmap(slot_mem, slot_size) from
include/kvm_syscalls.h to pair with kvm_mmap()?

> +
> +int main(void)
> +{
> +	TEST_REQUIRE(UCALL_EXIT_REASON == KVM_EXIT_MMIO);
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_READONLY_MEM));
> +
> +	test_slot1_protected();
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017110213.46199-1-tharitt97@gmail.com?part=5