Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: sashiko-bot@kernel.org
Date: 2026-07-15 14:35:49
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: Pre-existing issues: - [High] Concurrent execution of virtqueue interrupts during device teardown causes a use-after-free of the input device. - [High] If `input_register_device()` fails, the driver deletes virtqueues without first resetting the virtio device, violating the virtio specification. - [Medium] The driver fails to detach unused buffers from the `vi->evt` queue before deleting the virtqueues, leading to leaked DMA mappings. - [Medium] Missing null-termination for `vi->name` and `vi->serial` strings copied from the host. --
--- Patch [2]: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count ---commit 2551acf6fb14e59620dd29579313834ccc291ba0 Author: Hari Mishal [off-list ref] virtio_input: validate device-reported multitouch slot count The commit adds validation for the nslots value derived from the device-reported ABS_MT_SLOT maximum. It prevents registering a multitouch device with zero backing slots by rejecting non-positive slot counts before calling input_mt_init_slots().
quoted hunk ↗ jump to hunk
diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c index deec24e8e6828..2cc19782cdd33 100644 --- a/drivers/virtio/virtio_input.c +++ b/drivers/virtio/virtio_input.c@@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev) if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) { nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1; + if (nslots <= 0) { + err = -EINVAL; + goto err_mt_init_slots; + } err = input_mt_init_slots(vi->idev, nslots, 0); if (err) goto err_mt_init_slots;
[Severity: High]
This is a pre-existing issue, but can concurrent execution of virtqueue
interrupts during device teardown cause a use-after-free of the input device?
Looking at virtinput_remove(), the teardown unregisters the input device
before stopping the hardware and synchronizing interrupts:
drivers/virtio/virtio_input.c:virtinput_remove() {
...
input_unregister_device(vi->idev);
virtio_reset_device(vdev);
...
}
If virtinput_recv_events() runs concurrently with virtinput_remove(), the
interrupt handler temporarily drops vi->lock and calls input_event(vi->idev).
Meanwhile, virtinput_remove() can free vi->idev if there are no active users,
since it doesn't call virtio_reset_device() or del_vqs() until after
unregistering the input device.
[Severity: High]
This is a pre-existing issue, but does the error path in virtinput_probe()
violate the virtio specification by deleting virtqueues without resetting
an active device?
During virtinput_probe(), the device is marked ready and activated:
drivers/virtio/virtio_input.c:virtinput_probe() {
...
virtio_device_ready(vdev);
vi->ready = true;
err = input_register_device(vi->idev);
if (err)
goto err_input_register;
...
}
If input_register_device() fails, the code jumps to the error path and deletes
the virtqueues without first calling virtio_reset_device():
drivers/virtio/virtio_input.c:virtinput_probe() {
...
err_input_register:
spin_lock_irqsave(&vi->lock, flags);
vi->ready = false;
spin_unlock_irqrestore(&vi->lock, flags);
err_mt_init_slots:
input_free_device(vi->idev);
err_input_alloc:
vdev->config->del_vqs(vdev);
...
}
Could this lead to memory corruption or host crashes due to DMA into
freed queues?
[Severity: Medium]
This is a pre-existing issue, but does the driver leak DMA mappings for the
vi->evt queue during teardown?
In virtinput_remove() and virtinput_freeze(), the driver detaches and frees
buffers from the vi->sts queue, but ignores the vi->evt queue:
drivers/virtio/virtio_input.c:virtinput_remove() {
...
while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
kfree(buf);
vdev->config->del_vqs(vdev);
...
}
The buffers in vi->evt were mapped for DMA when queued in virtinput_fill_evt().
Could deleting the virtqueues without detaching these unused buffers leave
their DMA mappings active indefinitely?
[Severity: Medium]
This is a pre-existing issue, but does the device name copy lack
null-termination if the host provides a full-length string?
In virtinput_probe(), the host-provided string is copied into the fixed-size
vi->name array:
drivers/virtio/virtio_input.c:virtinput_probe() {
...
size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config,
u.string),
vi->name, min(size, sizeof(vi->name)));
...
}
If the host provides exactly 64 bytes of non-null characters, vi->name will
not be null-terminated.
Since this string is passed to vi->idev->name, could a subsequent sysfs read
of the device name read past the array boundary?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715142337.22811-1-harimishal1@gmail.com?part=2