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

[Halil Pasic <pasic@linux.ibm.com>]

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)?

Regards,
Halil