On Wed, 15 Jan 2025 14:35:02 -0500
Anthony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> >> +static int vfio_ap_set_cfg_change_irq(struct ap_matrix_mdev *matrix_mdev, unsigned long arg)
> >> +{
> >> + s32 fd;
> >> + void __user *data;
> >> + unsigned long minsz;
> >> + struct eventfd_ctx *cfg_chg_trigger;
> >> +
> >> + minsz = offsetofend(struct vfio_irq_set, count);
> >> + data = (void __user *)(arg + minsz);
> >> +
> >> + if (get_user(fd, (s32 __user *)data))
> >> + return -EFAULT;
> >> +
> >> + if (fd == -1) {
> >> + if (matrix_mdev->cfg_chg_trigger)
> >> + eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
> >> + matrix_mdev->cfg_chg_trigger = NULL;
> >> + } else if (fd >= 0) {
> >> + cfg_chg_trigger = eventfd_ctx_fdget(fd);
> >> + if (IS_ERR(cfg_chg_trigger))
> >> + return PTR_ERR(cfg_chg_trigger);
> >> +
> >> + if (matrix_mdev->cfg_chg_trigger)
> >> + eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
> >> +
> >> + matrix_mdev->cfg_chg_trigger = cfg_chg_trigger;
> >> + } else {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> > How does this guard against a use after free, such as the eventfd being
> > disabled or swapped concurrent to a config change? Thanks,
> >
> > Alex
>
> Hi Alex. I spent a great deal of time today trying to figure out exactly
> what
> you are asking here; reading about eventfd and digging through code.
> I looked at other places where eventfd is used to set up communication
> of events targetting a vfio device from KVM to userspace (e.g.,
> hw/vfio/ccw.c)
> and do not find anything much different than what is done here. In fact,
> this code looks identical to the code that sets up an eventfd for the
> VFIO_AP_REQ_IRQ_INDEX.
>
> Maybe you can explain how an eventfd is disabled or swapped, or maybe
> explain how we can guard against its use after free. Thanks.
Maybe I will try! The value of matrix_mdev->cfg_chg_trigger is used in:
* vfio_ap_set_cfg_change_irq() (rw, with matrix_dev->mdevs_lock)
* signal_guest_ap_cfg_changed()(r, takes no locks itself, )
* called by vfio_ap_mdev_update_guest_apcb()
* called at a bunch of places but AFAICT always with
matrix_dev->mdevs_lock held
* called by vfio_ap_mdev_unset_kvm() (with matrix_dev->mdevs_lock held
via get_update_locks_for_kvm())
* vfio_ap_mdev_probe() (w, assigns NULL to it)
If vfio_ap_set_cfg_change_irq() could change/destroy
matrix_mdev->cfg_chg_trigger while another thread of execution
is using it e.g. with signal_guest_ap_cfg_changed() that would be a
possible UAF and thus BAD.
Now AFAICT matrix_mdev->cfg_chg_trigger is protected by
matrix_dev->mdevs_lock on each access except for in vfio_ap_mdev_probe()
which is AFAIK just an initialization in a safe state where we are
guaranteed to have exclusive access.
The eventfd is swapped and disabled in vfio_ap_set_cfg_change_irq() with
userspace supplying a new valid fd or -1 respectively.
Tony does that answer your question to Alex?
Alex, does the above answer your question on what guards against UAF (the
short answer is: matrix_dev->mdevs_lock)?
Regards,
Halil
