Re: [PATCH] vfio/pci/vfio_pci_core.c: add IORESOURCE_EXCLUSIVE flag

[Alex Williamson]

On Tue,  7 Mar 2023 12:40:18 +0900
Seunggyun Lee <sglee97@xxxxxxxxxxxxx> wrote:

> While using a pci device (GPU) through the vfio-pci passthrough in QEMU
> VM, host can mmap the PCI device which used by the guest through sysfs.
> 
> In this case, when the guest used the PCI device, the host could also
> access the data stored in the PCI device memory.
> 
> Regarding this, there is a routine to check IORESOURCE_EXCLUSIVE through
> iomem_is_exclusive() in pci_mmap_resource() of pci-sysfs.c, but vfio-pci
> driver doesn't seem to set that flag.
> 
> Wouldn't it be better to use pci_request_selected_regions_exclusive() to
> set the IORESOURCE_EXCLUSIVE flag rather than
> pci_request_selected_regions() that was used previously?
> 
> Signed-off-by: Seunggyun Lee <sglee97@xxxxxxxxxxxxx>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 26a541cc64d1..9731ac35b3ad 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1779,7 +1779,7 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
>  	 * we need to request the region and the barmap tracks that.
>  	 */
>  	if (!vdev->barmap[index]) {
> -		ret = pci_request_selected_regions(pdev,
> +		ret = pci_request_selected_regions_exclusive(pdev,
>  						   1 << index, "vfio-pci");
>  		if (ret)
>  			return ret;

I don't understand why this request is so pervasive lately, there are
other means to lockdown sysfs access to a device, why are they not
sufficient (ex. LOCKDOWN_PCI_ACCESS).

If this is work towards confidential computing support with VFIO, I'm
afraid that sysfs access to the device is only one potential vector,
QEMU itself is in control of whether a VM directly maps device
resources.

Also, IORESOURCE_EXCLUSIVE is described as preventing userspace mapping
of the resource, so it's a bit ironic that a driver providing userspace
mappings of device resources would mark the resource in such a way.
Thanks,

Alex