Re: [PATCH v3 4/6] um: virtio_uml: implement synchronize_cbs()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Premature clearing of dynamically allocated IRQ bit in um_free_irq() causes unintended IRQ sharing and race conditions.
- [High] Resource leak and potential Use-After-Free on vhost_user_init() error path.
--

commit 88d671cc22ee3a67541ca7191122aa3e1f6d8c9f
Author: Karl Mehltretter <kmehltretter@gmail.com>

um: virtio_uml: implement synchronize_cbs()

This commit implements synchronize_cbs() for virtio_uml by using
synchronize_irq() instead of waiting for an RCU grace period. This
synchronizes with the IRQ handlers used by virtqueue callbacks.

> diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> index 7425a8548141a..1f2e62e29a277 100644
> --- a/arch/um/drivers/virtio_uml.c
> +++ b/arch/um/drivers/virtio_uml.c

[ ... ]

> @@ -869,6 +870,14 @@ static void vu_reset(struct virtio_device *vdev)
>  	vu_dev->status = 0;
>  }
>  
> +static void vu_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
> +
> +	if (vu_dev->irq != UM_IRQ_ALLOC)
> +		synchronize_irq(vu_dev->irq);
> +}
> +
>  static void vu_del_vq(struct virtqueue *vq)
>  {
>  	struct virtio_uml_vq_info *info = vq->priv;

[Severity: High]
This isn't a bug introduced by this patch, but it is a pre-existing issue in
the codebase. 

When a virtqueue is deleted during teardown, vu_del_vq() calls
um_free_irq(vu_dev->irq, vq) to remove the callback for that specific queue. 

However, looking at um_free_irq() in arch/um/kernel/irq.c, it lacks reference
counting and unconditionally clears the dynamically allocated IRQ bit:

arch/um/kernel/irq.c:um_free_irq() {
	free_irq(irq, dev);
	clear_bit(irq, irqs_allocated);
}

Since virtio_uml shares a single dynamically allocated IRQ (vu_dev->irq)
across multiple virtqueues and the slave request channel using IRQF_SHARED,
could this premature clearing of the IRQ bit mark it as free for global
reallocation while still in use? 

It seems concurrent requests for a new dynamic IRQ from other devices could be
assigned this prematurely freed IRQ, resulting in unintended forced sharing or
-EBUSY probe failures.

[ ... ]

[Severity: High]
This is also a pre-existing issue, but there appears to be a resource leak
and potential use-after-free in the probe error path of virtio_uml.

In virtio_uml_probe(), vhost_user_init() is called. If the slave request
channel is negotiated, vhost_user_init_slave_req() creates a pipe, assigns
req_fds[0] to vu_dev->req_fd, and requests an IRQ handler via
um_request_irq_tt():

arch/um/drivers/virtio_uml.c:vhost_user_init() {
	if (vu_dev->protocol_features &
			BIT_ULL(VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
		rc = vhost_user_init_slave_req(vu_dev);
		if (rc)
			return rc;
	}

	if (vu_dev->protocol_features &
			BIT_ULL(VHOST_USER_PROTOCOL_F_MQ)) {
		rc = vhost_user_get_queue_num(vu_dev, &vu_dev->max_vqs);
		if (rc)
			return rc;
}

If a subsequent step like vhost_user_get_queue_num() fails, it returns the
error code directly. Back in virtio_uml_probe(), the error path is taken:

arch/um/drivers/virtio_uml.c:virtio_uml_probe() {
error_init:
	os_close_file(vu_dev->sock);
error_free:
	kfree(vu_dev);
	return rc;
}

Does this error path free vu_dev without unregistering the active IRQ
handler or closing the vu_dev->req_fd file descriptor? 

If an untrusted userspace backend forces the probe to fail, could the leaked
IRQ handler subsequently fire and access the freed vu_dev memory, causing a
use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=4