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

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 10 Jun 2024 06:12:06 +0100
Al Viro <viro@zeniv.linux.org.uk> wrote:

> 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.

vfio_virqfd_enable() and vfio_virqfd_disable() are serialized by their
callers, I don't see that they have a UAF problem.  Thanks,

Alex

> 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?
>