Re: [RFC PATCH v3 4/6] virt: bao: add I/O dispatcher driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
- [High] Userspace can spoof the `dm_id` in `BAO_IOCTL_IO_REQUEST_COMPLETE`, allowing one device model instance to interfere with others.
- [High] Unrecoverable self-deadlock in the I/O client kernel thread during teardown.
- [High] Global interrupt handler pointer breaks multi-DM support by starving active DMs of interrupts.
- [High] TOCTOU Use-After-Free race in `bao_irqfd_assign()` when polling the eventfd.
- [High] Missing workqueue cancellation during irqfd shutdown causes Use-After-Free on asynchronous POLLHUP.
- [High] Device unbind frees `struct bao_dm` while userspace holds open file descriptors.
- [High] Calling `destroy_workqueue` before removing eventfd wait queues creates a Use-After-Free on the irqfd workqueue.
- [High] Passing a stack-allocated string as the 'name' argument to request_irq() leads to a Use-After-Free.
- [High] A missing return statement in `bao_ioeventfd_client_config` silently breaks ioeventfd deassignment.
- [High] The I/O dispatcher workqueue queues `bao_io_request` structures unboundedly, allowing a malicious frontend VM to cause a kernel OOM.
- [Medium] The `struct bao_dm_info` UAPI structure contains uninitialized implicit padding holes, leaking kernel stack memory to userspace.
- [Medium] Ioeventfd deassignment matches only by eventfd, breaking targeted range deregistration.
- [Medium] Signals are incorrectly handled in `bao_io_client_attach`, translating normal signal interruptions into spurious `-EFAULT` errors in userspace.
- [Medium] The `bao_io_client_destroy` function fails to free pending I/O requests.
- [Medium] Unused kernel memory mapping of backend shared memory wastes vmalloc space.
- [Medium] Silent discard of I/O requests when `copy_to_user` fails or memory allocation fails.
--

--- Patch [4]: [RFC PATCH v3 4/6] virt: bao: add I/O dispatcher driver ---
commit fc6ea3f8ec16168d50252a311b2bf53f0409c312
Author: João Peixoto <jpeixoto@osyx.tech>
Subject: virt: bao: add I/O dispatcher driver

Add the Bao I/O dispatcher, used by backend VMs to service I/O on behalf
of frontend guests. It bridges Bao's Remote I/O mechanism to userspace
VirtIO backend device models.

> --- a/include/uapi/linux/bao.h
> +++ b/include/uapi/linux/bao.h
> @@ -68,6 +68,13 @@ struct bao_irqfd {
>  };
>  
> +struct bao_dm_info {
> +	__u32 id;
> +	__u64 shmem_addr;
> +	__u64 shmem_size;
> +	__u32 irq;
> +};

[Severity: Medium]
Does this UAPI structure leak kernel stack memory to userspace? The structure
contains implicit padding holes after the 32-bit fields.

When it is allocated on the stack in bao_io_dispatcher_driver_probe() and
copied to userspace via bao_dm_ioctl() using copy_to_user(), the uninitialized
4-byte holes are leaked.

> --- a/drivers/virt/bao/io-dispatcher/dm.c
> +++ b/drivers/virt/bao/io-dispatcher/dm.c
> @@ -34,6 +34,107 @@ static int bao_dm_release(struct inode *inode, struct file *filp)
> +static long bao_dm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	struct bao_dm *dm = filp->private_data;

[ ... ]

> +	case BAO_IOCTL_IO_CLIENT_ATTACH: {
> +		struct bao_virtio_request *req;
> +
> +		req = memdup_user((void __user *)arg, sizeof(*req));
> +		if (IS_ERR(req)) {
> +			rc = PTR_ERR(req);
> +			break;
> +		}
> +
> +		if (!dm->control_client) {
> +			rc = -ENOENT;
> +			goto out_free;
> +		}
> +
> +		rc = bao_io_client_attach(dm->control_client);
> +		if (rc)
> +			goto out_free;
> +
> +		rc = bao_io_client_request(dm->control_client, req);
> +		if (rc)
> +			goto out_free;
> +
> +		if (copy_to_user((void __user *)arg, req, sizeof(*req))) {
> +			rc = -EFAULT;
> +			goto out_free;
> +		}

[Severity: Medium]
What happens to the I/O request if copy_to_user() fails here? The request
is permanently popped from the queue in bao_io_client_request().

If copying it to userspace fails, the request appears to be silently discarded,
which would leave the frontend guest VM hanging indefinitely.

> +	case BAO_IOCTL_IO_REQUEST_COMPLETE: {
> +		struct bao_virtio_request *req;
> +		struct bao_remio_hypercall_ctx ctx;
> +
> +		req = memdup_user((void __user *)arg, sizeof(*req));
> +		if (IS_ERR(req)) {
> +			rc = PTR_ERR(req);
> +			break;
> +		}
> +
> +		ctx.dm_id = req->dm_id;
> +		ctx.addr = req->addr;
> +		ctx.op = req->op;
> +		ctx.value = req->value;
> +		ctx.access_width = req->access_width;
> +		ctx.request_id = req->request_id;
> +
> +		rc = bao_remio_hypercall(&ctx);

[Severity: High]
Can userspace spoof the dm_id in this hypercall context? The request's dm_id
is blindly copied from userspace instead of being validated against the dm
associated with the file descriptor (dm->info.id).

Could this allow an unprivileged frontend process to complete or corrupt
requests belonging to other device models?

[ ... ]

> +struct bao_dm *bao_dm_create(struct bao_dm_info *info, struct device *dev)
> +{
> +	struct bao_dm *dm;
> +	int ret;

[ ... ]

> +	dm->shmem_base_addr = memremap(dm->info.shmem_addr, dm->info.shmem_size,
> +				       MEMREMAP_WB);

[Severity: Medium]
Is this kernel memory mapping necessary? The mapped physical memory pointer
dm->shmem_base_addr is never dereferenced by the kernel for reading or writing.

Since userspace maps the memory directly via remap_pfn_range() using the
physical address, does this just waste kernel virtual address space?

[ ... ]

> +void bao_dm_destroy(struct bao_dm *dm)
> +{
> +	if (WARN_ON_ONCE(!dm))
> +		return;
> +
> +	misc_deregister(&dm->miscdev);
> +
> +	if (dm->shmem_base_addr)
> +		memunmap(dm->shmem_base_addr);
> +
> +	bao_irqfd_server_destroy(dm);
> +	bao_io_clients_destroy(dm);
> +	bao_io_dispatcher_destroy(dm);
> +
> +	kfree(dm);
> +}

[Severity: High]
Is it safe to unconditionally free the dm structure here? This runs during the
driver's remove callback, but misc_deregister() does not revoke existing open
file descriptors.

If userspace holds the device open and issues an ioctl() or mmap() after the
driver unbinds, the VFS callbacks will dereference the freed dm pointer,
leading to a use-after-free.

> --- a/drivers/virt/bao/io-dispatcher/intc.c
> +++ b/drivers/virt/bao/io-dispatcher/intc.c
> @@ -13,6 +13,52 @@
> +/* Top-level handler registered by the Bao interrupt controller */
> +static void (*bao_intc_handler)(struct bao_dm *dm);

[Severity: High]
Does this global function pointer break multi-DM support? When any single DM
is paused or destroyed, bao_intc_remove_handler() sets this global pointer
to NULL.

This would silently disable I/O interrupt dispatching for all other active DMs
on the system.

[ ... ]

> +int bao_intc_init(struct bao_dm *dm)
> +{
> +	char name[BAO_NAME_MAX_LEN];
> +
> +	if (WARN_ON_ONCE(!dm))
> +		return -EINVAL;
> +
> +	scnprintf(name, sizeof(name), "bao-iodintc%d", dm->info.id);
> +
> +	return request_irq(dm->info.irq, bao_interrupt_handler, 0, name, dm);
> +}

[Severity: High]
Can this lead to a use-after-free when reading /proc/interrupts? The character
array name is allocated on the stack and passed to request_irq().

The IRQ subsystem stores this pointer directly without copying the string. When
this function returns and the stack frame is destroyed, any subsequent access
to the interrupt name will dereference a stale stack pointer.

> --- a/drivers/virt/bao/io-dispatcher/io_client.c
> +++ b/drivers/virt/bao/io-dispatcher/io_client.c
> @@ -45,6 +45,356 @@ static inline bool bao_io_client_is_destroying(struct bao_io_client *client)
> +bool bao_io_client_push_request(struct bao_io_client *client,
> +				struct bao_virtio_request *req)
> +{
> +	struct bao_io_request *io_req;
> +
> +	if (WARN_ON_ONCE(!client || !req))
> +		return false;
> +
> +	io_req = kzalloc_obj(*io_req, GFP_KERNEL);

[Severity: High]
Could this allow a malicious frontend VM to cause a host kernel OOM? The
io_dispatcher workqueue continually pulls pending I/O requests from the
hypervisor and pushes them here, which allocates memory with GFP_KERNEL.

There doesn't appear to be any limit or backpressure mechanism if userspace
processes requests slower than they are produced.

[ ... ]

> +static void bao_io_client_destroy(struct bao_io_client *client)
> +{
> +	struct bao_io_client *range;
> +	struct bao_io_client *next;
> +	struct bao_dm *dm;
> +
> +	if (WARN_ON_ONCE(!client))
> +		return;
> +
> +	dm = client->dm;
> +
> +	bao_io_dispatcher_pause(dm);
> +
> +	set_bit(BAO_IO_CLIENT_DESTROYING, &client->flags);
> +
> +	if (client->is_control) {
> +		wake_up_interruptible(&client->wq);
> +	} else {
> +		bao_ioeventfd_client_destroy(dm);
> +		if (client->thread)
> +			kthread_stop(client->thread);
> +	}

[Severity: High]
Could this cause an unrecoverable self-deadlock? In bao_io_client_attach(),
when kthread_should_stop() is true, the kthread recursively calls
bao_io_client_destroy() on itself.

Inside this call, it invokes kthread_stop() on its own thread context, which
blocks waiting for the thread to exit.

[ ... ]

> +	down_write(&dm->io_clients_lock);
> +	if (client->is_control)
> +		dm->control_client = NULL;
> +	else
> +		dm->ioeventfd_client = NULL;
> +
> +	list_del(&client->list);
> +	up_write(&dm->io_clients_lock);
> +
> +	bao_io_dispatcher_resume(dm);
> +
> +	kfree(client);
> +}

[Severity: Medium]
Is there a memory leak here? When the client is destroyed, its range_list is
cleaned up, but it does not iterate over the virtio_requests list to free
pending struct bao_io_request items before calling kfree(client).

> +int bao_io_client_attach(struct bao_io_client *client)
> +{
> +	if (WARN_ON_ONCE(!client))
> +		return -EINVAL;
> +
> +	if (client->is_control) {
> +		wait_event_interruptible(client->wq,
> +					 bao_io_client_has_pending_requests(client) ||
> +					 bao_io_client_is_destroying(client));
> +		if (bao_io_client_is_destroying(client))
> +			return -EPERM;

[Severity: Medium]
Does this mishandle signals? The wait_event_interruptible() macro returns
-ERESTARTSYS if interrupted by a signal, but this return value is ignored.

As a result, normal signal interruptions cause bao_dm_ioctl() to attempt to
pop an empty queue, failing and returning a spurious -EFAULT to userspace.

> --- a/drivers/virt/bao/io-dispatcher/io_dispatcher.c
> +++ b/drivers/virt/bao/io-dispatcher/io_dispatcher.c
> @@ -25,6 +25,156 @@ struct bao_io_dispatcher_work {
> +int bao_dispatch_io(struct bao_dm *dm)
> +{
> +	struct bao_io_client *client;
> +	struct bao_remio_hypercall_ctx ctx;
> +	struct bao_virtio_request req;

[ ... ]

> +	if (!bao_io_client_push_request(client, &req)) {
> +		up_read(&dm->io_clients_lock);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Are requests silently discarded here if bao_io_client_push_request() fails?
If memory exhaustion prevents the allocation, the request fetched from the
hypervisor is dropped, permanently hanging the frontend guest waiting for
I/O completion.

> --- a/drivers/virt/bao/io-dispatcher/ioeventfd.c
> +++ b/drivers/virt/bao/io-dispatcher/ioeventfd.c
> @@ -201,6 +201,122 @@ static int bao_ioeventfd_deassign(struct bao_dm *dm,
> +	list_for_each_entry(p, &dm->ioeventfds, list) {
> +		if (p->eventfd != eventfd)
> +			continue;
> +
> +		bao_io_client_range_del(dm->ioeventfd_client, p->addr,
> +					p->addr + p->length - 1);
> +
> +		bao_ioeventfd_shutdown(dm, p);
> +		break;
> +	}

[Severity: Medium]
Does this deassignment loop match solely on the eventfd context and ignore
the requested memory address range? If userspace registers multiple memory
ranges to trigger the same eventfd, this will arbitrarily delete the first
matching entry rather than the specific range requested by userspace.

[ ... ]

> +int bao_ioeventfd_client_config(struct bao_dm *dm, struct bao_ioeventfd *config)
> +{
> +	if (WARN_ON_ONCE(!dm || !config))
> +		return -EINVAL;
> +
> +	if (config->flags & BAO_IOEVENTFD_FLAG_DEASSIGN)
> +		bao_ioeventfd_deassign(dm, config);
> +
> +	return bao_ioeventfd_assign(dm, config);
> +}

[Severity: High]
Is a return statement missing here? Because there is no return after calling
bao_ioeventfd_deassign(), execution falls through to bao_ioeventfd_assign(),
immediately recreating the ioeventfd that was just destroyed.

> --- a/drivers/virt/bao/io-dispatcher/irqfd.c
> +++ b/drivers/virt/bao/io-dispatcher/irqfd.c
> @@ -34,6 +34,280 @@ struct irqfd {
> +static void bao_irqfd_shutdown(struct irqfd *irqfd)
> +{
> +	u64 cnt;
> +
> +	if (WARN_ON_ONCE(!irqfd || !irqfd->dm))
> +		return;
> +
> +	lockdep_assert_held(&irqfd->dm->irqfds_lock);
> +
> +	list_del_init(&irqfd->list);
> +
> +	eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
> +
> +	eventfd_ctx_put(irqfd->eventfd);
> +
> +	kfree(irqfd);
> +}

[Severity: High]
Is it safe to free the irqfd here without cancelling the asynchronous shutdown
work? If userspace closes the eventfd concurrently, a POLLHUP event queues
the shutdown work.

The irqfd is then freed, and when the workqueue executes irqfd_shutdown_work(),
it dereferences the freed memory to access irqfd->dm.

[ ... ]

> +static int bao_irqfd_assign(struct bao_dm *dm, struct bao_irqfd *args)
> +{
> +	struct eventfd_ctx *eventfd = NULL;
> +	struct irqfd *irqfd;

[ ... ]

> +	list_add_tail(&irqfd->list, &dm->irqfds);
> +	mutex_unlock(&dm->irqfds_lock);
> +
> +	events = vfs_poll(fd_file(f), &irqfd->pt);

[Severity: High]
Could this cause a use-after-free race? The newly created irqfd is published
to the globally visible dm->irqfds list, and the lock is dropped before
calling vfs_poll().

A concurrent thread calling deassign can remove and free this irqfd while
vfs_poll() proceeds to access the freed &irqfd->pt.

[ ... ]

> +void bao_irqfd_server_destroy(struct bao_dm *dm)
> +{
> +	struct irqfd *irqfd;
> +	struct irqfd *next;
> +
> +	if (WARN_ON_ONCE(!dm))
> +		return;
> +
> +	if (dm->irqfd_server)
> +		destroy_workqueue(dm->irqfd_server);
> +
> +	mutex_lock(&dm->irqfds_lock);
> +	list_for_each_entry_safe(irqfd, next, &dm->irqfds, list)
> +		bao_irqfd_shutdown(irqfd);
> +	mutex_unlock(&dm->irqfds_lock);
> +}

[Severity: High]
Can this lead to a use-after-free on the irqfd workqueue? Calling
destroy_workqueue() before shutting down the active eventfds leaves their
wait queues active.

If an eventfd receives an event in this window, bao_irqfd_wakeup() will execute
queue_work() on the destroyed and freed workqueue pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786010512.git.jpeixoto@osyx.tech?part=4