[PATCH] vfio/fsl-mc: keep logical vm_pgoff in MMIO region mmap

[PATCH] vfio/fsl-mc: keep logical vm_pgoff in MMIO region mmap

[Abdifatah Suruur]

vfio_fsl_mc_mmap_mmio() overwrites vma->vm_pgoff with the physical
frame number of the MMIO region and passes it to remap_pfn_range().
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()).

With a raw PFN in vm_pgoff the interval tree entry lands in the wrong
coordinate space and unmap_mapping_range() cannot find the VMA,
leaving stale MMIO mappings behind any revocation attempt.  Keep
vm_pgoff in the logical VFIO offset space, as vfio-pci does, and pass
the physical PFN to remap_pfn_range() explicitly.

Fixes: 67247289688d4 ("vfio/fsl-mc: Allow userspace to MMAP fsl-mc device MMIO regions")
Signed-off-by: Abdifatah Suruur <suruurism@gmail.com>

---
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -369,8 +369,7 @@
 	if (!region_cacheable)
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

-	vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
-
-	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+	return remap_pfn_range(vma, vma->vm_start,
+			       (region.addr >> PAGE_SHIFT) + pgoff,
 			       size, vma->vm_page_prot);
 }

Re: [PATCH] vfio/fsl-mc: keep logical vm_pgoff in MMIO region mmap

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch fails to address the exact same bug in the `vfio/cdx` and `vfio/platform` drivers. Both of these drivers continue to explicitly overwrite `vma->vm_pgoff` with the physical frame number before calling `io_remap_pfn_range()` / `remap_pfn_range()`. This places the VMA into the device file's mapping's interval tree using the physical PFN as the offset, rather than the logical file offset that VFIO expects. This breaks VMA revocation because `unmap_mapping_range()` uses the logical offset.
--

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

vfio/fsl-mc: keep logical vm_pgoff in MMIO region mmap

This commit modifies the VFIO fsl-mc driver to pass the physical PFN directly
to remap_pfn_range() instead of overwriting vma->vm_pgoff. Overwriting
vma->vm_pgoff broke the ability of unmap_mapping_range() to find and revoke
mappings because the VMA was inserted into the interval tree using the wrong
coordinate space.

> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index b4c3958201b25..9fc6f9136e01f 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -369,9 +369,8 @@ static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
>  	if (!region_cacheable)
>  		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  
> -	vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
> -
> -	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
logical offset clobbering issue in the fsl-mc driver, should the exact same
issue be fixed in the vfio/cdx and vfio/platform drivers as well?

Looking at drivers/vfio/cdx/main.c in vfio_cdx_mmap_mmio():

    vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;

And similarly in drivers/vfio/platform/vfio_platform_common.c in
vfio_platform_mmap_mmio():

    vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;

Both of these drivers continue to explicitly overwrite vma->vm_pgoff
with the physical frame number before mapping. Doesn't this also break
VMA revocation during device detach for these drivers, since
unmap_mapping_range() won't be able to find the mappings in the interval
tree? 

Could this leave stale MMIO mappings behind and allow userspace to retain
unrevoked access to hardware resources after a device is unbound?

> +	return remap_pfn_range(vma, vma->vm_start,
> +			       (region.addr >> PAGE_SHIFT) + pgoff,
>  			       size, vma->vm_page_prot);
>  }

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