Commit be8d3ada authored by Jason Gunthorpe's avatar Jason Gunthorpe Committed by Alex Williamson

vfio: Add missing locking for struct vfio_group::kvm

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
    [..]
        kvm_get_kvm(vgpu->kvm);
	    // UAF!

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 a consumer of
vgpu->kvm and trigger this same UAF, it just makes the notifier
self-consistent.

Fixes: ccd46dba ("vfio: support notifier chain in vfio_group")
Reviewed-by: default avatarKevin Tian <kevin.tian@intel.com>
Signed-off-by: default avatarJason Gunthorpe <jgg@nvidia.com>
Tested-by: default avatarNicolin Chen <nicolinc@nvidia.com>
Tested-by: default avatarMatthew Rosato <mjrosato@linux.ibm.com>
Link: https://lore.kernel.org/r/1-v2-d035a1842d81+1bf-vfio_group_locking_jgg@nvidia.comSigned-off-by: default avatarAlex Williamson <alex.williamson@redhat.com>
parent 6b17ca8e
...@@ -76,6 +76,7 @@ struct vfio_group { ...@@ -76,6 +76,7 @@ struct vfio_group {
atomic_t opened; atomic_t opened;
enum vfio_group_type type; enum vfio_group_type type;
unsigned int dev_counter; unsigned int dev_counter;
struct rw_semaphore group_rwsem;
struct kvm *kvm; struct kvm *kvm;
struct blocking_notifier_head notifier; struct blocking_notifier_head notifier;
}; };
...@@ -360,6 +361,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group, ...@@ -360,6 +361,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
group->cdev.owner = THIS_MODULE; group->cdev.owner = THIS_MODULE;
refcount_set(&group->users, 1); refcount_set(&group->users, 1);
init_rwsem(&group->group_rwsem);
INIT_LIST_HEAD(&group->device_list); INIT_LIST_HEAD(&group->device_list);
mutex_init(&group->device_lock); mutex_init(&group->device_lock);
group->iommu_group = iommu_group; group->iommu_group = iommu_group;
...@@ -1694,9 +1696,11 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm) ...@@ -1694,9 +1696,11 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
if (file->f_op != &vfio_group_fops) if (file->f_op != &vfio_group_fops)
return; return;
down_write(&group->group_rwsem);
group->kvm = kvm; group->kvm = kvm;
blocking_notifier_call_chain(&group->notifier, blocking_notifier_call_chain(&group->notifier,
VFIO_GROUP_NOTIFY_SET_KVM, kvm); VFIO_GROUP_NOTIFY_SET_KVM, kvm);
up_write(&group->group_rwsem);
} }
EXPORT_SYMBOL_GPL(vfio_file_set_kvm); EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
...@@ -2004,15 +2008,22 @@ static int vfio_register_group_notifier(struct vfio_group *group, ...@@ -2004,15 +2008,22 @@ static int vfio_register_group_notifier(struct vfio_group *group,
return -EINVAL; return -EINVAL;
ret = blocking_notifier_chain_register(&group->notifier, nb); ret = blocking_notifier_chain_register(&group->notifier, nb);
if (ret)
return ret;
/* /*
* The attaching of kvm and vfio_group might already happen, so * The attaching of kvm and vfio_group might already happen, so
* here we replay once upon registration. * here we replay once upon registration.
*/ */
if (!ret && set_kvm && group->kvm) if (set_kvm) {
blocking_notifier_call_chain(&group->notifier, down_read(&group->group_rwsem);
VFIO_GROUP_NOTIFY_SET_KVM, group->kvm); if (group->kvm)
return ret; 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, int vfio_register_notifier(struct vfio_device *device,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment