mbox series

[v7,0/4] Introduce mseal()

Message ID 20240122152905.2220849-1-jeffxu@chromium.org (mailing list archive)
Headers show
Series Introduce mseal() | expand

Message

Jeff Xu Jan. 22, 2024, 3:28 p.m. UTC
From: Jeff Xu <jeffxu@chromium.org>

This patchset proposes a new mseal() syscall for the Linux kernel.

In a nutshell, mseal() protects the VMAs of a given virtual memory
range against modifications, such as changes to their permission bits.

Modern CPUs support memory permissions, such as the read/write (RW)
and no-execute (NX) bits. Linux has supported NX since the release of
kernel version 2.6.8 in August 2004 [1]. The memory permission feature
improves the security stance on memory corruption bugs, as an attacker
cannot simply write to arbitrary memory and point the code to it. The
memory must be marked with the X bit, or else an exception will occur.
Internally, the kernel maintains the memory permissions in a data
structure called VMA (vm_area_struct). mseal() additionally protects
the VMA itself against modifications of the selected seal type.

Memory sealing is useful to mitigate memory corruption issues where a
corrupted pointer is passed to a memory management system. For
example, such an attacker primitive can break control-flow integrity
guarantees since read-only memory that is supposed to be trusted can
become writable or .text pages can get remapped. Memory sealing can
automatically be applied by the runtime loader to seal .text and
.rodata pages and applications can additionally seal security critical
data at runtime. A similar feature already exists in the XNU kernel
with the VM_FLAGS_PERMANENT [3] flag and on OpenBSD with the
mimmutable syscall [4]. Also, Chrome wants to adopt this feature for
their CFI work [2] and this patchset has been designed to be
compatible with the Chrome use case.

Two system calls are involved in sealing the map:  mmap() and mseal().

The new mseal() is an syscall on 64 bit CPU, and with
following signature:

int mseal(void addr, size_t len, unsigned long flags)
addr/len: memory range.
flags: reserved.

mseal() blocks following operations for the given memory range.

1> Unmapping, moving to another location, and shrinking the size,
   via munmap() and mremap(), can leave an empty space, therefore can
   be replaced with a VMA with a new set of attributes.

2> Moving or expanding a different VMA into the current location,
   via mremap().

3> Modifying a VMA via mmap(MAP_FIXED).

4> Size expansion, via mremap(), does not appear to pose any specific
   risks to sealed VMAs. It is included anyway because the use case is
   unclear. In any case, users can rely on merging to expand a sealed VMA.

5> mprotect() and pkey_mprotect().

6> Some destructive madvice() behaviors (e.g. MADV_DONTNEED) for anonymous
   memory, when users don't have write permission to the memory. Those
   behaviors can alter region contents by discarding pages, effectively a
   memset(0) for anonymous memory.

In addition: mmap() has two related changes.

The PROT_SEAL bit in prot field of mmap(). When present, it marks
the map sealed since creation.

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.

Applications that don't care about sealing will expect their behavior
unchanged. For those that need sealing support, opt-in by adding
MAP_SEALABLE in mmap().

The idea that inspired this patch comes from Stephen Röttger’s work in
V8 CFI [5]. Chrome browser in ChromeOS will be the first user of this
API.

Indeed, the Chrome browser has very specific requirements for sealing,
which are distinct from those of most applications. For example, in
the case of libc, sealing is only applied to read-only (RO) or
read-execute (RX) memory segments (such as .text and .RELRO) to
prevent them from becoming writable, the lifetime of those mappings
are tied to the lifetime of the process.

Chrome wants to seal two large address space reservations that are
managed by different allocators. The memory is mapped RW- and RWX
respectively but write access to it is restricted using pkeys (or in
the future ARM permission overlay extensions). The lifetime of those
mappings are not tied to the lifetime of the process, therefore, while
the memory is sealed, the allocators still need to free or discard the
unused memory. For example, with madvise(DONTNEED).

However, always allowing madvise(DONTNEED) on this range poses a
security risk. For example if a jump instruction crosses a page
boundary and the second page gets discarded, it will overwrite the
target bytes with zeros and change the control flow. Checking
write-permission before the discard operation allows us to control
when the operation is valid. In this case, the madvise will only
succeed if the executing thread has PKEY write permissions and PKRU
changes are protected in software by control-flow integrity.

Although the initial version of this patch series is targeting the
Chrome browser as its first user, it became evident during upstream
discussions that we would also want to ensure that the patch set
eventually is a complete solution for memory sealing and compatible
with other use cases. The specific scenario currently in mind is
glibc's use case of loading and sealing ELF executables. To this end,
Stephen is working on a change to glibc to add sealing support to the
dynamic linker, which will seal all non-writable segments at startup.
Once this work is completed, all applications will be able to
automatically benefit from these new protections.

In closing, I would like to formally acknowledge the valuable
contributions received during the RFC process, which were instrumental
in shaping this patch:

Jann Horn: raising awareness and providing valuable insights on the
destructive madvise operations.
Linus Torvalds: assisting in defining system call signature and scope.
Pedro Falcato: suggesting sealing in the mmap().
Theo de Raadt: sharing the experiences and insights gained from
implementing mimmutable() in OpenBSD.

Change history:
===============
V7:
- fix index.rst (Randy Dunlap)
- fix arm build (Randy Dunlap)
- return EPERM for blocked operations (Theo de Raadt)

V6:
- Drop RFC from subject, Given Linus's general approval.
- Adjust syscall number for mseal (main Jan.11/2024) 
- Code style fix (Matthew Wilcox)
- selftest: use ksft macros (Muhammad Usama Anjum)
- Document fix. (Randy Dunlap)
https://lore.kernel.org/all/20240111234142.2944934-1-jeffxu@chromium.org/

V5:
- fix build issue in mseal-Wire-up-mseal-syscall
  (Suggested by Linus Torvalds, and Greg KH)
- updates on selftest.
https://lore.kernel.org/lkml/20240109154547.1839886-1-jeffxu@chromium.org/#r

V4:
(Suggested by Linus Torvalds)
- new signature: mseal(start,len,flags)
- 32 bit is not supported. vm_seal is removed, use vm_flags instead.
- single bit in vm_flags for sealed state.
- CONFIG_MSEAL kernel config is removed.
- single bit of PROT_SEAL in the "Prot" field of mmap().
Other changes:
- update selftest (Suggested by Muhammad Usama Anjum)
- update documentation.
https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@chromium.org/

V3:
- Abandon per-syscall approach, (Suggested by Linus Torvalds).
- Organize sealing types around their functionality, such as
  MM_SEAL_BASE, MM_SEAL_PROT_PKEY.
- Extend the scope of sealing from calls originated in userspace to
  both kernel and userspace. (Suggested by Linus Torvalds)
- Add seal type support in mmap(). (Suggested by Pedro Falcato)
- Add a new sealing type: MM_SEAL_DISCARD_RO_ANON to prevent
  destructive operations of madvise. (Suggested by Jann Horn and
  Stephen Röttger)
- Make sealed VMAs mergeable. (Suggested by Jann Horn)
- Add MAP_SEALABLE to mmap()
- Add documentation - mseal.rst
https://lore.kernel.org/linux-mm/20231212231706.2680890-2-jeffxu@chromium.org/

v2:
Use _BITUL to define MM_SEAL_XX type.
Use unsigned long for seal type in sys_mseal() and other functions.
Remove internal VM_SEAL_XX type and convert_user_seal_type().
Remove MM_ACTION_XX type.
Remove caller_origin(ON_BEHALF_OF_XX) and replace with sealing bitmask.
Add more comments in code.
Add a detailed commit message.
https://lore.kernel.org/lkml/20231017090815.1067790-1-jeffxu@chromium.org/

v1:
https://lore.kernel.org/lkml/20231016143828.647848-1-jeffxu@chromium.org/

----------------------------------------------------------------
[1] https://kernelnewbies.org/Linux_2_6_8
[2] https://v8.dev/blog/control-flow-integrity
[3] https://github.com/apple-oss-distributions/xnu/blob/1031c584a5e37aff177559b9f69dbd3c8c3fd30a/osfmk/mach/vm_statistics.h#L274
[4] https://man.openbsd.org/mimmutable.2
[5] https://docs.google.com/document/d/1O2jwK4dxI3nRcOJuPYkonhTkNQfbmwdvxQMyXgeaRHo/edit#heading=h.bvaojj9fu6hc
[6] https://lore.kernel.org/lkml/CAG48ez3ShUYey+ZAFsU2i1RpQn0a5eOs2hzQ426FkcgnfUGLvA@mail.gmail.com/
[7] https://lore.kernel.org/lkml/20230515130553.2311248-1-jeffxu@chromium.org/

Jeff Xu (4):
  mseal: Wire up mseal syscall
  mseal: add mseal syscall
  selftest mm/mseal memory sealing
  mseal:add documentation

 Documentation/userspace-api/index.rst       |    1 +
 Documentation/userspace-api/mseal.rst       |  183 ++
 arch/alpha/kernel/syscalls/syscall.tbl      |    1 +
 arch/arm/tools/syscall.tbl                  |    1 +
 arch/arm64/include/asm/unistd.h             |    2 +-
 arch/arm64/include/asm/unistd32.h           |    2 +
 arch/m68k/kernel/syscalls/syscall.tbl       |    1 +
 arch/microblaze/kernel/syscalls/syscall.tbl |    1 +
 arch/mips/kernel/syscalls/syscall_n32.tbl   |    1 +
 arch/mips/kernel/syscalls/syscall_n64.tbl   |    1 +
 arch/mips/kernel/syscalls/syscall_o32.tbl   |    1 +
 arch/parisc/kernel/syscalls/syscall.tbl     |    1 +
 arch/powerpc/kernel/syscalls/syscall.tbl    |    1 +
 arch/s390/kernel/syscalls/syscall.tbl       |    1 +
 arch/sh/kernel/syscalls/syscall.tbl         |    1 +
 arch/sparc/kernel/syscalls/syscall.tbl      |    1 +
 arch/x86/entry/syscalls/syscall_32.tbl      |    1 +
 arch/x86/entry/syscalls/syscall_64.tbl      |    1 +
 arch/xtensa/kernel/syscalls/syscall.tbl     |    1 +
 include/linux/mm.h                          |   48 +
 include/linux/syscalls.h                    |    1 +
 include/uapi/asm-generic/mman-common.h      |    8 +
 include/uapi/asm-generic/unistd.h           |    5 +-
 kernel/sys_ni.c                             |    1 +
 mm/Makefile                                 |    4 +
 mm/madvise.c                                |   12 +
 mm/mmap.c                                   |   27 +
 mm/mprotect.c                               |   10 +
 mm/mremap.c                                 |   31 +
 mm/mseal.c                                  |  343 ++++
 tools/testing/selftests/mm/.gitignore       |    1 +
 tools/testing/selftests/mm/Makefile         |    1 +
 tools/testing/selftests/mm/mseal_test.c     | 1997 +++++++++++++++++++
 33 files changed, 2690 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/userspace-api/mseal.rst
 create mode 100644 mm/mseal.c
 create mode 100644 tools/testing/selftests/mm/mseal_test.c

Comments

Theo de Raadt Jan. 22, 2024, 3:49 p.m. UTC | #1
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.

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

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.
Jeff Xu Jan. 22, 2024, 10:10 p.m. UTC | #2
On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@openbsd.org> 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.

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@mail.gmail.com/

> 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@chromium.org/
[3] https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@chromium.org/

> 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.
Theo de Raadt Jan. 22, 2024, 10:34 p.m. UTC | #3
Jeff Xu <jeffxu@chromium.org> wrote:

> On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@openbsd.org> 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@mail.gmail.com/

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@chromium.org/
> [3] https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@chromium.org/

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.
Liam R. Howlett Jan. 23, 2024, 5:33 p.m. UTC | #4
* Theo de Raadt <deraadt@openbsd.org> [240122 17:35]:
> Jeff Xu <jeffxu@chromium.org> wrote:
> 
> > On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@openbsd.org> 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@mail.gmail.com/
> 
> Are you joking?  You don't have any code doing that today.  More feelings?

The 'no downside" is to combining two calls together; mmap() & mseal(),
at least that is how I read the linked discussion.

The common case (since there are no users today) of just calling
mmap()/munmap() will have the downside.

There will be a performance impact once you have can_modify_mm() doing
more than just returning true.  Certainly, the impact will be larger
in munmap where multiple VMAs may need to be checked (assuming that's
the plan?).

This will require a new and earlier walk of the vma tree while holding
the mmap_lock.  Since you are checking (potentially multiple) VMAs for
something, I don't think there is a way around holding the lock.

I'm not saying the cost will be large, but it will be a positive
non-zero number.

Thanks,
Liam
Theo de Raadt Jan. 23, 2024, 6:58 p.m. UTC | #5
Liam R. Howlett <Liam.Howlett@Oracle.com> wrote:

> * Theo de Raadt <deraadt@openbsd.org> [240122 17:35]:
> > Jeff Xu <jeffxu@chromium.org> wrote:
> > 
> > > On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@openbsd.org> 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@mail.gmail.com/
> > 
> > Are you joking?  You don't have any code doing that today.  More feelings?
> 
> The 'no downside" is to combining two calls together; mmap() & mseal(),
> at least that is how I read the linked discussion.
> 
> The common case (since there are no users today) of just calling
> mmap()/munmap() will have the downside.
> 
> There will be a performance impact once you have can_modify_mm() doing
> more than just returning true.  Certainly, the impact will be larger
> in munmap where multiple VMAs may need to be checked (assuming that's
> the plan?).
> 
> This will require a new and earlier walk of the vma tree while holding
> the mmap_lock.  Since you are checking (potentially multiple) VMAs for
> something, I don't think there is a way around holding the lock.
> 
> I'm not saying the cost will be large, but it will be a positive
> non-zero number.

For future glibc changes, I predict you will have zero cases where you
can call mmap+immutable or mprotect+immutable, I say so, because I ended
up having none.  You always have to fill the memory.  (At first glance
you might think it works for a new DSO's BSS, but RELRO overlaps it, and
since RELRO mprotect happens quite late, the permission locking is quite
delayed relative to the allocation).

I think chrome also won't lock memory at allocation.  I suspect the
generic allocator is quite seperate from the code using the allocation,
which knows which objects can have their permissions locked and which
objects can't.

In OpenBSD, the only cases where we could set immutable at the same time
as creating the mapping was in execve, for a new process's stack regions,
and that is kernel code, not the userland exposed system call APIs.
 
This change could skip adding PROT_MSEAL today, and add it later when
there are facts the need.


It's the same with MAP_MSEALABLE.  I don't get it. So now there are 3
memory types:
       - cannot be sealed, ever
       - not yet sealed
       - sealed

What purpose does the first type serve?  Please explain the use case.

Today, processes have control over their entire address space.

What is the purpose of "permissions cannot be locked".  Please supply
an example.  If I am wrong, I'd like to know where I went wrong.
Jeff Xu Jan. 24, 2024, 6:55 p.m. UTC | #6
On Mon, Jan 22, 2024 at 2:34 PM Theo de Raadt <deraadt@openbsd.org> wrote:
>
> Jeff Xu <jeffxu@chromium.org> wrote:
>
> > On Mon, Jan 22, 2024 at 7:49 AM Theo de Raadt <deraadt@openbsd.org> 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.
>
I'm sorry, I have never looked at one line of openBSD code, prototype
or not, nor did I install openBSD before.

Because of this situation on my side, I failed to understand why you
have such a strong opinion on PROC_SEAL in mmap() in linux kernel,
based on your own OpenBSD's experience ?

For PROT_SEAL in mmap(), I see it as a good and reasonable suggestion
raised during the RFC process, and incorporate it into the patch set,
there is nothing more and nothing less.

If openBSD doesn't want it, that is fine to me, it is not that I'm
trying to force this into openBSD's kernel, I understand it is a
different code base.

> > 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@mail.gmail.com/
>
> 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@chromium.org/
> > [3] https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@chromium.org/
>
> 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.
>
I respect your reasoning with OpenBSD, but do you have a real example
that this will be problematic for linux ?

In my opinion, the extra communication part with mmap()'s owner has
its pros and cons.

The cons is what you mentioned: extra time for convincing and approval.

The pro is that there won't be unexpected behavior from the code owner
point of view, once this communication process is completed. It can
reduce the possibility of introducing bugs.

So far, I do not have enough information to say this is a bad idea.
if you can provide a real example in the context of linux, e.g. DSO
and libc you mentioned with details, that will be helpful.
Jeff Xu Jan. 24, 2024, 6:56 p.m. UTC | #7
On Tue, Jan 23, 2024 at 10:58 AM Theo de Raadt <deraadt@openbsd.org> wrote:
>
> It's the same with MAP_MSEALABLE.  I don't get it. So now there are 3
> memory types:
>        - cannot be sealed, ever
>        - not yet sealed
>        - sealed
>
> What purpose does the first type serve?  Please explain the use case.
>
> Today, processes have control over their entire address space.
>
> What is the purpose of "permissions cannot be locked".  Please supply
> an example.  If I am wrong, I'd like to know where I went wrong.
>
The linux example is in the V3 and V4 cover letter [1] [2] of the open
discussion section.

[1] https://lore.kernel.org/linux-mm/20231212231706.2680890-1-jeffxu@chromium.org/T/
[2] https://lore.kernel.org/linux-mm/20240104185138.169307-3-jeffxu@chromium.org/T/

Copied below for ease of reading.
-----------------------------------------------------------------------------------------
During the development of V3, I had new questions and thoughts and
wished to discuss.

1> shm/aio
From reading the code, it seems to me that aio/shm can mmap/munmap
maps on behalf of userspace, e.g. ksys_shmdt() in shm.c. The lifetime
of those mapping are not tied to the lifetime of the process. If those
memories are sealed from userspace, then unmap will fail. This isn’t a
huge problem, since the memory will eventually be freed at exit or
exec. However, it feels like the solution is not complete, because of
the leaks in VMA address space during the lifetime of the process.

2> Brk (heap/stack)
Currently, userspace applications can seal parts of the heap by
calling malloc() and mseal(). This raises the question of what the
expected behavior is when sealing the heap is attempted.

let's assume following calls from user space:

ptr = malloc(size);
mprotect(ptr, size, RO);
mseal(ptr, size, SEAL_PROT_PKEY);
free(ptr);

Technically, before mseal() is added, the user can change the
protection of the heap by calling mprotect(RO). As long as the user
changes the protection back to RW before free(), the memory can be
reused.

Adding mseal() into picture, however, the heap is then sealed
partially, user can still free it, but the memory remains to be RO,
and the result of brk-shrink is nondeterministic, depending on if
munmap() will try to free the sealed memory.(brk uses munmap to shrink
the heap).

3> Above two cases led to the third topic:
There one option to address the problem mentioned above.
Option 1:  A “MAP_SEALABLE” flag in mmap().
If a map is created without this flag, the mseal() operation will
fail. Applications that are not concerned with sealing will expect
their behavior to be unchanged. For those that are concerned, adding a
flag at mmap time to opt in is not difficult. For the short term, this
solves problems 1 and 2 above. The memory in shm/aio/brk will not have
the MAP_SEALABLE flag at mmap(), and the same is true for the heap.

If we choose not to go with path, all mapping will by default
sealable. We could document above mentioned limitations so devs are
more careful at the time to choose what memory to seal. I think
deny of service through mseal() by attacker is probably not a concern,
if attackers have access to mseal() and unsealed memory, then they can
also do other harmful thing to the memory, such as munmap, etc.

4>
I think it might be possible to seal the stack or other special
mappings created at runtime (vdso, vsyscall, vvar). This means we can
enforce and seal W^X for certain types of application. For instance,
the stack is typically used in read-write mode, but in some cases, it
can become executable. To defend against unintented addition of
executable bit to stack, we could let the application to seal it.

Sealing the heap (for adding X) requires special handling, since the
heap can shrink, and shrink is implemented through munmap().

Indeed, it might be possible that all virtual memory accessible to user
space, regardless of its usage pattern, could be sealed. However, this
would require additional research and development work.

-----------------------------------------------------------------------------------------------------
Theo de Raadt Jan. 24, 2024, 7:17 p.m. UTC | #8
Jeff Xu <jeffxu@chromium.org> wrote:

> > 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.
> >
> I'm sorry, I have never looked at one line of openBSD code, prototype
> or not, nor did I install openBSD before.

That is really disingeneous.

It is obvious to everyone that mseal is a derivative of the mimmutable
mechanism, the raw idea stems directly from this and you didn't need to
stay at a Holiday Express Inn.

> Because of this situation on my side, I failed to understand why you
> have such a strong opinion on PROC_SEAL in mmap() in linux kernel,
> based on your own OpenBSD's experience ?

Portable and compatible interfaces are good.

Historically, incompatible interfaces are less good.

> For PROT_SEAL in mmap(), I see it as a good and reasonable suggestion
> raised during the RFC process, and incorporate it into the patch set,
> there is nothing more and nothing less.

Yet, you and those who suggested it don't have a single line of userland
code ready which will use this.
 
> If openBSD doesn't want it, that is fine to me, it is not that I'm
> trying to force this into openBSD's kernel, I understand it is a
> different code base.

This has nothing to do with code base.

It is about attempting to decrease differences between systems; this
approach which has always been valuable.

Divergence has always been painful.

> > > > 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@chromium.org/
> > > [3] https://lore.kernel.org/all/20240104185138.169307-1-jeffxu@chromium.org/
> >
> > 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.
> >
> I respect your reasoning with OpenBSD, but do you have a real example
> that this will be problematic for linux ?

See below.

> In my opinion, the extra communication part with mmap()'s owner has
> its pros and cons.

See below.

> The cons is what you mentioned: extra time for convincing and approval.

No, it is much worse than that.  See below.

> The pro is that there won't be unexpected behavior from the code owner
> point of view, once this communication process is completed. It can
> reduce the possibility of introducing bugs.
> 
> So far, I do not have enough information to say this is a bad idea.
> if you can provide a real example in the context of linux, e.g. DSO
> and libc you mentioned with details, that will be helpful.

Does the kernel map the main program's text segment, data segment, bss
segment, and stack with MAP_SEALABLE or without MAP_SEALABLE?

Once it is mapped, userland starts running.

If those objects don't have MAP_SEALABLE, then ld.so and libc cannot
perform locking of those mappings.  And ld.so or libc must do some of
those lockings later, some of these map lockings cannot be performed in
the kernel because userland makes data modifications and permission modifications
before proceeding into main().

This is unavoidable, because of RELRO; binaries with text relocation; binaries
with W|X mappings; it is probably required for IFUNC setup; and I strongly
suspect there are additional circumstances which require this, *just for glibc*
to use the mechanism.

If the kernel does map those regions with MAP_SEALABLE, then it seems
the most important parts of the address space are going to have MAP_SEALABLE
anyways.  So what were you trying to defend against?

So why are you doing this MAP_SEALABLE dance?   It makes no sense.

I'm sorry, but it is you who must justify these strange semantics which
you are introducing -- to change a mechanism previously engineered and
fully deployed in another operating system.  To me, not being able to
justify these behavious seems to be based on intentional ignorance.
"Not Invented Here", is what I see.

You say glibc will use this.  I call bollocks.  I see a specific behaviour
which will prevent use by glibc.  I designed my mechanism with libc specifically
considered -- it was a whole system environment.

You work on chrome.  You don't work on glibc.  The glibc people aren't publically
talking about this.  From my perspective, this is looking really dumb.
Jonathan Corbet Jan. 29, 2024, 10:36 p.m. UTC | #9
jeffxu@chromium.org writes:

> Although the initial version of this patch series is targeting the
> Chrome browser as its first user, it became evident during upstream
> discussions that we would also want to ensure that the patch set
> eventually is a complete solution for memory sealing and compatible
> with other use cases. The specific scenario currently in mind is
> glibc's use case of loading and sealing ELF executables. To this end,
> Stephen is working on a change to glibc to add sealing support to the
> dynamic linker, which will seal all non-writable segments at startup.
> Once this work is completed, all applications will be able to
> automatically benefit from these new protections.

Is this work posted somewhere?  Having a second - and more generally
useful - user for this API would do a lot to show that the design is, in
fact, right and useful beyond the Chrome browser.

Thanks,

jon
Jeff Xu Jan. 31, 2024, 5:49 p.m. UTC | #10
On Mon, Jan 29, 2024 at 2:37 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> jeffxu@chromium.org writes:
>
> > Although the initial version of this patch series is targeting the
> > Chrome browser as its first user, it became evident during upstream
> > discussions that we would also want to ensure that the patch set
> > eventually is a complete solution for memory sealing and compatible
> > with other use cases. The specific scenario currently in mind is
> > glibc's use case of loading and sealing ELF executables. To this end,
> > Stephen is working on a change to glibc to add sealing support to the
> > dynamic linker, which will seal all non-writable segments at startup.
> > Once this work is completed, all applications will be able to
> > automatically benefit from these new protections.
>
> Is this work posted somewhere?  Having a second - and more generally
> useful - user for this API would do a lot to show that the design is, in
> fact, right and useful beyond the Chrome browser.
>
Stephen conducted a PoC last year, it will be published once it is complete.
We're super excited about introducing this as a general safety measure
for all of Linux!

Thanks
-Jeff

> Thanks,
>
> jon
Jonathan Corbet Jan. 31, 2024, 8:51 p.m. UTC | #11
Jeff Xu <jeffxu@chromium.org> writes:

> On Mon, Jan 29, 2024 at 2:37 PM Jonathan Corbet <corbet@lwn.net> wrote:
>>
>> jeffxu@chromium.org writes:
>>
>> > Although the initial version of this patch series is targeting the
>> > Chrome browser as its first user, it became evident during upstream
>> > discussions that we would also want to ensure that the patch set
>> > eventually is a complete solution for memory sealing and compatible
>> > with other use cases. The specific scenario currently in mind is
>> > glibc's use case of loading and sealing ELF executables. To this end,
>> > Stephen is working on a change to glibc to add sealing support to the
>> > dynamic linker, which will seal all non-writable segments at startup.
>> > Once this work is completed, all applications will be able to
>> > automatically benefit from these new protections.
>>
>> Is this work posted somewhere?  Having a second - and more generally
>> useful - user for this API would do a lot to show that the design is, in
>> fact, right and useful beyond the Chrome browser.
>>
> Stephen conducted a PoC last year, it will be published once it is complete.
> We're super excited about introducing this as a general safety measure
> for all of Linux!

We're excited too, something like mseal() seems like a good thing to
have.  My point, though, is that it would be good to see this second
(and more general) user of the API *before* merging it.  As others have
noted, once mseal() is in a released kernel, it will be difficult to
change if adjustments turn out to be necessary.

Thanks,

jon