| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <74b2dad2-1165-9509-111a-48407e2a1319@amd.com>
Date: Fri, 7 Apr 2023 10:33:26 +0530
From: Nipun Gupta <nipun.gupta@....com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: alex.williamson@...hat.com, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, git@....com, harpreet.anand@....com,
michal.simek@....com, "Agarwal, Nikhil" <nikhil.agarwal@....com>,
Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@....com>,
okaya@...nel.org
Subject: Re: [PATCH] vfio/cdx: add support for CDX bus
On 4/5/2023 9:03 PM, Jason Gunthorpe wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
Sure. Will add this.
>
>> +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.
Agree
>
>> +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.
We will update the code to disable the device in case of failures.
>
>> + case VFIO_DEVICE_RESET:
>> + {
>> + return vfio_cdx_reset_device(vdev);
>> + }
>
> What happens to MMIO access during this reset?
There is no config space here; and access to mmap space will go as usual
but user may not see the expected behavior as the device is being reset.
It is responsibility of user of VFIO device to serialize such access and
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
Will update..
>
>> + 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.
Sure will remove these unnecessary 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
Okay
>
>> 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
Okay. Will update the code.
>
>> +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
There is work in progress which would add support of MSI in VFIO-CDX
driver, and where would be additional file/s. So we would like to keep
the header file separate.
Thanks,
Nipun
>
> Jason
Powered by blists - more mailing lists