Re: [PATCH v2 2/3] virtio_input: stop callbacks before unregistering input device

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Sep 05, 2026 at 03:33:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing `virtio_reset_device()` in the error path of `virtinput_probe()` before deleting virtqueues, potentially leading to use-after-free or memory corruption by the active hardware/hypervisor.

This one is fixed in my tree.

> - [High] The driver leaks DMA mappings for unconsumed buffers in the event virtqueue (`vi->evt`) upon device removal or suspend/freeze.
> --
> 
> commit 71bbbcca0b03e3c67fa905a8357e9d3ad23e7fe4
> Author: Karl Mehltretter <kmehltretter@gmail.com>
> 
> virtio_input: stop callbacks before unregistering input device
> 
> virtinput_remove() unregisters the input device before resetting the
> virtio device. virtinput_recv_events() drops vi->lock around input_event(),
> so clearing vi->ready does not stop a callback that passed the entry check.
> It can still use vi->idev, requeue buffers and kick the queue.
> 
> > diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> > index deec24e8e6828..7b654af0a42cb 100644
> > --- a/drivers/virtio/virtio_input.c
> > +++ b/drivers/virtio/virtio_input.c
> [ ... ]
> > @@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
> >  	vi->ready = false;
> >  	spin_unlock_irqrestore(&vi->lock, flags);
> >  
> > -	input_unregister_device(vi->idev);
> > +	/* Callbacks use vi->idev. */
> >  	virtio_reset_device(vdev);
> > +	input_unregister_device(vi->idev);
> >  	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
> >  		kfree(buf);
> >  	vdev->config->del_vqs(vdev);
> 
> [Severity: High]
> This is a pre-existing issue, but does this cleanup leak DMA mappings for
> the event virtqueue (vi->evt)?
> 
> The cleanup loop correctly detaches and unmaps vi->sts using
> virtqueue_detach_unused_buf(), but it ignores vi->evt. Since
> virtinput_fill_evt() populates vi->evt with buffers using
> virtqueue_add_inbuf_cache_clean(), which establishes DMA mappings under
> the hood, omitting the detachment step leaves these mappings active.
> 
> Does this result in a persistent DMA mapping leak every time the device is
> unbound? Furthermore, since the memory for these buffers (vi->evts) is freed,
> does this leave active DMA mappings pointing to freed memory, posing a memory
> corruption risk if delayed DMA occurs?
> 
> Does this exact same DMA mapping leak also occur during the power management
> suspend cycle in virtinput_freeze() which performs the same cleanup?
> 
> [Severity: High]
> This is also a pre-existing issue, but is there a missing
> virtio_reset_device() in the error path of virtinput_probe()?
> 
> Looking at virtinput_probe(), the device is marked as ready:
> 
> 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 error path deletes the virtqueues:
> 
> drivers/virtio/virtio_input.c:virtinput_probe() {
>     ...
> err_input_alloc:
> 	vdev->config->del_vqs(vdev);
>     ...
> }
> 
> Since virtio_device_ready() sets the device status to DRIVER_OK and enables
> the host/hardware to start processing virtqueues, does deleting the active
> virtqueue structures without first calling virtio_reset_device() to halt
> the device risk a use-after-free or memory corruption by the host backend
> if it continues to access the freed vring memory?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260905152059.89560-1-kmehltretter@gmail.com?part=2