Re: [PATCH v2 10/16] KVM: riscv: selftests: Add mmio test

[sashiko-bot@kernel.org]

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

New issues:
- [High] The test pattern for MMIO writes uses a 32-bit literal, causing it to evaluate to 0 when shifted on 64-bit architectures.
- [Medium] The compressed store instruction `c.sdsp` is entirely omitted from the test suite.
- [Medium] Standard 16-bit loads (`lh` and `lhu`) are erroneously restricted to 64-bit RISC-V builds.
- [Medium] The KVM device file descriptor returned by `kvm_create_device` is leaked, which also leaks the KVM VM.
- [Medium] The selftest fails to gracefully skip when KVM_DEV_TYPE_TEST is not supported by the kernel.

Pre-existing issues:
- [Critical] The KVM MMIO test device contains a heap buffer overflow and out-of-bounds read vulnerability due to a mismatch between the registered device size and its internal cache array capacity.
- [Critical] The mmio_test device fails to unregister itself from the KVM MMIO bus upon release, leading to a Use-After-Free vulnerability.
- [High] `mmio_test_create()` registers the device to the KVM IO bus without holding the required `kvm->slots_lock`.
- [High] The error cleanup path in `kvm_init()` unwinds initializations in the wrong order.
- [High] The return value of `kvm_io_bus_register_dev()` is ignored, potentially leaking an unregistered device.
- [High] The `struct kvm_device` allocated for the test device is permanently leaked.
--

commit 91109e879d023e4a9b60888f79545187037508bf
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>
KVM: riscv: selftests: Add mmio test

This commit adds a new MMIO selftest for KVM on RISC-V. It introduces test
cases for various read and write instructions, including compressed
instructions, to ensure KVM properly emulates MMIO accesses.

> 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..9726860a269a3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/riscv/mmio_test.c
> @@ -0,0 +1,184 @@
[ ... ]
> +test_standard_read(4, lw, lw, "")
> +test_standard_read(4, c.lw, c_lw, ".option arch,+c\n")
> +test_sp_read(4, c.lwsp, c_lwsp)
> +
> +#if __riscv_xlen == 64
> +test_standard_read(2, lh, lh, "")
> +test_standard_read(2, lhu, lhu, "")

[Severity: Medium]
Are lh and lhu intentionally restricted to 64-bit architectures here?

Since these are standard 16-bit loads in the RV32I base instruction set,
should they be moved outside the __riscv_xlen == 64 check so they are also
tested on 32-bit platforms?

> +test_standard_read(4, lwu, lwu, "")
> +test_standard_read(8, ld, ld, "")
[ ... ]
> +#define test_standard_write(len, write, read, name, options)					\
> +static void test_##name(void)									\
> +{												\
> +	unsigned long result;									\
> +	unsigned long reference = (0x55555555UL >> ((sizeof(long) - len) * 8));			\

[Severity: High]
Does this bitwise shift evaluate to 0 on 64-bit systems?

When __riscv_xlen == 64, sizeof(long) is 8. For a 32-bit store (len=4),
this shifts 0x55555555UL right by 32 bits, resulting in 0.

For len=1 and len=2, it shifts by 56 and 48 bits, also resulting in 0.

Could this mask sign-extension or truncation issues by only ever verifying
a value of 0?

Should this use a 64-bit literal like 0x5555555555555555ULL instead?

> +	/* Check that we can write and then read the same value. */				\
> +	asm volatile (										\
[ ... ]
> +#define test_sp_write(len, write, read, name)							\
> +static void test_##name(void)									\
> +{												\
> +	unsigned long result;									\
> +	unsigned long tmp;									\
> +	unsigned long reference = (0x55555555UL >> ((sizeof(long) - len) * 8));			\

[Severity: High]
Similar to test_standard_write, does this shift evaluate to 0 on 64-bit
systems for compressed stack-pointer relative writes?

> +	/* Check that we can write and then read the same value. */				\
[ ... ]
> +static void run(void *guest_code, char *instruction)
> +{
> +	struct ucall uc;
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	kvm_create_device(vm, KVM_DEV_TYPE_TEST);

[Severity: Medium]
Does this test need to gracefully skip if KVM_DEV_TYPE_TEST is unavailable?

kvm_create_device() uses TEST_ASSERT internally and will abort the test
if the kernel lacks support.

Would it be safer to check support with __kvm_create_device() and use
TEST_REQUIRE() to exit with a skip code in CI environments running older
kernels?

[Severity: Medium]
Does this leak the KVM device file descriptor returned by kvm_create_device()?

Since the file descriptor holds a reference to the KVM VM (kvm_get_kvm),
failing to close it will prevent the VM from being destroyed when
kvm_vm_free(vm) is called.

Since run() iterates for multiple instructions, will this accumulate file
descriptor and kernel memory leaks?

> +
> +	virt_map(vm, (unsigned long)MMIO_TEST_REGION, MMIO_TEST_REGION, 1);
[ ... ]
> +void test_mmio_read_sign_extension(void)
> +{
> +	run(test_lb, "lb");
[ ... ]
> +	run(test_c_lwsp, "c.lwsp");
> +
> +#if __riscv_xlen == 64
> +	run(test_lh, "lh");
> +	run(test_lhu, "lhu");

[Severity: Medium]
Are the executions for lh and lhu also intentionally restricted to 64-bit
builds here?

> +	run(test_lwu, "lwu");
[ ... ]
> +void test_mmio_write(void)
> +{
> +	run(test_sb, "sb");
> +	run(test_sh, "sh");
> +	run(test_sw, "sw");
> +	run(test_c_sw, "c.sw");
> +	run(test_c_swsp, "c.swsp");
> +
> +#if __riscv_xlen == 64
> +	run(test_sd, "sd");
> +	run(test_c_sd, "c.sd");

[Severity: Medium]
Is the compressed store instruction c.sdsp omitted from the 64-bit write
tests?

The commit message notes that all emulated load and store instructions are
checked, but c.sdsp appears to be missing an instantiation via
test_sp_write and execution via run(test_c_sdsp, "c.sdsp").

> +#endif
> +}

While reviewing how this selftest interacts with KVM_DEV_TYPE_TEST, I
noticed several pre-existing issues in the test device implementation itself
(virt/kvm/mmio_test.c and virt/kvm/kvm_main.c). I have included them here
for completeness:

virt/kvm/mmio_test.c:mmio_test_create():
    mmio_test->size = 0x16;

[Severity: Critical]
This is a pre-existing issue, but does mmio_test->size exceed the capacity
of the internal cache array?

The internal cache array is sized at 16 bytes (char cache[16];), but the
size field is initialized to 0x16 (22 bytes).

When a guest accesses an MMIO address near the end of the registered region,
the addr - start + len calculation inside mmio_write() and mmio_read()
can exceed 16.

Can this allow an unprivileged guest to trigger an out-of-bounds memcpy(),
leading to host heap corruption or information leaks?


virt/kvm/mmio_test.c:mmio_test_release():
static void mmio_test_release(struct kvm_device *dev)
{
    kfree(dev->private);
}

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free vulnerability here?

The release callback frees the backing dev->private structure, but it does
not call kvm_io_bus_unregister_dev().

The KVM MMIO bus retains a dangling pointer to the freed structure.

Will any subsequent MMIO access to the registered range by an active vCPU
dereference this freed pointer?


[Severity: High]
This is a pre-existing issue, but does mmio_test_release() leak the
struct kvm_device?

kvm_ioctl_create_device() dynamically allocates the struct kvm_device
using kzalloc().

The device implementation's .release or .destroy callback is responsible
for freeing this memory, but it appears to be omitted here.

Does this cause a permanent leak every time a test device is closed?


virt/kvm/mmio_test.c:mmio_test_create():
    kvm_io_bus_register_dev(dev->kvm, KVM_MMIO_BUS, mmio_test->start,
                            mmio_test->size, &mmio_test->dev);

[Severity: High]
This is a pre-existing issue, but is kvm_io_bus_register_dev() called
without holding the required kvm->slots_lock?

mmio_test_create() is invoked via kvm_ioctl_create_device(), which only
holds kvm->lock.

kvm_io_bus_register_dev() strictly requires kvm->slots_lock and enforces
it via lockdep.

Will this cause an immediate lockdep splat and a data race against concurrent
MMIO bus updates?


[Severity: High]
This is a pre-existing issue, but is the return value of
kvm_io_bus_register_dev() safely ignored?

The function can fail (e.g., due to -ENOMEM or hitting NR_IOBUS_DEVS).

Since mmio_test_create() ignores the return value and unconditionally
returns 0, will userspace receive a valid fd for a device that was never
actually attached to the MMIO bus?


virt/kvm/kvm_main.c:kvm_init():
err_gmem:
    kvm_vfio_ops_exit();
err_vfio:
    kvm_mmio_test_ops_exit();
err_mmio_test:
    kvm_async_pf_deinit();

[Severity: High]
This is a pre-existing issue, but are the error unwinding labels interleaved
incorrectly?

kvm_init() initializes components in the order: VFIO, MMIO_TEST, and GMEM.

If kvm_gmem_init() fails, it jumps to err_gmem which executes
kvm_vfio_ops_exit() followed by kvm_mmio_test_ops_exit(), which is the
reverse of the correct unwinding order.

Furthermore, if kvm_vfio_ops_init() fails, it jumps to err_vfio and
attempts to exit mmio_test (which hasn't been initialized yet).

If kvm_mmio_test_ops_init() fails, it skips unwinding VFIO entirely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=10