[PATCH] vfio/cdx: reject non-shared MMIO mmaps

[PATCH] vfio/cdx: reject non-shared MMIO mmaps

[Abdifatah Suruur]

vfio_cdx_mmap() accepts MAP_PRIVATE mappings.  A non-shared mapping
with VM_MAYWRITE is a COW mapping, and remap_pfn_range()'s COW special
case in get_remap_pgoff() then overwrites vma->vm_pgoff with the
physical frame number:

	if (is_cow) {
		if (addr != vm_start || end != vm_end)
			return -EINVAL;
		*vm_pgoff_p = pfn;
	}

The VMA is inserted into the device file's mapping->i_mmap interval
tree keyed by vm_pgoff, which VFIO expects to be the logical file
offset: the core links every device mmap to the device inode's
i_mapping precisely so that unmap_mapping_range() can revoke all
mappings associated with a device (see vfio_device_cdev_open()).

A raw PFN in vm_pgoff lands the interval tree entry in the wrong
coordinate space, so unmap_mapping_range() cannot find the VMA and a
stale MMIO mapping survives any revocation attempt.  Without this
check, a MAP_PRIVATE mapping reintroduces that corruption even with
the logical-vm_pgoff fix in place.

Require VM_SHARED as vfio-pci, vfio/fsl-mc and vfio/platform already
do.

Fixes: 234489ac56130 ("vfio/cdx: add support for CDX bus")
Signed-off-by: Abdifatah Suruur <suruurism@gmail.com>
---
 drivers/vfio/cdx/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
index b31ed4be7bdc1..6d208db0896d6 100644
--- a/drivers/vfio/cdx/main.c
+++ b/drivers/vfio/cdx/main.c
@@ -271,6 +271,9 @@ static int vfio_cdx_mmap(struct vfio_device *core_vdev,
 
 	index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
 
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+
 	if (index >= cdx_dev->res_count)
 		return -EINVAL;
 
-- 
2.53.0

Re: [PATCH] vfio/cdx: reject non-shared MMIO mmaps

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fails to prevent interval tree corruption because vfio_cdx_mmap_mmio() explicitly overwrites vma->vm_pgoff with a raw physical frame number.
- [High] The mmap handler fails to clear VM_MAYWRITE for read-only regions, allowing unauthorized write access via mprotect().
--

commit a72fc948fdc663ac850e54c9671f37f1c661b655
Author: Abdifatah Suruur <suruurism@gmail.com>

vfio/cdx: reject non-shared MMIO mmaps

This commit rejects non-shared mappings in vfio_cdx_mmap to prevent
remap_pfn_range from overwriting the logical offset with a physical
frame number. The core links device mappings to the interval tree using
this offset, so preserving it is necessary for device mapping revocation.

> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> index b31ed4be7bdc1..6d208db0896d6 100644
> --- a/drivers/vfio/cdx/main.c
> +++ b/drivers/vfio/cdx/main.c
> @@ -271,6 +271,9 @@ static int vfio_cdx_mmap(struct vfio_device *core_vdev,
>  
>  	index = vma->vm_pgoff >> (VFIO_CDX_OFFSET_SHIFT - PAGE_SHIFT);
>  
> +	if (!(vma->vm_flags & VM_SHARED))
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but does this patch fully prevent the interval
tree corruption? The commit message notes that placing a raw physical frame
number into the vm_pgoff field breaks revocation because subsystems like vfio
use the logical offset to search the interval tree.

However, if a user maps a region as shared, this check passes and the driver
eventually calls vfio_cdx_mmap_mmio. In that function, the logical offset is
explicitly overwritten with a physical frame number before inserting the vma
into the tree:

drivers/vfio/cdx/main.c:vfio_cdx_mmap_mmio() {
        ...
        vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
        vma->vm_page_prot = pgprot_device(vma->vm_page_prot);

        return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
                                  size, vma->vm_page_prot);
}

Will this cause stale mmio mappings to survive device teardown for shared
mappings as well, potentially leading to a use-after-free of mmio memory?

[Severity: High]
This is also a pre-existing issue, but does the mmap handler leave read-only
mappings vulnerable to an mprotect escalation?

Further down in vfio_cdx_mmap, the driver enforces read-only permissions by
checking if the write flag is set when the region info write flag is absent:

drivers/vfio/cdx/main.c:vfio_cdx_mmap() {
        ...
        if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE) &&
            (vma->vm_flags & VM_WRITE))
                return -EPERM;
        ...
}

Since the vfio device file is opened for read and write access, the core mm
layer initially sets the vm_maywrite flag on the vma. If a user maps a
read-only region with read permissions, the check above passes. Because
vm_flags_clear for vm_maywrite is never called, could the user subsequently
call mprotect to add write permissions, bypassing the hardware protection
and gaining write access to the region?

>  	if (index >= cdx_dev->res_count)
>  		return -EINVAL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906123245.10884-1-suruurism@gmail.com?part=1