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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Modifying VMA flags post-merge inside the .mmap() callback silently breaks future VMA merges, leading to fragmentation.

Pre-existing issues:
- [High] Incomplete mprotect() hardening allows unauthorized read access to write-only hardware regions.
- [High] Assigning a raw physical frame number (PFN) to `vma->vm_pgoff` for a file-backed VMA corrupts the VMA interval tree.
--

commit 6c3ce3b68057fa0dfd3b9b92fc8493eaac414153
Author: Abdifatah Suruur <suruurism@gmail.com>
Subject: vfio/platform: prevent read-only region mappings from becoming writable

vfio_platform_mmap() rejects writable mappings of regions without the
WRITE flag, but leaves VM_MAYWRITE set. Userspace can map such a region
read-only and then upgrade the mapping to writable with mprotect(). This
commit clears VM_MAYWRITE for regions without the WRITE flag.

> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index c72db5a99ebda..20d87c244ca28 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -595,6 +595,10 @@ int vfio_platform_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
>  			&& (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: High]
This isn't a bug introduced by this patch, but while this hardens the WRITE
path against mprotect upgrades, does it overlook hardening the READ path via
vm_flags_clear(vma, VM_MAYREAD)?

If a user maps a write-only region with PROT_NONE (or PROT_WRITE), they
bypass the initial vma->vm_flags & VM_READ check. Since the region lacks
the READ flag, the user should not be able to read it. However, because
VM_MAYREAD is not cleared, couldn't the user successfully call
mprotect(..., PROT_READ) to upgrade the VMA and read the memory?

Can this allow an attacker to gain unauthorized read access to write-only
hardware registers, which might corrupt hardware state machines, cause CPU
stalls, or trigger fatal bus errors (e.g., SError on ARM)?

[Severity: Medium]
Will modifying VMA flags post-merge inside the driver's mmap callback
silently break future VMA merges and lead to fragmentation?

When userspace mmaps a subsequent adjacent segment of the same region,
mmap_region() attempts to merge it using the proposed VMA flags, which
still contain VM_MAYWRITE by default. Because the existing adjacent VMA
had VM_MAYWRITE cleared by the driver's .mmap callback during its
creation, and VM_MAYWRITE is not in VMA_IGNORE_MERGE_FLAGS, the flags
will mismatch.

Won't is_mergeable_vma() reject the merge, causing VMA fragmentation for
adjacent mappings of the same read-only region?

>  
>  	vma->vm_private_data = vdev;
>  
>  	if (vdev->regions[index].type & VFIO_PLATFORM_REGION_TYPE_MMIO)
>  		return vfio_platform_mmap_mmio(vdev->regions[index], vma);

[Severity: High]
This is a pre-existing issue, but does assigning a raw physical frame
number (PFN) to vma->vm_pgoff for a file-backed VMA corrupt the VMA
interval tree?

vfio_platform_mmap_mmio() explicitly overwrites vma->vm_pgoff with the PFN
before calling remap_pfn_range():

drivers/vfio/platform/vfio_platform_common.c:vfio_platform_mmap_mmio() {
	...
	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,
			       req_len, vma->vm_page_prot);
}

The VMA is inserted into the file's mapping->i_mmap interval tree using
the raw PFN as the index instead of the file offset. Won't any core kernel
mechanism (such as unmap_mapping_range) that searches the interval tree by
file offset fail to find the VMA?

While remap_pfn_range() expects the PFN as an argument, it does not
require overwriting vma->vm_pgoff for non-COW shared mappings.

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