Re: [PATCH RFC v3 63/88] vfio/pci: Implement check_set_attributes 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/pci: missing BAR checks in
  vfio_pci_core_gmem_check_set_attributes()
- [High] vfio/pci: unguarded linux/kvm_host.h include breaks non-KVM
  arch builds
--

commit 2255a7dd0042c189f9db1a91b5da96def65f3e26
Author: Ackerley Tng <ackerleytng@google.com>

vfio/pci: Implement check_set_attributes provider op for guest_memfd

This commit implements the .gmem_check_set_attributes callback in vfio-pci
to reject private attribute conversions on ranges overlapping the MSI-X
vector table.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 9551a067b5689..894b1332f8a0c 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -16,6 +16,7 @@
>  #include <linux/eventfd.h>
>  #include <linux/file.h>
>  #include <linux/guest_memfd.h>
> +#include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
>  #include <linux/iommu.h>
>  #include <linux/module.h>

[Severity: High]
Does including <linux/kvm_host.h> unconditionally break builds with
CONFIG_VFIO_PCI_CORE on architectures without KVM support (such as 32-bit
arm, sparc, parisc, or m68k)?

include/linux/kvm_host.h pulls in <asm/kvm.h> and <asm/kvm_host.h>, which
only exist on KVM-capable architectures; drivers/vfio/vfio_main.c guards
this include with #if IS_ENABLED(CONFIG_KVM).  (A later patch in this
series, commit "QUESTION: KVM: guest_memfd: Disallow private mappings of
shared-only PFNs", removes this include, but intermediate commits fail to
build on non-KVM architectures.)

> @@ -2769,6 +2770,39 @@ int vfio_pci_core_gmem_get_pfn(struct vfio_device *core_vdev, pgoff_t index,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_gmem_get_pfn);
> +
> +int vfio_pci_core_gmem_check_set_attributes(struct vfio_device *core_vdev,
> +					    pgoff_t index, size_t nr_pages,
> +					    u64 attributes)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +	pgoff_t bar_pgoff;
> +	size_t size;
> +	loff_t pos;
> +	int bar;
> +
> +	if (!(attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE))
> +		return 0;
> +
> +	/*
> +	 * Prevent private conversion of pages containing the MSI-X vector
> +	 * table. The host kernel directly programs MSI-X table entries to
> +	 * configure and mask interrupts; converting these pages to private
> +	 * would cause host kernel writes to trigger fatal hardware protection
> +	 * faults (e.g. RMP or GPT violations) on confidential compute platforms.
> +	 */
> +	bar = index >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> +	bar_pgoff = index & ((1ULL << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> +	pos = (loff_t)bar_pgoff << PAGE_SHIFT;
> +	size = nr_pages << PAGE_SHIFT;
> +
> +	if (vfio_pci_range_overlaps_msix(vdev, bar, pos, size))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_core_gmem_check_set_attributes);
>  #endif

[Severity: High]
Does vfio_pci_core_gmem_check_set_attributes() miss checking that the BAR
is valid and supports mmap, and that [pos, pos + size) fits within
pci_resource_len(vdev->pdev, bar)?

The commit message states that this function verifies that the targeted BAR
is a standard memory BAR and that the range does not exceed the BAR's
physical resource size, as vfio_pci_core_gmem_get_pfn() does:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_gmem_get_pfn() {
    ...
	if (bar >= PCI_STD_NUM_BARS || !vdev->bar_mmap_supported[bar])
		return -EINVAL;

	if (pgoff >= (pci_resource_len(pdev, bar) >> PAGE_SHIFT))
		return -EINVAL;
    ...
}

Without those checks, non-mmappable regions and offsets past the end of a
BAR can be marked private.  Furthermore, because kvm_gmem_set_attributes()
only bounds the request against the inode size spanning the 40-bit region
offset space and bar is decoded solely from the starting index, a
KVM_SET_MEMORY_ATTRIBUTES2 range starting in an earlier BAR (bar <
vdev->msix_bar) and spanning across the 40-bit boundary into vdev->msix_bar
has bar != vdev->msix_bar in vfio_pci_range_overlaps_msix(), bypassing the
MSI-X overlap check and allowing the MSI-X table pages to be marked private
and later faulted in one page at a time via vfio_pci_core_gmem_get_pfn().
(Commit "QUESTION: KVM: guest_memfd: Disallow private mappings of
shared-only PFNs" later in the series replaces this callback, but these
checks are needed if the check_set_attributes approach is kept or when
bisecting.)

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