| 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: <77f9aac4-da7e-4a86-8e7f-685f4ceec270@lucifer.local>
Date: Thu, 22 May 2025 16:18:41 +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
On Thu, May 22, 2025 at 11:00:30PM +0800, Pu Lehui wrote:
>
> 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.
I can handle the backports as I'm best placed to figure this out due to being
the one to radically change the mremap code :P I'd rather do things optimally
for each revision, and just take the back port pain.
>
> >
> > 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.
>
> 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.
Yeah see above, we'll just have to bite the bullet and take some pain on this.
>
> >
> > >
> > > 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
> > >
>
Powered by blists - more mailing lists