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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f614bde5-6b35-4d6c-a373-f97671ea565d@linux.ibm.com>
Date: Fri, 17 Jan 2025 07:55:05 -0500
From: Anthony Krowiak <akrowiak@...ux.ibm.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Halil Pasic <pasic@...ux.ibm.com>, Rorie Reyes <rreyes@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, hca@...ux.ibm.com, borntraeger@...ibm.com,
        agordeev@...ux.ibm.com, gor@...ux.ibm.com, jjherne@...ux.ibm.com
Subject: Re: [PATCH v1] s390/vfio-ap: Signal eventfd when guest AP
 configuration is changed




On 1/16/25 11:52 AM, Alex Williamson wrote:
> On Thu, 16 Jan 2025 10:38:41 -0500
> Anthony Krowiak <akrowiak@...ux.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@...ux.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
>