Message-ID: <CABi2SkXO_O0E5EfB0VaoFuyJ4WG-n-P62wBC801Jj1ki4Tk1Mg@mail.gmail.com>
Date: Thu, 13 Mar 2025 15:50:25 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Kees Cook <kees@...nel.org>, akpm@...ux-foundation.org, vbabka@...e.cz, 
	Liam.Howlett@...cle.com, broonie@...nel.org, skhan@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org, jorgelo@...omium.org, 
	pedro.falcato@...il.com, rdunlap@...radead.org, jannh@...gle.com
Subject: Re: [RFC PATCH v1 2/2] mseal: allow noop mprotect

On Wed, Mar 12, 2025 at 10:29 PM Lorenzo Stoakes
<lorenzo.stoakes@...cle.com> wrote:
>
> On Wed, Mar 12, 2025 at 04:29:50PM -0700, Jeff Xu wrote:
> > On Wed, Mar 12, 2025 at 9:45 AM Kees Cook <kees@...nel.org> wrote:
> > >
> > > On Wed, Mar 12, 2025 at 03:50:40PM +0000, Lorenzo Stoakes wrote:
> > > > What about madvise() with MADV_DONTNEED on a r/o VMA that's not faulted in?
> > > > That's a no-op right? But it's not permitted.
> > >
> > Madvise's semantics are about behavior, while mprotect is about
> > attributes. To me:  madvise is like "make this VMA do that" while
> > mprotect is about "update this VMA's attributes to a new value".
> >
> > It is more difficult to determine if a behavior is no-op, so I don't
> > intend to apply the same no-op concept to madvise().
> >
> > > Hmm, yes, that's a good example. Thank you!
> > >
> > > > So now we have an inconsistency between the two calls.
> > >
> > > Yeah, I see your concern now.
> > >
> > > > I don't know what you mean by 'ergonomic'?
> > >
> > > I was thinking about idempotent-ness. Like, some library setting up a
> > > memory region, it can't call its setup routine twice if the second time
> > > through (where no changes are made) it gets rejected. But I think this
> > > is likely just a userspace problem: check for the VMAs before blindly
> > > trying to do it again. (This is strictly an imagined situation.)
> > >
> > Yes.
> >
> >  We also don't have a system call to query the "mprotect" attributes,
> > so it is understandable that userspace can rely on idempotents of the
> > mprotect.
>
> PROCMAP_QUERY ioctl, /proc/$pid/pagemap :) I mean hey - these are somewhat
> diagnostic-y, racey, un-fun interfaces that we'd rather you not use in
> anger when mapping stuff - but they do at least exist :)
>
> (an aside, been playing with PROCMAP_QUERY recently and very cool - we plan
> to make this useable under RCU lock rather than mmap lock which will make
> it _even more_ useful in future... exciting times :)
>
/proc/pid/maps only has a subset information of vm_flags, e.g. pkeys
is not part of it, however pkey_mprotect can update pkey. So the
suggestion of checking for the VMAs before calling mprotect won't work
for all cases. Besides,  the checking then updating pattern also has
the perf impact due to an extra syscall.

> > > trying to do it again. (

> It's possible, but it seems that it would be relying upon it purely because
> in some cases it would be modifying the mapping, right?
>
> It strikes me as very unlikely that an application would be looking to
> modify the attributes of a series of VMAs including ones that have a
> security feature enabled which says 'until this is unmapped do not modify
> the attributes of this VMA'.
>
> Yes it's _theoretically_ possible but that'd be quite silly no?
>
> >
> > > > My reply seemed to get truncated at the end here :) So let me ask again -
> > > > do you have a practical case in mind for this?
> > >
> > I noticed there were idempotent mprotects last year while working on
> > applying mseal on stack in Android. I assume this might not be the
> > only instance since mprotect gets called a lot in general.
> >
> > Blocking this won't improve security, it could actually hinder the
> > adoption of mseal, i.e. force apps to make code change.
>
> Thanks for the explanation it's appreciated!
>
> But I feel the drawbacks I mentioned previously and elucidated upon in my
> reply to Kees outweigh this theoretical concern.
>
> If we encounter actual real-world instances of this we can reconsider,
> presuming we are ok with the asymmetry vs. other seal-protected calls. We
> have this shipped with a uAPI already like this so there's no rush.
>
Sure. But I honestly think that you are overthinking on this. The
security benefit of mseal for pkey_mprotect is that an attacker can't
modify the VMA's attributes,  and this patch does not compromise on
that.

Best regards,
-Jeff


> >
> > -Jeff
> >
> > > Sorry, I didn't have any reply to that part, so I left it off. If Jeff
> > > has a specific case in mind, I'll let him answer that part. :)
> > >
> > > -Kees
> > >
> > > --
> > > Kees Cook
>
> Cheers, Lorenzo

Powered by blists - more mailing lists