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

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

On 1/16/25 11:52 AM, Alex Williamson wrote:
> 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,

He is a marksman!:)

>
> Alex
>