linux-kernel - Re: [PATCH v2 2/6] mm/munmap: Replace can_modify_mm with can_modify

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ljhdwgkfzjgdehfjgvdxa4wyxx4d32kpqqz2pwjse65rasuaeo@nncsuq3ku53o>
Date: Fri, 9 Aug 2024 15:24:51 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Pedro Falcato <pedro.falcato@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, oliver.sang@...el.com,
        torvalds@...ux-foundation.org, jeffxu@...gle.com,
        Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH v2 2/6] mm/munmap: Replace can_modify_mm with
 can_modify_vma

* Pedro Falcato <pedro.falcato@...il.com> [240809 14:53]:
> On Fri, Aug 9, 2024 at 5:48 PM Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
> >
> > * Liam R. Howlett <Liam.Howlett@...cle.com> [240809 12:15]:
> > > * Pedro Falcato <pedro.falcato@...il.com> [240807 17:13]:
> > > > We were doing an extra mmap tree traversal just to check if the entire
> > > > range is modifiable. This can be done when we iterate through the VMAs
> > > > instead.
> > > >
> > > > Signed-off-by: Pedro Falcato <pedro.falcato@...il.com>
> > > > ---
> > > >  mm/mmap.c | 13 +------------
> > > >  mm/vma.c  | 23 ++++++++++++-----------
> > > >  2 files changed, 13 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > index 4a9c2329b09..c1c7a7d00f5 100644
> > > > --- a/mm/mmap.c
> > > > +++ b/mm/mmap.c
> > > > @@ -1740,18 +1740,7 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > > >             unsigned long start, unsigned long end, struct list_head *uf,
> > > >             bool unlock)
> > > >  {
> > > > -   struct mm_struct *mm = vma->vm_mm;
> > > > -
> > > > -   /*
> > > > -    * Check if memory is sealed before arch_unmap.
> > > > -    * Prevent unmapping a sealed VMA.
> > > > -    * can_modify_mm assumes we have acquired the lock on MM.
> > > > -    */
> > > > -   if (unlikely(!can_modify_mm(mm, start, end)))
> > > > -           return -EPERM;
> > > > -
> > > > -   arch_unmap(mm, start, end);
> > > > -   return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
> > > > +   return do_vmi_align_munmap(vmi, vma, vma->vm_mm, start, end, uf, unlock);
> > > >  }
> > > >
> > > >  /*
> > > > diff --git a/mm/vma.c b/mm/vma.c
> > > > index bf0546fe6ea..7a121bcc907 100644
> > > > --- a/mm/vma.c
> > > > +++ b/mm/vma.c
> > > > @@ -712,6 +712,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > > >             if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
> > > >                     goto map_count_exceeded;
> > > >
> > > > +           /* Don't bother splitting the VMA if we can't unmap it anyway */
> > > > +           if (!can_modify_vma(vma)) {
> > > > +                   error = -EPERM;
> > > > +                   goto start_split_failed;
> > > > +           }
> > > > +
> > >
> > > Would this check be better placed in __split_vma()?  It could replace
> > > both this and the next chunk of code.
> >
> > not quite.
> 
> Yeah, I was going to say that splitting a sealed VMA is okay (and we
> allow it on mlock and madvise).

splitting a sealed vma wasn't supposed to be okay.. but it is Jeff's
feature.  Jeff?

> 
> >
> > >
> > > >             error = __split_vma(vmi, vma, start, 1);
> > > >             if (error)
> > > >                     goto start_split_failed;
> > > > @@ -723,6 +729,11 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > > >      */
> > > >     next = vma;
> > > >     do {
> > > > +           if (!can_modify_vma(vma)) {
> > > > +                   error = -EPERM;
> > > > +                   goto modify_vma_failed;
> > > > +           }
> > > > +
> >
> > This chunk would need to be moved below the end check so that we catch
> > full vma unmaps.
> 
> Why below the end check? I believe we can avoid the split? Is there
> something I'm missing?
> But I did find a bug, what I really seem to want is:
> 
>  +           if (!can_modify_vma(next)) {

Good catch.

> instead of (vma). It's somewhat concerning how the mseal selftests
> didn't trip on this?

the end check will call split and will fail in there, if you move the
code as I suggested.

That means, if we aren't splitting, we still have to check the vma, so
the check is necessary.