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

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

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

> On Mon, Jun 10, 2024 at 02:09:06PM -0600, Alex Williamson wrote:
> > > 
> > > 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  
> 
> Umm...  I agree that there's no UAF on vfio side; acrn and xen/privcmd
> counterparts, OTOH, look like they do have that...
> 
> OK, so the memory safety in there depends upon
> 	* external exclusion wrt vfio_virqfd_disable() on caller-specific
> locks (vfio_pci_core_device::ioeventfds_lock for vfio_pci_rdwr.c,
> vfio_pci_core_device::igate for the rest?  What about the path via
> vfio_pci_core_disable()?)

This is only called when the device is closed, therefore there's no
userspace access to generate a race.

> 	* no EPOLLHUP on eventfd while the file is pinned.  That's what
>         /*
>          * Do not drop the file until the irqfd is fully initialized,
>          * otherwise we might race against the EPOLLHUP.
>          */
> in there (that "irqfd" is a typo for "kirqfd", right?) refers to.

Sorry, I'm not fully grasping your comment.  "irqfd" is not a typo
here, "kirqfd" seems to be a Xen thing.  I believe the comment is
referring to holding a reference to the fd until everything is in place
to cleanup correctly if the user process is killed mid-setup.  Thanks,

Alex