[RFC] UAF in acrn_irqfd_assign() and vfio_virqfd_enable()

[Al Viro <viro@zeniv.linux.org.uk>]

In acrn_irqfd_assign():
	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
	...
	set it up
	...
        mutex_lock(&vm->irqfds_lock);
        list_for_each_entry(tmp, &vm->irqfds, list) {
                if (irqfd->eventfd != tmp->eventfd)
                        continue;
                ret = -EBUSY;
                mutex_unlock(&vm->irqfds_lock);
                goto fail;
        }
        list_add_tail(&irqfd->list, &vm->irqfds);
        mutex_unlock(&vm->irqfds_lock);
Now irqfd is visible in vm->irqfds.

        /* Check the pending event in this stage */
        events = vfs_poll(f.file, &irqfd->pt);

        if (events & EPOLLIN)
                acrn_irqfd_inject(irqfd);

OTOH, in

static int acrn_irqfd_deassign(struct acrn_vm *vm,
                               struct acrn_irqfd *args)
{
        struct hsm_irqfd *irqfd, *tmp;
        struct eventfd_ctx *eventfd;

        eventfd = eventfd_ctx_fdget(args->fd);
        if (IS_ERR(eventfd))
                return PTR_ERR(eventfd);

        mutex_lock(&vm->irqfds_lock);
        list_for_each_entry_safe(irqfd, tmp, &vm->irqfds, list) {
                if (irqfd->eventfd == eventfd) {
                        hsm_irqfd_shutdown(irqfd);

and

static void hsm_irqfd_shutdown(struct hsm_irqfd *irqfd)
{
        u64 cnt;

        lockdep_assert_held(&irqfd->vm->irqfds_lock);

        /* remove from wait queue */
        list_del_init(&irqfd->list);
        eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
        eventfd_ctx_put(irqfd->eventfd);
        kfree(irqfd);
}

Both acrn_irqfd_assign() and acrn_irqfd_deassign() are callable via
ioctl(2), with no serialization whatsoever.  Suppose deassign hits
as soon as we'd inserted the damn thing into the list.  By the
time we call vfs_poll() irqfd might have been freed.  The same
can happen if hsm_irqfd_wakeup() gets called with EPOLLHUP as a key
(incidentally, it ought to do
	__poll_t poll_bits = key_to_poll(key);
instead of
        unsigned long poll_bits = (unsigned long)key;
and check for EPOLLIN and EPOLLHUP instead of POLLIN and POLLHUP).

AFAICS, that's a UAF...

We could move vfs_poll() under vm->irqfds_lock, but that smells
like asking for deadlocks ;-/

vfio_virqfd_enable() has the same problem, except that there we
definitely can't move vfs_poll() under the lock - it's a spinlock.

Could we move vfs_poll() + inject to _before_ making the thing
public?  We'd need to delay POLLHUP handling there, but then
we need it until the moment with do inject anyway.  Something
like replacing
        if (!list_empty(&irqfd->list))
		hsm_irqfd_shutdown(irqfd);
in hsm_irqfd_shutdown_work() with
        if (!list_empty(&irqfd->list))
		hsm_irqfd_shutdown(irqfd);
	else
		irqfd->need_shutdown = true;
and doing
	if (unlikely(irqfd->need_shutdown))
		hsm_irqfd_shutdown(irqfd);
	else
		list_add_tail(&irqfd->list, &vm->irqfds);
when the sucker is made visible.

I'm *not* familiar with the area, though, so that might be unfeasible
for any number of reasons.

Suggestions?