Re: [RFC v1 02/19] uprobes: Correctly reject anonymous VMAs for breakpoint installation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Page tables are modified under mmap_write_lock without calling vma_start_write(vma), risking a race with concurrent per-VMA locked operations.
- [High] Several functions access vma->vm_file or use it for classification without first checking !vma_is_anonymous(vma), leaving them vulnerable to anonymous VMAs that have a vm_file set.
--

commit e85d99904cf4af95b805f715ec281558955b0733
Author: Andi Kleen <ak@kernel.org>

uprobes: Correctly reject anonymous VMAs for breakpoint installation

This patch updates valid_vma() to correctly check for anonymous VMAs 
using vma_is_anonymous() instead of relying only on vm_file.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7709ea8824778..4287c4ff4c0fd 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -139,7 +139,8 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
>  	if (is_register)
>  		flags |= VM_WRITE;
>  
> -	return vma->vm_file && (vma->vm_flags & flags) == VM_MAYEXEC;
> +	return !vma_is_anonymous(vma) && vma->vm_file &&
> +		(vma->vm_flags & flags) == VM_MAYEXEC;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
anonymous VMA check here in valid_vma(), do we need to apply this same
!vma_is_anonymous(vma) check to other functions in this file?

Several other functions, such as valid_ref_ctr_vma(), 
find_active_uprobe_speculative(), find_active_uprobe_rcu(), and 
uprobe_mmap(), appear to rely directly on vma->vm_file to determine if 
a VMA is file-backed. 

For example, valid_ref_ctr_vma() checks:

    return uprobe->ref_ctr_offset &&
           vma->vm_file &&
           file_inode(vma->vm_file) == uprobe->inode;

If an anonymous VMA happens to have a backing file (e.g., from a private 
mapping of /dev/zero or a driver mapping using vma_set_anonymous()), could 
this incorrectly match the inode and update reference counters in anonymous
memory, or compute incorrect file offsets in the speculative lookups?

[Severity: High]
This is a pre-existing issue, but while reviewing the VMA handling in
uprobes, does this file also need to call vma_start_write(vma) when
modifying page tables?

Paths like register_for_each_vma() and unapply_uprobe() take
mmap_write_lock() and eventually call uprobe_write() to modify page
tables via folio_walk_start() in __uprobe_write(). 

Because mmap_write_lock() does not drain per-VMA read locks, a concurrent 
page table operation (like MADV_DONTNEED) under a per-VMA lock could 
clear a PMD and free the PTE page while uprobe_write() is walking it.

Could this race lead to a use-after-free or page table corruption when
tracers manipulate the memory layout concurrently without the protection of 
vma_start_write() draining concurrent VMA-lock readers?

>  
>  static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150651.1134594-1-ak@kernel.org?part=2