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

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

Pre-existing issues:
- [High] Missing VM_SHARED check in vfio_cdx_mmap allows COW on device MMIO memory.
- [High] Missing VM_MAYWRITE clearing allows read-only VFIO platform and fsl-mc regions to be upgraded to writable via mprotect.
--

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

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

This commit clears the VM_MAYWRITE flag in vfio_cdx_mmap() for regions
without the WRITE flag. This prevents userspace from upgrading read-only
mappings to writable using mprotect().

Cc: [email protected]

> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> index b31ed4be7bdc1..b6b17a6a46811 100644
> --- a/drivers/vfio/cdx/main.c
> +++ b/drivers/vfio/cdx/main.c
> @@ -285,6 +285,10 @@ static int vfio_cdx_mmap(struct vfio_device *core_vdev,
>  	    (vma->vm_flags & VM_WRITE))
>  		return -EPERM;

[Severity: High]
This is a pre-existing issue, but does vfio_cdx_mmap() also need to reject
mappings that lack VM_SHARED?

If a user maps the region with MAP_PRIVATE, a write fault will trigger a
Copy-On-Write (COW). The COW reads from the device MMIO using bulk memory
instructions (via __wp_page_copy_user()), which could cause unsupported bus
transactions (like SError on ARM64) and crash the system. Additionally,
writes to the mapping will quietly hit RAM instead of the device, breaking
MMIO semantics.

Unlike other VFIO bus drivers, this function appears to be missing a
validation check to reject private mappings. Could the absence of a
`(vma->vm_flags & VM_SHARED) == 0` check here allow users to inadvertently
trigger these COW crashes?

>  
> +	/* 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: High]
This isn't a bug introduced by this patch, but does the same vulnerability
exist in other vfio drivers?

Looking at vfio_platform_mmap() in drivers/vfio/platform/vfio_platform_common.c
and vfio_fsl_mc_mmap() in drivers/vfio/fsl-mc/vfio_fsl_mc.c, both check for
VM_WRITE but neither explicitly clears VM_MAYWRITE.

If those drivers leave VM_MAYWRITE set, could they also allow users to upgrade
read-only regions to writable via mprotect() in the same way being fixed here?

>  	return vfio_cdx_mmap_mmio(vdev->regions[index], vma);
>  }
>

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