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

[Eric Auger <eric.auger@redhat.com>]

Hi,

On 9/3/26 11:48 AM, sashiko-bot@kernel.org wrote:
> 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)?
every MMIO region in vfio-platform is marked as
VFIO_REGION_INFO_FLAG_READ so write-only regions do not exist. I don't
think there is such symetrical need here.
>
> 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?
Isn't it pretty unlikely that the region gets mmapped in several
adjacent chunks? At least for devices officially/publicly integrated, I
don't think this happened.
>
>>  
>>  	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.
the same pattern also exists in cdx/main.c, fsl-mc/vfio_fsl_mc.c

Thanks

Eric
>