On Thu, 16 Jan 2025 10:38:41 -0500
Anthony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> On 1/15/25 7:17 PM, Halil Pasic wrote:
> > 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)?
Yes, that answers my question, thanks for untangling it. We might
consider a lockdep_assert_held() in the new
signal_guest_ap_cfg_changed() since it does get called from a variety
of paths and we need that lock to prevent the UAF.
> I agree that the matrix_dev->mdevs_lock does prevent changes to
> matrix_mdev->cfg_chg_trigger while it is being accessed by the
> vfio_ap device driver. My confusion arises from my interpretation of
> Alex's question; it seemed to me that he was talking its use outside
> of the vfio_ap driver and how to guard against that.
Nope, Halil zeroed in on the UAF possibility that concerned me. Thanks,
Alex
