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

[sashiko-bot@kernel.org]

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