> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Thursday, April 21, 2022 3:23 AM
>
> VFIO PCI does a security check as part of hot reset to prove that the user
> has permission to manipulate all the devices that will be impacted by the
> reset.
>
> Use a new API vfio_file_has_dev() to perform this security check against
> the struct file directly and remove the vfio_group from VFIO PCI.
>
> Since VFIO PCI was the last user of vfio_group_get_external_user() remove
> it as well.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci_core.c | 42 ++++++++++-----------
> drivers/vfio/vfio.c | 63 +++++++++-----------------------
> include/linux/vfio.h | 2 +-
> 3 files changed, 40 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index b7bb16f92ac628..465c42f53fd2fc 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -577,7 +577,7 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void
> *data)
>
> struct vfio_pci_group_info {
> int count;
> - struct vfio_group **groups;
> + struct file **files;
> };
>
> static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot
> *slot)
> @@ -1039,10 +1039,10 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
> } else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) {
> struct vfio_pci_hot_reset hdr;
> int32_t *group_fds;
> - struct vfio_group **groups;
> + struct file **files;
> struct vfio_pci_group_info info;
> bool slot = false;
> - int group_idx, count = 0, ret = 0;
> + int file_idx, count = 0, ret = 0;
>
> minsz = offsetofend(struct vfio_pci_hot_reset, count);
>
> @@ -1075,17 +1075,17 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
> return -EINVAL;
>
> group_fds = kcalloc(hdr.count, sizeof(*group_fds),
> GFP_KERNEL);
> - groups = kcalloc(hdr.count, sizeof(*groups), GFP_KERNEL);
> - if (!group_fds || !groups) {
> + files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL);
> + if (!group_fds || !files) {
> kfree(group_fds);
> - kfree(groups);
> + kfree(files);
> return -ENOMEM;
> }
>
> if (copy_from_user(group_fds, (void __user *)(arg + minsz),
> hdr.count * sizeof(*group_fds))) {
> kfree(group_fds);
> - kfree(groups);
> + kfree(files);
> return -EFAULT;
> }
>
> @@ -1094,22 +1094,22 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
> * user interface and store the group and iommu ID. This
> * ensures the group is held across the reset.
> */
> - for (group_idx = 0; group_idx < hdr.count; group_idx++) {
> - struct vfio_group *group;
> - struct fd f = fdget(group_fds[group_idx]);
> - if (!f.file) {
> + for (file_idx = 0; file_idx < hdr.count; file_idx++) {
> + struct file *file = fget(group_fds[file_idx]);
> +
> + if (!file) {
> ret = -EBADF;
> break;
> }
>
> - group = vfio_group_get_external_user(f.file);
> - fdput(f);
> - if (IS_ERR(group)) {
> - ret = PTR_ERR(group);
> + /* Ensure the FD is a vfio group FD.*/
> + if (!vfio_file_iommu_group(file)) {
> + fput(file);
> + ret = -EINVAL;
> break;
> }
>
> - groups[group_idx] = group;
> + files[file_idx] = file;
> }
>
> kfree(group_fds);
> @@ -1119,15 +1119,15 @@ long vfio_pci_core_ioctl(struct vfio_device
> *core_vdev, unsigned int cmd,
> goto hot_reset_release;
>
> info.count = hdr.count;
> - info.groups = groups;
> + info.files = files;
>
> ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info);
>
> hot_reset_release:
> - for (group_idx--; group_idx >= 0; group_idx--)
> - vfio_group_put_external_user(groups[group_idx]);
> + for (file_idx--; file_idx >= 0; file_idx--)
> + fput(files[file_idx]);
>
> - kfree(groups);
> + kfree(files);
> return ret;
> } else if (cmd == VFIO_DEVICE_IOEVENTFD) {
> struct vfio_device_ioeventfd ioeventfd;
> @@ -1964,7 +1964,7 @@ static bool vfio_dev_in_groups(struct
> vfio_pci_core_device *vdev,
> unsigned int i;
>
> for (i = 0; i < groups->count; i++)
> - if (groups->groups[i] == vdev->vdev.group)
> + if (vfio_file_has_dev(groups->files[i], &vdev->vdev))
> return true;
> return false;
> }
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 7d0fad02936f69..ff5f6e0f285faa 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1899,51 +1899,6 @@ static const struct file_operations
> vfio_device_fops = {
> .mmap = vfio_device_fops_mmap,
> };
>
> -/*
> - * External user API, exported by symbols to be linked dynamically.
> - *
> - * The protocol includes:
> - * 1. do normal VFIO init operation:
> - * - opening a new container;
> - * - attaching group(s) to it;
> - * - setting an IOMMU driver for a container.
> - * When IOMMU is set for a container, all groups in it are
> - * considered ready to use by an external user.
> - *
> - * 2. User space passes a group fd to an external user.
> - * The external user calls vfio_group_get_external_user()
> - * to verify that:
> - * - the group is initialized;
> - * - IOMMU is set for it.
> - * If both checks passed, vfio_group_get_external_user()
> - * increments the container user counter to prevent
> - * the VFIO group from disposal before KVM exits.
> - *
> - * 3. When the external KVM finishes, it calls
> - * vfio_group_put_external_user() to release the VFIO group.
> - * This call decrements the container user counter.
> - */
> -struct vfio_group *vfio_group_get_external_user(struct file *filep)
> -{
> - struct vfio_group *group = filep->private_data;
> - int ret;
> -
> - if (filep->f_op != &vfio_group_fops)
> - return ERR_PTR(-EINVAL);
> -
> - ret = vfio_group_add_container_user(group);
> - if (ret)
> - return ERR_PTR(ret);
> -
> - /*
> - * Since the caller holds the fget on the file group->users must be >= 1
> - */
> - vfio_group_get(group);
> -
> - return group;
> -}
> -EXPORT_SYMBOL_GPL(vfio_group_get_external_user);
> -
> /*
> * External user API, exported by symbols to be linked dynamically.
> * The external user passes in a device pointer
> @@ -2056,6 +2011,24 @@ void vfio_file_set_kvm(struct file *file, struct kvm
> *kvm)
> }
> EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
>
> +/**
> + * vfio_file_has_dev - True if the VFIO file is a handle for device
> + * @file: VFIO file to check
> + * @device: Device that must be part of the file
> + *
> + * Returns true if given file has permission to manipulate the given device.
> + */
> +bool vfio_file_has_dev(struct file *file, struct vfio_device *device)
> +{
> + struct vfio_group *group = file->private_data;
> +
> + if (file->f_op != &vfio_group_fops)
> + return false;
> +
> + return group == device->group;
> +}
> +EXPORT_SYMBOL_GPL(vfio_file_has_dev);
> +
> /*
> * Sub-module support
> */
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index cbd9103b5c1223..e8be8ec40f2b50 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -140,13 +140,13 @@ int vfio_mig_get_next_state(struct vfio_device
> *device,
> /*
> * External user API
> */
> -extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
> extern void vfio_group_put_external_user(struct vfio_group *group);
> extern struct vfio_group *vfio_group_get_external_user_from_dev(struct
> device
> *dev);
> extern struct iommu_group *vfio_file_iommu_group(struct file *file);
> extern bool vfio_file_enforced_coherent(struct file *file);
> extern void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
> +extern bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
>
> #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned
> long))
>
> --
> 2.36.0
