Re: [PATCH 3/4] mm/huge_memory: don't warn when a page table blocks a huge pfn insert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] mm/huge_memory: inaccurate Fixes trailer for insert_pmd() and
  insert_pud()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] mm: missing VMA bounds checks in insert_pmd() and
  insert_pud() [1]
--

commit d9bb1ebf27a3fa3fa5849404d8b2245f291d97f1
Author: Eliav Farber <farbere@amazon.com>

> mm/huge_memory: don't warn when a page table blocks a huge pfn insert
>
> insert_pmd() and insert_pud() treat a non-empty entry at their own level
> as a leaf and compare its PFN against the one being inserted. A page
> table installed by a concurrent order-0 fault also makes the entry
> non-empty, and pmd_pfn()/pud_pfn() then report the PFN of that table,
> which never matches. insert_pmd() warns unless the entry is the huge
> zero PMD, and insert_pud() warns on any mismatch.
>
> A VM_PFNMAP mapping with both ->fault and ->huge_fault can produce the
> mixed orders: an order-0 fault installs a page table over an address
> while a concurrent huge fault on the same address reaches insert_pmd().
> Changing an mm's THP policy while another thread faults the same PMD,
> for example with prctl(PR_SET_THP_DISABLE), is enough to do it.
>
> Compare the PFN only for a leaf entry. A page table is not an error, so
> take the existing out_unlock path, which returns VM_FAULT_NOPAGE and
> lets the access refault onto the smaller entry.
>
> Fixes: f9e54c3a2f5b ("vfio/pci: implement huge_fault support")
> Signed-off-by: Eliav Farber <farbere@amazon.com>

[Severity: Low]
In both insert_pmd() and insert_pud(), the PFN comparison and
WARN_ON_ONCE() only run when write is true.  f9e54c3a2f5b ("vfio/pci:
implement huge_fault support") always passes write = false to
vmf_insert_pfn_pmd() and vmf_insert_pfn_pud(), so it never enters the
if (write) block.  The unguarded PFN checks and warnings under if (write)
were added by c6f3c5ee40c1 ("mm/huge_memory.c: fix modifying of page
protection by insert_pfn_pmd()").  Should the Fixes tag reference that
commit instead?

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 1e5d68acf62a5..1ec124ea542a5 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1647,6 +1647,14 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
>  		const unsigned long pfn = fop.is_folio ? folio_pfn(fop.folio) :
>  					  fop.pfn;
>  
> +		/*
> +		 * A concurrent order-0 fault may have installed a PTE table
> +		 * here, which pmd_pfn() would report as the PFN of the table
> +		 * itself. Only a leaf entry can be compared.
> +		 */
> +		if (!pmd_leaf(*pmd))
> +			goto out_unlock;
> +
>  		if (write) {
>  			if (pmd_pfn(*pmd) != pfn) {
>  				WARN_ON_ONCE(!is_huge_zero_pmd(*pmd));

[Severity: Critical]
This problem wasn't introduced by this patch, but can insert_pmd() and
insert_pud() map physical memory past vma->vm_end [1]?

Both functions only check whether the starting address falls within the
VMA:

mm/huge_memory.c:insert_pmd() {
	...
	if (addr < vma->vm_start || addr >= vma->vm_end)
		return VM_FAULT_SIGBUS;
	...
}

Neither insert_pmd() nor insert_pud() verifies that addr + PMD_SIZE or
addr + PUD_SIZE fits within vma->vm_end.  If userspace creates a VMA
whose end is not aligned to a huge page boundary, faulting in a PMD or
PUD maps physical memory beyond the end of the VMA, allowing userspace to
access out-of-bounds memory or corrupt adjacent mappings.

[1] https://sashiko.dev/#/bug/linux-f9c89537-a4ba-4e65-a29a-5c77e15819b6

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111140655.34281-1-farbere@amazon.com?part=3