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