* [PATCH v22 2/4] random: introduce generic vDSO getrandom() implementation
From: Jason A. Donenfeld @ 2024-07-09 13:05 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86, Linus Torvalds,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240709130513.98102-1-Jason@zx2c4.com>
Provide a generic C vDSO getrandom() implementation, which operates on
an opaque state returned by vgetrandom_alloc() and produces random bytes
the same way as getrandom(). This has the following API signature:
ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags,
void *opaque_state, size_t opaque_len);
The return value and the first three arguments are the same as ordinary
getrandom(), while the last two arguments are a pointer to the opaque
allocated state and its size. Were all five arguments passed to the
getrandom() syscall, nothing different would happen, and the functions
would have the exact same behavior.
The actual vDSO RNG algorithm implemented is the same one implemented by
drivers/char/random.c, using the same fast-erasure techniques as that.
Should the in-kernel implementation change, so too will the vDSO one.
It requires an implementation of ChaCha20 that does not use any stack,
in order to maintain forward secrecy if a multi-threaded program forks
(though this does not account for a similar issue with SA_SIGINFO
copying registers to the stack), so this is left as an
architecture-specific fill-in. Stack-less ChaCha20 is an easy algorithm
to implement on a variety of architectures, so this shouldn't be too
onerous.
Initially, the state is keyless, and so the first call makes a
getrandom() syscall to generate that key, and then uses it for
subsequent calls. By keeping track of a generation counter, it knows
when its key is invalidated and it should fetch a new one using the
syscall. Later, more than just a generation counter might be used.
Since MADV_WIPEONFORK is set on the opaque state, the key and related
state is wiped during a fork(), so secrets don't roll over into new
processes, and the same state doesn't accidentally generate the same
random stream. The generation counter, as well, is always >0, so that
the 0 counter is a useful indication of a fork() or otherwise
uninitialized state.
If the kernel RNG is not yet initialized, then the vDSO always calls the
syscall, because that behavior cannot be emulated in userspace, but
fortunately that state is short lived and only during early boot. If it
has been initialized, then there is no need to inspect the `flags`
argument, because the behavior does not change post-initialization
regardless of the `flags` value.
Since the opaque state passed to it is mutated, vDSO getrandom() is not
reentrant, when used with the same opaque state, which libc should be
mindful of.
The function works over an opaque per-thread state of a particular size,
which must be marked VM_WIPEONFORK, VM_DONTDUMP, VM_NORESERVE, and
VM_DROPPABLE for proper operation. Over time, the nuances of these
allocations may change or grow or even differ based on architectural
features.
The opaque state passed to vDSO getrandom() must be allocated using the
mmap_flags and mmap_prot parameters provided by the vgetrandom_opaque_params
struct, which also contains the size of each state. That struct can be
obtained with a call to vgetrandom(NULL, 0, 0, ¶ms, ~0UL). Then,
libc can call mmap(2) and slice up the returned array into a state per
each thread, while ensuring that no single state straddles a page
boundary. Libc is expected to allocate a chunk of these on first use,
and then dole them out to threads as they're created, allocating more
when needed.
vDSO getrandom() provides the ability for userspace to generate random
bytes quickly and safely, and is intended to be integrated into libc's
thread management. As an illustrative example, the introduced code in
the vdso_test_getrandom self test later in this series might be used to
do the same outside of libc. In a libc the various pthread-isms are
expected to be elided into libc internals.
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
MAINTAINERS | 2 +
drivers/char/random.c | 18 ++-
include/uapi/linux/random.h | 15 +++
include/vdso/datapage.h | 11 ++
include/vdso/getrandom.h | 46 +++++++
lib/vdso/Kconfig | 5 +
lib/vdso/getrandom.c | 251 ++++++++++++++++++++++++++++++++++++
7 files changed, 347 insertions(+), 1 deletion(-)
create mode 100644 include/vdso/getrandom.h
create mode 100644 lib/vdso/getrandom.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 3c4fdf74a3f9..798158329ad8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18747,6 +18747,8 @@ T: git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
F: Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml
F: drivers/char/random.c
F: drivers/virt/vmgenid.c
+F: include/vdso/getrandom.h
+F: lib/vdso/getrandom.c
RAPIDIO SUBSYSTEM
M: Matt Porter <mporter@kernel.crashing.org>
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2597cb43f438..b02a12436750 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
/*
- * Copyright (C) 2017-2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
* Copyright Matt Mackall <mpm@selenic.com>, 2003, 2004, 2005
* Copyright Theodore Ts'o, 1994, 1995, 1996, 1997, 1998, 1999. All rights reserved.
*
@@ -56,6 +56,10 @@
#include <linux/sched/isolation.h>
#include <crypto/chacha.h>
#include <crypto/blake2s.h>
+#ifdef CONFIG_VDSO_GETRANDOM
+#include <vdso/getrandom.h>
+#include <vdso/datapage.h>
+#endif
#include <asm/archrandom.h>
#include <asm/processor.h>
#include <asm/irq.h>
@@ -271,6 +275,15 @@ static void crng_reseed(struct work_struct *work)
if (next_gen == ULONG_MAX)
++next_gen;
WRITE_ONCE(base_crng.generation, next_gen);
+#ifdef CONFIG_VDSO_GETRANDOM
+ /* base_crng.generation's invalid value is ULONG_MAX, while
+ * _vdso_rng_data.generation's invalid value is 0, so add one to the
+ * former to arrive at the latter. Use smp_store_release so that this
+ * is ordered with the write above to base_crng.generation. Pairs with
+ * the smp_rmb() before the syscall in the vDSO code.
+ */
+ smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
+#endif
if (!static_branch_likely(&crng_is_ready))
crng_init = CRNG_READY;
spin_unlock_irqrestore(&base_crng.lock, flags);
@@ -721,6 +734,9 @@ static void __cold _credit_init_bits(size_t bits)
if (static_key_initialized && system_unbound_wq)
queue_work(system_unbound_wq, &set_ready);
atomic_notifier_call_chain(&random_ready_notifier, 0, NULL);
+#ifdef CONFIG_VDSO_GETRANDOM
+ WRITE_ONCE(_vdso_rng_data.is_ready, true);
+#endif
wake_up_interruptible(&crng_init_wait);
kill_fasync(&fasync, SIGIO, POLL_IN);
pr_notice("crng init done\n");
diff --git a/include/uapi/linux/random.h b/include/uapi/linux/random.h
index e744c23582eb..2a3fe4c2cdc9 100644
--- a/include/uapi/linux/random.h
+++ b/include/uapi/linux/random.h
@@ -55,4 +55,19 @@ struct rand_pool_info {
#define GRND_RANDOM 0x0002
#define GRND_INSECURE 0x0004
+/**
+ * struct vgetrandom_opaque_params - arguments for allocating memory for vgetrandom
+ *
+ * @size_per_opaque_state: Size of each state that is to be passed to vgetrandom().
+ * @mmap_prot: Value of the prot argument in mmap(2).
+ * @mmap_flags: Value of the flags argument in mmap(2).
+ * @reserved: Reserved for future use.
+ */
+struct vgetrandom_opaque_params {
+ __u32 size_of_opaque_state;
+ __u32 mmap_prot;
+ __u32 mmap_flags;
+ __u32 reserved[13];
+};
+
#endif /* _UAPI_LINUX_RANDOM_H */
diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index d04d394db064..05e5787beb73 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -113,6 +113,16 @@ struct vdso_data {
struct arch_vdso_data arch_data;
};
+/**
+ * struct vdso_rng_data - vdso RNG state information
+ * @generation: counter representing the number of RNG reseeds
+ * @is_ready: boolean signaling whether the RNG is initialized
+ */
+struct vdso_rng_data {
+ u64 generation;
+ u8 is_ready;
+};
+
/*
* We use the hidden visibility to prevent the compiler from generating a GOT
* relocation. Not only is going through a GOT useless (the entry couldn't and
@@ -124,6 +134,7 @@ struct vdso_data {
*/
extern struct vdso_data _vdso_data[CS_BASES] __attribute__((visibility("hidden")));
extern struct vdso_data _timens_data[CS_BASES] __attribute__((visibility("hidden")));
+extern struct vdso_rng_data _vdso_rng_data __attribute__((visibility("hidden")));
/**
* union vdso_data_store - Generic vDSO data page
diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
new file mode 100644
index 000000000000..a8b7c14b0ae0
--- /dev/null
+++ b/include/vdso/getrandom.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#ifndef _VDSO_GETRANDOM_H
+#define _VDSO_GETRANDOM_H
+
+#include <linux/types.h>
+
+#define CHACHA_KEY_SIZE 32
+#define CHACHA_BLOCK_SIZE 64
+
+/**
+ * struct vgetrandom_state - State used by vDSO getrandom().
+ *
+ * @batch: One and a half ChaCha20 blocks of buffered RNG output.
+ *
+ * @key: Key to be used for generating next batch.
+ *
+ * @batch_key: Union of the prior two members, which is exactly two full
+ * ChaCha20 blocks in size, so that @batch and @key can be filled
+ * together.
+ *
+ * @generation: Snapshot of @rng_info->generation in the vDSO data page at
+ * the time @key was generated.
+ *
+ * @pos: Offset into @batch of the next available random byte.
+ *
+ * @in_use: Reentrancy guard for reusing a state within the same thread
+ * due to signal handlers.
+ */
+struct vgetrandom_state {
+ union {
+ struct {
+ u8 batch[CHACHA_BLOCK_SIZE * 3 / 2];
+ u32 key[CHACHA_KEY_SIZE / sizeof(u32)];
+ };
+ u8 batch_key[CHACHA_BLOCK_SIZE * 2];
+ };
+ u64 generation;
+ u8 pos;
+ bool in_use;
+};
+
+#endif /* _VDSO_GETRANDOM_H */
diff --git a/lib/vdso/Kconfig b/lib/vdso/Kconfig
index c46c2300517c..82fe827af542 100644
--- a/lib/vdso/Kconfig
+++ b/lib/vdso/Kconfig
@@ -38,3 +38,8 @@ config GENERIC_VDSO_OVERFLOW_PROTECT
in the hotpath.
endif
+
+config VDSO_GETRANDOM
+ bool
+ help
+ Selected by architectures that support vDSO getrandom().
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
new file mode 100644
index 000000000000..b230f0b10832
--- /dev/null
+++ b/lib/vdso/getrandom.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#include <linux/cache.h>
+#include <linux/kernel.h>
+#include <linux/time64.h>
+#include <vdso/datapage.h>
+#include <vdso/getrandom.h>
+#include <asm/vdso/getrandom.h>
+#include <asm/vdso/vsyscall.h>
+#include <asm/unaligned.h>
+#include <uapi/linux/mman.h>
+
+#define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do { \
+ while (len >= sizeof(type)) { \
+ __put_unaligned_t(type, __get_unaligned_t(type, src), dst); \
+ __put_unaligned_t(type, 0, src); \
+ dst += sizeof(type); \
+ src += sizeof(type); \
+ len -= sizeof(type); \
+ } \
+} while (0)
+
+static void memcpy_and_zero_src(void *dst, void *src, size_t len)
+{
+ if (IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)) {
+ if (IS_ENABLED(CONFIG_64BIT))
+ MEMCPY_AND_ZERO_SRC(u64, dst, src, len);
+ MEMCPY_AND_ZERO_SRC(u32, dst, src, len);
+ MEMCPY_AND_ZERO_SRC(u16, dst, src, len);
+ }
+ MEMCPY_AND_ZERO_SRC(u8, dst, src, len);
+}
+
+/**
+ * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
+ * @rng_info: Describes state of kernel RNG, memory shared with kernel.
+ * @buffer: Destination buffer to fill with random bytes.
+ * @len: Size of @buffer in bytes.
+ * @flags: Zero or more GRND_* flags.
+ * @opaque_state: Pointer to an opaque state area.
+ * @opaque_len: Length of opaque state area.
+ *
+ * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
+ * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
+ * schedule that the kernel's RNG is reseeded. If the kernel's RNG is not ready, then this always
+ * calls into the syscall.
+ *
+ * If @buffer, @len, and @flags are 0, and @opaque_len is ~0UL, then @opaque_state is populated
+ * with a struct vgetrandom_opaque_params and the function returns 0; if it does not return 0,
+ * this function should not be used.
+ *
+ * @opaque_state *must* be allocated by calling mmap(2) using the mmap_prot and mmap_flags fields
+ * from the struct vgetrandom_opaque_params, and states must not straddle pages. Unless external
+ * locking is used, one state must be allocated per thread, as it is not safe to call this function
+ * concurrently with the same @opaque_state. However, it is safe to call this using the same
+ * @opaque_state that is shared between main code and signal handling code, within the same thread.
+ *
+ * Returns: The number of random bytes written to @buffer, or a negative value indicating an error.
+ */
+static __always_inline ssize_t
+__cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
+ unsigned int flags, void *opaque_state, size_t opaque_len)
+{
+ ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
+ struct vgetrandom_state *state = opaque_state;
+ size_t batch_len, nblocks, orig_len = len;
+ bool in_use, have_retried = false;
+ unsigned long current_generation;
+ void *orig_buffer = buffer;
+ u32 counter[2] = { 0 };
+
+ if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags)) {
+ *(struct vgetrandom_opaque_params *)opaque_state = (struct vgetrandom_opaque_params) {
+ .size_of_opaque_state = sizeof(*state),
+ .mmap_prot = PROT_READ | PROT_WRITE,
+ .mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS
+ };
+ return 0;
+ }
+
+ /* The state must not straddle a page, since pages can be zeroed at any time. */
+ if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
+ return -EFAULT;
+
+ /* If the caller passes the wrong size, which might happen due to CRIU, fallback. */
+ if (unlikely(opaque_len != sizeof(*state)))
+ goto fallback_syscall;
+
+ /*
+ * If the kernel's RNG is not yet ready, then it's not possible to provide random bytes from
+ * userspace, because A) the various @flags require this to block, or not, depending on
+ * various factors unavailable to userspace, and B) the kernel's behavior before the RNG is
+ * ready is to reseed from the entropy pool at every invocation.
+ */
+ if (unlikely(!READ_ONCE(rng_info->is_ready)))
+ goto fallback_syscall;
+
+ /*
+ * This condition is checked after @rng_info->is_ready, because before the kernel's RNG is
+ * initialized, the @flags parameter may require this to block or return an error, even when
+ * len is zero.
+ */
+ if (unlikely(!len))
+ return 0;
+
+ /*
+ * @state->in_use is basic reentrancy protection against this running in a signal handler
+ * with the same @opaque_state, but obviously not atomic wrt multiple CPUs or more than one
+ * level of reentrancy. If a signal interrupts this after reading @state->in_use, but before
+ * writing @state->in_use, there is still no race, because the signal handler will run to
+ * its completion before returning execution.
+ */
+ in_use = READ_ONCE(state->in_use);
+ if (unlikely(in_use))
+ /* The syscall simply fills the buffer and does not touch @state, so fallback. */
+ goto fallback_syscall;
+ WRITE_ONCE(state->in_use, true);
+
+retry_generation:
+ /*
+ * @rng_info->generation must always be read here, as it serializes @state->key with the
+ * kernel's RNG reseeding schedule.
+ */
+ current_generation = READ_ONCE(rng_info->generation);
+
+ /*
+ * If @state->generation doesn't match the kernel RNG's generation, then it means the
+ * kernel's RNG has reseeded, and so @state->key is reseeded as well.
+ */
+ if (unlikely(state->generation != current_generation)) {
+ /*
+ * Write the generation before filling the key, in case of fork. If there is a fork
+ * just after this line, the parent and child will get different random bytes from
+ * the syscall, which is good. However, were this line to occur after the getrandom
+ * syscall, then both child and parent could have the same bytes and the same
+ * generation counter, so the fork would not be detected. Therefore, write
+ * @state->generation before the call to the getrandom syscall.
+ */
+ WRITE_ONCE(state->generation, current_generation);
+
+ /*
+ * Prevent the syscall from being reordered wrt current_generation. Pairs with the
+ * smp_store_release(&_vdso_rng_data.generation) in random.c.
+ */
+ smp_rmb();
+
+ /* Reseed @state->key using fresh bytes from the kernel. */
+ if (getrandom_syscall(state->key, sizeof(state->key), 0) != sizeof(state->key)) {
+ /*
+ * If the syscall failed to refresh the key, then @state->key is now
+ * invalid, so invalidate the generation so that it is not used again, and
+ * fallback to using the syscall entirely.
+ */
+ WRITE_ONCE(state->generation, 0);
+
+ /*
+ * Set @state->in_use to false only after the last write to @state in the
+ * line above.
+ */
+ WRITE_ONCE(state->in_use, false);
+
+ goto fallback_syscall;
+ }
+
+ /*
+ * Set @state->pos to beyond the end of the batch, so that the batch is refilled
+ * using the new key.
+ */
+ state->pos = sizeof(state->batch);
+ }
+
+ /* Set len to the total amount of bytes that this function is allowed to read, ret. */
+ len = ret;
+more_batch:
+ /*
+ * First use bytes out of @state->batch, which may have been filled by the last call to this
+ * function.
+ */
+ batch_len = min_t(size_t, sizeof(state->batch) - state->pos, len);
+ if (batch_len) {
+ /* Zeroing at the same time as memcpying helps preserve forward secrecy. */
+ memcpy_and_zero_src(buffer, state->batch + state->pos, batch_len);
+ state->pos += batch_len;
+ buffer += batch_len;
+ len -= batch_len;
+ }
+
+ if (!len) {
+ /* Prevent the loop from being reordered wrt ->generation. */
+ barrier();
+
+ /*
+ * Since @rng_info->generation will never be 0, re-read @state->generation, rather
+ * than using the local current_generation variable, to learn whether a fork
+ * occurred or if @state was zeroed due to memory pressure. Primarily, though, this
+ * indicates whether the kernel's RNG has reseeded, in which case generate a new key
+ * and start over.
+ */
+ if (unlikely(READ_ONCE(state->generation) != READ_ONCE(rng_info->generation))) {
+ /*
+ * Prevent this from looping forever in case of low memory or racing with a
+ * user force-reseeding the kernel's RNG using the ioctl.
+ */
+ if (have_retried) {
+ WRITE_ONCE(state->in_use, false);
+ goto fallback_syscall;
+ }
+
+ have_retried = true;
+ buffer = orig_buffer;
+ goto retry_generation;
+ }
+
+ /*
+ * Set @state->in_use to false only when there will be no more reads or writes of
+ * @state.
+ */
+ WRITE_ONCE(state->in_use, false);
+ return ret;
+ }
+
+ /* Generate blocks of RNG output directly into @buffer while there's enough room left. */
+ nblocks = len / CHACHA_BLOCK_SIZE;
+ if (nblocks) {
+ __arch_chacha20_blocks_nostack(buffer, state->key, counter, nblocks);
+ buffer += nblocks * CHACHA_BLOCK_SIZE;
+ len -= nblocks * CHACHA_BLOCK_SIZE;
+ }
+
+ BUILD_BUG_ON(sizeof(state->batch_key) % CHACHA_BLOCK_SIZE != 0);
+
+ /* Refill the batch and overwrite the key, in order to preserve forward secrecy. */
+ __arch_chacha20_blocks_nostack(state->batch_key, state->key, counter,
+ sizeof(state->batch_key) / CHACHA_BLOCK_SIZE);
+
+ /* Since the batch was just refilled, set the position back to 0 to indicate a full batch. */
+ state->pos = 0;
+ goto more_batch;
+
+fallback_syscall:
+ return getrandom_syscall(orig_buffer, orig_len, flags);
+}
+
+static __always_inline ssize_t
+__cvdso_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len)
+{
+ return __cvdso_getrandom_data(__arch_get_vdso_rng_data(), buffer, len, flags, opaque_state, opaque_len);
+}
--
2.45.2
^ permalink raw reply related
* [PATCH v22 1/4] mm: add MAP_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-07-09 13:05 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86, Linus Torvalds,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <20240709130513.98102-1-Jason@zx2c4.com>
The vDSO getrandom() implementation works with a buffer allocated with a
new system call that has certain requirements:
- It shouldn't be written to core dumps.
* Easy: VM_DONTDUMP.
- It should be zeroed on fork.
* Easy: VM_WIPEONFORK.
- It shouldn't be written to swap.
* Uh-oh: mlock is rlimited.
* Uh-oh: mlock isn't inherited by forks.
It turns out that the vDSO getrandom() function has three really nice
characteristics that we can exploit to solve this problem:
1) Due to being wiped during fork(), the vDSO code is already robust to
having the contents of the pages it reads zeroed out midway through
the function's execution.
2) In the absolute worst case of whatever contingency we're coding for,
we have the option to fallback to the getrandom() syscall, and
everything is fine.
3) The buffers the function uses are only ever useful for a maximum of
60 seconds -- a sort of cache, rather than a long term allocation.
These characteristics mean that we can introduce VM_DROPPABLE, which
has the following semantics:
a) It never is written out to swap.
b) Under memory pressure, mm can just drop the pages (so that they're
zero when read back again).
c) It is inherited by fork.
d) It doesn't count against the mlock budget, since nothing is locked.
This is fairly simple to implement, with the one snag that we have to
use 64-bit VM_* flags, but this shouldn't be a problem, since the only
consumers will probably be 64-bit anyway.
This way, allocations used by vDSO getrandom() can use:
VM_DROPPABLE | VM_DONTDUMP | VM_WIPEONFORK | VM_NORESERVE
And there will be no problem with using memory when not in use, not
wiping on fork(), coredumps, or writing out to swap.
In order to let vDSO getrandom() use this, expose these via mmap(2) as
MAP_DROPPABLE.
Finally, the provided self test ensures that this is working as desired.
Cc: linux-mm@kvack.org
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
fs/proc/task_mmu.c | 1 +
include/linux/mm.h | 7 ++++
include/trace/events/mmflags.h | 7 ++++
include/uapi/linux/mman.h | 1 +
mm/madvise.c | 5 ++-
mm/mlock.c | 2 +-
mm/mmap.c | 30 +++++++++++++++
mm/rmap.c | 22 +++++++++--
tools/include/uapi/linux/mman.h | 1 +
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/droppable.c | 53 ++++++++++++++++++++++++++
12 files changed, 126 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/mm/droppable.c
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 71e5039d940d..46f0b0fe9ee3 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -708,6 +708,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
[ilog2(VM_SHADOW_STACK)] = "ss",
#endif
#ifdef CONFIG_64BIT
+ [ilog2(VM_DROPPABLE)] = "dp",
[ilog2(VM_SEALED)] = "sl",
#endif
};
diff --git a/include/linux/mm.h b/include/linux/mm.h
index eb7c96d24ac0..e078c2890bf8 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -406,6 +406,13 @@ extern unsigned int kobjsize(const void *objp);
#define VM_ALLOW_ANY_UNCACHED VM_NONE
#endif
+#ifdef CONFIG_64BIT
+#define VM_DROPPABLE_BIT 40
+#define VM_DROPPABLE BIT(VM_DROPPABLE_BIT)
+#else
+#define VM_DROPPABLE VM_NONE
+#endif
+
#ifdef CONFIG_64BIT
/* VM is sealed, in vm_flags */
#define VM_SEALED _BITUL(63)
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index e46d6e82765e..b63d211bd141 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -165,6 +165,12 @@ IF_HAVE_PG_ARCH_X(arch_3)
# define IF_HAVE_UFFD_MINOR(flag, name)
#endif
+#ifdef CONFIG_64BIT
+# define IF_HAVE_VM_DROPPABLE(flag, name) {flag, name},
+#else
+# define IF_HAVE_VM_DROPPABLE(flag, name)
+#endif
+
#define __def_vmaflag_names \
{VM_READ, "read" }, \
{VM_WRITE, "write" }, \
@@ -197,6 +203,7 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" ) \
{VM_MIXEDMAP, "mixedmap" }, \
{VM_HUGEPAGE, "hugepage" }, \
{VM_NOHUGEPAGE, "nohugepage" }, \
+IF_HAVE_VM_DROPPABLE(VM_DROPPABLE, "droppable" ) \
{VM_MERGEABLE, "mergeable" } \
#define show_vma_flags(flags) \
diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
index a246e11988d5..e89d00528f2f 100644
--- a/include/uapi/linux/mman.h
+++ b/include/uapi/linux/mman.h
@@ -17,6 +17,7 @@
#define MAP_SHARED 0x01 /* Share changes */
#define MAP_PRIVATE 0x02 /* Changes are private */
#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
+#define MAP_DROPPABLE 0x08 /* Zero memory under memory pressure. */
/*
* Huge page size encoding when MAP_HUGETLB is specified, and a huge page
diff --git a/mm/madvise.c b/mm/madvise.c
index a77893462b92..cba5bc652fc4 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1068,13 +1068,16 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
new_flags |= VM_WIPEONFORK;
break;
case MADV_KEEPONFORK:
+ if (vma->vm_flags & VM_DROPPABLE)
+ return -EINVAL;
new_flags &= ~VM_WIPEONFORK;
break;
case MADV_DONTDUMP:
new_flags |= VM_DONTDUMP;
break;
case MADV_DODUMP:
- if (!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL)
+ if ((!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL) ||
+ (vma->vm_flags & VM_DROPPABLE))
return -EINVAL;
new_flags &= ~VM_DONTDUMP;
break;
diff --git a/mm/mlock.c b/mm/mlock.c
index 30b51cdea89d..b87b3d8cc9cc 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -485,7 +485,7 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
if (newflags == oldflags || (oldflags & VM_SPECIAL) ||
is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) ||
- vma_is_dax(vma) || vma_is_secretmem(vma))
+ vma_is_dax(vma) || vma_is_secretmem(vma) || (oldflags & VM_DROPPABLE))
/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
goto out;
diff --git a/mm/mmap.c b/mm/mmap.c
index 83b4682ec85c..8aeedeb784c2 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1369,6 +1369,36 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
pgoff = 0;
vm_flags |= VM_SHARED | VM_MAYSHARE;
break;
+ case MAP_DROPPABLE:
+ if (VM_DROPPABLE == VM_NONE)
+ return -ENOTSUPP;
+ /*
+ * A locked or stack area makes no sense to be droppable.
+ *
+ * Also, since droppable pages can just go away at any time
+ * it makes no sense to copy them on fork or dump them.
+ *
+ * And don't attempt to combine with hugetlb for now.
+ */
+ if (flags & (MAP_LOCKED | MAP_HUGETLB))
+ return -EINVAL;
+ if (vm_flags & (VM_GROWSDOWN | VM_GROWSUP))
+ return -EINVAL;
+
+ vm_flags |= VM_DROPPABLE;
+
+ /*
+ * If the pages can be dropped, then it doesn't make
+ * sense to reserve them.
+ */
+ vm_flags |= VM_NORESERVE;
+
+ /*
+ * Likewise, they're volatile enough that they
+ * shouldn't survive forks or coredumps.
+ */
+ vm_flags |= VM_WIPEONFORK | VM_DONTDUMP;
+ fallthrough;
case MAP_PRIVATE:
/*
* Set pgoff according to addr for anon_vma.
diff --git a/mm/rmap.c b/mm/rmap.c
index e8fc5ecb59b2..1f9b5a9cb121 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1397,7 +1397,12 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
VM_BUG_ON_VMA(address < vma->vm_start ||
address + (nr << PAGE_SHIFT) > vma->vm_end, vma);
- __folio_set_swapbacked(folio);
+ /*
+ * VM_DROPPABLE mappings don't swap; instead they're just dropped when
+ * under memory pressure.
+ */
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ __folio_set_swapbacked(folio);
__folio_set_anon(folio, vma, address, true);
if (likely(!folio_test_large(folio))) {
@@ -1841,7 +1846,13 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* plus the rmap(s) (dropped by discard:).
*/
if (ref_count == 1 + map_count &&
- !folio_test_dirty(folio)) {
+ (!folio_test_dirty(folio) ||
+ /*
+ * Unlike MADV_FREE mappings, VM_DROPPABLE
+ * ones can be dropped even if they've
+ * been dirtied.
+ */
+ (vma->vm_flags & VM_DROPPABLE))) {
dec_mm_counter(mm, MM_ANONPAGES);
goto discard;
}
@@ -1851,7 +1862,12 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* discarded. Remap the page to page table.
*/
set_pte_at(mm, address, pvmw.pte, pteval);
- folio_set_swapbacked(folio);
+ /*
+ * Unlike MADV_FREE mappings, VM_DROPPABLE ones
+ * never get swap backed on failure to drop.
+ */
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ folio_set_swapbacked(folio);
ret = false;
page_vma_mapped_walk_done(&pvmw);
break;
diff --git a/tools/include/uapi/linux/mman.h b/tools/include/uapi/linux/mman.h
index a246e11988d5..e89d00528f2f 100644
--- a/tools/include/uapi/linux/mman.h
+++ b/tools/include/uapi/linux/mman.h
@@ -17,6 +17,7 @@
#define MAP_SHARED 0x01 /* Share changes */
#define MAP_PRIVATE 0x02 /* Changes are private */
#define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
+#define MAP_DROPPABLE 0x08 /* Zero memory under memory pressure. */
/*
* Huge page size encoding when MAP_HUGETLB is specified, and a huge page
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 0b9ab987601c..a8beeb43c2b5 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -49,3 +49,4 @@ hugetlb_fault_after_madv
hugetlb_madv_vs_map
mseal_test
seal_elf
+droppable
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 3b49bc3d0a3b..e3e5740e13e1 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -73,6 +73,7 @@ TEST_GEN_FILES += ksm_functional_tests
TEST_GEN_FILES += mdwe_test
TEST_GEN_FILES += hugetlb_fault_after_madv
TEST_GEN_FILES += hugetlb_madv_vs_map
+TEST_GEN_FILES += droppable
ifneq ($(ARCH),arm64)
TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/droppable.c b/tools/testing/selftests/mm/droppable.c
new file mode 100644
index 000000000000..f3d9ecf96890
--- /dev/null
+++ b/tools/testing/selftests/mm/droppable.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#include <assert.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <signal.h>
+#include <sys/mman.h>
+#include <linux/mman.h>
+
+#include "../kselftest.h"
+
+int main(int argc, char *argv[])
+{
+ size_t alloc_size = 134217728;
+ size_t page_size = getpagesize();
+ void *alloc;
+ pid_t child;
+
+ ksft_print_header();
+ ksft_set_plan(1);
+
+ alloc = mmap(0, alloc_size, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_DROPPABLE, -1, 0);
+ assert(alloc != MAP_FAILED);
+ memset(alloc, 'A', alloc_size);
+ for (size_t i = 0; i < alloc_size; i += page_size)
+ assert(*(uint8_t *)(alloc + i));
+
+ child = fork();
+ assert(child >= 0);
+ if (!child) {
+ for (;;)
+ *(char *)malloc(page_size) = 'B';
+ }
+
+ for (bool done = false; !done;) {
+ for (size_t i = 0; i < alloc_size; i += page_size) {
+ if (!*(uint8_t *)(alloc + i)) {
+ done = true;
+ break;
+ }
+ }
+ }
+ kill(child, SIGTERM);
+
+ ksft_test_result_pass("MAP_DROPPABLE: PASS\n");
+ exit(KSFT_PASS);
+}
--
2.45.2
^ permalink raw reply related
* [PATCH v22 0/4] implement getrandom() in vDSO
From: Jason A. Donenfeld @ 2024-07-09 13:05 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86, Linus Torvalds,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
The plan for this series is to take it through my random.git tree for 6.11.
It's cooking in linux-next now.
Changes v21->v22:
- Only add MAP_DROPPABLE, not the other MAP_*s, but make it imply the other
relevant flags.
- Ensure that mlock() and madvise() can't undo MAP_DROPPABLE implications.
- Since MAP_DROPPABLE is generally useful, remove conditional Kconfig
scafolding around it.
- Follow mm/ standards on comment style.
- Base atop latest selftest PR, to avoid merge conflicts in 6.11.
- Update glibc patches.
Changes v20->v21:
- After extensive conversation with Linus, we're nixing the entire
vgetrandom_alloc() syscall, in favor of just exposing the functionality
needed through mmap() and having the kernel communicate to the caller what
arguments/sizes it should pass to mmap(). This simplifies the series
considerably. It also means that the first commit adds some new MAP_*
constants for mmap().
- Separate vDSO selftests out into separate commit.
--------------
Useful links:
- This series:
- https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/
- Glibc patches by Adhemerval and me against glibc-2.39:
- https://git.zx2c4.com/glibc/log/?h=vdso
- In case you're actually interested in the v≤14 design where faults were
non-fatal and instructions were skipped (which I think is more coherent, even
if the implementation is controversial), this lives in my branch here:
- https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/vdso-skip-insn
Note that I'm *not* actually proposing this for upstream at this time. But it
may be of conversational interest.
-------------
Two statements:
1) Userspace wants faster cryptographically secure random numbers of
arbitrary size, big or small.
2) Userspace is currently unable to safely roll its own RNG with the
same security profile as getrandom().
Statement (1) has been debated for years, with arguments ranging from
"we need faster cryptographically secure card shuffling!" to "the only
things that actually need good randomness are keys, which are few and
far between" to "actually, TLS CBC nonces are frequent" and so on. I
don't intend to wade into that debate substantially, except to note that
recently glibc added arc4random(), whose goal is to return a
cryptographically secure uint32_t, and there are real user reports of it
being too slow. So here we are.
Statement (2) is more interesting. The kernel is the nexus of all
entropic inputs that influence the RNG. It is in the best position, and
probably the only position, to decide anything at all about the current
state of the RNG and of its entropy. One of the things it uniquely knows
about is when reseeding is necessary.
For example, when a virtual machine is forked, restored, or duplicated,
it's imparative that the RNG doesn't generate the same outputs. For this
reason, there's a small protocol between hypervisors and the kernel that
indicates this has happened, alongside some ID, which the RNG uses to
immediately reseed, so as not to return the same numbers. Were userspace
to expand a getrandom() seed from time T1 for the next hour, and at some
point T2 < hour, the virtual machine forked, userspace would continue to
provide the same numbers to two (or more) different virtual machines,
resulting in potential cryptographic catastrophe. Something similar
happens on resuming from hibernation (or even suspend), with various
compromise scenarios there in mind.
There's a more general reason why userspace rolling its own RNG from a
getrandom() seed is fraught. There's a lot of attention paid to this
particular Linuxism we have of the RNG being initialized and thus
non-blocking or uninitialized and thus blocking until it is initialized.
These are our Two Big States that many hold to be the holy
differentiating factor between safe and not safe, between
cryptographically secure and garbage. The fact is, however, that the
distinction between these two states is a hand-wavy wishy-washy inexact
approximation. Outside of a few exceptional cases (e.g. a HW RNG is
available), we actually don't really ever know with any rigor at all
when the RNG is safe and ready (nor when it's compromised). We do the
best we can to "estimate" it, but entropy estimation is fundamentally
impossible in the general case. So really, we're just doing guess work,
and hoping it's good and conservative enough. Let's then assume that
there's always some potential error involved in this differentiator.
In fact, under the surface, the RNG is engineered around a different
principle, and that is trying to *use* new entropic inputs regularly and
at the right specific moments in time. For example, close to boot time,
the RNG reseeds itself more often than later. At certain events, like VM
fork, the RNG reseeds itself immediately. The various heuristics for
when the RNG will use new entropy and how often is really a core aspect
of what the RNG has some potential to do decently enough (and something
that will probably continue to improve in the future from random.c's
present set of algorithms). So in your mind, put away the metal
attachment to the Two Big States, which represent an approximation with
a potential margin of error. Instead keep in mind that the RNG's primary
operating heuristic is how often and exactly when it's going to reseed.
So, if userspace takes a seed from getrandom() at point T1, and uses it
for the next hour (or N megabytes or some other meaningless metric),
during that time, potential errors in the Two Big States approximation
are amplified. During that time potential reseeds are being lost,
forgotten, not reflected in the output stream. That's not good.
The simplest statement you could make is that userspace RNGs that expand
a getrandom() seed at some point T1 are nearly always *worse*, in some
way, than just calling getrandom() every time a random number is
desired.
For those reasons, after some discussion on libc-alpha, glibc's
arc4random() now just calls getrandom() on each invocation. That's
trivially safe, and gives us latitude to then make the safe thing faster
without becoming unsafe at our leasure. Card shuffling isn't
particularly fast, however.
How do we rectify this? By putting a safe implementation of getrandom()
in the vDSO, which has access to whatever information a
particular iteration of random.c is using to make its decisions. I use
that careful language of "particular iteration of random.c", because the
set of things that a vDSO getrandom() implementation might need for making
decisions as good as the kernel's will likely change over time. This
isn't just a matter of exporting certain *data* to userspace. We're not
going to commit to a "data API" where the various heuristics used are
exposed, locking in how the kernel works for decades to come, and then
leave it to various userspaces to roll something on top and shoot
themselves in the foot and have all sorts of complexity disasters.
Rather, vDSO getrandom() is supposed to be the *same exact algorithm*
that runs in the kernel, except it's been hoisted into userspace as
much as possible. And so vDSO getrandom() and kernel getrandom() will
always mirror each other hermetically.
API-wise, the vDSO gains this function:
ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags,
void *opaque_state, size_t opaque_len);
The return value and the first 3 arguments are the same as ordinary
getrandom(), while the penultimate argument is a pointer to some state
allocated with the right flags passed to mmap(2), explained below. Were all
five arguments passed to the getrandom syscall, nothing different would happen,
and the functions would have the exact same behavior.
If vgetrandom(NULL, 0, 0, ¶ms, ~0UL) is called, then params gets populated
with information about what flags and prot fields to pass to mmap(2), as well
as how big each state should be, so that the caller can slice up returned
memory from mmap(2) into chunks for passing to vgetrandom().
Libc is expected to allocate a chunk of these on first use, and then
dole them out to threads as they're created, allocating more when
needed.
The interesting meat of the implementation is in lib/vdso/getrandom.c,
as generic C code, and it aims to mainly follow random.c's buffered fast
key erasure logic. Before the RNG is initialized, it falls back to the
syscall. Right now it uses a simple generation counter to make its decisions
on reseeding (though this could be made more extensive over time).
The actual place that has the most work to do is in all of the other
files. Most of the vDSO shared page infrastructure is centered around
gettimeofday, and so the main structs are all in arrays for different
timestamp types, and attached to time namespaces, and so forth. I've
done the best I could to add onto this in an unintrusive way.
In my test results, performance is pretty stellar (around 15x for uint32_t
generation), and it seems to be working. There's an extended example in the
last commit of this series, showing how the syscall and the vDSO function
are meant to be used together.
Cc: linux-crypto@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: x86@kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Hildenbrand <dhildenb@redhat.com>
Jason A. Donenfeld (4):
mm: add MAP_DROPPABLE for designating always lazily freeable mappings
random: introduce generic vDSO getrandom() implementation
x86: vdso: Wire up getrandom() vDSO implementation
selftests/vDSO: add tests for vgetrandom
MAINTAINERS | 4 +
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/entry/vdso/vdso.lds.S | 2 +
arch/x86/entry/vdso/vgetrandom-chacha.S | 178 +++++++++++
arch/x86/entry/vdso/vgetrandom.c | 17 ++
arch/x86/include/asm/vdso/getrandom.h | 55 ++++
arch/x86/include/asm/vdso/vsyscall.h | 2 +
arch/x86/include/asm/vvar.h | 16 +
drivers/char/random.c | 18 +-
fs/proc/task_mmu.c | 1 +
include/linux/mm.h | 7 +
include/trace/events/mmflags.h | 7 +
include/uapi/linux/mman.h | 1 +
include/uapi/linux/random.h | 15 +
include/vdso/datapage.h | 11 +
include/vdso/getrandom.h | 46 +++
lib/vdso/Kconfig | 5 +
lib/vdso/getrandom.c | 251 +++++++++++++++
mm/madvise.c | 5 +-
mm/mlock.c | 2 +-
mm/mmap.c | 30 ++
mm/rmap.c | 22 +-
tools/include/asm/rwonce.h | 0
tools/include/uapi/linux/mman.h | 1 +
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/droppable.c | 53 ++++
tools/testing/selftests/vDSO/.gitignore | 2 +
tools/testing/selftests/vDSO/Makefile | 18 ++
.../testing/selftests/vDSO/vdso_test_chacha.c | 43 +++
.../selftests/vDSO/vdso_test_getrandom.c | 288 ++++++++++++++++++
32 files changed, 1099 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/entry/vdso/vgetrandom-chacha.S
create mode 100644 arch/x86/entry/vdso/vgetrandom.c
create mode 100644 arch/x86/include/asm/vdso/getrandom.h
create mode 100644 include/vdso/getrandom.h
create mode 100644 lib/vdso/getrandom.c
create mode 100644 tools/include/asm/rwonce.h
create mode 100644 tools/testing/selftests/mm/droppable.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_chacha.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_getrandom.c
base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
prerequisite-patch-id: 9a45c4b77033012b2c2cbbec24fd8b2a7a5daf84
prerequisite-patch-id: 8b773921433de1e8b9fd5a8f3d6107258c133c2a
prerequisite-patch-id: afd1b07bd24fe3c93d1fef782ba9064e95d1534c
prerequisite-patch-id: a5cbcafe6072a173a8f20eac5cc7e545be50ae20
prerequisite-patch-id: 59640753e9c60e5d23ede9a20ed5c933a47b3f97
--
2.45.2
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Florian Weimer @ 2024-07-09 10:05 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jeff Xu, Al Viro, Christian Brauner, Kees Cook, Linus Torvalds,
Paul Moore, Theodore Ts'o, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240709.gae4cu4Aiv6s@digikod.net>
* Mickaël Salaün:
>> > If we want to avoid that, we could have an agreed-upon error code which
>> > the LSM can signal that it'll never fail AT_CHECK checks, so we only
>> > have to perform the extra system call once.
>
> I'm not sure to follow. Either we check executable code or we don't,
> but it doesn't make sense to only check some parts (except for migration
> of user space code in a system, which is one purpose of the securebits
> added with the next patch).
>
> The idea with AT_CHECK is to unconditionnaly check executable right the
> same way it is checked when a file is executed. User space can decide
> to check that or not according to its policy (i.e. securebits).
I meant it purely as a performance optimization, to skip future system
calls if we know they won't provide any useful information for this
process. In the grand scheme of things, the extra system call probably
does not matter because we already have to do costly things like mmap.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-07-09 9:18 UTC (permalink / raw)
To: Jeff Xu
Cc: Florian Weimer, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <CALmYWFvA7VPz06Tg8E-R_Jqn2cxMiWPPC6Vhy+vgqnofT0GELg@mail.gmail.com>
On Mon, Jul 08, 2024 at 10:52:36AM -0700, Jeff Xu wrote:
> On Mon, Jul 8, 2024 at 10:33 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Jeff Xu:
> >
> > > On Mon, Jul 8, 2024 at 9:26 AM Florian Weimer <fweimer@redhat.com> wrote:
> > >>
> > >> * Jeff Xu:
> > >>
> > >> > Will dynamic linkers use the execveat(AT_CHECK) to check shared
> > >> > libraries too ? or just the main executable itself.
> > >>
> > >> I expect that dynamic linkers will have to do this for everything they
> > >> map.
> > > Then all the objects (.so, .sh, etc.) will go through the check from
> > > execveat's main to security_bprm_creds_for_exec(), some of them might
> > > be specific for the main executable ?
Yes, we should check every executable code (including seccomp filters)
to get a consistent policy.
What do you mean by "specific for the main executable"?
> >
> > If we want to avoid that, we could have an agreed-upon error code which
> > the LSM can signal that it'll never fail AT_CHECK checks, so we only
> > have to perform the extra system call once.
I'm not sure to follow. Either we check executable code or we don't,
but it doesn't make sense to only check some parts (except for migration
of user space code in a system, which is one purpose of the securebits
added with the next patch).
The idea with AT_CHECK is to unconditionnaly check executable right the
same way it is checked when a file is executed. User space can decide
to check that or not according to its policy (i.e. securebits).
> >
> Right, something like that.
> I would prefer not having AT_CHECK specific code in LSM code as an
> initial goal, if that works, great.
LSMs should not need to change anything, but they are free to implement
new access right according to AT_CHECK.
>
> -Jeff
>
> > Thanks,
> > Florian
> >
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-07-09 2:17 UTC (permalink / raw)
To: David Hildenbrand
Cc: Linus Torvalds, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <75d6c45d-deea-464d-b0fd-b36e5d73b898@redhat.com>
Hi David,
On Mon, Jul 08, 2024 at 10:21:09PM +0200, David Hildenbrand wrote:
> BTW, I was just trying to understand how MADV_FREE + MAP_DROPPABLE would
> behave without any swap space around.
>
> Did you experiment with that?
You mean on a system without any swap configured? That's actually my
primary test environment for this. It behaves as expected: when ram
fills up and the scanner is trying to reclaim what it can,
folio_test_swapbacked(folio) is false, and the memory gets freed. After,
reads fault in a zero page. So it's working as expected.
Jason
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Jeff Xu @ 2024-07-08 22:07 UTC (permalink / raw)
To: Steve Dower
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Luca Boccassi,
Luis Chamberlain, Madhavan T . Venkataraman, Matt Bobrowski,
Matthew Garrett, Matthew Wilcox, Miklos Szeredi, Mimi Zohar,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <ef3281ad-48a5-4316-b433-af285806540d@python.org>
On Mon, Jul 8, 2024 at 2:25 PM Steve Dower <steve.dower@python.org> wrote:
>
> On 08/07/2024 22:15, Jeff Xu wrote:
> > IIUC:
> > CHECK=0, RESTRICT=0: do nothing, current behavior
> > CHECK=1, RESTRICT=0: permissive mode - ignore AT_CHECK results.
> > CHECK=0, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, no exception.
> > CHECK=1, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, except
> > those in the "checked-and-allowed" list.
>
> I had much the same question for Mickaël while working on this.
>
> Essentially, "CHECK=0, RESTRICT=1" means to restrict without checking.
> In the context of a script or macro interpreter, this just means it will
> never interpret any scripts. Non-binary code execution is fully disabled
> in any part of the process that respects these bits.
>
I see, so Mickaël does mean this will block all scripts.
I guess, in the context of dynamic linker, this means: no more .so
loading, even "dlopen" is called by an app ? But this will make the
execve() fail.
> "CHECK=1, RESTRICT=1" means to restrict unless AT_CHECK passes. This
> case is the allow list (or whatever mechanism is being used to determine
> the result of an AT_CHECK check). The actual mechanism isn't the
> business of the script interpreter at all, it just has to refuse to
> execute anything that doesn't pass the check. So a generic interpreter
> can implement a generic mechanism and leave the specifics to whoever
> configures the machine.
>
In the context of dynamic linker. this means:
if .so passed the AT_CHECK, ldopen() can still load it.
If .so fails the AT_CHECK, ldopen() will fail too.
Thanks
-Jeff
> The other two case are more obvious. "CHECK=0, RESTRICT=0" is the
> zero-overhead case, while "CHECK=1, RESTRICT=0" might log, warn, or
> otherwise audit the result of the check, but it won't restrict execution.
>
> Cheers,
> Steve
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Steve Dower @ 2024-07-08 21:25 UTC (permalink / raw)
To: Jeff Xu, Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CALmYWFuVJiRZgB0ye9eR95dvBOigoOVShgS9i_ESjEre-H5pLA@mail.gmail.com>
On 08/07/2024 22:15, Jeff Xu wrote:
> IIUC:
> CHECK=0, RESTRICT=0: do nothing, current behavior
> CHECK=1, RESTRICT=0: permissive mode - ignore AT_CHECK results.
> CHECK=0, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, no exception.
> CHECK=1, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, except
> those in the "checked-and-allowed" list.
I had much the same question for Mickaël while working on this.
Essentially, "CHECK=0, RESTRICT=1" means to restrict without checking.
In the context of a script or macro interpreter, this just means it will
never interpret any scripts. Non-binary code execution is fully disabled
in any part of the process that respects these bits.
"CHECK=1, RESTRICT=1" means to restrict unless AT_CHECK passes. This
case is the allow list (or whatever mechanism is being used to determine
the result of an AT_CHECK check). The actual mechanism isn't the
business of the script interpreter at all, it just has to refuse to
execute anything that doesn't pass the check. So a generic interpreter
can implement a generic mechanism and leave the specifics to whoever
configures the machine.
The other two case are more obvious. "CHECK=0, RESTRICT=0" is the
zero-overhead case, while "CHECK=1, RESTRICT=0" might log, warn, or
otherwise audit the result of the check, but it won't restrict execution.
Cheers,
Steve
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Jeff Xu @ 2024-07-08 21:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240708.quoe8aeSaeRi@digikod.net>
On Mon, Jul 8, 2024 at 11:48 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Mon, Jul 08, 2024 at 10:53:11AM -0700, Jeff Xu wrote:
> > On Mon, Jul 8, 2024 at 9:17 AM Jeff Xu <jeffxu@google.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, Jul 4, 2024 at 12:02 PM Mickaël Salaün <mic@digikod.net> wrote:
> > > >
> > > > These new SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT, and
> > > > their *_LOCKED counterparts are designed to be set by processes setting
> > > > up an execution environment, such as a user session, a container, or a
> > > > security sandbox. Like seccomp filters or Landlock domains, the
> > > > securebits are inherited across proceses.
> > > >
> > > > When SECBIT_SHOULD_EXEC_CHECK is set, programs interpreting code should
> > > > check executable resources with execveat(2) + AT_CHECK (see previous
> > > > patch).
> > > >
> > > > When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
> > > > execution of approved resources, if any (see SECBIT_SHOULD_EXEC_CHECK).
> > > >
> > > Do we need both bits ?
> > > When CHECK is set and RESTRICT is not, the "check fail" executable
> > > will still get executed, so CHECK is for logging ?
> > > Does RESTRICT imply CHECK is set, e.g. What if CHECK=0 and RESTRICT = 1 ?
> > >
> > The intention might be "permissive mode"? if so, consider reuse
> > existing selinux's concept, and still with 2 bits:
> > SECBIT_SHOULD_EXEC_RESTRICT
> > SECBIT_SHOULD_EXEC_RESTRICT_PERMISSIVE
>
> SECBIT_SHOULD_EXEC_CHECK is for user space to check with execveat+AT_CHECK.
>
> SECBIT_SHOULD_EXEC_RESTRICT is for user space to restrict execution by
> default, and potentially allow some exceptions from the list of
> checked-and-allowed files, if SECBIT_SHOULD_EXEC_CHECK is set.
>
> Without SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT is to deny
> any kind of execution/interpretation.
>
Do you mean "deny any kinds of executable/interpretation" or just
those that failed with "AT_CHECK" ( I assume this)?
> With only SECBIT_SHOULD_EXEC_CHECK, user space should just check and log
> any denied access, but ignore them. So yes, it is similar to the
> SELinux's permissive mode.
>
IIUC:
CHECK=0, RESTRICT=0: do nothing, current behavior
CHECK=1, RESTRICT=0: permissive mode - ignore AT_CHECK results.
CHECK=0, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, no exception.
CHECK=1, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, except
those in the "checked-and-allowed" list.
So CHECK is basically trying to form a allowlist?
If there is a need for a allowlist, that is the task of "interruptor
or dynamic linker" to maintain this list, and the list is known in
advance, i.e. not something from execveat(AT_CHECK), and kernel
shouldn't have the knowledge of this allowlist.
Secondly, the concept of allow-list seems to be an attack factor for
me, I would rather it be fully enforced, or permissive mode.
And Check=1 and RESTRICT=1 is less secure than CHECK=0, RESTRICT=1,
this might also be not obvious to dev.
Unless I understood the CHECK wrong.
> This is explained in the next patch as comments.
>
The next patch is a selftest patch, it is better to define them in the
current commit and in the securebits.h.
> The *_LOCKED variants are useful and part of the securebits concept.
>
The locked state is easy to understand.
Thanks
Best regards
-Jeff
> >
> >
> > -Jeff
> >
> >
> >
> >
> > > > For a secure environment, we might also want
> > > > SECBIT_SHOULD_EXEC_CHECK_LOCKED and SECBIT_SHOULD_EXEC_RESTRICT_LOCKED
> > > > to be set. For a test environment (e.g. testing on a fleet to identify
> > > > potential issues), only the SECBIT_SHOULD_EXEC_CHECK* bits can be set to
> > > > still be able to identify potential issues (e.g. with interpreters logs
> > > > or LSMs audit entries).
> > > >
> > > > It should be noted that unlike other security bits, the
> > > > SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT bits are
> > > > dedicated to user space willing to restrict itself. Because of that,
> > > > they only make sense in the context of a trusted environment (e.g.
> > > > sandbox, container, user session, full system) where the process
> > > > changing its behavior (according to these bits) and all its parent
> > > > processes are trusted. Otherwise, any parent process could just execute
> > > > its own malicious code (interpreting a script or not), or even enforce a
> > > > seccomp filter to mask these bits.
> > > >
> > > > Such a secure environment can be achieved with an appropriate access
> > > > control policy (e.g. mount's noexec option, file access rights, LSM
> > > > configuration) and an enlighten ld.so checking that libraries are
> > > > allowed for execution e.g., to protect against illegitimate use of
> > > > LD_PRELOAD.
> > > >
> > > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > > environment variables), but that is outside the scope of the kernel.
> > > >
> > > > The only restriction enforced by the kernel is the right to ptrace
> > > > another process. Processes are denied to ptrace less restricted ones,
> > > > unless the tracer has CAP_SYS_PTRACE. This is mainly a safeguard to
> > > > avoid trivial privilege escalations e.g., by a debugging process being
> > > > abused with a confused deputy attack.
> > > >
> > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > Link: https://lore.kernel.org/r/20240704190137.696169-3-mic@digikod.net
> > > > ---
^ permalink raw reply
* Re: [RFC PATCH v19 0/5] Script execution control (was O_MAYEXEC)
From: Mimi Zohar @ 2024-07-08 20:35 UTC (permalink / raw)
To: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o
Cc: Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
Hi Mickaël,
On Thu, 2024-07-04 at 21:01 +0200, Mickaël Salaün wrote:
> Hi,
>
> The ultimate goal of this patch series is to be able to ensure that
> direct file execution (e.g. ./script.sh) and indirect file execution
> (e.g. sh script.sh) lead to the same result, especially from a security
> point of view.
>
> Overview
> --------
>
> This patch series is a new approach of the initial O_MAYEXEC feature,
> and a revamp of the previous patch series. Taking into account the last
> reviews [1], we now stick to the kernel semantic for file executability.
> One major change is the clear split between access check and policy
> management.
>
> The first patch brings the AT_CHECK flag to execveat(2). The goal is to
> enable user space to check if a file could be executed (by the kernel).
> Unlike stat(2) that only checks file permissions, execveat2(2) +
> AT_CHECK take into account the full context, including mount points
> (noexec), caller's limits, and all potential LSM extra checks (e.g.
> argv, envp, credentials).
>
> The second patch brings two new securebits used to set or get a security
> policy for a set of processes. For this to be meaningful, all
> executable code needs to be trusted. In practice, this means that
> (malicious) users can be restricted to only run scripts provided (and
> trusted) by the system.
>
> [1] https://lore.kernel.org/r/CAHk-=wjPGNLyzeBMWdQu+kUdQLHQugznwY7CvWjmvNW47D5sog@mail.gmail.com
>
> Script execution
> ----------------
>
> One important thing to keep in mind is that the goal of this patch
> series is to get the same security restrictions with these commands:
> * ./script.py
> * python script.py
> * python < script.py
> * python -m script.pyT
This is really needed, but is it the "only" purpose of this patch set or can it
be used to also monitor files the script opens (for read) with the intention of
executing.
>
> However, on secure systems, we should be able to forbid these commands
> because there is no way to reliably identify the origin of the script:
> * xargs -a script.py -d '\r' -- python -c
> * cat script.py | python
> * python
>
> Background
> ----------
>
> Compared to the previous patch series, there is no more dedicated
> syscall nor sysctl configuration. This new patch series only add new
> flags: one for execveat(2) and four for prctl(2).
>
> This kind of script interpreter restriction may already be used in
> hardened systems, which may need to fork interpreters and install
> different versions of the binaries. This mechanism should enable to
> avoid the use of duplicate binaries (and potential forked source code)
> for secure interpreters (e.g. secure Python [2]) by making it possible
> to dynamically enforce restrictions or not.
>
> The ability to control script execution is also required to close a
> major IMA measurement/appraisal interpreter integrity [3].
Definitely. But it isn't limited to controlling script execution, but also
measuring the script. Will it be possible to measure and appraise the indirect
script calls with this patch set?
Mimi
> This new execveat + AT_CHECK should not be confused with the O_EXEC flag
> (for open) which is intended for execute-only, which obviously doesn't
> work for scripts.
>
> I gave a talk about controlling script execution where I explain the
> previous approaches [4]. The design of the WIP RFC I talked about
> changed quite a bit since then.
>
> [2] https://github.com/zooba/spython
> [3] https://lore.kernel.org/lkml/20211014130125.6991-1-zohar@linux.ibm.com/
> [4] https://lssna2023.sched.com/event/1K7bO
>
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: David Hildenbrand @ 2024-07-08 20:26 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linus Torvalds, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <75d6c45d-deea-464d-b0fd-b36e5d73b898@redhat.com>
On 08.07.24 22:21, David Hildenbrand wrote:
> On 08.07.24 16:40, Jason A. Donenfeld wrote:
>> Hi David, Linus,
>>
>> Below is what I understand the suggestions about the UX to be. The full
>> commit is in https://git.zx2c4.com/linux-rng/log/ but here's the part
>> we've been discussing. I've held off on David's suggestion changing
>> "DROPPABLE" to "VOLATILE" to give Linus some time to wake up on the west
>> coast and voice his preference for "DROPPABLE". But the rest is in
>> place.
>>
>> Jason
>>
>> diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
>> index a246e11988d5..e89d00528f2f 100644
>> --- a/include/uapi/linux/mman.h
>> +++ b/include/uapi/linux/mman.h
>> @@ -17,6 +17,7 @@
>> #define MAP_SHARED 0x01 /* Share changes */
>> #define MAP_PRIVATE 0x02 /* Changes are private */
>> #define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
>> +#define MAP_DROPPABLE 0x08 /* Zero memory under memory pressure. */
>>
>> /*
>> * Huge page size encoding when MAP_HUGETLB is specified, and a huge page
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index a77893462b92..cba5bc652fc4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1068,13 +1068,16 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
>> new_flags |= VM_WIPEONFORK;
>> break;
>> case MADV_KEEPONFORK:
>> + if (vma->vm_flags & VM_DROPPABLE)
>> + return -EINVAL;
>> new_flags &= ~VM_WIPEONFORK;
>> break;
>> case MADV_DONTDUMP:
>> new_flags |= VM_DONTDUMP;
>> break;
>> case MADV_DODUMP:
>> - if (!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL)
>> + if ((!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL) ||
>> + (vma->vm_flags & VM_DROPPABLE))
>> return -EINVAL;
>> new_flags &= ~VM_DONTDUMP;
>> break;
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 30b51cdea89d..b87b3d8cc9cc 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -485,7 +485,7 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>>
>> if (newflags == oldflags || (oldflags & VM_SPECIAL) ||
>> is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) ||
>> - vma_is_dax(vma) || vma_is_secretmem(vma))
>> + vma_is_dax(vma) || vma_is_secretmem(vma) || (oldflags & VM_DROPPABLE))
>> /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
>> goto out;
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 83b4682ec85c..b3d38179dd42 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1369,6 +1369,34 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>> pgoff = 0;
>> vm_flags |= VM_SHARED | VM_MAYSHARE;
>> break;
>> + case MAP_DROPPABLE:
>> + /*
>> + * A locked or stack area makes no sense to be droppable.
>> + *
>> + * Also, since droppable pages can just go away at any time
>> + * it makes no sense to copy them on fork or dump them.
>> + *
>> + * And don't attempt to combine with hugetlb for now.
>> + */
>> + if (flags & (MAP_LOCKED | MAP_HUGETLB))
>> + return -EINVAL;
>> + if (vm_flags & (VM_GROWSDOWN | VM_GROWSUP))
>> + return -EINVAL;
>> +
>> + vm_flags |= VM_DROPPABLE;
>> +
>> + /*
>> + * If the pages can be dropped, then it doesn't make
>> + * sense to reserve them.
>> + */
>> + vm_flags |= VM_NORESERVE;
>
> That is certainly interesting. Nothing that we might not be able to
"Nothing" -> "I'll note that" :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: David Hildenbrand @ 2024-07-08 20:24 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, patches, tglx, linux-crypto, linux-api, x86,
Linus Torvalds, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <ZotE3-VwI0SJlM6w@zx2c4.com>
On 08.07.24 03:46, Jason A. Donenfeld wrote:
> Hi David,
Hi Jason,
just catching up on mails here. Most of the stuff is now clear from the
other subthread.
[...]
>>> @@ -1851,7 +1858,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>> * discarded. Remap the page to page table.
>>> */
>>> set_pte_at(mm, address, pvmw.pte, pteval);
>>> - folio_set_swapbacked(folio);
>>> + /* Unlike MADV_FREE mappings, VM_DROPPABLE ones
>>> + * never get swap backed on failure to drop. */
>>> + if (!(vma->vm_flags & VM_DROPPABLE))
>>> + folio_set_swapbacked(folio);
>>> ret = false;
>>> page_vma_mapped_walk_done(&pvmw);
>>> break;
>>
>> A note that in mm/mm-stable, "madvise_free_huge_pmd" exists to optimize
>> MADV_FREE on PMDs. I suspect we'd want to extend that one as well for
>> dropping support, but likely it would also only be a performance
>> improvmeent and not affect functonality if not handled.
>
> That's for doing the freeing of PTEs after the fact, right? If the
> mapping was created, got filled with some data, and then sometime later
> it got MADV_FREE'd, which is the pattern people follow typically with
> MADV_FREE. If we do this as PROT_/MAP_, then that's not a case we need
> to worry about, if I understand this code correctly.
We essentially now have code to handle PMD-mapped THP: instead of first
remapping them using PTEs to then unmap+discard via 512 PTEs (due to
MADV_FREE being set on the folio), we can now simply unmap+discard a
single PMD. So performance wise, this might be interesting for this
mechanism as well (when used in combination with THP).
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: David Hildenbrand @ 2024-07-08 20:21 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linus Torvalds, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <Zov6SZZCKrqmigua@zx2c4.com>
On 08.07.24 16:40, Jason A. Donenfeld wrote:
> Hi David, Linus,
>
> Below is what I understand the suggestions about the UX to be. The full
> commit is in https://git.zx2c4.com/linux-rng/log/ but here's the part
> we've been discussing. I've held off on David's suggestion changing
> "DROPPABLE" to "VOLATILE" to give Linus some time to wake up on the west
> coast and voice his preference for "DROPPABLE". But the rest is in
> place.
>
> Jason
>
> diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
> index a246e11988d5..e89d00528f2f 100644
> --- a/include/uapi/linux/mman.h
> +++ b/include/uapi/linux/mman.h
> @@ -17,6 +17,7 @@
> #define MAP_SHARED 0x01 /* Share changes */
> #define MAP_PRIVATE 0x02 /* Changes are private */
> #define MAP_SHARED_VALIDATE 0x03 /* share + validate extension flags */
> +#define MAP_DROPPABLE 0x08 /* Zero memory under memory pressure. */
>
> /*
> * Huge page size encoding when MAP_HUGETLB is specified, and a huge page
> diff --git a/mm/madvise.c b/mm/madvise.c
> index a77893462b92..cba5bc652fc4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1068,13 +1068,16 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> new_flags |= VM_WIPEONFORK;
> break;
> case MADV_KEEPONFORK:
> + if (vma->vm_flags & VM_DROPPABLE)
> + return -EINVAL;
> new_flags &= ~VM_WIPEONFORK;
> break;
> case MADV_DONTDUMP:
> new_flags |= VM_DONTDUMP;
> break;
> case MADV_DODUMP:
> - if (!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL)
> + if ((!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL) ||
> + (vma->vm_flags & VM_DROPPABLE))
> return -EINVAL;
> new_flags &= ~VM_DONTDUMP;
> break;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 30b51cdea89d..b87b3d8cc9cc 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -485,7 +485,7 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>
> if (newflags == oldflags || (oldflags & VM_SPECIAL) ||
> is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) ||
> - vma_is_dax(vma) || vma_is_secretmem(vma))
> + vma_is_dax(vma) || vma_is_secretmem(vma) || (oldflags & VM_DROPPABLE))
> /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
> goto out;
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 83b4682ec85c..b3d38179dd42 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1369,6 +1369,34 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> pgoff = 0;
> vm_flags |= VM_SHARED | VM_MAYSHARE;
> break;
> + case MAP_DROPPABLE:
> + /*
> + * A locked or stack area makes no sense to be droppable.
> + *
> + * Also, since droppable pages can just go away at any time
> + * it makes no sense to copy them on fork or dump them.
> + *
> + * And don't attempt to combine with hugetlb for now.
> + */
> + if (flags & (MAP_LOCKED | MAP_HUGETLB))
> + return -EINVAL;
> + if (vm_flags & (VM_GROWSDOWN | VM_GROWSUP))
> + return -EINVAL;
> +
> + vm_flags |= VM_DROPPABLE;
> +
> + /*
> + * If the pages can be dropped, then it doesn't make
> + * sense to reserve them.
> + */
> + vm_flags |= VM_NORESERVE;
That is certainly interesting. Nothing that we might not be able to
reclaim these pages reliably in all cases: for example when long-term
pinning them.
In some environments (OVERCOMMIT_NEVER) MAP_NORESERE would never be
effective. I wonder if we want to stick to the same behavior here ...
but in theory I agree that we can set this here unconditionally, it's
just the corner case of "there are ways to prohibit reclaim" that makes
me wonder.
BTW, I was just trying to understand how MADV_FREE + MAP_DROPPABLE would
behave without any swap space around.
Did you experiment with that?
I'm reading can_reclaim_anon_pages(), and I'm wondering how
good/reliable that works when there is no swap configured.
Also, the comment in get_scan_count(): "If we have no swap space, do not
bother scanning anon folios." makes me wonder if some work in that area
is needed.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: David Hildenbrand @ 2024-07-08 20:06 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linus Torvalds, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <Zovv4lzM38EHtnms@zx2c4.com>
On 08.07.24 15:55, Jason A. Donenfeld wrote:
> Hi David,
>
> On Mon, Jul 08, 2024 at 10:11:24AM +0200, David Hildenbrand wrote:
>> The semantics are much more intuitive. No need for separate mmap flags.
>
> Agreed.
>
>> Likely we'll have to adjust mlock() as well. Also, I think we should
>> just bail out with hugetlb as well.
>
> Ack.
>
>> Further, maybe we want to disallow madvise() clearing these flags here,
>> just to be consistent.
>
> Good thinking.
>
>> As a side note, I'll raise that I am not a particular fan of the
>> "droppable" terminology, at least with the "read 0s" approach.
>>
>> From a user perspective, the memory might suddenly lose its state and
>> read as 0s just like volatile memory when it loses power. "dropping
>> pages" sounds more like an implementation detail.
>>
>> Something like MAP_VOLATILE might be more intuitive (similar to the
>> proposed MADV_VOLATILE).
>>
>> But naming is hard, just mentioning to share my thought :)
>
> Naming is hard, but *renaming* is annoying. I like droppable simply
> because that's what I've been calling it in my head. MAP_VOLATILE is
> fine with me though, and seems reasonable enough. So I'll name it that,
> and then please don't change your mind about it later so I won't have to
> rename everything again. :)
:) Nah. But wait with any remaining until more than one person thinks
it's a good idea.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: David Hildenbrand @ 2024-07-08 20:05 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linus Torvalds, linux-kernel, patches, tglx, linux-crypto,
linux-api, x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <ZovwO8KJ5Cv5fa26@zx2c4.com>
On 08.07.24 15:57, Jason A. Donenfeld wrote:
> On Mon, Jul 08, 2024 at 10:23:10AM +0200, David Hildenbrand wrote:
>>> As a side note, I'll raise that I am not a particular fan of the
>>> "droppable" terminology, at least with the "read 0s" approach.
>>>
>>> From a user perspective, the memory might suddenly lose its state and
>>> read as 0s just like volatile memory when it loses power. "dropping
>>> pages" sounds more like an implementation detail.
>>
>> Long story short: it's the hypervisor that could be effectively
>> dropping/zeroing out that memory, not the guest VM. "NONVOLATILE" might
>> be clearer than "DROPPABLE".
>
> Surely you mean "VOLATILE", not "NONVOLATILE", right?
Yes, typo :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [RFC PATCH v19 5/5] samples/should-exec: Add set-should-exec
From: Mimi Zohar @ 2024-07-08 19:40 UTC (permalink / raw)
To: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o
Cc: Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-6-mic@digikod.net>
Hi Mickaël,
On Thu, 2024-07-04 at 21:01 +0200, Mickaël Salaün wrote:
> Add a simple tool to set SECBIT_SHOULD_EXEC_CHECK,
> SECBIT_SHOULD_EXEC_RESTRICT, and their lock counterparts before
> executing a command. This should be useful to easily test against
> script interpreters.
The print_usage() provides the calling syntax. Could you provide an example of
how to use it and what to expect?
thanks,
Mimi
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Kees Cook @ 2024-07-08 19:38 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240705.uch1saeNi6mo@digikod.net>
On Fri, Jul 05, 2024 at 07:53:10PM +0200, Mickaël Salaün wrote:
> On Thu, Jul 04, 2024 at 05:04:03PM -0700, Kees Cook wrote:
> > On Thu, Jul 04, 2024 at 09:01:33PM +0200, Mickaël Salaün wrote:
> > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > > allowed for execution. The main use case is for script interpreters and
> > > dynamic linkers to check execution permission according to the kernel's
> > > security policy. Another use case is to add context to access logs e.g.,
> > > which script (instead of interpreter) accessed a file. As any
> > > executable code, scripts could also use this check [1].
> > >
> > > This is different than faccessat(2) which only checks file access
> > > rights, but not the full context e.g. mount point's noexec, stack limit,
> > > and all potential LSM extra checks (e.g. argv, envp, credentials).
> > > Since the use of AT_CHECK follows the exact kernel semantic as for a
> > > real execution, user space gets the same error codes.
> >
> > Nice! I much prefer this method of going through the exec machinery so
> > we always have a single code path for these kinds of checks.
> >
> > > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > > make sense for the kernel to parse the checked files, look for
> > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > if the format is unknown. Because of that, security_bprm_check() is
> > > never called when AT_CHECK is used.
> >
> > I'd like some additional comments in the code that reminds us that
> > access control checks have finished past a certain point.
>
> Where in the code? Just before the bprm->is_check assignment?
Yeah, that's what I was thinking.
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Mickaël Salaün @ 2024-07-08 18:48 UTC (permalink / raw)
To: Jeff Xu
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CALmYWFsLUhkU5u1NKH8XWvSxbFKFOEq+A_eqLeDsN29xOEAYgg@mail.gmail.com>
On Mon, Jul 08, 2024 at 10:53:11AM -0700, Jeff Xu wrote:
> On Mon, Jul 8, 2024 at 9:17 AM Jeff Xu <jeffxu@google.com> wrote:
> >
> > Hi
> >
> > On Thu, Jul 4, 2024 at 12:02 PM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > These new SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT, and
> > > their *_LOCKED counterparts are designed to be set by processes setting
> > > up an execution environment, such as a user session, a container, or a
> > > security sandbox. Like seccomp filters or Landlock domains, the
> > > securebits are inherited across proceses.
> > >
> > > When SECBIT_SHOULD_EXEC_CHECK is set, programs interpreting code should
> > > check executable resources with execveat(2) + AT_CHECK (see previous
> > > patch).
> > >
> > > When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
> > > execution of approved resources, if any (see SECBIT_SHOULD_EXEC_CHECK).
> > >
> > Do we need both bits ?
> > When CHECK is set and RESTRICT is not, the "check fail" executable
> > will still get executed, so CHECK is for logging ?
> > Does RESTRICT imply CHECK is set, e.g. What if CHECK=0 and RESTRICT = 1 ?
> >
> The intention might be "permissive mode"? if so, consider reuse
> existing selinux's concept, and still with 2 bits:
> SECBIT_SHOULD_EXEC_RESTRICT
> SECBIT_SHOULD_EXEC_RESTRICT_PERMISSIVE
SECBIT_SHOULD_EXEC_CHECK is for user space to check with execveat+AT_CHECK.
SECBIT_SHOULD_EXEC_RESTRICT is for user space to restrict execution by
default, and potentially allow some exceptions from the list of
checked-and-allowed files, if SECBIT_SHOULD_EXEC_CHECK is set.
Without SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT is to deny
any kind of execution/interpretation.
With only SECBIT_SHOULD_EXEC_CHECK, user space should just check and log
any denied access, but ignore them. So yes, it is similar to the
SELinux's permissive mode.
This is explained in the next patch as comments.
The *_LOCKED variants are useful and part of the securebits concept.
>
>
> -Jeff
>
>
>
>
> > > For a secure environment, we might also want
> > > SECBIT_SHOULD_EXEC_CHECK_LOCKED and SECBIT_SHOULD_EXEC_RESTRICT_LOCKED
> > > to be set. For a test environment (e.g. testing on a fleet to identify
> > > potential issues), only the SECBIT_SHOULD_EXEC_CHECK* bits can be set to
> > > still be able to identify potential issues (e.g. with interpreters logs
> > > or LSMs audit entries).
> > >
> > > It should be noted that unlike other security bits, the
> > > SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT bits are
> > > dedicated to user space willing to restrict itself. Because of that,
> > > they only make sense in the context of a trusted environment (e.g.
> > > sandbox, container, user session, full system) where the process
> > > changing its behavior (according to these bits) and all its parent
> > > processes are trusted. Otherwise, any parent process could just execute
> > > its own malicious code (interpreting a script or not), or even enforce a
> > > seccomp filter to mask these bits.
> > >
> > > Such a secure environment can be achieved with an appropriate access
> > > control policy (e.g. mount's noexec option, file access rights, LSM
> > > configuration) and an enlighten ld.so checking that libraries are
> > > allowed for execution e.g., to protect against illegitimate use of
> > > LD_PRELOAD.
> > >
> > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > environment variables), but that is outside the scope of the kernel.
> > >
> > > The only restriction enforced by the kernel is the right to ptrace
> > > another process. Processes are denied to ptrace less restricted ones,
> > > unless the tracer has CAP_SYS_PTRACE. This is mainly a safeguard to
> > > avoid trivial privilege escalations e.g., by a debugging process being
> > > abused with a confused deputy attack.
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20240704190137.696169-3-mic@digikod.net
> > > ---
^ permalink raw reply
* Re: [PATCH] binfmt_elf: Fail execution of shared objects with ELIBEXEC
From: Eric W. Biederman @ 2024-07-08 17:34 UTC (permalink / raw)
To: Florian Weimer
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, linux-mm
In-Reply-To: <878qybet6t.fsf_-_@oldenburg.str.redhat.com>
Florian Weimer <fweimer@redhat.com> writes:
> * Mickaël Salaün:
>
>> On Sat, Jul 06, 2024 at 05:32:12PM +0200, Florian Weimer wrote:
>>> * Mickaël Salaün:
>>>
>>> > On Fri, Jul 05, 2024 at 08:03:14PM +0200, Florian Weimer wrote:
>>> >> * Mickaël Salaün:
>>> >>
>>> >> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
>>> >> > allowed for execution. The main use case is for script interpreters and
>>> >> > dynamic linkers to check execution permission according to the kernel's
>>> >> > security policy. Another use case is to add context to access logs e.g.,
>>> >> > which script (instead of interpreter) accessed a file. As any
>>> >> > executable code, scripts could also use this check [1].
>>> >>
>>> >> Some distributions no longer set executable bits on most shared objects,
>>> >> which I assume would interfere with AT_CHECK probing for shared objects.
>>> >
>>> > A file without the execute permission is not considered as executable by
>>> > the kernel. The AT_CHECK flag doesn't change this semantic. Please
>>> > note that this is just a check, not a restriction. See the next patch
>>> > for the optional policy enforcement.
>>> >
>>> > Anyway, we need to define the policy, and for Linux this is done with
>>> > the file permission bits. So for systems willing to have a consistent
>>> > execution policy, we need to rely on the same bits.
>>>
>>> Yes, that makes complete sense. I just wanted to point out the odd
>>> interaction with the old binutils bug and the (sadly still current)
>>> kernel bug.
>>>
>>> >> Removing the executable bit is attractive because of a combination of
>>> >> two bugs: a binutils wart which until recently always set the entry
>>> >> point address in the ELF header to zero, and the kernel not checking for
>>> >> a zero entry point (maybe in combination with an absent program
>>> >> interpreter) and failing the execve with ELIBEXEC, instead of doing the
>>> >> execve and then faulting at virtual address zero. Removing the
>>> >> executable bit is currently the only way to avoid these confusing
>>> >> crashes, so I understand the temptation.
>>> >
>>> > Interesting. Can you please point to the bug report and the fix? I
>>> > don't see any ELIBEXEC in the kernel.
>>>
>>> The kernel hasn't been fixed yet. I do think this should be fixed, so
>>> that distributions can bring back the executable bit.
>>
>> Can you please point to the mailing list discussion or the bug report?
>
> I'm not sure if this was ever reported upstream as an RFE to fail with
> ELIBEXEC. We have downstream bug report:
>
> Prevent executed .so files with e_entry == 0 from attempting to become
> a process.
> <https://bugzilla.redhat.com/show_bug.cgi?id=2004942>
>
> I've put together a patch which seems to work, see below.
>
> I don't think there's any impact on AT_CHECK with execveat because that
> mode will never get to this point.
>
> Thanks,
> Florian
>
> ---8<-----------------------------------------------------------------
> Subject: binfmt_elf: Fail execution of shared objects with ELIBEXEC
>
> Historically, binutils has used the start of the text segment as the
> entry point if _start was not defined. Executing such files results
> in crashes with random effects, depending on what code resides there.
> However, starting with binutils 2.38, BFD ld uses a zero entry point,
> due to commit 5226a6a892f922ea672e5775c61776830aaf27b7 ("Change the
> linker's heuristic for computing the entry point for binaries so that
> shared libraries default to an entry point of 0."). This means
> that shared objects with zero entry points are becoming more common,
> and it makes sense for the kernel to recognize them and refuse
> to execute them.
>
> For backwards compatibility, if a load segment does not map the ELF
> header at file offset zero, the kernel still proceeds as before, in
> case the file is very non-standard and can actually start executing
> at virtual offset zero.
As written I find the logic of the patch confusing, and slightly wrong.
The program header value e_entry is a virtual address, possibly adjusted
by load_bias. Which makes testing it against the file offset of a
PT_LOAD segment wrong. It needs to test against elf_ppnt->p_vaddr.
I think performing an early sanity check to avoid very confusing crashes
seems sensible (as long as it is inexpensive). This appears inexpensive
enough that we don't care. This code is also before begin_new_exec
so it is early enough to be meaningful.
I think the check should simply test if e_entry is mapped. So a range
check please to see if e_entry falls in a PT_LOAD segment.
Having code start at virtual address 0 is a perfectly fine semantically
and might happen in embedded scenarios.
The program header is not required to be mapped or be first, (AKA
p_offset and p_vaddr can have a somewhat arbitrary relationship) so any
mention of the program header in your logic seems confusing to me.
I think your basic structure will work. Just the first check needs to
check if e_entry is lands inside the virtual address of a PT_LOAD
segment. The second check should just be checking a variable to see if
e_entry was inside any PT_LOAD segment, and there is no interpreter.
Does that make sense?
Eric
>
> Signed-off-by: Florian Weimer <fweimer@redhat.com>
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index a43897b03ce9..ebd7052eb616 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -830,6 +830,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> unsigned long e_entry;
> unsigned long interp_load_addr = 0;
> unsigned long start_code, end_code, start_data, end_data;
> + bool elf_header_mapped = false;
> unsigned long reloc_func_desc __maybe_unused = 0;
> int executable_stack = EXSTACK_DEFAULT;
> struct elfhdr *elf_ex = (struct elfhdr *)bprm->buf;
> @@ -865,6 +866,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
> continue;
> }
>
> + if (elf_ppnt->p_type == PT_LOAD && !elf_ppnt->p_offset)
> + elf_header_mapped = true;
> +
> if (elf_ppnt->p_type != PT_INTERP)
> continue;
>
> @@ -921,6 +925,20 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_ph;
> }
>
> + /*
> + * A zero value for e_entry means that the ELF file has no
> + * entry point. If the ELF header is mapped, this is
> + * guaranteed to crash (often even on the first instruction),
> + * so fail the execve system call instead. (This is most
> + * likely to happen for a shared object.) If the object has a
> + * program interpreter, dealing with the situation is its
> + * responsibility.
> + */
> + if (elf_header_mapped && !elf_ex->e_entry && !interpreter) {
> + retval = -ELIBEXEC;
> + goto out_free_dentry;
> + }
> +
> elf_ppnt = elf_phdata;
> for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++)
> switch (elf_ppnt->p_type) {
^ permalink raw reply
* Re: [PATCH] binfmt_elf: Fail execution of shared objects with ELIBEXEC
From: Florian Weimer @ 2024-07-08 17:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, linux-mm
In-Reply-To: <87cynn3hzv.fsf@email.froward.int.ebiederm.org>
* Eric W. Biederman:
> As written I find the logic of the patch confusing, and slightly wrong.
>
> The program header value e_entry is a virtual address, possibly adjusted
> by load_bias. Which makes testing it against the file offset of a
> PT_LOAD segment wrong. It needs to test against elf_ppnt->p_vaddr.
I think we need to test both against zero, or maybe invert the logic: if
something is mapped at virtual address zero that doesn't come from a
zero file offset, we disable the ELIBEXEC check.
> I think performing an early sanity check to avoid very confusing crashes
> seems sensible (as long as it is inexpensive). This appears inexpensive
> enough that we don't care. This code is also before begin_new_exec
> so it is early enough to be meaningful.
Yeah, it was quite confusing when it was after begin_new_exec because
the ELIBEXEC error is visible under strace, and then the SIGSEGV comes …
> I think the check should simply test if e_entry is mapped. So a range
> check please to see if e_entry falls in a PT_LOAD segment.
It's usually mapped even with e_entry ==0 because the ELF header is
loaded at virtual address zero for ET_DYN using the default linker flags
(and this is the case we care about). With -z noseparate-code, it is
even mapped executable.
> Having code start at virtual address 0 is a perfectly fine semantically
> and might happen in embedded scenarios.
To keep supporting this case, we need to check that the ELF header is at
address zero, because we make a leap of faith and assume it's not really
executable even if it is mapped as such because due to its role in the
file format, it does not contain executable instructions. That's why
the patch is focused on the ELF header.
I could remove all these checks and just return ELIBEXEC for a zero
entry point. I think this is valid based on the ELF specification, but
it may have a backwards compatibility impact.
> The program header is not required to be mapped or be first, (AKA
> p_offset and p_vaddr can have a somewhat arbitrary relationship) so any
> mention of the program header in your logic seems confusing to me.
It's the ELF header.
> I think your basic structure will work. Just the first check needs to
> check if e_entry is lands inside the virtual address of a PT_LOAD
> segment. The second check should just be checking a variable to see if
> e_entry was inside any PT_LOAD segment, and there is no interpreter.
I think the range check doesn't help here. Just checking p_vaddr for
zero in addition to p_offset should be sufficient. If you agree, can
test and send an updated patch.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Jeff Xu @ 2024-07-08 17:53 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CALmYWFscz5W6xSXD-+dimzbj=TykNJEDa0m5gvBx93N-J+3nKA@mail.gmail.com>
On Mon, Jul 8, 2024 at 9:17 AM Jeff Xu <jeffxu@google.com> wrote:
>
> Hi
>
> On Thu, Jul 4, 2024 at 12:02 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > These new SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT, and
> > their *_LOCKED counterparts are designed to be set by processes setting
> > up an execution environment, such as a user session, a container, or a
> > security sandbox. Like seccomp filters or Landlock domains, the
> > securebits are inherited across proceses.
> >
> > When SECBIT_SHOULD_EXEC_CHECK is set, programs interpreting code should
> > check executable resources with execveat(2) + AT_CHECK (see previous
> > patch).
> >
> > When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
> > execution of approved resources, if any (see SECBIT_SHOULD_EXEC_CHECK).
> >
> Do we need both bits ?
> When CHECK is set and RESTRICT is not, the "check fail" executable
> will still get executed, so CHECK is for logging ?
> Does RESTRICT imply CHECK is set, e.g. What if CHECK=0 and RESTRICT = 1 ?
>
The intention might be "permissive mode"? if so, consider reuse
existing selinux's concept, and still with 2 bits:
SECBIT_SHOULD_EXEC_RESTRICT
SECBIT_SHOULD_EXEC_RESTRICT_PERMISSIVE
-Jeff
> > For a secure environment, we might also want
> > SECBIT_SHOULD_EXEC_CHECK_LOCKED and SECBIT_SHOULD_EXEC_RESTRICT_LOCKED
> > to be set. For a test environment (e.g. testing on a fleet to identify
> > potential issues), only the SECBIT_SHOULD_EXEC_CHECK* bits can be set to
> > still be able to identify potential issues (e.g. with interpreters logs
> > or LSMs audit entries).
> >
> > It should be noted that unlike other security bits, the
> > SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT bits are
> > dedicated to user space willing to restrict itself. Because of that,
> > they only make sense in the context of a trusted environment (e.g.
> > sandbox, container, user session, full system) where the process
> > changing its behavior (according to these bits) and all its parent
> > processes are trusted. Otherwise, any parent process could just execute
> > its own malicious code (interpreting a script or not), or even enforce a
> > seccomp filter to mask these bits.
> >
> > Such a secure environment can be achieved with an appropriate access
> > control policy (e.g. mount's noexec option, file access rights, LSM
> > configuration) and an enlighten ld.so checking that libraries are
> > allowed for execution e.g., to protect against illegitimate use of
> > LD_PRELOAD.
> >
> > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > environment variables), but that is outside the scope of the kernel.
> >
> > The only restriction enforced by the kernel is the right to ptrace
> > another process. Processes are denied to ptrace less restricted ones,
> > unless the tracer has CAP_SYS_PTRACE. This is mainly a safeguard to
> > avoid trivial privilege escalations e.g., by a debugging process being
> > abused with a confused deputy attack.
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20240704190137.696169-3-mic@digikod.net
> > ---
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > include/uapi/linux/securebits.h | 56 ++++++++++++++++++++++++++++-
> > security/commoncap.c | 63 ++++++++++++++++++++++++++++-----
> > 2 files changed, 110 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > index d6d98877ff1a..3fdb0382718b 100644
> > --- a/include/uapi/linux/securebits.h
> > +++ b/include/uapi/linux/securebits.h
> > @@ -52,10 +52,64 @@
> > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> >
> > +/*
> > + * When SECBIT_SHOULD_EXEC_CHECK is set, a process should check all executable
> > + * files with execveat(2) + AT_CHECK. However, such check should only be
> > + * performed if all to-be-executed code only comes from regular files. For
> > + * instance, if a script interpreter is called with both a script snipped as
> > + * argument and a regular file, the interpreter should not check any file.
> > + * Doing otherwise would mislead the kernel to think that only the script file
> > + * is being executed, which could for instance lead to unexpected permission
> > + * change and break current use cases.
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_SHOULD_EXEC_CHECK_LOCKED bit should also be set.
> > + *
> > + * Ptracing another process is deny if the tracer has SECBIT_SHOULD_EXEC_CHECK
> > + * but not the tracee. SECBIT_SHOULD_EXEC_CHECK_LOCKED also checked.
> > + */
> > +#define SECURE_SHOULD_EXEC_CHECK 8
> > +#define SECURE_SHOULD_EXEC_CHECK_LOCKED 9 /* make bit-8 immutable */
> > +
> > +#define SECBIT_SHOULD_EXEC_CHECK (issecure_mask(SECURE_SHOULD_EXEC_CHECK))
> > +#define SECBIT_SHOULD_EXEC_CHECK_LOCKED \
> > + (issecure_mask(SECURE_SHOULD_EXEC_CHECK_LOCKED))
> > +
> > +/*
> > + * When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
> > + * execution of approved files, if any (see SECBIT_SHOULD_EXEC_CHECK). For
> > + * instance, script interpreters called with a script snippet as argument
> > + * should always deny such execution if SECBIT_SHOULD_EXEC_RESTRICT is set.
> > + * However, if a script interpreter is called with both
> > + * SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT, they should
> > + * interpret the provided script files if no unchecked code is also provided
> > + * (e.g. directly as argument).
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_SHOULD_EXEC_RESTRICT_LOCKED bit should also be set.
> > + *
> > + * Ptracing another process is deny if the tracer has
> > + * SECBIT_SHOULD_EXEC_RESTRICT but not the tracee.
> > + * SECBIT_SHOULD_EXEC_RESTRICT_LOCKED is also checked.
> > + */
> > +#define SECURE_SHOULD_EXEC_RESTRICT 10
> > +#define SECURE_SHOULD_EXEC_RESTRICT_LOCKED 11 /* make bit-8 immutable */
> > +
> > +#define SECBIT_SHOULD_EXEC_RESTRICT (issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
> > +#define SECBIT_SHOULD_EXEC_RESTRICT_LOCKED \
> > + (issecure_mask(SECURE_SHOULD_EXEC_RESTRICT_LOCKED))
> > +
> > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > issecure_mask(SECURE_KEEP_CAPS) | \
> > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > + issecure_mask(SECURE_SHOULD_EXEC_CHECK) | \
> > + issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
> > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> >
> > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_SHOULD_EXEC_CHECK) | \
> > + issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
> > +
> > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 162d96b3a676..34b4493e2a69 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -117,6 +117,33 @@ int cap_settime(const struct timespec64 *ts, const struct timezone *tz)
> > return 0;
> > }
> >
> > +static bool ptrace_secbits_allowed(const struct cred *tracer,
> > + const struct cred *tracee)
> > +{
> > + const unsigned long tracer_secbits = SECURE_ALL_UNPRIVILEGED &
> > + tracer->securebits;
> > + const unsigned long tracee_secbits = SECURE_ALL_UNPRIVILEGED &
> > + tracee->securebits;
> > + /* Ignores locking of unset secure bits (cf. SECURE_ALL_LOCKS). */
> > + const unsigned long tracer_locked = (tracer_secbits << 1) &
> > + tracer->securebits;
> > + const unsigned long tracee_locked = (tracee_secbits << 1) &
> > + tracee->securebits;
> > +
> > + /* The tracee must not have less constraints than the tracer. */
> > + if ((tracer_secbits | tracee_secbits) != tracee_secbits)
> > + return false;
> > +
> > + /*
> > + * Makes sure that the tracer's locks for restrictions are the same for
> > + * the tracee.
> > + */
> > + if ((tracer_locked | tracee_locked) != tracee_locked)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > /**
> > * cap_ptrace_access_check - Determine whether the current process may access
> > * another
> > @@ -146,7 +173,8 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
> > else
> > caller_caps = &cred->cap_permitted;
> > if (cred->user_ns == child_cred->user_ns &&
> > - cap_issubset(child_cred->cap_permitted, *caller_caps))
> > + cap_issubset(child_cred->cap_permitted, *caller_caps) &&
> > + ptrace_secbits_allowed(cred, child_cred))
> > goto out;
> > if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
> > goto out;
> > @@ -178,7 +206,8 @@ int cap_ptrace_traceme(struct task_struct *parent)
> > cred = __task_cred(parent);
> > child_cred = current_cred();
> > if (cred->user_ns == child_cred->user_ns &&
> > - cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
> > + cap_issubset(child_cred->cap_permitted, cred->cap_permitted) &&
> > + ptrace_secbits_allowed(cred, child_cred))
> > goto out;
> > if (has_ns_capability(parent, child_cred->user_ns, CAP_SYS_PTRACE))
> > goto out;
> > @@ -1302,21 +1331,39 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > & (old->securebits ^ arg2)) /*[1]*/
> > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > - || (cap_capable(current_cred(),
> > - current_cred()->user_ns,
> > - CAP_SETPCAP,
> > - CAP_OPT_NONE) != 0) /*[4]*/
> > /*
> > * [1] no changing of bits that are locked
> > * [2] no unlocking of locks
> > * [3] no setting of unsupported bits
> > - * [4] doing anything requires privilege (go read about
> > - * the "sendmail capabilities bug")
> > */
> > )
> > /* cannot change a locked bit */
> > return -EPERM;
> >
> > + /*
> > + * Doing anything requires privilege (go read about the
> > + * "sendmail capabilities bug"), except for unprivileged bits.
> > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > + * restrictions enforced by the kernel but by user space on
> > + * itself. The kernel is only in charge of protecting against
> > + * privilege escalation with ptrace protections.
> > + */
> > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > + const unsigned long unpriv_and_locks =
> > + SECURE_ALL_UNPRIVILEGED |
> > + SECURE_ALL_UNPRIVILEGED << 1;
> > + const unsigned long changed = old->securebits ^ arg2;
> > +
> > + /* For legacy reason, denies non-change. */
> > + if (!changed)
> > + return -EPERM;
> > +
> > + /* Denies privileged changes. */
> > + if (changed & ~unpriv_and_locks)
> > + return -EPERM;
> > + }
> > +
> > new = prepare_creds();
> > if (!new)
> > return -ENOMEM;
> > --
> > 2.45.2
> >
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-07-08 17:52 UTC (permalink / raw)
To: Florian Weimer
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <87r0c3dc1c.fsf@oldenburg.str.redhat.com>
On Mon, Jul 8, 2024 at 10:33 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Jeff Xu:
>
> > On Mon, Jul 8, 2024 at 9:26 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Jeff Xu:
> >>
> >> > Will dynamic linkers use the execveat(AT_CHECK) to check shared
> >> > libraries too ? or just the main executable itself.
> >>
> >> I expect that dynamic linkers will have to do this for everything they
> >> map.
> > Then all the objects (.so, .sh, etc.) will go through the check from
> > execveat's main to security_bprm_creds_for_exec(), some of them might
> > be specific for the main executable ?
>
> If we want to avoid that, we could have an agreed-upon error code which
> the LSM can signal that it'll never fail AT_CHECK checks, so we only
> have to perform the extra system call once.
>
Right, something like that.
I would prefer not having AT_CHECK specific code in LSM code as an
initial goal, if that works, great.
-Jeff
> Thanks,
> Florian
>
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Florian Weimer @ 2024-07-08 17:33 UTC (permalink / raw)
To: Jeff Xu
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <CALmYWFvkUnevm=npBeaZVkK_PXm=A8MjgxFXkASnERxoMyhYBg@mail.gmail.com>
* Jeff Xu:
> On Mon, Jul 8, 2024 at 9:26 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Jeff Xu:
>>
>> > Will dynamic linkers use the execveat(AT_CHECK) to check shared
>> > libraries too ? or just the main executable itself.
>>
>> I expect that dynamic linkers will have to do this for everything they
>> map.
> Then all the objects (.so, .sh, etc.) will go through the check from
> execveat's main to security_bprm_creds_for_exec(), some of them might
> be specific for the main executable ?
If we want to avoid that, we could have an agreed-upon error code which
the LSM can signal that it'll never fail AT_CHECK checks, so we only
have to perform the extra system call once.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-07-08 17:05 UTC (permalink / raw)
To: Jeff Xu
Cc: Florian Weimer, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <CALmYWFvkUnevm=npBeaZVkK_PXm=A8MjgxFXkASnERxoMyhYBg@mail.gmail.com>
On Mon, Jul 08, 2024 at 09:40:45AM -0700, Jeff Xu wrote:
> On Mon, Jul 8, 2024 at 9:26 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Jeff Xu:
> >
> > > Will dynamic linkers use the execveat(AT_CHECK) to check shared
> > > libraries too ? or just the main executable itself.
> >
> > I expect that dynamic linkers will have to do this for everything they
> > map.
Correct, that would enable to safely handle LD_PRELOAD for instance.
> Then all the objects (.so, .sh, etc.) will go through the check from
> execveat's main to security_bprm_creds_for_exec(), some of them might
> be specific for the main executable ?
> e.g. ChromeOS uses security_bprm_creds_for_exec to block executable
> memfd [1], applying this means automatically extending the block to
> the .so object.
That's a good example of how this AT_CHECK check makes sense.
Landlock will probably get a similar (optional) restriction too:
https://github.com/landlock-lsm/linux/issues/37
>
> I'm not sure if other LSMs need to be updated ? e.g. will SELINUX
> check for .so with its process transaction policy ?
LSM should not need to be updated with this patch series. However,
systems/components/containers enabling this new check should make sure
it works with their current policy.
>
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3834992
>
> -Jeff
>
>
> > Usually, that does not include the maim program, but this can
> > happen with explicit loader invocations (“ld.so /bin/true”).
> >
> > Thanks,
> > Florian
> >
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-07-08 16:40 UTC (permalink / raw)
To: Florian Weimer
Cc: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Eric Biggers,
Eric Chiang, Fan Wu, Geert Uytterhoeven, James Morris, Jan Kara,
Jann Horn, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <87ed83etpk.fsf@oldenburg.str.redhat.com>
On Mon, Jul 8, 2024 at 9:26 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Jeff Xu:
>
> > Will dynamic linkers use the execveat(AT_CHECK) to check shared
> > libraries too ? or just the main executable itself.
>
> I expect that dynamic linkers will have to do this for everything they
> map.
Then all the objects (.so, .sh, etc.) will go through the check from
execveat's main to security_bprm_creds_for_exec(), some of them might
be specific for the main executable ?
e.g. ChromeOS uses security_bprm_creds_for_exec to block executable
memfd [1], applying this means automatically extending the block to
the .so object.
I'm not sure if other LSMs need to be updated ? e.g. will SELINUX
check for .so with its process transaction policy ?
[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3834992
-Jeff
> Usually, that does not include the maim program, but this can
> happen with explicit loader invocations (“ld.so /bin/true”).
>
> Thanks,
> Florian
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox