Re: [PATCH RFC v3 61/88] vfio: Implement mmap provider op for guest_memfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] vfio: uncached and decrypted vma->vm_page_prot lost in
  guest_memfd fault
- [Medium] vfio: vfio_device_fops_mmap() returns -EOPNOTSUPP instead of
  -EINVAL
--

commit 6c78c8834b1a41506b88de16d4e56626d4ac8609
Author: Ackerley Tng <ackerleytng@google.com>

vfio: Implement mmap provider op for guest_memfd

This commit extracts a vfio_device_mmap() helper from
vfio_device_fops_mmap() and wires up the .mmap provider callback in
vfio_device_gmem_provider_ops to forward guest_memfd mappings to the bus
driver's .mmap handler.

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 93bd0a200ad22..979ed69c5ebf7 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -1503,20 +1503,25 @@ static ssize_t vfio_device_fops_write(struct file *filep,
>  	return device->ops->write(device, buf, count, ppos);
>  }
>  
> +static int vfio_device_mmap(struct vfio_device *device,
> +			    struct vm_area_struct *vma)
> +{
> +	if (unlikely(!device->ops->mmap))
> +		return -EOPNOTSUPP;
> +
> +	return device->ops->mmap(device, vma);
> +}
> +
>  static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
>  {
>  	struct vfio_device_file *df = filep->private_data;
> -	struct vfio_device *device = df->device;
>  	int ret;
>  
>  	ret = vfio_df_check_full_access(df);
>  	if (ret)
>  		return ret;
>  
> -	if (unlikely(!device->ops->mmap))
> -		return -EINVAL;
> -
> -	return device->ops->mmap(device, vma);
> +	return vfio_device_mmap(df->device, vma);
>  }

[Severity: Medium]
Does extracting vfio_device_mmap() here cause a regression where
vfio_device_fops_mmap() returns -EOPNOTSUPP instead of -EINVAL when a bus
driver does not implement device->ops->mmap?

Direct mmap(2) calls on full-access VFIO device fds for drivers without an
.mmap callback (such as vfio-ccw or vfio-ap) previously returned -EINVAL,
matching vfio_device_fops_read(), vfio_device_fops_write(), and
vfio_device_fops_unl_ioctl().  Returning -EOPNOTSUPP changes the user-visible
errno and makes a missing driver .mmap callback indistinguishable from a
file descriptor bound with VFIO_DEVICE_BIND_FLAG_CTRL_ONLY.

> @@ -1604,10 +1609,23 @@ static int vfio_device_gmem_get_pfn(void *provider, pgoff_t index,
>  					 pfn_attrs);
>  }
>  
> +static int vfio_device_gmem_mmap(void *provider, struct vm_area_struct *vma)
> +{
> +	struct file *file = provider;
> +	struct vfio_device_file *df = file->private_data;
> +
> +	/*
> +	 * No control- vs. data-plane filtering is required because
> +	 * vfio_device_mmap() already only allows mmap-ing the data plane.
> +	 */
> +	return vfio_device_mmap(df->device, vma);
> +}
> +
>  static const struct guest_memfd_provider_operations vfio_device_gmem_provider_ops = {
>  	.attach  = vfio_device_gmem_attach,
>  	.release = vfio_device_gmem_release,
>  	.get_pfn = vfio_device_gmem_get_pfn,
> +	.mmap    = vfio_device_gmem_mmap,
>  };
>  #endif

[Severity: High]
When kvm_gmem_mmap() overrides vma->vm_ops with kvm_gmem_pfn_vm_ops after
vfio_device_gmem_mmap() returns, does the guest_memfd fault handler discard
the uncached and decrypted protections that vfio_pci_core_mmap() sets on
vma->vm_page_prot?

vfio_pci_core_mmap() configures vma->vm_page_prot with pgprot_noncached()
and pgprot_decrypted().  However, on a user fault,
kvm_gmem_fault_user_mapping() -> gmem_insert_pfn() recomputes page
protections from vma->vm_flags instead of using vma->vm_page_prot:

virt/kvm/guest_memfd.c:gmem_insert_pfn() {
    ...
	return vmf_insert_pfn_prot(vma, vmf->address, pfn,
				   vm_get_page_prot(flags));
}

Because vm_get_page_prot(flags) derives a fresh pgprot_t from
protection_map[], both pgprot_noncached() and pgprot_decrypted() are lost.
On architectures without PAT PFNMAP tracking in pfnmap_setup_cachemode_pfn()
(such as arm64), PCI BAR MMIO pages are mapped into host userspace as
write-back cacheable normal memory rather than Device memory and without
PROT_NS_SHARED, which can corrupt MMIO accesses or trigger synchronous
external aborts and SError.  On x86 systems with SME or SEV active,
vm_get_page_prot() sets _PAGE_ENC, causing host userspace MMIO accesses to
unencrypted device BARs to be performed with the encryption bit set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=61