Re: [PATCH] vfio/fsl-mc: prevent read-only region mappings from becoming writable

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

New issues:
- [Low] Modifying VMA flags (`vm_flags_clear(vma, VM_MAYWRITE)`) inside the `f_op->mmap()` handler violates the VMA flags modification API guidelines regarding merging.

Pre-existing issues:
- [High] The patch fixes a vulnerability where read-only region mappings can be upgraded to writable via mprotect(), but misses identical pre-existing vulnerabilities in sibling drivers vfio_cdx and vfio_platform.
--

--- Patch [1]: [PATCH] vfio/fsl-mc: prevent read-only region mappings from becoming writable ---
commit f3aa2053ad6dcc721a152db22b46cec93266b9df
Author: Abdifatah Suruur <[email protected]>

vfio/fsl-mc: prevent read-only region mappings from becoming writable

This commit clears VM_MAYWRITE for regions without the WRITE flag.
It prevents userspace from mapping read-only regions and upgrading them
to writable using mprotect().

Fixes: fb1ff4c194157 ("vfio/fsl-mc: Add VFIO framework skeleton for fsl-mc devices")
Cc: [email protected]

> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index b4c3958201b25..85f479db936c6 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -407,6 +407,10 @@ static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
>  			&& (vma->vm_flags & VM_WRITE))
>  		return -EINVAL;
>  
> +	/* Prevent read-only region mappings from being upgraded with mprotect() */
> +	if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE))
> +		vm_flags_clear(vma, VM_MAYWRITE);
> +

[Severity: Low]
Does clearing VMA flags directly inside the f_op->mmap() handler violate the
VMA merge rules? Modifying flags post-merge can silently break future merges.

While remap_pfn_range() later adds VM_DONTEXPAND (making these VMAs
unmergeable anyway), is this still the correct place to modify the flags, or
should this be handled earlier via a .mmap_prepare() callback?

[Severity: High]
This isn't a bug introduced by this patch, but it appears sibling drivers
have this exact same vulnerability.

For example, in 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;

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

    if (!(vdev->regions[index].flags & VFIO_REGION_INFO_FLAG_WRITE)
            && (vma->vm_flags & VM_WRITE))
        return -EINVAL;

Both drivers reject PROT_WRITE but fail to clear VM_MAYWRITE. Could
unprivileged userspace map read-only regions as PROT_READ and then use
mprotect() to add PROT_WRITE, allowing unauthorized writes to device MMIO?

>  	vma->vm_private_data = mc_dev;
>  
>  	return vfio_fsl_mc_mmap_mmio(vdev->regions[index], vma);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1