Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when expanding vma during mremap

[Pu Lehui <pulehui@huaweicloud.com>]

On 2025/5/21 21:13, Lorenzo Stoakes wrote:
> Overall need to dig into this a bit before we come up with a final fix.
> 
> Has this _always_ been the case? Or could you bisect this to a particular kernel
> commit?
> 
> If you haven't dug into that, could you do so?

Thanks for your review. The directly related commit is 78a320542e6c 
("uprobes: Change valid_vma() to demand VM_MAYEXEC rather than VM_EXEC") 
# v3.7-rc3, but in fact, I think the issue was introduced in the 
original commit 2b1444983508 ("uprobes, mm, x86: Add the ability to 
install and remove uprobes breakpoints") # v3.5-rc1. This issue is quite 
old, but the mremap code differs a lot between linux versions. Perhaps 
we need to fix it in multiple versions separately.

> 
> Thanks!
> 
> On Wed, May 21, 2025 at 09:25:03AM +0000, Pu Lehui wrote:
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> We encountered a BUG alert triggered by Syzkaller as follows:
>>     BUG: Bad rss-counter state mm:00000000b4a60fca type:MM_ANONPAGES val:1
>>
>> And we can reproduce it with the following steps:
>> 1. register uprobe on file at zero offset
>> 2. mmap the file at zero offset:
>>     addr1 = mmap(NULL, 2 * 4096, PROT_NONE, MAP_PRIVATE, fd, 0);
>> 3. mremap part of vma1 to new vma2:
>>     addr2 = mremap(addr1, 4096, 2 * 4096, MREMAP_MAYMOVE);
>> 4. mremap back to orig addr1:
>>     mremap(addr2, 4096, 4096, MREMAP_MAYMOVE | MREMAP_FIXED, addr1);
>>
>> In the step 3, the vma1 range [addr1, addr1 + 4096] will be remap to new
>> vma2 with range [addr2, addr2 + 8192], and remap uprobe anon page from
>> the vma1 to vma2, then unmap the vma1 range [addr1, addr1 + 4096].
>> In tht step 4, the vma2 range [addr2, addr2 + 4096] will be remap back
>> to the addr range [addr1, addr1 + 4096]. Since the addr range [addr1 +
>> 4096, addr1 + 8192] still maps the file, it will take
>> vma_merge_new_range to merge these two addr ranges, and then do
>> uprobe_mmap in vma_complete. Since the merged vma pgoff is also zero
>> offset, it will install uprobe anon page to the merged vma. However, the
>> upcomming move_page_tables step, which use set_pte_at to remap the vma2
>> uprobe anon page to the merged vma, will over map the old uprobe anon
>> page in the merged vma, and lead the old uprobe anon page to be orphan.
>>
>> Since the uprobe anon page will be remapped to the merged vma, we can
>> remove the unnecessary uprobe_mmap at merged vma, that is, do not
>> perform uprobe_mmap when there is no vma in the addr range to be
>> expaned.
> 
> Good spot... lord.
> 
>>
>> This problem was first find in linux-6.6.y and also exists in the
>> community syzkaller:
>> https://lore.kernel.org/all/000000000000ada39605a5e71711@google.com/T/
>>
>> The complete Syzkaller C reproduction program is as follows:
>>
>> #define _GNU_SOURCE
>> #include <sys/mman.h>
>> #include <linux/perf_event.h>
>> #include <linux/hw_breakpoint.h>
>>
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdint.h>
>> #include <string.h>
>> #include <syscall.h>
>> #include <unistd.h>
>>
>> int main(int argc, char *argv[])
>> {
>>      // Find out what type id we need for uprobes
>>      int perf_type_pmu_uprobe;
>>      {
>>          FILE *fp = fopen("/sys/bus/event_source/devices/uprobe/type", "r");
>>          fscanf(fp, "%d", &perf_type_pmu_uprobe);
>>          fclose(fp);
>>      }
>>
>>      const char *filename = "./bus";
>>
>>      int fd = open(filename, O_RDWR|O_CREAT, 0600);
>>      write(fd, "x", 1);
>>
>>      void *addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE | PROT_EXEC,
>>                        MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>
>>      // Register a perf uprobe on "./bus"
>>      struct perf_event_attr attr = {};
>>      attr.type = perf_type_pmu_uprobe;
>>      attr.uprobe_path = (unsigned long) filename;
>>      syscall(__NR_perf_event_open, &attr, 0, 0, -1, 0);
>>
>>      void *addr2 = mmap(NULL, 2 * 4096, PROT_NONE, MAP_PRIVATE, fd, 0);
>>      void *addr3 = mremap((void *) addr2, 4096, 2 * 4096, MREMAP_MAYMOVE);
>>      mremap(addr3, 4096, 4096, MREMAP_MAYMOVE | MREMAP_FIXED, (void *) addr2);
>>
>>      return 0;
>> }
> 
> Thanks for including this.
> 
> We can probably refine this down a bit though (let me have a tinker around).
> 
>>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>> ---
>>   mm/vma.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/vma.c b/mm/vma.c
>> index 3ff6cfbe3338..9a8d84b12918 100644
>> --- a/mm/vma.c
>> +++ b/mm/vma.c
>> @@ -325,7 +325,7 @@ static void vma_prepare(struct vma_prepare *vp)
>>    * @mm: The mm_struct
>>    */
>>   static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
>> -			 struct mm_struct *mm)
>> +			 struct mm_struct *mm, bool handle_vma_uprobe)
> 
> Nity, but please do not add a field here, add it to the vma_prepare struct.

ok

> 
>>   {
>>   	if (vp->file) {
>>   		if (vp->adj_next)
>> @@ -358,7 +358,8 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
>>
>>   	if (vp->file) {
>>   		i_mmap_unlock_write(vp->mapping);
>> -		uprobe_mmap(vp->vma);
>> +		if (handle_vma_uprobe)
>> +			uprobe_mmap(vp->vma);
> 
> You could perhaps make this simpler by making uprobe_mmap() take a vma_prepare *
> parameter, where you can check this?

got it

> 
>>
>>   		if (vp->adj_next)
>>   			uprobe_mmap(vp->adj_next);
>> @@ -549,7 +550,7 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
>>   	}
>>
>>   	/* vma_complete stores the new vma */
>> -	vma_complete(&vp, vmi, vma->vm_mm);
>> +	vma_complete(&vp, vmi, vma->vm_mm, true);
>>   	validate_mm(vma->vm_mm);
>>
>>   	/* Success. */
>> @@ -715,6 +716,7 @@ static int commit_merge(struct vma_merge_struct *vmg)
>>   {
>>   	struct vm_area_struct *vma;
>>   	struct vma_prepare vp;
>> +	bool handle_vma_uprobe = !!vma_lookup(vmg->mm, vmg->start);
> 
> This is really inefficient. We can't be doing a maple tree search on every
> commit_merge(), especially not just for the sake of uprobe.
> 
> Let me have a tinker around with this and see how we can do this more sensibly,
> I'd say as part of the vmg.

so, we can add `bool handle_vma_uprobe` member in vmg, then set true 
before enter vma_merge_new_range in copy_vma, and then pass it to vp 
struct. wdyt?

But this may not suit for other version, such as linux-6.6.y.

> 
>>
>>   	if (vmg->__adjust_next_start) {
>>   		/* We manipulate middle and adjust next, which is the target. */
>> @@ -748,7 +750,7 @@ static int commit_merge(struct vma_merge_struct *vmg)
>>   	vmg_adjust_set_range(vmg);
>>   	vma_iter_store_overwrite(vmg->vmi, vmg->target);
>>
>> -	vma_complete(&vp, vmg->vmi, vma->vm_mm);
>> +	vma_complete(&vp, vmg->vmi, vma->vm_mm, handle_vma_uprobe);
> 
> Again, having fields means we can drop this and the below changes.

ok

> 
>>
>>   	return 0;
>>   }
>> @@ -1201,7 +1203,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
>>
>>   	vma_iter_clear(vmi);
>>   	vma_set_range(vma, start, end, pgoff);
>> -	vma_complete(&vp, vmi, vma->vm_mm);
>> +	vma_complete(&vp, vmi, vma->vm_mm, true);
>>   	validate_mm(vma->vm_mm);
>>   	return 0;
>>   }
>> --
>> 2.34.1
>>