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: <CAKbZUD2VV=FOeGhCOb3o5CKBiaV+6JSPoDRwzV1-3t2hZX7rQw@mail.gmail.com>
Date: Fri, 9 Aug 2024 19:53:18 +0100
From: Pedro Falcato <pedro.falcato@...il.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, Pedro Falcato <pedro.falcato@...il.com>, 
	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

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).

>
> >
> > >             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)) {
instead of (vma). It's somewhat concerning how the mseal selftests
didn't trip on this?

-- 
Pedro