Re: [PATCH v3 11/17] KVM: riscv: selftests: Add mmio test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The selftest unconditionally creates a KVM_DEV_TYPE_TEST device without checking for host support, causing hard failures on standard kernels.
- [Medium] Missing "memory" clobber in inline assembly allows compiler to reorder or eliminate MMIO stores and local variable initializations.
- [Low] Overly broad inline assembly constraints for RISC-V compressed instructions will cause intermittent assembler errors.

Pre-existing issues:
- [High] The KVM MMIO test device fails to unregister its MMIO bus device on teardown, leading to a Use-After-Free when the memory is subsequently freed.
--

--- Patch [11]: [PATCH v3 11/17] KVM: riscv: selftests: Add mmio test ---
commit 39a55e4a273642e2ad8e96502e3f7d6eb84c69ad
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

KVM: riscv: selftests: Add mmio test

This commit adds a test to validate that KVM properly emulates reads
and writes to a KVM test device. It checks all load and store
instructions emulated by KVM for RISC-V.

> diff --git a/tools/testing/selftests/kvm/riscv/mmio_test.c b/tools/testing/selftests/kvm/riscv/mmio_test.c
> new file mode 100644
> index 0000000000000..ea3837771a333
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/riscv/mmio_test.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +#define test_standard_read(len, instruction, name, options)					\
> +static void test_##name(void)									\
> +{												\
> +	unsigned long mmio_result, reference_result;						\
> +	/* Configure the MMIO to return 0xff for each byte to check sign extension */		\
> +	unsigned long reference = ((unsigned long)-1 >> ((sizeof(long) - len) * 8));		\
> +	*((unsigned long *)MMIO_TEST_REGION) = reference;					\
> +	/* Check that reads through MMIO are equivalent to standard reads. */			\
> +	asm volatile (										\
> +		".option push\n"								\
> +		options										\
> +		#instruction "	%[mmio_res], 0(%[region])\n"					\
> +		#instruction "	%[ref_res], 0(%[ref])\n"					\
> +		".option pop\n"									\
> +		: [mmio_res] "=&cr" (mmio_result), [ref_res] "=&cr" (reference_result)		\
> +		: [region] "cr" (MMIO_TEST_REGION), [ref] "cr" (&reference)			\
> +	);											\

[Severity: Medium]
Since the pointer to MMIO_TEST_REGION lacks a volatile qualifier and the
asm volatile block lacks a "memory" clobber, can the compiler optimize away
or reorder the store to MMIO_TEST_REGION?

GCC's dead store elimination might eliminate the initialization store since
it does not see any memory inputs to the assembly block, which could cause
the guest to execute with uninitialized memory and fail intermittently.

[Severity: Low]
Will using the "cr" constraint cause intermittent assembler errors for
compressed instructions?

For instructions like c.lw and c.sw, the assembler strictly requires
registers in the x8-x15 range. If GCC allocates a general register outside
this range, will the GNU Assembler fail with illegal operands?

> +	GUEST_ASSERT_EQ(mmio_result, reference_result);						\
> +	GUEST_DONE();										\
> +}
> +
> +#define test_sp_read(len, instruction, name)							\
> +static void test_##name(void)									\
> +{												\
> +	unsigned long mmio_result, reference_result;						\
> +	unsigned long tmp;									\
> +	/* Configure the MMIO to return 0xff for each byte to check sign extension */		\
> +	unsigned long reference = ((unsigned long)-1 >> ((sizeof(long) - len) * 8));		\
> +	*(((unsigned long *)MMIO_TEST_REGION)) = reference;					\

[Severity: Medium]
Similarly to test_standard_read(), could the missing "memory" clobber or
volatile qualifier here allow the compiler to eliminate the reference store?

[ ... ]
> +#define test_standard_write(len, write, read, name, options)					\
> +static void test_##name(void)									\
> +{												\
> +	unsigned long result;									\
> +	unsigned long reference = (0x5555555555555555ULL >> ((sizeof(long) - len) * 8));	\
> +	/* Check that we can write and then read the same value. */				\
> +	asm volatile (										\
> +		".option push\n"								\
> +		options										\
> +		#write "	%[ref], 0(%[region])\n"						\
> +		#read "		%[res], 0(%[region])\n"						\
> +		".option pop\n"									\
> +		: [res] "=&cr" (result)								\
> +		: [region] "cr" (MMIO_TEST_REGION), [ref] "cr" (reference)			\
> +	);											\

[Severity: Low]
Could the "cr" constraint also cause assembler errors here when the macro
is expanded for compressed instructions like c.sw and c.lw?

[ ... ]
> +static void run(void *guest_code, char *instruction)
> +{
> +	int fd;
> +	struct ucall uc;
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	fd = kvm_create_device(vm, KVM_DEV_TYPE_TEST);

[Severity: Medium]
If the host kernel does not have CONFIG_KVM_MMIO_TEST enabled, will
kvm_create_device() fail with -ENODEV and cause the test to abort?

Would it be better to dynamically check for capabilities and skip
gracefully (for example, by using TEST_REQUIRE() or similar test helpers)
instead of failing?


[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free
during teardown in virt/kvm/mmio_test.c.

In mmio_test_destroy():

virt/kvm/mmio_test.c:mmio_test_destroy() {
        ...
        struct mmio_test *mmio_test = kvm_to_mmio_test_dev(dev);
        ...
        kvm_io_bus_unregister_dev(dev->kvm, KVM_MMIO_BUS, &mmio_test->dev);
        kfree(dev->private);
}

Does kvm_to_mmio_test_dev() expect a struct kvm_io_device pointer rather
than a struct kvm_device pointer?

If so, container_of() might compute an invalid pointer, causing
kvm_io_bus_unregister_dev() to fail to unregister the actual mmio_test->dev.
When the structure is later freed, will this leave a dangling pointer in
the KVM MMIO bus that triggers a use-after-free on subsequent MMIO
accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=11