> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Friday, May 6, 2022 8:25 AM
>
> Without locking userspace can trigger a UAF by racing
> KVM_DEV_VFIO_GROUP_DEL with VFIO_GROUP_GET_DEVICE_FD:
>
> CPU1 CPU2
> ioctl(KVM_DEV_VFIO_GROUP_DEL)
> ioctl(VFIO_GROUP_GET_DEVICE_FD)
> vfio_group_get_device_fd
> open_device()
> intel_vgpu_open_device()
> vfio_register_notifier()
> vfio_register_group_notifier()
> blocking_notifier_call_chain(&group->notifier,
> VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
>
> set_kvm()
> group->kvm = NULL
> close()
> kfree(kvm)
>
> intel_vgpu_group_notifier()
> vdev->kvm = data
> [..]
> kvmgt_guest_init()
> kvm_get_kvm(info->kvm);
> // UAF!
>
this doesn't match the latest code since kvmgt_guest_init() has
been removed.
With the new code UAF will occur in an earlier place:
ret = -ESRCH;
if (!vgpu->kvm || vgpu->kvm->mm != current->mm) {
gvt_vgpu_err("KVM is required to use Intel vGPU\n");
goto undo_register;
}
...
kvm_get_kvm(vgpu->kvm);
> Add a simple rwsem in the group to protect the kvm while the notifier is
> using it.
>
> Note this doesn't fix the race internal to i915 where userspace can
> trigger two VFIO_GROUP_NOTIFY_SET_KVM's before we reach
> kvmgt_guest_init()
> and trigger this same UAF.
>
> Fixes: ccd46dbae77d ("vfio: support notifier chain in vfio_group")
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
with above flow updated:
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> drivers/vfio/vfio.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e6ea3981bc7c4a..0477df3a50a3d6 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -77,6 +77,7 @@ struct vfio_group {
> wait_queue_head_t container_q;
> enum vfio_group_type type;
> unsigned int dev_counter;
> + struct rw_semaphore group_rwsem;
> struct kvm *kvm;
> struct blocking_notifier_head notifier;
> };
> @@ -361,6 +362,7 @@ static struct vfio_group *vfio_group_alloc(struct
> iommu_group *iommu_group,
> group->cdev.owner = THIS_MODULE;
>
> refcount_set(&group->users, 1);
> + init_rwsem(&group->group_rwsem);
> INIT_LIST_HEAD(&group->device_list);
> mutex_init(&group->device_lock);
> init_waitqueue_head(&group->container_q);
> @@ -1714,9 +1716,11 @@ void vfio_file_set_kvm(struct file *file, struct kvm
> *kvm)
> if (file->f_op != &vfio_group_fops)
> return;
>
> + down_write(&group->group_rwsem);
> group->kvm = kvm;
> blocking_notifier_call_chain(&group->notifier,
> VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> + up_write(&group->group_rwsem);
> }
> EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
>
> @@ -2024,15 +2028,22 @@ static int vfio_register_group_notifier(struct
> vfio_group *group,
> return -EINVAL;
>
> ret = blocking_notifier_chain_register(&group->notifier, nb);
> + if (ret)
> + return ret;
>
> /*
> * The attaching of kvm and vfio_group might already happen, so
> * here we replay once upon registration.
> */
> - if (!ret && set_kvm && group->kvm)
> - blocking_notifier_call_chain(&group->notifier,
> - VFIO_GROUP_NOTIFY_SET_KVM,
> group->kvm);
> - return ret;
> + if (set_kvm) {
> + down_read(&group->group_rwsem);
> + if (group->kvm)
> + blocking_notifier_call_chain(&group->notifier,
> +
> VFIO_GROUP_NOTIFY_SET_KVM,
> + group->kvm);
> + up_read(&group->group_rwsem);
> + }
> + return 0;
> }
>
> int vfio_register_notifier(struct vfio_device *device,
> --
> 2.36.0
