On Mon, 10 Jun 2024 06:12:06 +0100
Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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?
>
