Re: [PATCH v3 10/17] KVM: device: Add test device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `kvm_io_bus_unregister_dev` is called without holding `kvm->slots_lock`, which can lead to a lockdep splat and data races.
- [High] Passing a `struct kvm_device *` to a function expecting `struct kvm_io_device *` causes silent unregistration failure and a Use-After-Free.
- [Medium] Off-by-one bounds check in `mmio_read` and `mmio_write` incorrectly rejects valid memory accesses at the upper bound of the device.
- [Medium] Missing `.release` callback allows userspace to leak memory and exhaust IO bus slots.
- [Medium] Concurrent lockless reads and writes to the shared `mmio_test->cache` buffer result in data races.
- [Low] Undeclared variables `ret` and `kvm` in `mmio_test_create()` cause hard build failures.
--

--- Patch [10]: [PATCH v3 10/17] KVM: device: Add test device ---
commit 245981a653b22b71de31531543fae6c504801423
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/mmio_test.c b/virt/kvm/mmio_test.c
> new file mode 100644
> index 0000000000000..bd5f21a43ba87
> --- /dev/null
> +++ b/virt/kvm/mmio_test.c
> @@ -0,0 +1,104 @@

[ ... ]

> +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 + len) >= mmio_test->size)
> +		return -1;

[Severity: Medium]
Does this bounds check incorrectly reject valid memory accesses?

If an MMIO read ends exactly at the size boundary, the offset plus length
would equal the size, causing the greater-than-or-equal check to fail.
Should this be a strictly greater-than check instead?

> +
> +	/* Write back cached value */
> +	memcpy(val, &mmio_test->cache[(addr - mmio_test->start)], len);

[Severity: Medium]
Can concurrent lockless reads and writes to the shared cache buffer result
in data races? 

Multiple vCPUs performing concurrent MMIO reads or writes to the same
physical address might cause torn reads or corrupted states.

> +	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 + len) >= mmio_test->size)
> +		return -1;

[Severity: Medium]
Does this bounds check improperly reject legitimate writes?

Similar to the read path, a write ending exactly at the boundary evaluates
to true for the greater-than-or-equal check and is rejected.

> +
> +	/* Cache value */
> +	memcpy(&mmio_test->cache[(addr - mmio_test->start)], val, len);

[Severity: Medium]
Are writes to the shared cache susceptible to data races from concurrent
vCPUs accessing the same device region?

> +	return 0;
> +}
> +

[ ... ]

> +static int mmio_test_create(struct kvm_device *dev, u32 type)
> +{
> +	struct mmio_test *mmio_test;
> +
> +	mmio_test = kzalloc_obj(struct mmio_test, GFP_KERNEL);
> +	if (!mmio_test)
> +		return -ENOMEM;
> +
> +	mmio_test->start = 0x20000000;
> +	mmio_test->size = 16;
> +
> +	dev->private = mmio_test;
> +
> +	kvm_iodevice_init(&mmio_test->dev, &mmio_ops);
> +	mutex_lock(&kvm->slots_lock);
> +	ret = kvm_io_bus_register_dev(dev->kvm, KVM_MMIO_BUS, mmio_test->start,
> +				mmio_test->size, &mmio_test->dev);

[Severity: Low]
Will this cause build failures due to undeclared variables?

It looks like kvm and ret are not declared in the scope of mmio_test_create.
Should this be dev->kvm->slots_lock and a local int ret declaration?

> +	mutex_unlock(&kvm->slots_lock);
> +
> +	if (ret < 0)
> +		kfree(mmio_test);
> +
> +	return ret;
> +}
> +
> +static void mmio_test_destroy(struct kvm_device *dev)
> +{
> +	struct mmio_test *mmio_test = kvm_to_mmio_test_dev(dev);

[Severity: High]
Does passing a struct kvm_device pointer to kvm_to_mmio_test_dev lead to
silent unregistration failures and a use-after-free?

The kvm_to_mmio_test_dev function uses container_of expecting a
struct kvm_io_device pointer, but dev here is a struct kvm_device. This
calculates an invalid pointer base.

> +
> +	kvm_io_bus_unregister_dev(dev->kvm, KVM_MMIO_BUS, &mmio_test->dev);

[Severity: High]
Is kvm_io_bus_unregister_dev being called without holding the required
kvm->slots_lock?

Modifying the KVM IO bus without the required lock can corrupt the shared
bus array or cause lost RCU pointer updates.

> +	kfree(dev->private);
> +	kfree(dev);
> +}
> +
> +struct kvm_device_ops kvm_riscv_mmio_test_device_ops = {
> +	.name = "kvm-riscv-mmio_test",
> +	.create = mmio_test_create,
> +	.destroy = mmio_test_destroy,
> +};

[Severity: Medium]
Is it intentional to omit the .release callback here?

Without it, userspace might leak memory and exhaust IO bus slots by repeatedly
creating and closing test devices using the KVM_CREATE_DEVICE ioctl.

[ ... ]

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