Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[sashiko-bot@kernel.org]

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 <harimishal1@gmail.com>

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().

> 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