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: <3hzwtm7jw25ng5gemkp42k5ypkfky25fxeevccnk2d6gcpft32@qwkwofgauqna>
Date: Fri, 9 Aug 2024 12:48:33 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: 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

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

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

> >  		/* Does it split the end? */
> >  		if (next->vm_end > end) {
> >  			error = __split_vma(vmi, next, end, 0);
> > @@ -815,6 +826,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  	__mt_destroy(&mt_detach);
> >  	return 0;
> >  
> > +modify_vma_failed:
> >  clear_tree_failed:
> >  userfaultfd_error:
> >  munmap_gather_failed:
> > @@ -860,17 +872,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
> >  	if (end == start)
> >  		return -EINVAL;
> >  
> > -	/*
> > -	 * 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() might do unmaps itself.  */
> > -	arch_unmap(mm, start, end);
> > -
> >  	/* Find the first overlapping VMA */
> >  	vma = vma_find(vmi, end);
> >  	if (!vma) {
> > -- 
> > 2.46.0
> >