Re: [PATCH v7 0/4] Introduce mseal()

[Theo de Raadt]

Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:

> On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@xxxxxxxxxxx> wrote:
> >
> > Regarding these pieces
> >
> > > The PROT_SEAL bit in prot field of mmap(). When present, it marks
> > > the map sealed since creation.
> >
> > OpenBSD won't be doing this.  I had PROT_IMMUTABLE as a draft.  In my
> > research I found basically zero circumstances when you userland does
> > that.  The most common circumstance is you create a RW mapping, fill it,
> > and then change to a more restrictve mapping, and lock it.
> >
> > There are a few regions in the addressspace that can be locked while RW.
> > For instance, the stack.  But the kernel does that, not userland.  I
> > found regions where the kernel wants to do this to the address space,
> > but there is no need to export useless functionality to userland.
> >
> I have a feeling that most apps that need to use mmap() in their code
> are likely using RW mappings. Adding sealing to mmap() could stop
> those mappings from being executable. Of course, those apps would
> need to change their code. We can't do it for them.

I don't have a feeling about it.

I spent a year engineering a complete system which exercises the maximum
amount of memory you can lock.

I saw nothing like what you are describing.  I had PROT_IMMUTABLE in my
drafts, and saw it turning into a dangerous anti-pattern.

> Also, I believe adding this to mmap() has no downsides, only
> performance gain, as Pedro Falcato pointed out in [1].
> 
> [1] https://lore.kernel.org/lkml/CAKbZUD2A+=bp_sd+Q0Yif7NJqMu8p__eb4yguq0agEcmLH8SDQ@xxxxxxxxxxxxxx/

Are you joking?  You don't have any code doing that today.  More feelings?

OpenBSD userland has zero places it can use mmap() MAP_IMMUTABLE.

It has two places where it has mprotect() + mimmutable() adjacent to each
other, two codepaths for late mprotect() of RELRO, and then make the RELRO
immutable.

I think this idea is a premature optimization, and intentionally incompatible.

Like I say, I had a similar MAP_ flag for mprotect() and mmap() in my
development trees, and I recognized it was pointless, distracting developers
into the wrong patterns, and I threw it out.

> > OpenBSD now uses this for a high percent of the address space.  It might
> > be worth re-reading a description of the split of responsibility regarding
> > who locks different types of memory in a process;
> > - kernel (the majority, based upon what ELF layout tell us),
> > - shared library linker (the next majority, dealing with shared
> >   library mappings and left-overs not determinable at kernel time),
> > - libc (a small minority, mostly regarding forced mutable objects)
> > - and the applications themselves (only 1 application today)
> >
> >     https://lwn.net/Articles/915662/
> >
> > > The MAP_SEALABLE bit in the flags field of mmap(). When present, it marks
> > > the map as sealable. A map created without MAP_SEALABLE will not support
> > > sealing, i.e. mseal() will fail.
> >
> > We definately won't be doing this.  We allow a process to lock any and all
> > it's memory that isn't locked already, even if it means it is shooting
> > itself in the foot.
> >
> > I think you are going to severely hurt the power of this mechanism,
> > because you won't be able to lock memory that has been allocated by a
> > different callsite not under your source-code control which lacks the
> > MAP_SEALABLE flag.  (Which is extremely common with the system-parts of
> > a process, meaning not just libc but kernel allocated objects).
> >
> MAP_SEALABLE was an open discussion item called out on V3 [2] and V4 [3].
> 
> I acknowledge that additional coordination would be required if
> mapping were to be allocated by one software component and sealed in
> another. However, this is feasible.
> 
> Considering the side effect of not having this flag (as discussed in
> V3/V4) and the significant implications of altering the lifetime of
> the mapping (since unmapping would not be possible), I believe it is
> reasonable to expect developers to exercise additional care and
> caution when utilizing memory sealing.
>
> [2] https://lore.kernel.org/linux-mm/20231212231706.2680890-2-jeffxu@xxxxxxxxxxxx/
> [3] https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@xxxxxxxxxxxx/

I disagree *strongly*.  Developers need to exercise additional care on
memory, period.  Memory sealing issues is the least of their worries.

(Except for handling RELRO, but only the ld.so developers will lose
their hair).


OK, so mseal and mimmutable are very different.

mimmutable can be used by any developer on the address space easily.

mseal requires control of the whole stack between allocation and consumption.

I'm sorry, but I don't think you understand how dangerous this MAP_SEALABLE
proposal is because of the difficulties it will create for use.

The immutable memory management we have today in OpenBSD would completely
impossible with such a flag.  Seperation between allocator (that doesn't know
what is going to happen), and consumer (that does know), is completely common
in the systems environment (meaning the interaction between DSO, libc, other
libraries, and the underside of applications).

This is not not like an application where you can simply sprinkle the flag
into the mmap() calls that cause you problems.  That mmap() call is now in
someone else's code, and you CANNOT gain security advantage unless you
convince them to gain an understanding of what that flag means -- and it is
a flag that other Linux variants don't have, not even in their #include
files.

> > It may be fine inside a program like chrome, but I expect that flag to make
> > it harder to use in libc, and it will hinder adoption.
> >
> In the case of glibc and linux, as stated in the cover letter, Stephen
> is working on a change to glibc to add sealing support to the dynamic
> linker,  also I plan to make necessary code changes in the linux kernel.

How will ld.so seal memory which the kernel mapped?  The kernel will now
automatically puts MAP_SEALABLE on the text segment and stack?  Why not
put it on all mmap() allocations?  Why not just skip the flag entirely?

To me, this is all very bizzare.

I don't understand what the MAP_SEALABLE flag is trying to solve.