Re: [PATCH v1] s390/vfio-ap: Signal eventfd when guest AP configuration is changed

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

On Thu, 16 Jan 2025 10:38:41 -0500
Anthony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 1/15/25 7:17 PM, Halil Pasic wrote:
> > On Wed, 15 Jan 2025 14:35:02 -0500
> > Anthony Krowiak <akrowiak@linux.ibm.com> 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