linux-kernel - Re: [PATCH 4/7] mm/mremap: 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: <CALmYWFuufH+mCOng=G+EROo7R-GNBH2w11fva-3qfYh1JCR0=w@mail.gmail.com>
Date: Tue, 6 Aug 2024 18:47:31 -0700
From: Jeff Xu <jeffxu@...gle.com>
To: Pedro Falcato <pedro.falcato@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	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, Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH 4/7] mm/mremap: Replace can_modify_mm with can_modify_vma

On Tue, Aug 6, 2024 at 5:59 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
>
> On Wed, Aug 7, 2024 at 12:09 AM Jeff Xu <jeffxu@...gle.com> wrote:
> >
> > On Tue, Aug 6, 2024 at 2:28 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
> > >
> > > Delegate all can_modify checks to the proper places. Unmap checks are
> > > done in do_unmap (et al).
> > >
> > > This patch allows for mremap partial failure in certain cases (for
> > > instance, when destination VMAs aren't sealed, but the source VMA is).
> > > It shouldn't be too troublesome, as you'd need to go out of your way to
> > > do illegal operations on a VMA.
> > >
> > > Signed-off-by: Pedro Falcato <pedro.falcato@...il.com>
> > > ---
> > >  mm/mremap.c | 33 +++++++--------------------------
> > >  1 file changed, 7 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index e7ae140fc64..8af877d7bb0 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -676,6 +676,9 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> > >         if (unlikely(flags & MREMAP_DONTUNMAP))
> > >                 to_account = new_len;
> > >
> > > +       if (!can_modify_vma(vma))
> > > +               return -EPERM;
> > > +
> > I m not 100% sure, but I suspect you don't need this check? Is
> > vma_to_resize already checking the src address ?
>
> Hmm, yes, good point.
>
> >
> > PS. Is it possible to consolidate all the related changes (except the
> > fix for madvise) to a single commit ?
>
> The patch set is organized logically, in simple clear steps. As a
> maintainer, I would prefer to review something like this vs a big
> confusing patch that touches many things at once.
> Of course if the maintainers think this is too coarse (it's not
> exactly a large patch set, just moves a lot of code back and forth),
> I'm happy to merge these into larger chunks.
>
Yes. The patch is not exactly large. (The original mseal patch  has
one single patch for adding mseal code logic, and one for test).

Also the mseal has been backported to  the 6.6 kernel for ChromeOS,
if you keep the code change in one , it will make future backport
easier for me.

> >  It would be easier to look for dependency, e.g. the remap depends on munmap().
>
> All patches depend on the previous (and a single patch would make it
> harder to see these dependencies). The kernel should build and the
> selftests should pass for every patch in the set.
>
> >
> > Also selftest is helpful to prove the correctness of the change. (And
> > I can also test it)
>
> I have run selftests, and it is.
>
> --
> Pedro