Re: [PATCH v2 09/16] KVM: device: Add test device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Guest-triggerable out-of-bounds memory access in mmio_read and mmio_write due to invalid size constraints and missing length validation.
- [Critical] Use-after-free vulnerability due to missing IO bus unregistration during device release.
- [High] Registering the KVM IO device without holding `kvm->slots_lock`.
- [High] Memory leak of the parent `struct kvm_device` in the device release callback.
- [High] Ignoring the return value of `kvm_io_bus_register_dev()` leaves the device silently non-functional on failure.
- [High] Incorrect error handling sequence in `kvm_init()` leads to a resource leak of VFIO ops or uninitialized teardown.
--

commit 9fe1265e6415d3c7d4c794808d6fc306d06a4e0e
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

KVM: device: Add test device

Create a KVM test device to help verify mmio reads and write emulation.
This is a simple device that will store the data in a buffer on writes
and echo back that stored data on a read.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 881f92d7a469e..d9bbe570eddd5 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -6559,6 +6564,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  err_gmem:
>  	kvm_vfio_ops_exit();
>  err_vfio:
> +	kvm_mmio_test_ops_exit();
> +err_mmio_test:
>  	kvm_async_pf_deinit();

[Severity: High]
Does this error handling sequence leak VFIO ops or tear down uninitialized
state?

If kvm_mmio_test_ops_init() fails, the code jumps to err_mmio_test, which
only executes kvm_async_pf_deinit() and skips kvm_vfio_ops_exit().

Conversely, if kvm_vfio_ops_init() fails, the code jumps to err_vfio and
executes kvm_mmio_test_ops_exit() before it was initialized. Should the labels
reverse the initialization sequence perfectly?

> diff --git a/virt/kvm/mmio_test.c b/virt/kvm/mmio_test.c
> new file mode 100644
> index 0000000000000..fa84c2b4c5fcf
> --- /dev/null
> +++ b/virt/kvm/mmio_test.c
[ ... ]
> +struct mmio_test {
> +	struct kvm *kvm;
> +	struct kvm_io_device dev;
> +	unsigned long start;
> +	unsigned long size;
> +	char cache[16];
> +};
[ ... ]
> +static int mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> +		     gpa_t addr, int len, void *val)
> +{
> +	struct mmio_test *mmio_test = kvm_to_mmio_test_dev(dev);
> +
> +	if ((addr - mmio_test->start) >= mmio_test->size)
> +		return -1;
> +
> +	/* Write back cached value */
> +	memcpy(val, &mmio_test->cache[(addr - mmio_test->start)], len);

[Severity: Critical]
Can a guest trigger an out-of-bounds read of host memory here?

The cache array is 16 bytes long, but mmio_test->size is configured to 0x16
(22 bytes) in mmio_test_create(). The boundary check permits accesses up to
offset 21.

Furthermore, is there a check to ensure that the access offset plus len
does not exceed the bounds of the array?

> +	return 0;
> +}
> +
> +static int mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> +		      gpa_t addr, int len, const void *val)
> +{
> +	struct mmio_test *mmio_test = kvm_to_mmio_test_dev(dev);
> +
> +	if ((addr - mmio_test->start) >= mmio_test->size)
> +		return -1;
> +
> +	/* Cache value */
> +	memcpy(&mmio_test->cache[(addr - mmio_test->start)], val, len);

[Severity: Critical]
Similarly, can this allow a malicious guest to trigger a heap buffer overflow?

Without a check verifying that the access length fits within the remaining
space in the cache array, a large or offset write could overwrite adjacent
host kernel memory.

> +	return 0;
> +}
[ ... ]
> +static int mmio_test_create(struct kvm_device *dev, u32 type)
> +{
> +	struct mmio_test *mmio_test;
> +
> +	mmio_test = kzalloc(sizeof(*mmio_test), GFP_KERNEL);
> +	if (!mmio_test)
> +		return -ENOMEM;
> +
> +	mmio_test->start = 0x20000000;
> +	mmio_test->size = 0x16;
> +
> +	dev->private = mmio_test;
> +
> +	kvm_iodevice_init(&mmio_test->dev, &mmio_ops);
> +	kvm_io_bus_register_dev(dev->kvm, KVM_MMIO_BUS, mmio_test->start,
> +				mmio_test->size, &mmio_test->dev);

[Severity: High]
Does kvm_io_bus_register_dev() require kvm->slots_lock to be held?

The function modifies RCU-protected bus arrays and has a lockdep assertion
for kvm->slots_lock. This is called under kvm->lock but appears to be missing
the slots_lock acquisition.

[Severity: High]
What happens if kvm_io_bus_register_dev() fails here?

The function could return -ENOMEM or -ENOSPC, but its return value is not
checked. Could this leave the test device reported as fully instantiated
without being able to receive MMIO traps?

> +
> +	return 0;
> +}
> +
> +static void mmio_test_release(struct kvm_device *dev)
> +{
> +	kfree(dev->private);

[Severity: Critical]
Does freeing dev->private without unregistering the io device create a
use-after-free?

When the file descriptor is closed, the device state is freed, but the
embedded kvm_io_device remains registered on the KVM MMIO bus. Any subsequent
guest MMIO access to that region would route the request to this freed memory.
Should this call kvm_io_bus_unregister_dev() first?

[Severity: High]
Is the parent kvm_device leaked here?

When a KVM device implements the release callback instead of destroy, it is
responsible for freeing the core kvm_device object allocated by KVM core.
Should there be a call to kfree(dev) before returning?

> +}

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