| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a55fd3c0-cd53-43d9-a200-290d64dcf04f@lucifer.local>
Date: Wed, 21 May 2025 14:13:34 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Pu Lehui <pulehui@...weicloud.com>
Cc: mhiramat@...nel.org, oleg@...hat.com, peterz@...radead.org,
akpm@...ux-foundation.org, Liam.Howlett@...cle.com, vbabka@...e.cz,
jannh@...gle.com, pfalcato@...e.de, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, pulehui@...wei.com
Subject: Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when
expanding vma during mremap
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!
On Wed, May 21, 2025 at 09:25:03AM +0000, Pu Lehui wrote:
> From: Pu Lehui <pulehui@...wei.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@...wei.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.
> {
> 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?
>
> 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.
>
> 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.
>
> 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
>
Powered by blists - more mailing lists