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

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 [off-list ref]

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.

[ ... ]

[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