Re: [PATCH RFC 09/11] misc/sgi-gru: use ptep_get() for page-table reads

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Lockless page table walk in `atomic_pte_lookup` reads page table entries multiple times, causing TOCTOU vulnerabilities (UAF, Wild Read, Privilege Escalation).
- [Critical] atomic_pte_lookup maps the base of an anonymous THP instead of the requested offset, causing silent data corruption.
--

--- Patch [9]: [PATCH RFC 09/11] misc/sgi-gru: use ptep_get() for page-table reads ---
commit 24fd6151a0088920cebe4cf33a4840e02a2f4e62
Author: Muhammad Usama Anjum <usama.anjum@arm.com>

misc/sgi-gru: use ptep_get() for page-table reads

A leaf PMD is being read through ptep_get() by treating the PMD address
as PTE-sized table storage. ptep_get() now accepts hw_pte_t *, so update
the cast accordingly.

> diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c
> index 3557d78ee47a2..ff89d34ad2aa4 100644
> --- a/drivers/misc/sgi-gru/grufault.c
> +++ b/drivers/misc/sgi-gru/grufault.c
> @@ -228,10 +228,10 @@ static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr,
>  		goto err;
>  #ifdef CONFIG_X86_64
>  	if (unlikely(pmd_leaf(*pmdp)))
> -		pte = ptep_get((pte_t *)pmdp);
> +		pte = ptep_get((hw_pte_t *)pmdp);
>  	else
>  #endif
> -		pte = *pte_offset_kernel(pmdp, vaddr);
> +		pte = ptep_get(pte_offset_kernel(pmdp, vaddr));

[Severity: Critical]
This is a pre-existing issue, but does this lockless page table walk
have time-of-check to time-of-use vulnerabilities?

When atomic_pte_lookup() evaluates pmd_leaf(*pmdp) and then rereads *pmdp
via ptep_get(), if a THP split occurs concurrently (which only takes the
PMD lock and can run concurrently with mmap_read_lock()), the second read
could see a PMD pointing to a PTE table. The function would then treat this
non-leaf PMD as a leaf PTE, extracting the PFN of the PTE table itself.
Could this cause the GRU hardware TLB to be programmed to grant user-space
read/write access to its own kernel page tables?

Similarly, if a concurrent MADV_DONTNEED (which runs under mmap_read_lock())
clears the PMD between pmd_none() and pte_offset_kernel(), could
pte_offset_kernel() compute a direct-map address of PFN 0 (PAGE_OFFSET),
resulting in a wild read of physical memory to use as a PTE?

[Severity: Critical]
This is another pre-existing issue, but does atomic_pte_lookup() map the
base of an anonymous THP instead of the requested offset, potentially causing
silent data corruption?

Further down in this function, the code does:

	*paddr = pte_pfn(pte) << PAGE_SHIFT;
#ifdef CONFIG_HUGETLB_PAGE
	*pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT;
#else
	*pageshift = PAGE_SHIFT;
#endif

When processing a TLB miss for a virtual address mapped by an anonymous THP,
pmd_leaf(*pmdp) is true. The function extracts the base PFN of the 2MB page
from the PMD. However, because is_vm_hugetlb_page(vma) evaluates to false
for anonymous THPs (it only checks for hugetlbfs VMAs), *pageshift is set
to PAGE_SHIFT (4KB). 

Because the base PFN of the THP is extracted without adding the offset for
vaddr, and a 4KB page size is used, does this cause reads and writes to any
4KB chunk within an anonymous THP to incorrectly access the first 4KB chunk
of that THP?

>  
>  	if (unlikely(!pte_present(pte) ||
>  		     (write && (!pte_write(pte) || !pte_dirty(pte)))))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727164715.2866609-1-usama.anjum@arm.com?part=9