Re: [PATCH 1/6] vfio: Add missing locking for struct vfio_group::kvm

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, May 13, 2022 at 09:08:06AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 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);

Right thanks

I just dropped the kvmgt_guest_init() since it is all in the same
function now.

This needs fixing as Christoph suggested by making the KVM some
integral part of the VFIO so it can't just randomly disappear under a
VFIO driver that requires it.

Jason