Re: [PATCH v6 2/3] virt: bao: add I/O dispatcher driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] virt: bao: race in bao_irqfd_queue_shutdown() causes UAF on
  teardown
--

commit a54931ee06339bcb9128b2ac3b46c8d5cb3569c1
Author: João Peixoto <jpeixoto@osyx.tech>

virt: bao: add I/O dispatcher driver

This commit adds the Bao hypervisor I/O dispatcher driver, exposing
/dev/bao and per-DM file descriptors to bridge Remote I/O requests,
shared memory, ioeventfds, and irqfds between frontend guests and
userspace VirtIO device models.

> diff --git a/drivers/virt/bao/io-dispatcher/irqfd.c b/drivers/virt/bao/io-dispatcher/irqfd.c
> new file mode 100644
> index 0000000000000..8d01bef4e7abd
> --- /dev/null
> +++ b/drivers/virt/bao/io-dispatcher/irqfd.c
> @@ -0,0 +1,330 @@

[ ... ]

> +/* Queue the cleanup work at most once. Safe from atomic context. */
> +static void bao_irqfd_queue_shutdown(struct irqfd *irqfd)
> +{
> +	if (!test_and_set_bit(BAO_IRQFD_SHUTDOWN, &irqfd->flags))
> +		queue_work(irqfd->dm->irqfd_server, &irqfd->shutdown);
> +}

[ ... ]

> +static int bao_irqfd_wakeup(wait_queue_entry_t *wait, unsigned int mode,
> +			    int sync, void *key)
> +{
> +	struct irqfd *irqfd = container_of(wait, struct irqfd, wait);
> +	__poll_t flags = key_to_poll(key);
> +	u64 cnt;
> +
> +	if (flags & EPOLLIN) {
> +		eventfd_ctx_do_read(irqfd->eventfd, &cnt);
> +		bao_irqfd_inject(irqfd->dm->info.id);
> +	}
> +
> +	if (flags & EPOLLHUP)
> +		/* Defer teardown to the cleanup work; can't sleep here. */
> +		bao_irqfd_queue_shutdown(irqfd);
> +
> +	return 0;
> +}

[ ... ]

> +void bao_irqfd_server_destroy(struct bao_dm *dm)
> +{
> +	struct irqfd *irqfd;
> +	struct irqfd *next;
> +
> +	if (WARN_ON_ONCE(!dm))
> +		return;
> +
> +	mutex_lock(&dm->irqfds_lock);
> +	list_for_each_entry_safe(irqfd, next, &dm->irqfds, list) {
> +		list_del_init(&irqfd->list);
> +		dm->nr_irqfds--;
> +		bao_irqfd_queue_shutdown(irqfd);
> +	}
> +	mutex_unlock(&dm->irqfds_lock);
> +
> +	/* Drain all cleanup work before tearing the workqueue down. */
> +	if (dm->irqfd_server) {
> +		flush_workqueue(dm->irqfd_server);
> +		destroy_workqueue(dm->irqfd_server);
> +	}
> +}

[Severity: Medium]
Can bao_irqfd_queue_shutdown() race with bao_irqfd_server_destroy() and
cause a use-after-free of dm and dm->irqfd_server?

bao_irqfd_wakeup() runs on EPOLLHUP without holding dm->irqfds_lock.  Once
it sets BAO_IRQFD_SHUTDOWN in bao_irqfd_queue_shutdown(), a concurrent
teardown caller sees the bit already set and moves on to flush and destroy
dm->irqfd_server before the work item is actually queued:

CPU0                                  CPU1
eventfd_release()                     bao_dm_release()
  wake_up_poll(..., EPOLLHUP)           bao_dm_destroy()
    bao_irqfd_wakeup()                    bao_irqfd_server_destroy()
      bao_irqfd_queue_shutdown()            mutex_lock(&dm->irqfds_lock)
        test_and_set_bit(...) -> 0          bao_irqfd_queue_shutdown()
                                              test_and_set_bit(...) -> 1
                                            mutex_unlock(&dm->irqfds_lock)
                                            flush_workqueue(dm->irqfd_server)
                                            destroy_workqueue(dm->irqfd_server)
                                          kfree(dm)
        queue_work(irqfd->dm->irqfd_server, ...)
          <-- use-after-free

The same race with bao_irqfd_deassign() lets flush_workqueue() return
before irqfd->shutdown has been queued and run.

Since queue_work() already ignores a work_struct that is currently
pending, bao_irqfd_queue_shutdown() can call queue_work() unconditionally
without BAO_IRQFD_SHUTDOWN so that both paths guarantee the work is queued
before returning.

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