Re: [PATCH v2 1/1] mm/memory: constrain generic_access_phys() to page boundary

["David Hildenbrand (Arm)" <david@kernel.org>]

On 9/5/26 16:12, Ren Wei wrote:
> From: Luxiao Xu <rakukuip@gmail.com>
> 
> generic_access_phys() improperly validates the memory access range: it
> only validates the start address using follow_pfnmap_start() and passes
> PAGE_ALIGN(len + offset) to ioremap_prot().
> 
> This poses two problems:
> 1. In PFNMAP VMAs, consecutive virtual pages are not guaranteed to be
>    physically contiguous, and individual PTEs may have different access
>    permissions or writability.
> 2. The mapping may cross VMA boundaries if len extends beyond vma->vm_end.
> 
> Since __access_remote_vm() already loops over the requested length and
> accesses normal (struct page) memory page-by-page, constrain the ->access()
> callback in __access_remote_vm() to at most the current page boundary.
> Because VMA boundaries are always page-aligned, this also ensures the
> access never exceeds the current VMA.
> 
> In generic_access_phys(), defensively clamp len to the page boundary as
> well, map only a single PAGE_SIZE via ioremap_prot(), and add a missing
> (resource_size_t) cast during PFN re-validation to avoid truncation on
> 32-bit PAE systems.
> 
> Fixes: 9cb12d7b4cca ("mm/memory.c: actually remap enough memory")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: LLM
> Suggested-by: David Hildenbrand <david@kernel.org>
> Signed-off-by: Luxiao Xu <rakukuip@gmail.com>
> Signed-off-by: Ren Wei <weir@nebusec.ai>
> ---
> v1 -> v2:
> - Drop internal multi-page loop in generic_access_phys(); clamp the chunk
>   size in __access_remote_vm() to PAGE_SIZE - offset instead, leveraging
>   the existing caller loop (David Hildenbrand).
> - Defensively clamp len in generic_access_phys() and only map PAGE_SIZE.
> - Add missing (resource_size_t) cast when checking args.pfn (Andrew Morton).
> Link: https://lore.kernel.org/r/f618e2f57ad0086a8d7a95cb17dcf5a1f0cf0e45.1784428532.git.rakukuip@gmail.com
> ---
>  mm/memory.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 8b0c2c735d3d..09390e51432f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -7164,6 +7164,11 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>  	bool writable;
>  	struct follow_pfnmap_args args = { .vma = vma, .address = addr };
>  
> +	if (len <= 0)
> +		return 0;

Why is that required? Why would it be valid to call this function with a size of 0?

> +

We should add a comment here explaining the situation:

/*
 * Limit access to one page at a time, as that's what follow_pfnmap_start()
 * guarantees; expect the caller to retry to read larger ranges.
 */

> +	len = min_t(int, len, PAGE_SIZE - offset);

Do we really need the "min_t" here? (can't we use min?)

> +
>  retry:
>  	if (follow_pfnmap_start(&args))
>  		return -EINVAL;
> @@ -7175,7 +7180,7 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>  	if ((write & FOLL_WRITE) && !writable)
>  		return -EINVAL;
>  
> -	maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot);
> +	maddr = ioremap_prot(phys_addr, PAGE_SIZE, prot);
>  	if (!maddr)
>  		return -ENOMEM;
>  
> @@ -7183,7 +7188,7 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>  		goto out_unmap;
>  
>  	if ((pgprot_val(prot) != pgprot_val(args.pgprot)) ||
> -	    (phys_addr != (args.pfn << PAGE_SHIFT)) ||
> +	    (phys_addr != ((resource_size_t)args.pfn << PAGE_SHIFT)) ||
>  	    (writable != args.writable)) {
>  		follow_pfnmap_end(&args);
>  		iounmap(maddr);
> @@ -7254,7 +7259,8 @@ static int __access_remote_vm(struct mm_struct *mm, unsigned long addr,
>  #ifdef CONFIG_HAVE_IOREMAP_PROT
>  			if (vma->vm_ops && vma->vm_ops->access)
>  				bytes = vma->vm_ops->access(vma, addr, buf,
> -							    len, write);
> +							    min_t(int, len, PAGE_SIZE - offset_in_page(addr)),

Why isn't it sufficient to cap in generic_access_phys() ?


-- 
Cheers,

David