On Mon, Apr 03, 2023 at 07:55:25PM +0530, Nipun Gupta wrote:
> +enum {
> + CDX_ID_F_VFIO_DRIVER_OVERRIDE = 1,
> +};
This seems to be missing the file2alias part.
> +static void vfio_cdx_regions_cleanup(struct vfio_cdx_device *vdev)
> +{
> + kfree(vdev->regions);
> +}
> +
> +static int vfio_cdx_reset_device(struct vfio_cdx_device *vdev)
> +{
> + return cdx_dev_reset(&vdev->cdx_dev->dev);
> +}
Wrapper functions should be avoided.
> +static void vfio_cdx_close_device(struct vfio_device *core_vdev)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> + int ret;
> +
> + vfio_cdx_regions_cleanup(vdev);
> +
> + /* reset the device before cleaning up the interrupts */
> + ret = vfio_cdx_reset_device(vdev);
> + if (WARN_ON(ret))
> + dev_warn(core_vdev->dev,
> + "VFIO_CDX: reset device has failed (%d)\n", ret);
This is pretty problematic.. if the reset can fail the device is
returned to the system in an unknown state and it seems pretty likely
that it can be a way to attack the kernel.
> + case VFIO_DEVICE_RESET:
> + {
> + return vfio_cdx_reset_device(vdev);
> + }
What happens to MMIO access during this reset?
> +static int vfio_cdx_mmap_mmio(struct vfio_cdx_region region,
> + struct vm_area_struct *vma)
> +{
> + u64 size = vma->vm_end - vma->vm_start;
> + u64 pgoff, base;
> +
> + pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + base = pgoff << PAGE_SHIFT;
> +
> + if (region.size < PAGE_SIZE || base + size > region.size)
> + return -EINVAL;
> +
> + vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
pgprot_device
> + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> + size, vma->vm_page_prot);
io_remap_pfn_range
> +static int vfio_cdx_mmap(struct vfio_device *core_vdev,
> + struct vm_area_struct *vma)
> +{
> + struct vfio_cdx_device *vdev =
> + container_of(core_vdev, struct vfio_cdx_device, vdev);
> + struct cdx_device *cdx_dev = vdev->cdx_dev;
> + unsigned int index;
> +
> + index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
> +
> + if (vma->vm_end < vma->vm_start)
> + return -EINVAL;
> + if (vma->vm_start & ~PAGE_MASK)
> + return -EINVAL;
> + if (vma->vm_end & ~PAGE_MASK)
> + return -EINVAL;
The core code already assures these checks.
> + if (!(vma->vm_flags & VM_SHARED))
> + return -EINVAL;
> + if (index >= cdx_dev->res_count)
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_MMAP))
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_READ) &&
> + (vma->vm_flags & VM_READ))
> + return -EINVAL;
> +
> + if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE) &&
> + (vma->vm_flags & VM_WRITE))
> + return -EINVAL;
> +
> + vma->vm_private_data = cdx_dev;
not needed
> diff --git a/drivers/vfio/cdx/vfio_cdx_private.h b/drivers/vfio/cdx/vfio_cdx_private.h
> new file mode 100644
> index 000000000000..8b6f1ee3f5cd
> --- /dev/null
> +++ b/drivers/vfio/cdx/vfio_cdx_private.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef VFIO_CDX_PRIVATE_H
> +#define VFIO_CDX_PRIVATE_H
> +
> +#define VFIO_CDX_OFFSET_SHIFT 40
> +#define VFIO_CDX_OFFSET_MASK (((u64)(1) << VFIO_CDX_OFFSET_SHIFT) - 1)
> +
> +#define VFIO_CDX_OFFSET_TO_INDEX(off) ((off) >> VFIO_CDX_OFFSET_SHIFT)
> +
> +#define VFIO_CDX_INDEX_TO_OFFSET(index) \
> + ((u64)(index) << VFIO_CDX_OFFSET_SHIFT)
use static inlines for function-line macros
> +struct vfio_cdx_region {
> + u32 flags;
> + u32 type;
> + u64 addr;
> + resource_size_t size;
> + void __iomem *ioaddr;
> +};
> +
> +struct vfio_cdx_device {
> + struct vfio_device vdev;
> + struct cdx_device *cdx_dev;
> + struct device *dev;
> + struct vfio_cdx_region *regions;
> +};
This header file does not seem necessary right now
Jason
