Re: [PATCH] xen/privcmd: fix double free when an irqfd is shut down twice

[Juergen Gross <jgross@xxxxxxxx>]

On 01.11.26 17:32, Qihang wrote:
> irqfd_deactivate() takes the irqfd off irqfds_list and queues
> irqfd_shutdown().  queue_work() is not a once-only gate: it refuses to
> enqueue a work item only while WORK_STRUCT_PENDING is set, and
> process_one_work() clears that bit before it calls the work function.  So
> nothing records that the shutdown work has already been queued, and
> nothing protects the window between the start of irqfd_shutdown() and the
> kfree() at its end.
>
> Two paths call irqfd_deactivate() for the same irqfd:
>
>    - privcmd_irqfd_deassign(), from the PRIVCMD_IRQFD_FLAG_DEASSIGN ioctl;
>
>    - irqfd_wakeup(), with EPOLLHUP set, when the last reference to the
>      eventfd file goes away, which runs wake_up_poll(&ctx->wqh, EPOLLHUP)
>      from eventfd_release().
>
> irqfds_lock serialises the two calls, but it does not make the second one
> a no-op, and eventfd_ctx_fdget() takes a reference to the eventfd_ctx,
> not to the struct file, so the last fd can still be closed while the
> ioctl is running:
>
>    CPU0                                 CPU1
>
>    eventfd_ctx_fdget()
>    spin_lock(irqfds_lock)
>    list_del_init(&kirqfd->list)
>    queue_work(&kirqfd->shutdown)        close(fd)
>                                           eventfd_release()
>                                             wake_up_poll(EPOLLHUP)
>                                           irqfd_wakeup()
>                                           spin_lock(irqfds_lock)
>                                           irqfd_deactivate() again
>
> If the work item has already started by then, its pending bit has been
> cleared and the second queue_work() succeeds.  irqfd_shutdown() then runs
> twice, so eventfd_ctx_remove_wait_queue(), eventfd_ctx_put() and kfree()
> each hit the same object a second time; and when the second queue_work()
> lands after the irqfd has been freed, the workqueue list operations write
> to freed memory.
>
> virt/kvm/eventfd.c has the same two callers and has had this guard since
> the original irqfd support (721eecbf4fe99): irqfd_wakeup() calls
> irqfd_deactivate() only if irqfd_is_active().  The comment there explains
> why that is enough: the other side deactivates the irqfd before it takes
> it off the waitqueue, and irqfd_shutdown() needs the waitqueue lock,
> which this callback holds, so the irqfd cannot be freed under it.
>
> Do the same here.  list_del_init() leaves the list head empty and the
> irqfd stays off the list until kfree(), so it serves as a one-way
> "active" flag.  The test is in irqfd_deactivate() rather than in
> irqfd_wakeup() because privcmd_irqfd_deassign() walks irqfds_list and can
> therefore never reach an inactive irqfd; only the wakeup path can, and a
> single test covers both callers.
>
> Fixes: f8941e6c4c712 ("xen: privcmd: Add support for irqfd")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Qihang <q.h.hack.winter@xxxxxxxxx>
Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature