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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

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

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.

>
> Regards,
> Halil
>
>
>