* [PATCH 5/6] landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is disabled.
From: Justin Suess @ 2025-11-20 22:23 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251120222346.1157004-1-utilityemal77@gmail.com>
This was necessary when fixing the no inherit patch in the implementation of kunit tests.
When compiled without the audit flag enabled, kunit tests would fail to compile because of
the missing quiet flag field. This fixes this issue.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
security/landlock/audit.h | 3 ++-
security/landlock/fs.c | 4 +++-
security/landlock/net.h | 2 +-
security/landlock/ruleset.c | 2 ++
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/security/landlock/audit.h b/security/landlock/audit.h
index 950365cd223d..1a5a9cd28cf4 100644
--- a/security/landlock/audit.h
+++ b/security/landlock/audit.h
@@ -69,7 +69,8 @@ landlock_log_drop_domain(const struct landlock_hierarchy *const hierarchy)
static inline void
landlock_log_denial(const struct landlock_cred_security *const subject,
- const struct landlock_request *const request)
+ const struct landlock_request *const request,
+ const struct collected_rule_flags rule_flags)
{
}
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index ebeee080ea7a..20a24f3988bc 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -2094,7 +2094,7 @@ static int hook_path_unlink(const struct path *const dir,
const struct landlock_cred_security *const subject =
landlock_get_applicable_subject(current_cred(), any_fs, NULL);
int err;
-
+
if (subject) {
err = deny_no_inherit_topology_change(subject, dentry);
if (err)
@@ -2155,6 +2155,7 @@ get_required_file_open_access(const struct file *const file)
static void build_check_file_security(void)
{
+#ifdef CONFIG_AUDIT
const struct landlock_file_security file_sec = {
.quiet_optional_accesses = ~0,
.fown_layer = ~0,
@@ -2171,6 +2172,7 @@ static void build_check_file_security(void)
__const_hweight64(_LANDLOCK_ACCESS_FS_OPTIONAL));
/* Makes sure all layers can be identified. */
BUILD_BUG_ON(file_sec.fown_layer < LANDLOCK_MAX_NUM_LAYERS - 1);
+#endif /* CONFIG_AUDIT */
}
static int hook_file_alloc_security(struct file *const file)
diff --git a/security/landlock/net.h b/security/landlock/net.h
index 799cedd5d0b7..72c47f4d6803 100644
--- a/security/landlock/net.h
+++ b/security/landlock/net.h
@@ -25,7 +25,7 @@ static inline void landlock_add_net_hooks(void)
static inline int
landlock_append_net_rule(struct landlock_ruleset *const ruleset, const u16 port,
- access_mask_t access_rights)
+ access_mask_t access_rights, const int flags)
{
return -EAFNOSUPPORT;
}
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 63aa420ab593..5d190d6779da 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -660,7 +660,9 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
if (err)
return ERR_PTR(err);
+#ifdef CONFIG_AUDIT
new_dom->hierarchy->quiet_masks = ruleset->quiet_masks;
+#endif /* CONFIG_AUDIT */
return no_free_ptr(new_dom);
}
--
2.51.2
^ permalink raw reply related
* [PATCH 6/6] landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-20 22:23 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251120222346.1157004-1-utilityemal77@gmail.com>
Add a unit test for rule_flag collection, ensuring that access masks are properly
propagated with the flags.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
security/landlock/ruleset.c | 85 +++++++++++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 5d190d6779da..c407ec264207 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -22,6 +22,7 @@
#include <linux/spinlock.h>
#include <linux/workqueue.h>
#include <uapi/linux/landlock.h>
+#include <kunit/test.h>
#include "access.h"
#include "audit.h"
@@ -835,3 +836,87 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
}
return handled_accesses;
}
+
+#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
+
+static void test_unmask_layers_no_inherit(struct kunit *const test)
+{
+ struct landlock_rule *rule;
+ layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS];
+ struct collected_rule_flags rule_flags;
+ const access_mask_t access_request = BIT_ULL(0) | BIT_ULL(1);
+ const layer_mask_t layers_initialized = BIT_ULL(0) | BIT_ULL(1);
+ size_t i;
+
+ rule = kzalloc(struct_size(rule, layers, 2), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, rule);
+
+ rule->num_layers = 2;
+
+ /* Layer 1: allows access 0, no_inherit */
+ rule->layers[0].level = 1;
+ rule->layers[0].access = BIT_ULL(0);
+ rule->layers[0].flags.no_inherit = 1;
+
+ /* Layer 2: allows access 1 */
+ rule->layers[1].level = 2;
+ rule->layers[1].access = BIT_ULL(1);
+
+ /* Case 1: No rule_flags provided (should behave normally) */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), NULL);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* Case 2: rule_flags provided, no existing no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* rule_flags should collect no_inherit from layer 1 */
+ KUNIT_EXPECT_EQ(test, rule_flags.no_inherit_masks, (layer_mask_t)BIT_ULL(0));
+
+ /* Case 3: rule_flags provided, layer 1 is masked by no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+ rule_flags.no_inherit_masks = BIT_ULL(0); /* Mask layer 1 */
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should NOT be unmasked by layer 1 because it is skipped */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized);
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ kfree(rule);
+}
+
+static struct kunit_case ruleset_test_cases[] = {
+ KUNIT_CASE(test_unmask_layers_no_inherit),
+ {}
+};
+
+static struct kunit_suite ruleset_test_suite = {
+ .name = "landlock_ruleset",
+ .test_cases = ruleset_test_cases,
+};
+
+kunit_test_suite(ruleset_test_suite);
+
+#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
--
2.51.2
^ permalink raw reply related
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Mimi Zohar @ 2025-11-20 22:58 UTC (permalink / raw)
To: Anirudh Venkataramanan, linux-integrity
Cc: Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <20251119213359.39397-1-anirudhve@linux.microsoft.com>
On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
>
>
> 2. The kernel uses the userspace supplied PCR values to trim the IMA
> measurements list at a specific point, and so these are referred to as
> "trim-to PCR values" in this context.
>
> Note that the kernel doesn't really understand what these userspace
> provided PCR values mean or what IMA event they correspond to, and so
> it does its own IMA event replay till either the replayed PCR values
> match with the userspace provided ones, or it runs out of events.
>
> If a match is found, the kernel can proceed with trimming the IMA
> measurements list. This is done in two steps, to keep locking context
> minimal.
>
> step 1: Find and return the list entry (as a count from head) of exact
> match. This does not lock the measurements list mutex, ensuring
> new events can be appended to the log.
>
> step 2: Lock the measurements list mutex and trim the measurements list
> at the previously identified list entry.
>
> If the trim is successful, the trim-to PCR values are saved as "starting
> PCR values". The next time userspace wants to replay the IMA event log,
> it will use the starting PCR values as the base for the IMA event log
> replay.
Depending on the IMA policy, the IMA measurement list may contain integrity
violations, such as "ToM/ToU" (Time of Measurement/Time of Use) or "open-
writer". Either the userspace supplied PCR values will not match any measurement
record or the PCR values will be "fixed" to match the well known violation hash
value extended into the TPM. Depending on how the userspace PCR values will
subsequently be used, saving the "fixed" PCR values could result in whitewashing
the integrity of the measurement list.
--
thanks,
Mimi
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-20 23:50 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <87h5uoxw06.fsf_-_@email.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> Instead of computing the new cred before we pass the point of no
> return compute the new cred just before we use it.
>
> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>
> I am not certain why we wanted to compute the cred for the new
> executable so early. Perhaps I missed something but I did not see any
> common errors being signaled. So I don't think we loose anything by
> computing the new cred later.
I should add that the permission checks happen in open_exec,
everything that follows credential wise is just about representing in
struct cred the credentials the new executable will have.
So I am really at a loss why we have had this complicated way of
computing of computed the credentials all of these years full of
time of check to time of use problems.
Eric
^ permalink raw reply
* Re: [PATCH v4 02/35] compiler-context-analysis: Add infrastructure for Context Analysis with Clang
From: Marco Elver @ 2025-11-20 23:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Bart Van Assche,
Christoph Hellwig, Dmitry Vyukov, Eric Dumazet,
Frederic Weisbecker, Greg Kroah-Hartman, Herbert Xu, Ian Rogers,
Jann Horn, Joel Fernandes, Johannes Berg, Jonathan Corbet,
Josh Triplett, Justin Stitt, Kees Cook, Kentaro Takeda,
Lukas Bulwahn, Mark Rutland, Mathieu Desnoyers, Miguel Ojeda,
Nathan Chancellor, Neeraj Upadhyay, Nick Desaulniers,
Steven Rostedt, Tetsuo Handa, Thomas Gleixner, Thomas Graf,
Uladzislau Rezki, Waiman Long, kasan-dev, linux-crypto, linux-doc,
linux-kbuild, linux-kernel, linux-mm, linux-security-module,
linux-sparse, linux-wireless, llvm, rcu
In-Reply-To: <CAHk-=whyKteNtcLON-gScv6tu8ssvKWdNw-k371ufDrjOv374g@mail.gmail.com>
On Thu, Nov 20, 2025 at 10:14AM -0800, Linus Torvalds wrote:
> On Thu, 20 Nov 2025 at 07:13, Marco Elver <elver@google.com> wrote:
[..]
> > +#if defined(WARN_CONTEXT_ANALYSIS)
>
> Note the 400+ added lines to this header...
>
[..]
> Please let's *not* do it this way, where the header contents basically
> get enabled or not based on a compiler flag, but then everybody
> includes this 400+ line file whether they need it or not.
Note, there are a good amount of kernel-doc comments in there, so we
have 125 real code lines.
% cloc include/linux/compiler-context-analysis.h
1 text file.
1 unique file.
0 files ignored.
github.com/AlDanial/cloc v 2.06 T=0.01 s (97.1 files/s, 41646.9 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C/C++ Header 1 37 267 125
-------------------------------------------------------------------------------
> Can we please just make the header file *itself* not have any
> conditionals, and what happens is that the header file is included (or
> not) using a pattern something like
>
> -include $(srctree)/include/linux/$(context-analysis-header)
>
> instead.
>
> IOW, we'd have three different header files entirely: the "no context
> analysis", the "sparse" and the "clang context analysis" header, and
> instead of having a "-DWARN_CONTEXT_ANALYSIS" define, we'd just
> include the appropriate header automatically.
>
> We already use that "-include" pattern for <linux/kconfig.h> and
> <linux/compiler-version.h>. It's probably what we should have done for
> <linux/compiler.h> and friends too.
>
> The reason I react to things like this is that I've actually seen just
> the parsing of header files being a surprisingly big cost in build
> times. People think that optimizations are expensive, and yes, some of
> them really are, but when a lot of the code we parse is never actually
> *used*, but just hangs out in header files that gets included by
> everybody, the parsing overhead tends to be noticeable. There's a
> reason why most C compilers end up integrating the C pre-processor: it
> avoids parsing and tokenizing things multiple times.
>
> The other reason is that I often use "git grep" for looking up
> definitions of things, and when there are multiple definitions of the
> same thing, I actually find it much more informative when they are in
> two different files than when I see two different definitions (or
> declarations) in the same file and then I have to go look at what the
> #ifdef condition is. In contrast, when it's something where there are
> per-architecture definitions, you *see* that, because the grep results
> come from different header files.
>
> I dunno. This is not a huge deal, but I do think that it would seem to
> be much simpler and more straightforward to treat this as a kind of "N
> different baseline header files" than as "include this one header file
> in everything, and then we'll have #ifdef's for the configuration".
>
> Particularly when that config is not even a global config, but a per-file one.
>
> Hmm? Maybe there's some reason why this suggestion is very
> inconvenient, but please at least consider it.
Fair points; I gave this a shot, as a patch on top so we can skip the
Sparse version.
Reduced version below:
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C/C++ Header 1 26 189 80
-------------------------------------------------------------------------------
My suspicion (or I'm doing it wrong): there really isn't all that much
we can conditionally -include, because we need at least the no-op stubs
everywhere regardless because of annotations provided by common headers
(spinlock, mutex, rcu, etc. etc.).
If we assume that in the common case we need the no-op macros
everywhere, thus every line in <linux/compiler-context-analysis.h> is
required in the common case with the below version, the below experiment
should be be close to what we can achieve.
However, it might still be worthwhile for the code organization aspect?
Thoughts?
Thanks,
-- Marco
------ >8 ------
From: Marco Elver <elver@google.com>
Date: Thu, 20 Nov 2025 22:37:52 +0100
Subject: [PATCH] compiler-context-analysis: Move Clang definitions to separate
header
In the interest of improving compile-times, it makes sense to move the
conditionally enabled definitions when the analysis is enabled to a
separate file and include it only with -include.
A very unscientific comparison, on a system with 72 CPUs; before:
125.67 wallclock secs = ( 5681.04 usr secs + 367.63 sys secs / 4815.83% CPU )
After:
125.61 wallclock secs = ( 5684.80 usr secs + 366.53 sys secs / 4817.95% CPU )
[ Work in progress - with this version, there is no measurable
difference in compile times. ]
Signed-off-by: Marco Elver <elver@google.com>
---
Documentation/dev-tools/context-analysis.rst | 10 +-
.../linux/compiler-context-analysis-clang.h | 144 ++++++++++++++++++
include/linux/compiler-context-analysis.h | 136 +----------------
scripts/Makefile.context-analysis | 3 +-
4 files changed, 153 insertions(+), 140 deletions(-)
create mode 100644 include/linux/compiler-context-analysis-clang.h
diff --git a/Documentation/dev-tools/context-analysis.rst b/Documentation/dev-tools/context-analysis.rst
index e53f089d0c52..71b9c5e57eb4 100644
--- a/Documentation/dev-tools/context-analysis.rst
+++ b/Documentation/dev-tools/context-analysis.rst
@@ -99,10 +99,7 @@ Keywords
~~~~~~~~
.. kernel-doc:: include/linux/compiler-context-analysis.h
- :identifiers: context_guard_struct
- token_context_guard token_context_guard_instance
- __guarded_by __pt_guarded_by
- __must_hold
+ :identifiers: __must_hold
__must_not_hold
__acquires
__cond_acquires
@@ -119,6 +116,11 @@ Keywords
__acquire_shared_ret
context_unsafe
__context_unsafe
+
+.. kernel-doc:: include/linux/compiler-context-analysis-clang.h
+ :identifiers: __guarded_by __pt_guarded_by
+ context_guard_struct
+ token_context_guard token_context_guard_instance
disable_context_analysis enable_context_analysis
.. note::
diff --git a/include/linux/compiler-context-analysis-clang.h b/include/linux/compiler-context-analysis-clang.h
new file mode 100644
index 000000000000..534a41a25596
--- /dev/null
+++ b/include/linux/compiler-context-analysis-clang.h
@@ -0,0 +1,144 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Macros and attributes for compiler-based static context analysis that map to
+ * Clang's "Thread Safety Analysis".
+ */
+
+#ifndef _LINUX_COMPILER_CONTEXT_ANALYSIS_CLANG_H
+#define _LINUX_COMPILER_CONTEXT_ANALYSIS_CLANG_H
+
+#ifndef WARN_CONTEXT_ANALYSIS
+#error "This header should not be included"
+#endif
+
+/*
+ * These attributes define new context guard (Clang: capability) types.
+ * Internal only.
+ */
+#define __ctx_guard_type(name) __attribute__((capability(#name)))
+#define __reentrant_ctx_guard __attribute__((reentrant_capability))
+#define __acquires_ctx_guard(...) __attribute__((acquire_capability(__VA_ARGS__)))
+#define __acquires_shared_ctx_guard(...) __attribute__((acquire_shared_capability(__VA_ARGS__)))
+#define __try_acquires_ctx_guard(ret, var) __attribute__((try_acquire_capability(ret, var)))
+#define __try_acquires_shared_ctx_guard(ret, var) __attribute__((try_acquire_shared_capability(ret, var)))
+#define __releases_ctx_guard(...) __attribute__((release_capability(__VA_ARGS__)))
+#define __releases_shared_ctx_guard(...) __attribute__((release_shared_capability(__VA_ARGS__)))
+#define __assumes_ctx_guard(...) __attribute__((assert_capability(__VA_ARGS__)))
+#define __assumes_shared_ctx_guard(...) __attribute__((assert_shared_capability(__VA_ARGS__)))
+#define __returns_ctx_guard(var) __attribute__((lock_returned(var)))
+
+/*
+ * The below are used to annotate code being checked. Internal only.
+ */
+#define __excludes_ctx_guard(...) __attribute__((locks_excluded(__VA_ARGS__)))
+#define __requires_ctx_guard(...) __attribute__((requires_capability(__VA_ARGS__)))
+#define __requires_shared_ctx_guard(...) __attribute__((requires_shared_capability(__VA_ARGS__)))
+
+/**
+ * __guarded_by - struct member and globals attribute, declares variable
+ * only accessible within active context
+ *
+ * Declares that the struct member or global variable is only accessible within
+ * the context entered by the given context guard. Read operations on the data
+ * require shared access, while write operations require exclusive access.
+ *
+ * .. code-block:: c
+ *
+ * struct some_state {
+ * spinlock_t lock;
+ * long counter __guarded_by(&lock);
+ * };
+ */
+#define __guarded_by(...) __attribute__((guarded_by(__VA_ARGS__)))
+
+/**
+ * __pt_guarded_by - struct member and globals attribute, declares pointed-to
+ * data only accessible within active context
+ *
+ * Declares that the data pointed to by the struct member pointer or global
+ * pointer is only accessible within the context entered by the given context
+ * guard. Read operations on the data require shared access, while write
+ * operations require exclusive access.
+ *
+ * .. code-block:: c
+ *
+ * struct some_state {
+ * spinlock_t lock;
+ * long *counter __pt_guarded_by(&lock);
+ * };
+ */
+#define __pt_guarded_by(...) __attribute__((pt_guarded_by(__VA_ARGS__)))
+
+/**
+ * context_guard_struct() - declare or define a context guard struct
+ * @name: struct name
+ *
+ * Helper to declare or define a struct type that is also a context guard.
+ *
+ * .. code-block:: c
+ *
+ * context_guard_struct(my_handle) {
+ * int foo;
+ * long bar;
+ * };
+ *
+ * struct some_state {
+ * ...
+ * };
+ * // ... declared elsewhere ...
+ * context_guard_struct(some_state);
+ *
+ * Note: The implementation defines several helper functions that can acquire
+ * and release the context guard.
+ */
+#define context_guard_struct(name, ...) \
+ struct __ctx_guard_type(name) __VA_ARGS__ name; \
+ static __always_inline void __acquire_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __no_context_analysis __acquires_ctx_guard(var) { } \
+ static __always_inline void __acquire_shared_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __no_context_analysis __acquires_shared_ctx_guard(var) { } \
+ static __always_inline bool __try_acquire_ctx_guard(const struct name *var, bool ret) \
+ __attribute__((overloadable)) __no_context_analysis __try_acquires_ctx_guard(1, var) \
+ { return ret; } \
+ static __always_inline bool __try_acquire_shared_ctx_guard(const struct name *var, bool ret) \
+ __attribute__((overloadable)) __no_context_analysis __try_acquires_shared_ctx_guard(1, var) \
+ { return ret; } \
+ static __always_inline void __release_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __no_context_analysis __releases_ctx_guard(var) { } \
+ static __always_inline void __release_shared_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __no_context_analysis __releases_shared_ctx_guard(var) { } \
+ static __always_inline void __assume_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __assumes_ctx_guard(var) { } \
+ static __always_inline void __assume_shared_ctx_guard(const struct name *var) \
+ __attribute__((overloadable)) __assumes_shared_ctx_guard(var) { } \
+ struct name
+
+/**
+ * disable_context_analysis() - disables context analysis
+ *
+ * Disables context analysis. Must be paired with a later
+ * enable_context_analysis().
+ */
+#define disable_context_analysis() \
+ __diag_push(); \
+ __diag_ignore_all("-Wunknown-warning-option", "") \
+ __diag_ignore_all("-Wthread-safety", "") \
+ __diag_ignore_all("-Wthread-safety-pointer", "")
+
+/**
+ * enable_context_analysis() - re-enables context analysis
+ *
+ * Re-enables context analysis. Must be paired with a prior
+ * disable_context_analysis().
+ */
+#define enable_context_analysis() __diag_pop()
+
+/**
+ * __no_context_analysis - function attribute, disables context analysis
+ *
+ * Function attribute denoting that context analysis is disabled for the
+ * whole function. Prefer use of `context_unsafe()` where possible.
+ */
+#define __no_context_analysis __attribute__((no_thread_safety_analysis))
+
+#endif /* _LINUX_COMPILER_CONTEXT_ANALYSIS_CLANG_H */
diff --git a/include/linux/compiler-context-analysis.h b/include/linux/compiler-context-analysis.h
index 03056f87a86f..33ad367fef3f 100644
--- a/include/linux/compiler-context-analysis.h
+++ b/include/linux/compiler-context-analysis.h
@@ -6,140 +6,7 @@
#ifndef _LINUX_COMPILER_CONTEXT_ANALYSIS_H
#define _LINUX_COMPILER_CONTEXT_ANALYSIS_H
-#if defined(WARN_CONTEXT_ANALYSIS)
-
-/*
- * These attributes define new context guard (Clang: capability) types.
- * Internal only.
- */
-# define __ctx_guard_type(name) __attribute__((capability(#name)))
-# define __reentrant_ctx_guard __attribute__((reentrant_capability))
-# define __acquires_ctx_guard(...) __attribute__((acquire_capability(__VA_ARGS__)))
-# define __acquires_shared_ctx_guard(...) __attribute__((acquire_shared_capability(__VA_ARGS__)))
-# define __try_acquires_ctx_guard(ret, var) __attribute__((try_acquire_capability(ret, var)))
-# define __try_acquires_shared_ctx_guard(ret, var) __attribute__((try_acquire_shared_capability(ret, var)))
-# define __releases_ctx_guard(...) __attribute__((release_capability(__VA_ARGS__)))
-# define __releases_shared_ctx_guard(...) __attribute__((release_shared_capability(__VA_ARGS__)))
-# define __assumes_ctx_guard(...) __attribute__((assert_capability(__VA_ARGS__)))
-# define __assumes_shared_ctx_guard(...) __attribute__((assert_shared_capability(__VA_ARGS__)))
-# define __returns_ctx_guard(var) __attribute__((lock_returned(var)))
-
-/*
- * The below are used to annotate code being checked. Internal only.
- */
-# define __excludes_ctx_guard(...) __attribute__((locks_excluded(__VA_ARGS__)))
-# define __requires_ctx_guard(...) __attribute__((requires_capability(__VA_ARGS__)))
-# define __requires_shared_ctx_guard(...) __attribute__((requires_shared_capability(__VA_ARGS__)))
-
-/**
- * __guarded_by - struct member and globals attribute, declares variable
- * only accessible within active context
- *
- * Declares that the struct member or global variable is only accessible within
- * the context entered by the given context guard. Read operations on the data
- * require shared access, while write operations require exclusive access.
- *
- * .. code-block:: c
- *
- * struct some_state {
- * spinlock_t lock;
- * long counter __guarded_by(&lock);
- * };
- */
-# define __guarded_by(...) __attribute__((guarded_by(__VA_ARGS__)))
-
-/**
- * __pt_guarded_by - struct member and globals attribute, declares pointed-to
- * data only accessible within active context
- *
- * Declares that the data pointed to by the struct member pointer or global
- * pointer is only accessible within the context entered by the given context
- * guard. Read operations on the data require shared access, while write
- * operations require exclusive access.
- *
- * .. code-block:: c
- *
- * struct some_state {
- * spinlock_t lock;
- * long *counter __pt_guarded_by(&lock);
- * };
- */
-# define __pt_guarded_by(...) __attribute__((pt_guarded_by(__VA_ARGS__)))
-
-/**
- * context_guard_struct() - declare or define a context guard struct
- * @name: struct name
- *
- * Helper to declare or define a struct type that is also a context guard.
- *
- * .. code-block:: c
- *
- * context_guard_struct(my_handle) {
- * int foo;
- * long bar;
- * };
- *
- * struct some_state {
- * ...
- * };
- * // ... declared elsewhere ...
- * context_guard_struct(some_state);
- *
- * Note: The implementation defines several helper functions that can acquire
- * and release the context guard.
- */
-# define context_guard_struct(name, ...) \
- struct __ctx_guard_type(name) __VA_ARGS__ name; \
- static __always_inline void __acquire_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __no_context_analysis __acquires_ctx_guard(var) { } \
- static __always_inline void __acquire_shared_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __no_context_analysis __acquires_shared_ctx_guard(var) { } \
- static __always_inline bool __try_acquire_ctx_guard(const struct name *var, bool ret) \
- __attribute__((overloadable)) __no_context_analysis __try_acquires_ctx_guard(1, var) \
- { return ret; } \
- static __always_inline bool __try_acquire_shared_ctx_guard(const struct name *var, bool ret) \
- __attribute__((overloadable)) __no_context_analysis __try_acquires_shared_ctx_guard(1, var) \
- { return ret; } \
- static __always_inline void __release_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __no_context_analysis __releases_ctx_guard(var) { } \
- static __always_inline void __release_shared_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __no_context_analysis __releases_shared_ctx_guard(var) { } \
- static __always_inline void __assume_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __assumes_ctx_guard(var) { } \
- static __always_inline void __assume_shared_ctx_guard(const struct name *var) \
- __attribute__((overloadable)) __assumes_shared_ctx_guard(var) { } \
- struct name
-
-/**
- * disable_context_analysis() - disables context analysis
- *
- * Disables context analysis. Must be paired with a later
- * enable_context_analysis().
- */
-# define disable_context_analysis() \
- __diag_push(); \
- __diag_ignore_all("-Wunknown-warning-option", "") \
- __diag_ignore_all("-Wthread-safety", "") \
- __diag_ignore_all("-Wthread-safety-pointer", "")
-
-/**
- * enable_context_analysis() - re-enables context analysis
- *
- * Re-enables context analysis. Must be paired with a prior
- * disable_context_analysis().
- */
-# define enable_context_analysis() __diag_pop()
-
-/**
- * __no_context_analysis - function attribute, disables context analysis
- *
- * Function attribute denoting that context analysis is disabled for the
- * whole function. Prefer use of `context_unsafe()` where possible.
- */
-# define __no_context_analysis __attribute__((no_thread_safety_analysis))
-
-#else /* !WARN_CONTEXT_ANALYSIS */
-
+#if !defined(WARN_CONTEXT_ANALYSIS)
# define __ctx_guard_type(name)
# define __reentrant_ctx_guard
# define __acquires_ctx_guard(...)
@@ -168,7 +35,6 @@
# define disable_context_analysis()
# define enable_context_analysis()
# define __no_context_analysis
-
#endif /* WARN_CONTEXT_ANALYSIS */
/**
diff --git a/scripts/Makefile.context-analysis b/scripts/Makefile.context-analysis
index cd3bb49d3f09..6f94b555af14 100644
--- a/scripts/Makefile.context-analysis
+++ b/scripts/Makefile.context-analysis
@@ -2,7 +2,8 @@
context-analysis-cflags := -DWARN_CONTEXT_ANALYSIS \
-fexperimental-late-parse-attributes -Wthread-safety \
- -Wthread-safety-pointer -Wthread-safety-beta
+ -Wthread-safety-pointer -Wthread-safety-beta \
+ -include $(srctree)/include/linux/compiler-context-analysis-clang.h
ifndef CONFIG_WARN_CONTEXT_ANALYSIS_ALL
context-analysis-cflags += --warning-suppression-mappings=$(srctree)/scripts/context-analysis-suppression.txt
--
2.52.0.rc2.455.g230fcf2819-goog
^ permalink raw reply related
* Re: Re: [BUG] landlock: sleeping function called from invalid context in hook_sb_delete()
From: 许佳凯 @ 2025-11-21 1:01 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, linux-kernel, linux-security-module,
Günther Noack, Serge E. Hallyn
In-Reply-To: <20251120.hoT5ye5thooR@digikod.net>
Hello Mickaël,
Thanks a lot for your reply.
Best regards,
-Jiakai
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Bernd Edlinger @ 2025-11-21 2:59 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <87a50gxo0i.fsf@email.froward.int.ebiederm.org>
Hi Eric,
thanks for you valuable input on the topic.
On 11/21/25 00:50, Eric W. Biederman wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> Instead of computing the new cred before we pass the point of no
>> return compute the new cred just before we use it.
>>
>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>
>> I am not certain why we wanted to compute the cred for the new
>> executable so early. Perhaps I missed something but I did not see any
>> common errors being signaled. So I don't think we loose anything by
>> computing the new cred later.
>
> I should add that the permission checks happen in open_exec,
> everything that follows credential wise is just about representing in
> struct cred the credentials the new executable will have.
>
> So I am really at a loss why we have had this complicated way of
> computing of computed the credentials all of these years full of
> time of check to time of use problems.
>
Well, I think I see a problem with your patch:
When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
or the smack security engines at least. Previously that callback
was called before the point of no return, and the return code should
be returned as a return code the the caller of execve. But if we move
that check after the point of no return, the caller will get killed
due to the failed security check.
Or did I miss something?
Thanks
Bernd.
> Eric
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-21 7:18 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <GV2PPF74270EBEEAD4CACA124C05BE1CE45E4D5A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> Hi Eric,
>
> thanks for you valuable input on the topic.
>
> On 11/21/25 00:50, Eric W. Biederman wrote:
>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>
>>> Instead of computing the new cred before we pass the point of no
>>> return compute the new cred just before we use it.
>>>
>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>
>>> I am not certain why we wanted to compute the cred for the new
>>> executable so early. Perhaps I missed something but I did not see any
>>> common errors being signaled. So I don't think we loose anything by
>>> computing the new cred later.
>>
>> I should add that the permission checks happen in open_exec,
>> everything that follows credential wise is just about representing in
>> struct cred the credentials the new executable will have.
>>
>> So I am really at a loss why we have had this complicated way of
>> computing of computed the credentials all of these years full of
>> time of check to time of use problems.
>>
>
> Well, I think I see a problem with your patch:
>
> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
> or the smack security engines at least. Previously that callback
> was called before the point of no return, and the return code should
> be returned as a return code the the caller of execve. But if we move
> that check after the point of no return, the caller will get killed
> due to the failed security check.
>
> Or did I miss something?
I think we definitely need to document this change in behavior. I would
call ending the exec with SIGSEGV vs -EPERM a quality of implementation
issue. The exec is failing one way or the other so I don't see it as a
correctness issue.
In the case of ptrace in general I think it is a bug if the mere act of
debugging a program changes it's behavior. So which buggy behavior
should we prefer? SIGSEGV where it is totally clear that the behavior
has changed or -EPERM and ask the debugged program to handle it.
I lean towards SIGSEGV because then it is clear the code should not
handle it.
In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
handle unexpected things happening is to terminate the application.
In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
of CLONE_THREAD.
Plus all of these things are only considerations if we are exec'ing a
program that transitions to a different set of credentials. Something
that happens but is quite rare itself.
In practice I don't expect there is anything that depends on the exact
behavior of what happens when exec'ing a suid executable to gain
privileges when ptraced. The closes I can imagine is upstart and
I think upstart ran as root when ptracing other programs so there is no
gaining of privilege and thus no reason for a security module to
complain.
Who knows I could be wrong, and someone could actually care. Which is
hy I think we should document it.
Eric
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Bernd Edlinger @ 2025-11-21 9:35 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <87o6ovx38h.fsf@email.froward.int.ebiederm.org>
On 11/21/25 08:18, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>
>> Hi Eric,
>>
>> thanks for you valuable input on the topic.
>>
>> On 11/21/25 00:50, Eric W. Biederman wrote:
>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>
>>>> Instead of computing the new cred before we pass the point of no
>>>> return compute the new cred just before we use it.
>>>>
>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>>
>>>> I am not certain why we wanted to compute the cred for the new
>>>> executable so early. Perhaps I missed something but I did not see any
>>>> common errors being signaled. So I don't think we loose anything by
>>>> computing the new cred later.
>>>
>>> I should add that the permission checks happen in open_exec,
>>> everything that follows credential wise is just about representing in
>>> struct cred the credentials the new executable will have.
>>>
>>> So I am really at a loss why we have had this complicated way of
>>> computing of computed the credentials all of these years full of
>>> time of check to time of use problems.
>>>
>>
>> Well, I think I see a problem with your patch:
>>
>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
>> or the smack security engines at least. Previously that callback
>> was called before the point of no return, and the return code should
>> be returned as a return code the the caller of execve. But if we move
>> that check after the point of no return, the caller will get killed
>> due to the failed security check.
>>
>> Or did I miss something?
>
> I think we definitely need to document this change in behavior. I would
> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
> issue. The exec is failing one way or the other so I don't see it as a
> correctness issue.
>
> In the case of ptrace in general I think it is a bug if the mere act of
> debugging a program changes it's behavior. So which buggy behavior
> should we prefer? SIGSEGV where it is totally clear that the behavior
> has changed or -EPERM and ask the debugged program to handle it.
> I lean towards SIGSEGV because then it is clear the code should not
> handle it.
>
> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
> handle unexpected things happening is to terminate the application.
>
> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
> of CLONE_THREAD.
>
>
> Plus all of these things are only considerations if we are exec'ing a
> program that transitions to a different set of credentials. Something
> that happens but is quite rare itself.
>
> In practice I don't expect there is anything that depends on the exact
> behavior of what happens when exec'ing a suid executable to gain
> privileges when ptraced. The closes I can imagine is upstart and
> I think upstart ran as root when ptracing other programs so there is no
> gaining of privilege and thus no reason for a security module to
> complain.
>
> Who knows I could be wrong, and someone could actually care. Which is> hy I think we should document it.
>
Well, I dont know for sure, but the security engine could deny the execution
for any reason, not only because of being ptraced.
Maybe there can be a policy which denies user X to execute e.g. any suid programs.
Bernd.
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Bernd Edlinger @ 2025-11-21 11:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <GV2PPF74270EBEEFA106F4EF26B087ED898E4D5A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
On 11/21/25 10:35, Bernd Edlinger wrote:
> On 11/21/25 08:18, Eric W. Biederman wrote:
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> Hi Eric,
>>>
>>> thanks for you valuable input on the topic.
>>>
>>> On 11/21/25 00:50, Eric W. Biederman wrote:
>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>>
>>>>> Instead of computing the new cred before we pass the point of no
>>>>> return compute the new cred just before we use it.
>>>>>
>>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>>>
>>>>> I am not certain why we wanted to compute the cred for the new
>>>>> executable so early. Perhaps I missed something but I did not see any
>>>>> common errors being signaled. So I don't think we loose anything by
>>>>> computing the new cred later.
>>>>
>>>> I should add that the permission checks happen in open_exec,
>>>> everything that follows credential wise is just about representing in
>>>> struct cred the credentials the new executable will have.
>>>>
>>>> So I am really at a loss why we have had this complicated way of
>>>> computing of computed the credentials all of these years full of
>>>> time of check to time of use problems.
>>>>
>>>
>>> Well, I think I see a problem with your patch:
>>>
>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
>>> or the smack security engines at least. Previously that callback
>>> was called before the point of no return, and the return code should
>>> be returned as a return code the the caller of execve. But if we move
>>> that check after the point of no return, the caller will get killed
>>> due to the failed security check.
>>>
>>> Or did I miss something?
>>
>> I think we definitely need to document this change in behavior. I would
>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
>> issue. The exec is failing one way or the other so I don't see it as a
>> correctness issue.
>>
>> In the case of ptrace in general I think it is a bug if the mere act of
>> debugging a program changes it's behavior. So which buggy behavior
>> should we prefer? SIGSEGV where it is totally clear that the behavior
>> has changed or -EPERM and ask the debugged program to handle it.
>> I lean towards SIGSEGV because then it is clear the code should not
>> handle it.
>>
>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
>> handle unexpected things happening is to terminate the application.
>>
>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
>> of CLONE_THREAD.
>>
>>
>> Plus all of these things are only considerations if we are exec'ing a
>> program that transitions to a different set of credentials. Something
>> that happens but is quite rare itself.
>>
>> In practice I don't expect there is anything that depends on the exact
>> behavior of what happens when exec'ing a suid executable to gain
>> privileges when ptraced. The closes I can imagine is upstart and
>> I think upstart ran as root when ptracing other programs so there is no
>> gaining of privilege and thus no reason for a security module to
>> complain.
>>
>> Who knows I could be wrong, and someone could actually care. Which is
>> hy I think we should document it.>>
>
>
> Well, I dont know for sure, but the security engine could deny the execution
> for any reason, not only because of being ptraced.
> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
>
>
> Bernd.
>
Hmm, funny..
I installed this patch on top of
commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
Merge: c966813ea120 7b6216baae75
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu Nov 20 11:04:37 2025 -0800
but it does panic when I try to boot:
[ 0.870539] TERM=1inux
[ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
[ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
[ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
[ 0.870580] Call Trace:
[ 0.870590] <TASK>
[ 0.870592] vpanic+0x36d/0x380
[ 0.870607] ? __pfx_kernel_init+0x10/0x10
[ 0.870615] panic+0x5b/0x60
[ 0.870617] kernel_init+0x17d/0x1c0
[ 0.870623] ret_from_fork+0x124/0x150
[ 0.870625} ? __pfx_kernel_init+0x10/0x10
[ 0.870627] ret_from_fork_asm+0x1a/0x30
[ 0.870632] </TASK>
[ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
[ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
Is that a known problem?
Bernd.
^ permalink raw reply
* Re: [PATCH v4 02/10] landlock: Add API support and docs for the quiet flags
From: Mickaël Salaün @ 2025-11-21 15:27 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <cd79fcf71e6d38ab4280c0de2500fa5f6b06cd9d.1763330228.git.m@maowtm.org>
On Sun, Nov 16, 2025 at 09:59:32PM +0000, Tingmao Wang wrote:
> Adds the UAPI for the quiet flags feature (but not the implementation
> yet).
>
> According to pahole, even after adding the struct access_masks quiet_masks
> in struct landlock_hierarchy, the u32 log_* bitfield still only has a size
> of 2 bytes, so there's minimal wasted space.
>
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>
> Changes since v3:
> - Minor update to this commit message.
> - Fix minor formatting
>
> Changes since v2:
> - Updated docs from Mickaël's suggestions.
>
> Changes since v1:
> - Per suggestion, added support for quieting only certain access bits,
> controlled by extra quiet_access_* fields in the ruleset_attr.
> - Added docs for the extra fields and made updates to doc changes in v1.
> In particular, call out that the effect of LANDLOCK_ADD_RULE_QUIET is
> independent from the access bits passed in rule_attr
> - landlock_add_rule will return -EINVAL when LANDLOCK_ADD_RULE_QUIET is
> used but the ruleset does not have any quiet access bits set for the
> given rule type.
> - ABI version bump to v8
> - Syntactic and comment changes per suggestion.
>
> include/uapi/linux/landlock.h | 64 +++++++++++++++++
> security/landlock/domain.h | 5 ++
> security/landlock/fs.c | 4 +-
> security/landlock/fs.h | 2 +-
> security/landlock/net.c | 5 +-
> security/landlock/net.h | 3 +-
> security/landlock/ruleset.c | 10 ++-
> security/landlock/ruleset.h | 8 ++-
> security/landlock/syscalls.c | 72 +++++++++++++++-----
> tools/testing/selftests/landlock/base_test.c | 4 +-
> 10 files changed, 150 insertions(+), 27 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f030adc462ee..50f0806b7e33 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -32,6 +32,19 @@
> * *handle* a wide range or all access rights that they know about at build time
> * (and that they have tested with a kernel that supported them all).
> *
> + * @quiet_access_fs and @quiet_access_net are bitmasks of actions for
> + * which a denial by this layer will not trigger an audit log if the
> + * corresponding object (or its children, for filesystem rules) is marked
> + * with the "quiet" bit via %LANDLOCK_ADD_RULE_QUIET, even if logging
> + * would normally take place per landlock_restrict_self() flags.
> + * quiet_scoped is similar, except that it does not require marking any
> + * objects as quiet - if the ruleset is created with any bits set in
> + * quiet_scoped, then denial of such scoped resources will not trigger any
> + * log. These 3 fields are available since Landlock ABI version 8.
> + *
> + * @quiet_access_fs, @quiet_access_net and @quiet_scoped must be a subset
> + * of @handled_access_fs, @handled_access_net and @scoped respectively.
> + *
> * This structure can grow in future Landlock versions.
> */
> struct landlock_ruleset_attr {
> @@ -51,6 +64,24 @@ struct landlock_ruleset_attr {
> * resources (e.g. IPCs).
> */
> __u64 scoped;
> +
> + /* Since ABI 8: */
> +
> + /**
> + * @quiet_access_fs: Bitmask of filesystem actions which should not be
> + * audit logged if per-object quiet flag is set.
> + */
> + __u64 quiet_access_fs;
> + /**
> + * @quiet_access_net: Bitmask of network actions which should not be
> + * audit logged if per-object quiet flag is set.
> + */
> + __u64 quiet_access_net;
> + /**
> + * @quiet_scoped: Bitmask of scoped actions which should not be audit
> + * logged.
> + */
> + __u64 quiet_scoped;
> };
>
> /**
> @@ -69,6 +100,39 @@ struct landlock_ruleset_attr {
> #define LANDLOCK_CREATE_RULESET_ERRATA (1U << 1)
> /* clang-format on */
>
> +/**
> + * DOC: landlock_add_rule_flags
> + *
> + * **Flags**
> + *
> + * %LANDLOCK_ADD_RULE_QUIET
> + * Together with the quiet_* fields in struct landlock_ruleset_attr,
> + * this flag controls whether Landlock will log audit messages when
> + * access to the objects covered by this rule is denied by this layer.
> + *
> + * If audit logging is enabled, when Landlock denies an access, it will
> + * suppress the audit log if all of the following are true:
> + *
> + * - This layer is the innermost layer that denied the access;
Because these items follows ":" they should not start with a capital
letter (e.g. "- this layer ...").
> + * - All requested accesses are part of the quiet_* fields in the
> + * related struct landlock_ruleset_attr;
This should be updated to reflect my comment in the next patch about
landlock_log_denial().
> + * - The object (or one of its parents, for filesystem rules) is
> + * marked as "quiet" via %LANDLOCK_ADD_RULE_QUIET.
> + *
> + * Because logging is only suppressed by a layer if the layer denies
> + * access, a sandboxed program cannot use this flag to "hide" access
> + * denials, without denying itself the access in the first place.
> + *
> + * The effect of this flag does not depend on the value of
> + * allowed_access in the passed in rule_attr. When this flag is
> + * present, the caller is also allowed to pass in an empty
> + * allowed_access.
> + */
> +
> +/* clang-format off */
> +#define LANDLOCK_ADD_RULE_QUIET (1U << 0)
> +/* clang-format on */
> +
> /**
> * DOC: landlock_restrict_self_flags
> *
> diff --git a/security/landlock/domain.h b/security/landlock/domain.h
> index 7fb70b25f85a..aadbf53505c0 100644
> --- a/security/landlock/domain.h
> +++ b/security/landlock/domain.h
> @@ -114,6 +114,11 @@ struct landlock_hierarchy {
> * %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON. Set to false by default.
> */
> log_new_exec : 1;
> + /**
> + * @quiet_masks: Bitmasks of access that should be quieted (i.e. not
> + * logged) if the related object is marked as quiet.
> + */
> + struct access_masks quiet_masks;
Please update the above @work_free doc.
> #endif /* CONFIG_AUDIT */
> };
>
^ permalink raw reply
* Re: [PATCH -next] ima: Handle error code returned by ima_filter_rule_match()
From: Mimi Zohar @ 2025-11-21 15:38 UTC (permalink / raw)
To: Zhao Yipeng, roberto.sassu, dmitry.kasatkin, eric.snowberg
Cc: lujialin4, linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20251120071805.1604864-1-zhaoyipeng5@huawei.com>
On Thu, 2025-11-20 at 15:18 +0800, Zhao Yipeng wrote:
> In ima_match_rules(), if ima_filter_rule_match() returns -ENOENT due to
> the rule being NULL, the function incorrectly skips the 'if (!rc)' check
> and sets 'result = true'. The LSM rule is considered a match, causing
> extra files to be measured by IMA.
>
> This issue can be reproduced in the following scenario:
> After unloading the SELinux policy module via 'semodule -d', if an IMA
> measurement is triggered before ima_lsm_rules is updated,
> in ima_match_rules(), the first call to ima_filter_rule_match() returns
> -ESTALE. This causes the code to enter the 'if (rc == -ESTALE &&
> !rule_reinitialized)' block, perform ima_lsm_copy_rule() and retry. In
> ima_lsm_copy_rule(), since the SELinux module has been removed, the rule
> becomes NULL, and the second call to ima_filter_rule_match() returns
> -ENOENT. This bypasses the 'if (!rc)' check and results in a false match.
>
> Call trace:
> selinux_audit_rule_match+0x310/0x3b8
> security_audit_rule_match+0x60/0xa0
> ima_match_rules+0x2e4/0x4a0
> ima_match_policy+0x9c/0x1e8
> ima_get_action+0x48/0x60
> process_measurement+0xf8/0xa98
> ima_bprm_check+0x98/0xd8
> security_bprm_check+0x5c/0x78
> search_binary_handler+0x6c/0x318
> exec_binprm+0x58/0x1b8
> bprm_execve+0xb8/0x130
> do_execveat_common.isra.0+0x1a8/0x258
> __arm64_sys_execve+0x48/0x68
> invoke_syscall+0x50/0x128
> el0_svc_common.constprop.0+0xc8/0xf0
> do_el0_svc+0x24/0x38
> el0_svc+0x44/0x200
> el0t_64_sync_handler+0x100/0x130
> el0t_64_sync+0x3c8/0x3d0
>
> Fix this by changing 'if (!rc)' to 'if (rc <= 0)' to ensure that error
> codes like -ENOENT do not bypass the check and accidentally result in a
> successful match.
>
> Fixes: 4af4662fa4a9d ("integrity: IMA policy")
> Signed-off-by: Zhao Yipeng <zhaoyipeng5@huawei.com>
Thank you. The patch is now queued in next-integrity.
Mimi
^ permalink raw reply
* Re: [PATCH v4 03/10] landlock: Suppress logging when quiet flag is present
From: Mickaël Salaün @ 2025-11-21 15:27 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <5c0de8ee7e00aff1aceb3a80f5af162eeaaa06db.1763330228.git.m@maowtm.org>
On Sun, Nov 16, 2025 at 09:59:33PM +0000, Tingmao Wang wrote:
> The quietness behaviour is as documented in the previous patch.
>
> For optional accesses, since the existing deny_masks can only store 2x4bit
> of layer index, with no way to represent "no layer", we need to either
> expand it or have another field to correctly handle quieting of those.
> This commit uses the latter approach - we add another field to store which
> optional access (of the 2) are covered by quiet rules in their respective
> layers as stored in deny_masks.
>
> We can avoid making struct landlock_file_security larger by converting the
> existing fown_layer to a 4bit field. This commit does that, and adds test
> to ensure that it is large enough for LANDLOCK_MAX_NUM_LAYERS-1.
>
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>
> Changes since v2:
> - Renamed patch title from "Check for quiet flag in landlock_log_denial"
> to this given the growth.
> - Moved quiet bit check after domain_exec check
> - Rename, style and comment fixes suggested by Mickaël.
> - Squashed patch 6/6 from v2 "Implement quiet for optional accesses" into
> this one. Changes to that below:
> - Refactor the quiet flag setting in get_layer_from_deny_masks() to be
> more clear.
> - Add KUnit tests
> - Fix comments, add WARN_ON_ONCE, use __const_hweight64() as suggested by
> review
> - Move build_check_file_security to fs.c
> - Use a typedef for quiet_optional_accesses, add static_assert, and
> improve docs on landlock_get_quiet_optional_accesses.
>
> Changes since v1:
> - Supports the new quiet access masks.
> - Support quieting scope requests (but not ptrace and attempted mounting
> for now)
>
> security/landlock/access.h | 5 +
> security/landlock/audit.c | 255 +++++++++++++++++++++++++++++++++---
> security/landlock/audit.h | 4 +-
> security/landlock/domain.c | 33 +++++
> security/landlock/domain.h | 5 +
> security/landlock/fs.c | 45 +++++--
> security/landlock/fs.h | 17 ++-
> security/landlock/net.c | 3 +-
> security/landlock/ruleset.h | 5 +
> security/landlock/task.c | 12 +-
> 10 files changed, 344 insertions(+), 40 deletions(-)
>
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d..db47edc88afa 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -97,4 +97,9 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)
> return access_masks;
> }
>
> +/* A bitmask that is large enough to hold set of optional accesses. */
> +typedef u8 optional_access_t;
> +static_assert(BITS_PER_TYPE(optional_access_t) >=
> + HWEIGHT(_LANDLOCK_ACCESS_FS_OPTIONAL));
> +
> #endif /* _SECURITY_LANDLOCK_ACCESS_H */
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index c52d079cdb77..505b13c8f4c2 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -273,7 +273,7 @@ static void test_get_denied_layer(struct kunit *const test)
> static size_t
> get_layer_from_deny_masks(access_mask_t *const access_request,
> const access_mask_t all_existing_optional_access,
> - const deny_masks_t deny_masks)
> + const deny_masks_t deny_masks, u8 quiet_optional_accesses, bool *quiet)
> {
> const unsigned long access_opt = all_existing_optional_access;
> const unsigned long access_req = *access_request;
> @@ -285,18 +285,29 @@ get_layer_from_deny_masks(access_mask_t *const access_request,
> /* This will require change with new object types. */
> WARN_ON_ONCE(access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL);
>
> + *quiet = false;
I'd prefer to have a local variable and just at before returning,
setting the quiet parameter, similar to access_request.
> for_each_set_bit(access_bit, &access_opt,
> BITS_PER_TYPE(access_mask_t)) {
> if (access_req & BIT(access_bit)) {
> const size_t layer =
> (deny_masks >> (access_index * 4)) &
> (LANDLOCK_MAX_NUM_LAYERS - 1);
> + const bool layer_has_quiet =
> + !!(quiet_optional_accesses & BIT(access_index));
>
> if (layer > youngest_layer) {
> youngest_layer = layer;
> + *quiet = layer_has_quiet;
> missing = BIT(access_bit);
> } else if (layer == youngest_layer) {
> missing |= BIT(access_bit);
> + /*
> + * Whether the layer has rules with quiet flag covering
> + * the file accessed does not depend on the access, and so
> + * if this fails, quiet_optional_accesses is corrupted.
> + */
> + WARN_ON_ONCE(*quiet && !layer_has_quiet);
> + *quiet = layer_has_quiet;
In this case, why update *quiet?
> }
> }
> access_index++;
> @@ -312,42 +323,188 @@ static void test_get_layer_from_deny_masks(struct kunit *const test)
> {
> deny_masks_t deny_mask;
> access_mask_t access;
> + u8 quiet_optional_accesses;
> + bool quiet;
>
> /* truncate:0 ioctl_dev:2 */
> deny_mask = 0x20;
> + quiet_optional_accesses = 0;
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE;
> KUNIT_EXPECT_EQ(test, 0,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* layer denying truncate: quiet, ioctl: not quiet */
> + quiet_optional_accesses = 0b01;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* Reverse order - truncate:2 ioctl_dev:0 */
> + deny_mask = 0x02;
> + quiet_optional_accesses = 0;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* layer denying truncate: quiet, ioctl: not quiet */
> + quiet_optional_accesses = 0b01;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> KUNIT_EXPECT_EQ(test, 2,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + /* layer denying truncate: not quiet, ioctl: quiet */
> + quiet_optional_accesses = 0b10;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
>
> /* truncate:15 ioctl_dev:15 */
> deny_mask = 0xff;
> + quiet_optional_accesses = 0;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 15,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 15,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access,
> + LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* Both quiet (same layer so quietness must be the same) */
> + quiet_optional_accesses = 0b11;
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE;
> KUNIT_EXPECT_EQ(test, 15,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> KUNIT_EXPECT_EQ(test, 15,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access,
> LANDLOCK_ACCESS_FS_TRUNCATE |
> LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> @@ -381,19 +538,39 @@ static bool is_valid_request(const struct landlock_request *const request)
> return true;
> }
>
> +static access_mask_t
> +pick_access_mask_for_request_type(const enum landlock_request_type type,
> + const struct access_masks access_masks)
> +{
> + switch (type) {
> + case LANDLOCK_REQUEST_FS_ACCESS:
> + return access_masks.fs;
> + case LANDLOCK_REQUEST_NET_ACCESS:
> + return access_masks.net;
> + default:
> + WARN_ONCE(1, "Invalid request type %d passed to %s", type,
> + __func__);
> + return 0;
> + }
> +}
> +
> /**
> * landlock_log_denial - Create audit records related to a denial
> *
> * @subject: The Landlock subject's credential denying an action.
> * @request: Detail of the user space request.
> + * @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if
> + * this is a scope request (no particular object involved).
> */
> void landlock_log_denial(const struct landlock_cred_security *const subject,
> - const struct landlock_request *const request)
> + const struct landlock_request *const request,
> + const struct collected_rule_flags rule_flags)
It would be simpler and limit code change to move rule_flags/quiet_flags
into struct landlock_request, which means we can also remove
no_rule_flags.
> {
> struct audit_buffer *ab;
> struct landlock_hierarchy *youngest_denied;
> size_t youngest_layer;
> - access_mask_t missing;
> + access_mask_t missing, quiet_mask;
> + bool object_quiet_flag = false, quiet_applicable_to_access = false;
>
> if (WARN_ON_ONCE(!subject || !subject->domain ||
> !subject->domain->hierarchy || !request))
> @@ -409,10 +586,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> youngest_layer = get_denied_layer(
> subject->domain, &missing, request->layer_masks,
> request->layer_masks_size);
> + object_quiet_flag = !!(rule_flags.quiet_masks & BIT(youngest_layer));
> } else {
> youngest_layer = get_layer_from_deny_masks(
> &missing, request->all_existing_optional_access,
> - request->deny_masks);
> + request->deny_masks,
> + request->quiet_optional_accesses,
> + &object_quiet_flag);
> }
> youngest_denied =
> get_hierarchy(subject->domain, youngest_layer);
> @@ -447,6 +627,49 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> return;
> }
>
> + /*
> + * Checks if the object is marked quiet by the layer that denied the
> + * request. If it's a different layer that marked it as quiet, but
> + * that layer is not the one that denied the request, we should still
> + * audit log the denial.
> + */
> + if (object_quiet_flag) {
> + /*
> + * We now check if the denied requests are all covered by the
> + * layer's quiet access bits.
> + */
> + quiet_mask = pick_access_mask_for_request_type(
This quiet_mask is only used in this branch, so we can declare it here
and make it const:
const access_mask_t quiet_mask = pick_access_mask_for_request_type(
> + request->type, youngest_denied->quiet_masks);
> + quiet_applicable_to_access = (quiet_mask & missing) == missing;
I think it should be:
quiet_applicable_to_access = (quiet_mask & missing) == (handled_mask & missing);
We should have a test for this case: an access request (e.g. read-write)
is denied, half by one layer (e.g. read) and half by another (e.g.
write). Tests should cover this matrix.
> + } else {
> + /*
> + * Either the object is not quiet, or this is a scope request. We
> + * check request->type to distinguish between the two cases.
> + */
> + quiet_mask = youngest_denied->quiet_masks.scope;
const access_mask_t quiet_mask = youngest_denied->quiet_masks.scope;
> + switch (request->type) {
> + case LANDLOCK_REQUEST_SCOPE_SIGNAL:
> + quiet_applicable_to_access =
> + !!(quiet_mask & LANDLOCK_SCOPE_SIGNAL);
> + break;
> + case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET:
> + quiet_applicable_to_access =
> + !!(quiet_mask &
> + LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
> + break;
> + /*
> + * Leave LANDLOCK_REQUEST_PTRACE and
> + * LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are
> + * never quiet.
> + */
> + default:
> + break;
> + }
> + }
> +
> + if (quiet_applicable_to_access)
> + return;
> +
> /* Uses consistent allocation flags wrt common_lsm_audit(). */
> ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
> AUDIT_LANDLOCK_ACCESS);
^ permalink raw reply
* [PATCH v2] security: Rename functions and add namespace mapping tests
From: Ryan Foster @ 2025-11-21 17:48 UTC (permalink / raw)
To: serge, paul, linux-security-module; +Cc: linux-kernel, Ryan Foster
In-Reply-To: <20251110143748.4144288-1-foster.ryan.r@gmail.com>
Rename rootid_owns_currentns() to uid_owns_currentns() and
rootid_owns_userns() to uid_owns_ns() for clarity, as the function checks
any UID, not just root. Update all call sites accordingly.
Add tests that create actual user namespaces with different UID mappings
to verify namespace traversal logic. The tests create namespaces where
uid 0 maps to different kuids (e.g., kuid 1000, 2000) and verify that
uid_owns_ns() correctly identifies ownership based on the namespace
hierarchy traversal.
This addresses feedback to use clearer function naming and test actual
namespace functionality with real user namespace creation and mappings,
rather than just basic input validation.
---
security/commoncap.c | 26 ++--
security/commoncap_test.c | 286 ++++++++++++++++++++++++++++++++------
2 files changed, 254 insertions(+), 58 deletions(-)
diff --git a/security/commoncap.c b/security/commoncap.c
index 15d8147a34c4..cca291df9551 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -359,16 +359,16 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry)
}
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
-bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
-bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
+bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
+bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
#else
-static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
+static bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
#endif
{
struct user_namespace *iter;
for (iter = ns;; iter = iter->parent) {
- if (from_kuid(iter, kroot) == 0)
+ if (from_kuid(iter, kuid) == 0)
return true;
if (iter == &init_user_ns)
break;
@@ -378,19 +378,19 @@ static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
}
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
-bool rootid_owns_currentns(vfsuid_t rootvfsuid);
-bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+bool uid_owns_currentns(vfsuid_t vfsuid);
+bool uid_owns_currentns(vfsuid_t vfsuid)
#else
-static bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+static bool uid_owns_currentns(vfsuid_t vfsuid)
#endif
{
- kuid_t kroot;
+ kuid_t kuid;
- if (!vfsuid_valid(rootvfsuid))
+ if (!vfsuid_valid(vfsuid))
return false;
- kroot = vfsuid_into_kuid(rootvfsuid);
- return rootid_owns_userns(current_user_ns(), kroot);
+ kuid = vfsuid_into_kuid(vfsuid);
+ return uid_owns_ns(current_user_ns(), kuid);
}
static __u32 sansflags(__u32 m)
@@ -497,7 +497,7 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
goto out_free;
}
- if (!rootid_owns_currentns(vfsroot)) {
+ if (!uid_owns_currentns(vfsroot)) {
size = -EOVERFLOW;
goto out_free;
}
@@ -738,7 +738,7 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap,
/* Limit the caps to the mounter of the filesystem
* or the more limited uid specified in the xattr.
*/
- if (!rootid_owns_currentns(rootvfsuid))
+ if (!uid_owns_currentns(rootvfsuid))
return -ENODATA;
cpu_caps->permitted.val = le32_to_cpu(caps->data[0].permitted);
diff --git a/security/commoncap_test.c b/security/commoncap_test.c
index 962aa899455d..7f066dc0df5d 100644
--- a/security/commoncap_test.c
+++ b/security/commoncap_test.c
@@ -10,6 +10,8 @@
#include <linux/user_namespace.h>
#include <linux/uidgid.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/refcount.h>
/* Forward declare types and functions we need from mnt_idmapping.h
* We avoid including the full header because it contains inline functions
@@ -50,38 +52,38 @@ static inline kuid_t vfsuid_into_kuid(vfsuid_t vfsuid)
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
/* Forward declarations - functions are exported when KUNIT_TEST is enabled */
-extern bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
-extern bool rootid_owns_currentns(vfsuid_t rootvfsuid);
+extern bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
+extern bool uid_owns_currentns(vfsuid_t vfsuid);
/**
- * test_rootid_owns_currentns_init_ns - Test rootid_owns_currentns with init ns
+ * test_uid_owns_currentns_init_ns - Test uid_owns_currentns with init ns
*
- * Verifies that a root ID in the init namespace correctly owns the current
+ * Verifies that UID 0 in the init namespace correctly owns the current
* namespace when running in init_user_ns.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_init_ns(struct kunit *test)
+static void test_uid_owns_currentns_init_ns(struct kunit *test)
{
- vfsuid_t root_vfsuid;
- kuid_t root_kuid;
+ vfsuid_t vfsuid;
+ kuid_t kuid;
- /* Create a root UID in init namespace */
- root_kuid = KUIDT_INIT(0);
- root_vfsuid = VFSUIDT_INIT(root_kuid);
+ /* Create UID 0 in init namespace */
+ kuid = KUIDT_INIT(0);
+ vfsuid = VFSUIDT_INIT(kuid);
- /* In init namespace, root should own current namespace */
- KUNIT_EXPECT_TRUE(test, rootid_owns_currentns(root_vfsuid));
+ /* In init namespace, UID 0 should own current namespace */
+ KUNIT_EXPECT_TRUE(test, uid_owns_currentns(vfsuid));
}
/**
- * test_rootid_owns_currentns_invalid - Test rootid_owns_currentns with invalid vfsuid
+ * test_uid_owns_currentns_invalid - Test uid_owns_currentns with invalid vfsuid
*
* Verifies that an invalid vfsuid correctly returns false.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_invalid(struct kunit *test)
+static void test_uid_owns_currentns_invalid(struct kunit *test)
{
vfsuid_t invalid_vfsuid;
@@ -89,74 +91,268 @@ static void test_rootid_owns_currentns_invalid(struct kunit *test)
invalid_vfsuid = INVALID_VFSUID;
/* Invalid vfsuid should return false */
- KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(invalid_vfsuid));
+ KUNIT_EXPECT_FALSE(test, uid_owns_currentns(invalid_vfsuid));
}
/**
- * test_rootid_owns_currentns_nonroot - Test rootid_owns_currentns with non-root UID
+ * test_uid_owns_currentns_nonzero - Test uid_owns_currentns with non-zero UID
*
- * Verifies that a non-root UID correctly returns false.
+ * Verifies that a non-zero UID correctly returns false.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_nonroot(struct kunit *test)
+static void test_uid_owns_currentns_nonzero(struct kunit *test)
{
- vfsuid_t nonroot_vfsuid;
- kuid_t nonroot_kuid;
+ vfsuid_t vfsuid;
+ kuid_t kuid;
- /* Create a non-root UID */
- nonroot_kuid = KUIDT_INIT(1000);
- nonroot_vfsuid = VFSUIDT_INIT(nonroot_kuid);
+ /* Create a non-zero UID */
+ kuid = KUIDT_INIT(1000);
+ vfsuid = VFSUIDT_INIT(kuid);
- /* Non-root UID should return false */
- KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(nonroot_vfsuid));
+ /* Non-zero UID should return false */
+ KUNIT_EXPECT_FALSE(test, uid_owns_currentns(vfsuid));
}
/**
- * test_rootid_owns_userns_init_ns - Test rootid_owns_userns with init namespace
+ * test_uid_owns_ns_init_ns_uid0 - Test uid_owns_ns with init namespace and UID 0
*
- * Verifies that rootid_owns_userns correctly identifies root UID in init namespace.
- * This tests the core namespace traversal logic.
+ * Verifies that uid_owns_ns correctly identifies UID 0 in init namespace.
+ * This tests the core namespace traversal logic. In init namespace, UID 0
+ * maps to itself, so it should own the namespace.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_userns_init_ns(struct kunit *test)
+static void test_uid_owns_ns_init_ns_uid0(struct kunit *test)
{
- kuid_t root_kuid;
+ kuid_t kuid;
struct user_namespace *init_ns;
- root_kuid = KUIDT_INIT(0);
+ kuid = KUIDT_INIT(0);
init_ns = &init_user_ns;
- /* Root UID should own init namespace */
- KUNIT_EXPECT_TRUE(test, rootid_owns_userns(init_ns, root_kuid));
+ /* UID 0 should own init namespace */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
}
/**
- * test_rootid_owns_userns_nonroot - Test rootid_owns_userns with non-root UID
+ * test_uid_owns_ns_init_ns_nonzero - Test uid_owns_ns with init namespace and non-zero UID
*
- * Verifies that rootid_owns_userns correctly rejects non-root UIDs.
+ * Verifies that uid_owns_ns correctly rejects non-zero UIDs in init namespace.
+ * Only UID 0 should own a namespace.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_userns_nonroot(struct kunit *test)
+static void test_uid_owns_ns_init_ns_nonzero(struct kunit *test)
{
- kuid_t nonroot_kuid;
+ kuid_t kuid;
struct user_namespace *init_ns;
- nonroot_kuid = KUIDT_INIT(1000);
+ kuid = KUIDT_INIT(1000);
init_ns = &init_user_ns;
- /* Non-root UID should not own namespace */
- KUNIT_EXPECT_FALSE(test, rootid_owns_userns(init_ns, nonroot_kuid));
+ /* Non-zero UID should not own namespace */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+}
+
+/**
+ * test_uid_owns_ns_init_ns_various_uids - Test uid_owns_ns with various UIDs
+ *
+ * Verifies that uid_owns_ns correctly identifies only UID 0 as owning
+ * the namespace, regardless of the UID value tested.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_init_ns_various_uids(struct kunit *test)
+{
+ struct user_namespace *init_ns;
+ kuid_t kuid;
+
+ init_ns = &init_user_ns;
+
+ /* UID 0 should own the namespace */
+ kuid = KUIDT_INIT(0);
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
+
+ /* Other UIDs should not own the namespace */
+ kuid = KUIDT_INIT(1);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+
+ kuid = KUIDT_INIT(1000);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+
+ kuid = KUIDT_INIT(65534);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+}
+
+/**
+ * create_test_user_ns_with_mapping - Create a test user namespace with uid mapping
+ *
+ * Creates a minimal user namespace for testing where uid 0 in the namespace
+ * maps to the specified kuid in the parent namespace.
+ *
+ * The mapping semantics:
+ * - first: uid in this namespace (0)
+ * - lower_first: kuid in parent namespace (mapped_kuid)
+ * - count: range size (1)
+ *
+ * This means: from_kuid(ns, mapped_kuid) will return 0
+ * because map_id_up looks for kuid in [lower_first, lower_first+count)
+ * and returns first + (kuid - lower_first) = 0 + (mapped_kuid - mapped_kuid) = 0
+ *
+ * @test: KUnit test context
+ * @parent_ns: Parent user namespace
+ * @mapped_kuid: The kuid that uid 0 in the new namespace maps to
+ *
+ * Returns: The new user namespace, or NULL on failure
+ */
+static struct user_namespace *create_test_user_ns_with_mapping(struct kunit *test,
+ struct user_namespace *parent_ns,
+ kuid_t mapped_kuid)
+{
+ struct user_namespace *ns;
+ struct uid_gid_extent extent;
+
+ /* Allocate a test namespace - use kzalloc to zero all fields */
+ ns = kunit_kzalloc(test, sizeof(*ns), GFP_KERNEL);
+ if (!ns)
+ return NULL;
+
+ /* Initialize basic namespace structure fields */
+ ns->parent = parent_ns;
+ ns->level = parent_ns ? parent_ns->level + 1 : 0;
+ ns->owner = mapped_kuid;
+ ns->group = KGIDT_INIT(0);
+
+ /* Initialize ns_common structure */
+ refcount_set(&ns->ns.__ns_ref, 1);
+
+ /* Set up uid mapping: uid 0 in this namespace maps to mapped_kuid in parent
+ * Format: first (uid in ns) : lower_first (kuid in parent) : count
+ * So: uid 0 in ns -> kuid mapped_kuid in parent
+ * This means from_kuid(ns, mapped_kuid) returns 0
+ */
+ extent.first = 0; /* uid 0 in this namespace */
+ extent.lower_first = __kuid_val(mapped_kuid); /* maps to this kuid in parent */
+ extent.count = 1;
+
+ ns->uid_map.extent[0] = extent;
+ ns->uid_map.nr_extents = 1;
+
+ /* Set up gid mapping: gid 0 maps to gid 0 in parent (simplified) */
+ extent.first = 0;
+ extent.lower_first = 0;
+ extent.count = 1;
+
+ ns->gid_map.extent[0] = extent;
+ ns->gid_map.nr_extents = 1;
+
+ return ns;
+}
+
+/**
+ * test_uid_owns_ns_with_mapping - Test uid_owns_ns with namespace where uid 0
+ * maps to different kuid
+ *
+ * Creates a user namespace where uid 0 maps to kuid 1000 in the parent namespace.
+ * Verifies that uid_owns_ns correctly identifies kuid 1000 as owning the namespace.
+ *
+ * Note: uid_owns_ns walks up the namespace hierarchy, so it checks the current
+ * namespace first, then parent, then parent's parent, etc. So:
+ * - kuid 1000 owns test_ns because from_kuid(test_ns, 1000) == 0
+ * - kuid 0 also owns test_ns because from_kuid(init_user_ns, 0) == 0
+ * (checked in parent)
+ *
+ * This tests the actual functionality as requested: creating namespaces with
+ * different values for the namespace's uid 0.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_with_mapping(struct kunit *test)
+{
+ struct user_namespace *test_ns;
+ struct user_namespace *parent_ns;
+ kuid_t mapped_kuid, other_kuid;
+
+ parent_ns = &init_user_ns;
+ mapped_kuid = KUIDT_INIT(1000); /* uid 0 in test_ns maps to kuid 1000 */
+ other_kuid = KUIDT_INIT(2000); /* This should not own the namespace */
+
+ /* Create test namespace where uid 0 maps to kuid 1000 */
+ test_ns = create_test_user_ns_with_mapping(test, parent_ns, mapped_kuid);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_ns);
+
+ /* kuid 1000 should own the namespace (because uid 0 in test_ns maps to it) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, mapped_kuid));
+
+ /* kuid 0 also owns the namespace because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, KUIDT_INIT(0)));
+
+ /* Other kuids that don't map to 0 in test_ns or any parent should not own */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, other_kuid));
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, KUIDT_INIT(500)));
+}
+
+/**
+ * test_uid_owns_ns_with_different_mappings - Test with multiple namespaces
+ * having different mappings
+ *
+ * Creates multiple test namespaces with different uid 0 mappings to verify
+ * the function correctly identifies ownership based on the mapping.
+ *
+ * Since uid_owns_ns walks up the hierarchy, kuids that map to 0 in init_user_ns
+ * (like kuid 0) will own all namespaces. But we can still verify that the
+ * specific mapped kuids own their respective namespaces.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_with_different_mappings(struct kunit *test)
+{
+ struct user_namespace *ns1, *ns2, *ns3;
+ struct user_namespace *parent_ns;
+
+ parent_ns = &init_user_ns;
+
+ /* Namespace 1: uid 0 maps to kuid 1000 */
+ ns1 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(1000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns1);
+ /* kuid 1000 owns ns1 because it maps to uid 0 in ns1 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(1000)));
+ /* kuid 0 also owns ns1 because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(0)));
+ /* kuid 2000 doesn't map to 0 in ns1 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns1, KUIDT_INIT(2000)));
+
+ /* Namespace 2: uid 0 maps to kuid 2000 */
+ ns2 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(2000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns2);
+ /* kuid 2000 owns ns2 because it maps to uid 0 in ns2 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(2000)));
+ /* kuid 0 also owns ns2 because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(0)));
+ /* kuid 1000 doesn't map to 0 in ns2 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns2, KUIDT_INIT(1000)));
+
+ /* Namespace 3: uid 0 maps to kuid 0 (identity mapping) */
+ ns3 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(0));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns3);
+ /* kuid 0 owns ns3 because it maps to uid 0 in ns3 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns3, KUIDT_INIT(0)));
+ /* kuid 1000 doesn't map to 0 in ns3 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(1000)));
+ /* kuid 2000 doesn't map to 0 in ns3 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(2000)));
}
static struct kunit_case commoncap_test_cases[] = {
- KUNIT_CASE(test_rootid_owns_currentns_init_ns),
- KUNIT_CASE(test_rootid_owns_currentns_invalid),
- KUNIT_CASE(test_rootid_owns_currentns_nonroot),
- KUNIT_CASE(test_rootid_owns_userns_init_ns),
- KUNIT_CASE(test_rootid_owns_userns_nonroot),
+ KUNIT_CASE(test_uid_owns_currentns_init_ns),
+ KUNIT_CASE(test_uid_owns_currentns_invalid),
+ KUNIT_CASE(test_uid_owns_currentns_nonzero),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_uid0),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_nonzero),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_various_uids),
+ KUNIT_CASE(test_uid_owns_ns_with_mapping),
+ KUNIT_CASE(test_uid_owns_ns_with_different_mappings),
{}
};
--
2.43.0
^ permalink raw reply related
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Anirudh Venkataramanan @ 2025-11-21 19:13 UTC (permalink / raw)
To: Roberto Sassu, linux-integrity
Cc: Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <7722dff4e68ef5fb7f39bd732a8a77422bad5549.camel@huaweicloud.com>
On 11/20/2025 3:02 AM, Roberto Sassu wrote:
> On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
>> ==========================================================================
>>> A. Introduction |
>> ==========================================================================
>>
>> IMA events are kept in kernel memory and preserved across kexec soft
>> reboots. This can lead to increased kernel memory usage over time,
>> especially with aggressive IMA policies that measure everything. To reduce
>> memory pressure, it becomes necessary to discard IMA events but given that
>> IMA events are extended into PCRs in the TPM, just discarding events will
>> break the PCR extension chain, making future verification of the IMA event
>> log impossible.
>>
>> This patch series proposes a method to discard IMA events while keeping the
>> log verifiable. While reducing memory pressure is the primary objective,
>> the second order benefit of trimming the IMA log is that IMA log verifiers
>> (local userspace daemon or a remote cloud service) can process smaller IMA
>> logs on a rolling basis, thus avoiding re-verification of previously
>> verified events.
>
> Hi Anirudh
Hi Roberto,
Thanks for the feedback! Few questions below.
>
> I will rephrase this paragraph, to be sure that I understood it
> correctly.
>
> You are proposing a method to trim the measurement list and, at the
> same time, to keep the measurement list verifiable after trimming. The
> way you would like to achieve that is to keep the verification state in
> the kernel in the form of PCR values.
>
> Those values mean what verifiers have already verified. Thus, for the
> next verification attempt, verifiers take the current PCR values as
> starting values, replay the truncated IMA measurement list, and again
> you match with the current PCR values and you trim until that point.
>
> So the benefit of this proposal is that you keep the verification of
> the IMA measurement list self-contained by using the last verification
> state (PCR starting value) and the truncated IMA measurement list as
> the inputs of your verification.
Your understanding as described above is correct.
>
> Let me reiterate on the trusted computing principles IMA relies on for
> providing the evidence about a system's integrity.
>
> Unless you are at the beginning of the measurement chain, where the
> Root of Trust for Measurement (RTM) is trusted by assumption, the
> measurements done by a component can be trusted because that component
> was already measured by the previous component in the boot chain,
> before it had any chance to corrupt the system.
>
> In the context of IMA, IMA can be trusted to make new measurements
> because it measures every file before those files could cause any harm
> to the system. So, potentially IMA and the kernel can be corrupted by
> any file.
>
> What you are proposing would not work, because you are placing trust in
> an input (the PCR starting value) that can be manipulated at any time
> by a corrupted kernel, before you had the chance to detect such
> corruption.
If starting PCR values can be corrupted, the IMA measurements list can
also be corrupted, right?
More generally, what integrity guarantees can be provided (if any) if
the kernel itself is corrupted?
>
> Let me describe a scenario where I could take advantage of such
> weakness. After the first measurement list trim, I perform an attack on
> the system such that it corrupts the kernel. IMA added a new entry in
> the measurement list, which would reveal the attack.
>
> But, since I have control of the kernel, I conveniently update the PCR
> starting value to replay the new measurement entry, and remove the
> measurement entry from the measurement list.
>
> Now, from the perspective of the user space verifiers everything is
> fine, the truncated IMA measurement list is clean, no attack, and the
> current PCR values match by replaying the new PCR starting value with
> the remaining of the IMA measurement list.
Wouldn't the verifier detect the attack when it sees that its
recalculated PCR values don't match up to the PCR digest in the TPM quote?
>
> So, in my opinion the kernel should just offer the ability to trim the
> measurement list, and a remote verifier should be responsible to verify
> the measurement list, without relying on anything from the system being
> evaluated.
>
> Sure, the remote verifier can verify just the trimmed IMA measurement
> list, but the remote verifier must solely rely on state information
> maintained internally.
>
> Roberto
>
>> The method has other advantages too:
>>
>> 1. It provides a userspace interface that can be used to precisely control
>> trim point, allowing for trim points to be optionally aligned with
>> userspace IMA event log validation.
>>
>> 2. It ensures that all necessary information required for continued IMA
>> log validation is made available via the userspace interface at all
>> times.
>>
>> 3. It provides a simple mechanism for userspace applications to determine
>> if the event log has been unexpectedly trimmed.
>>
>> 4. The duration for which the IMA Measurement list mutex must be held (for
>> trimming) is minimal.
>>
>> ==========================================================================
>>> B. Solution |
>> ==========================================================================
>>
>> --------------------------------------------------------------------------
>>> B.1 Overview |
>> --------------------------------------------------------------------------
>>
>> The kernel trims the IMA event log based on PCR values supplied by userspace.
>> The core principles leveraged are as follows:
>>
>> - Given an IMA event log, PCR values for each IMA event can be obtained by
>> recalulating the PCR extension for each event. Thus processing N events
>> from the start will yield PCR values as of event N. This is referred to
>> as "IMA event log replay".
>>
>> - To get the PCR value for event N + 1, only the PCR value as of event N
>> is needed. If this can be known, events till and including N can be
>> safely purged.
>>
>> Putting it all together, we get the following userspace + kernel flow:
>>
>> 1. A userspace application replays the IMA event log to generate PCR
>> values and then triggers a trim by providing these values to the kernel
>> (by writing to a pseudo file).
>>
>> Optionally, the userspace application may verify these PCR values
>> against the corresponding TPM quote, and trigger trimming only if
>> the calculated PCR values match up to the expectations in the quote's
>> PCR digest.
>>
>> 2. The kernel uses the userspace supplied PCR values to trim the IMA
>> measurements list at a specific point, and so these are referred to as
>> "trim-to PCR values" in this context.
>>
>> Note that the kernel doesn't really understand what these userspace
>> provided PCR values mean or what IMA event they correspond to, and so
>> it does its own IMA event replay till either the replayed PCR values
>> match with the userspace provided ones, or it runs out of events.
>>
>> If a match is found, the kernel can proceed with trimming the IMA
>> measurements list. This is done in two steps, to keep locking context
>> minimal.
>>
>> step 1: Find and return the list entry (as a count from head) of exact
>> match. This does not lock the measurements list mutex, ensuring
>> new events can be appended to the log.
>>
>> step 2: Lock the measurements list mutex and trim the measurements list
>> at the previously identified list entry.
>>
>> If the trim is successful, the trim-to PCR values are saved as "starting
>> PCR values". The next time userspace wants to replay the IMA event log,
>> it will use the starting PCR values as the base for the IMA event log
>> replay.
>>
>> --------------------------------------------------------------------------
>>> B.2 Kernel Interfaces |
>> --------------------------------------------------------------------------
>>
>> A new configfs pseudo file /sys/kernel/config/ima/pcrs that supports the
>> following operations is exposed.
>>
>> read: returns starting PCR values stored in the kernel (within IMA
>> specifically).
>>
>> write: writes trim-to PCR values to trigger trimming. If trimming is
>> successful, trim-to PCR values are stored as starting PCR values.
>> requires root privileges.
>>
>> --------------------------------------------------------------------------
>>> B.3 Walk-through with a real example |
>> --------------------------------------------------------------------------
>>
>> This is a real example from a test run.
>>
>> Suppose this IMA policy is deployed:
>>
>> measure func=FILE_CHECK mask=MAY_READ pcr=10
>> measure func=FILE_CHECK mask=MAY_WRITE pcr=11
>>
>> When the policy is deployed, a zero digest starting PCR value will be set
>> for each PCR used. If the TPM supports multiple hashbanks, there will be
>> one starting PCR value per PCR, per TPM hashbank. This can be seen in the
>> following hexdump:
>>
>> $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
>> 00000000 70 63 72 31 30 3a 73 68 61 31 3a 00 00 00 00 00 |pcr10:sha1:.....|
>> 00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 |...............p|
>> 00000020 63 72 31 31 3a 73 68 61 31 3a 00 00 00 00 00 00 |cr11:sha1:......|
>> 00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 63 |..............pc|
>> 00000040 72 31 30 3a 73 68 61 32 35 36 3a 00 00 00 00 00 |r10:sha256:.....|
>> 00000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000060 00 00 00 00 00 00 00 00 00 00 00 70 63 72 31 31 |...........pcr11|
>> 00000070 3a 73 68 61 32 35 36 3a 00 00 00 00 00 00 00 00 |:sha256:........|
>> 00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000090 00 00 00 00 00 00 00 00 70 63 72 31 30 3a 73 68 |........pcr10:sh|
>> 000000a0 61 33 38 34 3a 00 00 00 00 00 00 00 00 00 00 00 |a384:...........|
>> 000000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 000000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 000000d0 00 00 00 00 00 70 63 72 31 31 3a 73 68 61 33 38 |.....pcr11:sha38|
>> 000000e0 34 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |4:..............|
>> 000000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000100 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000110 00 00 |..|
>> 00000112
>>
>> Let's say that a userspace utility replays the IMA event log, and triggers
>> trimming by writing the following PCR values (i.e. trim-to PCR values) to the
>> pseudo file:
>>
>> pcr10:sha256:8268782906555cf3aefc179f815c878527dd4e67eaa836572ebabab31977922c
>> pcr11:sha256:4c7f31927183eacb53d51d95b0162916fd3fca51a8d1efc6dde3805eb891fe41
>>
>> The trim is successful,
>>
>> 1. Some number of entries from the measurements log will disappear. This
>> can be verified by reading out the ASCII or binary IMA measurements
>> file.
>>
>> 2. The trim-to PCR values are saved as starting PCR values. This can be
>> verified by reading out the pseudo file again as shown below. Note that
>> even through only sha256 PCR values were written, the kernel populated
>> sha1 and sha384 starting values as well.
>>
>> $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
>>
>> 00000000 70 63 72 31 30 3a 73 68 61 31 3a c4 7f 9d 00 68 |pcr10:sha1:....h|
>> 00000010 e4 86 71 bf bc ae f0 10 12 ff 68 e2 9e 74 e4 70 |..q.......h..t.p|
>> 00000020 63 72 31 31 3a 73 68 61 31 3a 90 d7 17 ac 60 4d |cr11:sha1:....`M|
>> 00000030 c8 25 ce 77 7d 9d 94 cf 44 7b b2 2e 2e e2 70 63 |.%.w}...D{....pc|
>> 00000040 72 31 30 3a 73 68 61 32 35 36 3a 82 68 78 29 06 |r10:sha256:.hx).|
>> 00000050 55 5c f3 ae fc 17 9f 81 5c 87 85 27 dd 4e 67 ea |U\......\..'.Ng.|
>> 00000060 a8 36 57 2e ba ba b3 19 77 92 2c 70 63 72 31 31 |.6W.....w.,pcr11|
>> 00000070 3a 73 68 61 32 35 36 3a 4c 7f 31 92 71 83 ea cb |:sha256:L.1.q...|
>> 00000080 53 d5 1d 95 b0 16 29 16 fd 3f ca 51 a8 d1 ef c6 |S.....)..?.Q....|
>> 00000090 dd e3 80 5e b8 91 fe 41 70 63 72 31 30 3a 73 68 |...^...Apcr10:sh|
>> 000000a0 61 33 38 34 3a 8e d6 12 18 b1 d6 cd 95 16 98 33 |a384:..........3|
>> 000000b0 2b 7d a2 d6 d9 05 c7 e8 5b 15 b0 91 c5 fc 23 d1 |+}......[.....#.|
>> 000000c0 f9 a8 8d 60 50 5c e9 64 5f d7 b3 b2 f1 9c 90 0a |...`P\.d_.......|
>> 000000d0 45 53 5d b2 57 70 63 72 31 31 3a 73 68 61 33 38 |ES].Wpcr11:sha38|
>> 000000e0 34 3a 25 fc 21 28 31 5a f7 c6 fb 0f 40 c9 06 e6 |4:%.!(1Z....@...|
>> 000000f0 c5 da ed 20 61 a1 03 54 4f 67 18 88 82 0f 48 d1 |... a..TOg....H.|
>> 00000100 2f e0 3d 36 46 5e 94 a4 88 51 f8 91 39 7e e5 97 |/.=6F^...Q..9~..|
>> 00000110 2c c5 |,.|
>> 00000112
>>
>> --------------------------------------------------------------------------
>>> C. Footnotes |
>> --------------------------------------------------------------------------
>>
>> 1. The 'pcrs' pseudo file is currently part of configfs. This was due to
>> some early internal feedback in a different context. This can as well be
>> in securityfs with the rest of the IMA pseudo files.
>>
>> 2. PCR values are never read out of the TPM at any point. All PCR values
>> used are derived from IMA event log replay.
>>
>> 3. Code is "RFC quality". Refinements can be made if the method is accepted.
>>
>> 4. For functional validation, base kernel version was 6.17 stable, with the
>> most recent tested version being 6.17.8.
>>
>> 5. Code has been validated to some degree using a python-based internal test
>> tool. This can be published if there is community interest.
>>
>> Steven Chen (1):
>> ima: Implement IMA event log trimming
>>
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/ima/Kconfig | 13 +
>> drivers/ima/Makefile | 2 +
>> drivers/ima/ima_config_pcrs.c | 291 ++++++++++++++++++
>> include/linux/ima.h | 27 ++
>> security/integrity/ima/Makefile | 4 +
>> security/integrity/ima/ima.h | 8 +
>> security/integrity/ima/ima_init.c | 44 +++
>> security/integrity/ima/ima_log_trim.c | 421 ++++++++++++++++++++++++++
>> security/integrity/ima/ima_policy.c | 7 +-
>> security/integrity/ima/ima_queue.c | 5 +-
>> 12 files changed, 821 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/ima/Kconfig
>> create mode 100644 drivers/ima/Makefile
>> create mode 100644 drivers/ima/ima_config_pcrs.c
>> create mode 100644 security/integrity/ima/ima_log_trim.c
>>
>
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-21 19:19 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <GV2PPF74270EBEED0840E45459881C0EDD4E4D5A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> On 11/21/25 10:35, Bernd Edlinger wrote:
>> On 11/21/25 08:18, Eric W. Biederman wrote:
>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>
>>>> Hi Eric,
>>>>
>>>> thanks for you valuable input on the topic.
>>>>
>>>> On 11/21/25 00:50, Eric W. Biederman wrote:
>>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>>>
>>>>>> Instead of computing the new cred before we pass the point of no
>>>>>> return compute the new cred just before we use it.
>>>>>>
>>>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>>>>
>>>>>> I am not certain why we wanted to compute the cred for the new
>>>>>> executable so early. Perhaps I missed something but I did not see any
>>>>>> common errors being signaled. So I don't think we loose anything by
>>>>>> computing the new cred later.
>>>>>
>>>>> I should add that the permission checks happen in open_exec,
>>>>> everything that follows credential wise is just about representing in
>>>>> struct cred the credentials the new executable will have.
>>>>>
>>>>> So I am really at a loss why we have had this complicated way of
>>>>> computing of computed the credentials all of these years full of
>>>>> time of check to time of use problems.
>>>>>
>>>>
>>>> Well, I think I see a problem with your patch:
>>>>
>>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
>>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
>>>> or the smack security engines at least. Previously that callback
>>>> was called before the point of no return, and the return code should
>>>> be returned as a return code the the caller of execve. But if we move
>>>> that check after the point of no return, the caller will get killed
>>>> due to the failed security check.
>>>>
>>>> Or did I miss something?
>>>
>>> I think we definitely need to document this change in behavior. I would
>>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
>>> issue. The exec is failing one way or the other so I don't see it as a
>>> correctness issue.
>>>
>>> In the case of ptrace in general I think it is a bug if the mere act of
>>> debugging a program changes it's behavior. So which buggy behavior
>>> should we prefer? SIGSEGV where it is totally clear that the behavior
>>> has changed or -EPERM and ask the debugged program to handle it.
>>> I lean towards SIGSEGV because then it is clear the code should not
>>> handle it.
>>>
>>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
>>> handle unexpected things happening is to terminate the application.
>>>
>>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
>>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
>>> of CLONE_THREAD.
>>>
>>>
>>> Plus all of these things are only considerations if we are exec'ing a
>>> program that transitions to a different set of credentials. Something
>>> that happens but is quite rare itself.
>>>
>>> In practice I don't expect there is anything that depends on the exact
>>> behavior of what happens when exec'ing a suid executable to gain
>>> privileges when ptraced. The closes I can imagine is upstart and
>>> I think upstart ran as root when ptracing other programs so there is no
>>> gaining of privilege and thus no reason for a security module to
>>> complain.
>>>
>>> Who knows I could be wrong, and someone could actually care. Which is
>>> hy I think we should document it.>>
>>
>>
>> Well, I dont know for sure, but the security engine could deny the execution
>> for any reason, not only because of being ptraced.
>> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
>>
>>
>> Bernd.
>>
>
> Hmm, funny..
>
> I installed this patch on top of
>
> commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
> Merge: c966813ea120 7b6216baae75
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Thu Nov 20 11:04:37 2025 -0800
>
> but it does panic when I try to boot:
>
> [ 0.870539] TERM=1inux
> [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
> [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
> [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
> [ 0.870580] Call Trace:
> [ 0.870590] <TASK>
> [ 0.870592] vpanic+0x36d/0x380
> [ 0.870607] ? __pfx_kernel_init+0x10/0x10
> [ 0.870615] panic+0x5b/0x60
> [ 0.870617] kernel_init+0x17d/0x1c0
> [ 0.870623] ret_from_fork+0x124/0x150
> [ 0.870625} ? __pfx_kernel_init+0x10/0x10
> [ 0.870627] ret_from_fork_asm+0x1a/0x30
> [ 0.870632] </TASK>
> [ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
> [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
>
>
> Is that a known problem?
Nope. It looks like the code needs a little bit bug fixing testing.
I will take see about taking a look.
Eric
^ permalink raw reply
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Anirudh Venkataramanan @ 2025-11-21 19:53 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <629289c119a1a71d8d7a1208ec3676e006d4d527.camel@linux.ibm.com>
On 11/20/2025 2:58 PM, Mimi Zohar wrote:
> On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
>>
>>
>> 2. The kernel uses the userspace supplied PCR values to trim the IMA
>> measurements list at a specific point, and so these are referred to as
>> "trim-to PCR values" in this context.
>>
>> Note that the kernel doesn't really understand what these userspace
>> provided PCR values mean or what IMA event they correspond to, and so
>> it does its own IMA event replay till either the replayed PCR values
>> match with the userspace provided ones, or it runs out of events.
>>
>> If a match is found, the kernel can proceed with trimming the IMA
>> measurements list. This is done in two steps, to keep locking context
>> minimal.
>>
>> step 1: Find and return the list entry (as a count from head) of exact
>> match. This does not lock the measurements list mutex, ensuring
>> new events can be appended to the log.
>>
>> step 2: Lock the measurements list mutex and trim the measurements list
>> at the previously identified list entry.
>>
>> If the trim is successful, the trim-to PCR values are saved as "starting
>> PCR values". The next time userspace wants to replay the IMA event log,
>> it will use the starting PCR values as the base for the IMA event log
>> replay.
>
> Depending on the IMA policy, the IMA measurement list may contain integrity
> violations, such as "ToM/ToU" (Time of Measurement/Time of Use) or "open-
> writer". Either the userspace supplied PCR values will not match any measurement
> record or the PCR values will be "fixed" to match the well known violation hash
> value extended into the TPM. Depending on how the userspace PCR values will
> subsequently be used, saving the "fixed" PCR values could result in whitewashing
> the integrity of the measurement list.
>
Yes, we account for this. IMA documentation [1] notes that:
"An all zeros hash indicates a measurement log violation. IMA is
invalidating an entry. Trust in entries after that are up to the end
user. If the Template Data Hash is all zeros, an all ones digest is
extended."
A userspace verifier could be designed with a user option like
"--ignore-violations". This would extend a digest of all ones in the
event replay process and arrive at the same per-event PCR values that
the TPM originally did. The trimming logic in the kernel would do the
same thing.
By "whitewashing", you mean that violations will also get trimmed? Given
that trust in entries post violation is up to the user, is this a
problem? The IMA log itself can still be saved by userspace for further
analysis, auditing, etc. The point of trimming is to get it out of the
kernel memory.
Thanks!
Ani
[1]
https://ima-doc.readthedocs.io/en/latest/event-log-format.html#template-data-hash
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use pr_fmt()
From: Jarkko Sakkinen @ 2025-11-21 20:10 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <83C83079-0354-4642-A980-DBC7AE572A53@linux.dev>
On Wed, Nov 19, 2025 at 03:45:02PM +0100, Thorsten Blum wrote:
> On 19. Nov 2025, at 03:48, Jarkko Sakkinen wrote:
> > On Thu, Nov 13, 2025 at 01:35:44PM +0100, Thorsten Blum wrote:
> >> Use pr_fmt() to automatically prefix all pr_<level>() log messages with
> >
> > This fails to describe what "use" means.
>
> I don't understand what you mean. What's wrong with "use ... to ..."?
I think e.g., "Rewrite the definition of ..." describes better what
you're doing.
>
> >> "encrypted_key: " and remove all manually added prefixes.
> >>
> >> Reformat the code accordingly and avoid line breaks in log messages.
> >>
> >> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >> ---
> >> security/keys/encrypted-keys/encrypted.c | 74 +++++++++++-------------
> >> security/keys/encrypted-keys/encrypted.h | 2 +-
> >> 2 files changed, 35 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> >> index 513c09e2b01c..a8e8bf949b4b 100644
> >> --- a/security/keys/encrypted-keys/encrypted.c
> >> +++ b/security/keys/encrypted-keys/encrypted.c
> >> @@ -11,6 +11,8 @@
> >> * See Documentation/security/keys/trusted-encrypted.rst
> >> */
> >>
> >
> > Should have undef prepending.
>
> Why is this necessary when the #define is at the top of a source file?
> The kernel documentation [1] doesn't mention this anywhere. Isn't #undef
> only needed when redefining 'pr_fmt' in the middle of a file to avoid a
> compiler warning/error?
>
> >> +#define pr_fmt(fmt) "encrypted_key: " fmt
> >> +
> >> [...]
>
> Thanks,
> Thorsten
>
> [1] https://docs.kernel.org/core-api/printk-basics.html
>
BR, Jarkko
^ permalink raw reply
* [GIT PULL] selinux/selinux-pr-20251121
From: Paul Moore @ 2025-11-21 20:57 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Three SELinux patches for v6.18 to fix issues around accessing the
per-task decision cache that we introduced in v6.16 to help reduce
SELinux overhead on path walks. The problem was that despite the
cache being located in the SELinux "task_security_struct", the
parent struct wasn't actually tied to the task, it was tied to a cred.
Historically SELinux did locate the task_security_struct in the
task_struct's security blob, but it was later relocated to the cred
struct when the cred work happened, as it made the most sense at the
time. Unfortunately we never did the task_security_struct to
cred_security_struct rename work (avoid code churn maybe? who knows)
because it didn't really matter at the time. However, it suddenly
became a problem when we added a per-task cache to a per-cred object
and didn't notice because of the old, no-longer-correct struct naming.
Thanks to KCSAN for flagging this, as the silly humans running
things forgot that the task_security_struct was a big lie.
This pull request contains three patches, only one of which actually
fixes the problem described above and moves the SELinux decision cache
from the per-cred struct to a newly (re)created per-task struct. The
other two patches, which form the bulk of the diffstat, take care of
the associated renaming tasks so we can hopefully avoid making the
same stupid mistake in the future. For the record, I did contemplate
sending just a fix for the cache, leaving the renaming patches for the
upcoming merge window, but the type/variable naming ended up being
pretty awful and would have made v6.18 an outlier stuck between the
"old" names and the "new" names in v6.19. The renaming patches are
also fairly mechanical/trivial and shouldn't pose much risk despite
their size.
TLDR; naming things may be hard, but if you mess it up bad things
happen.
Paul
--
The following changes since commit 211ddde0823f1442e4ad052a2f30f050145ccada:
Linux 6.18-rc2 (2025-10-19 15:19:16 -1000)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20251121
for you to fetch changes up to 3ded250b97c3ae94a642bc2e710a95700e72dfb0:
selinux: rename the cred_security_struct variables to "crsec"
(2025-11-20 16:47:50 -0500)
----------------------------------------------------------------
selinux/stable-6.18 PR 20251121
----------------------------------------------------------------
Paul Moore (1):
selinux: rename the cred_security_struct variables to "crsec"
Stephen Smalley (2):
selinux: rename task_security_struct to cred_security_struct
selinux: move avdcache to per-task security struct
security/selinux/hooks.c | 251 +++++++++++++++---------------
security/selinux/include/objsec.h | 22 +-
2 files changed, 144 insertions(+), 129 deletions(-)
--
paul-moore.com
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Ryan Lee @ 2025-11-21 23:06 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, apparmor
In-Reply-To: <87ikf3w5us.fsf@email.froward.int.ebiederm.org>
On Fri, Nov 21, 2025 at 11:20 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>
> > On 11/21/25 10:35, Bernd Edlinger wrote:
> >> On 11/21/25 08:18, Eric W. Biederman wrote:
> >>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>>
> >>>> Hi Eric,
> >>>>
> >>>> thanks for you valuable input on the topic.
> >>>>
> >>>> On 11/21/25 00:50, Eric W. Biederman wrote:
> >>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> >>>>>
> >>>>>> Instead of computing the new cred before we pass the point of no
> >>>>>> return compute the new cred just before we use it.
> >>>>>>
> >>>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
> >>>>>>
> >>>>>> I am not certain why we wanted to compute the cred for the new
> >>>>>> executable so early. Perhaps I missed something but I did not see any
> >>>>>> common errors being signaled. So I don't think we loose anything by
> >>>>>> computing the new cred later.
> >>>>>
> >>>>> I should add that the permission checks happen in open_exec,
> >>>>> everything that follows credential wise is just about representing in
> >>>>> struct cred the credentials the new executable will have.
> >>>>>
> >>>>> So I am really at a loss why we have had this complicated way of
> >>>>> computing of computed the credentials all of these years full of
> >>>>> time of check to time of use problems.
> >>>>>
> >>>>
> >>>> Well, I think I see a problem with your patch:
> >>>>
> >>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
> >>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
> >>>> or the smack security engines at least. Previously that callback
> >>>> was called before the point of no return, and the return code should
> >>>> be returned as a return code the the caller of execve. But if we move
> >>>> that check after the point of no return, the caller will get killed
> >>>> due to the failed security check.
> >>>>
> >>>> Or did I miss something?
> >>>
> >>> I think we definitely need to document this change in behavior. I would
> >>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
> >>> issue. The exec is failing one way or the other so I don't see it as a
> >>> correctness issue.
> >>>
> >>> In the case of ptrace in general I think it is a bug if the mere act of
> >>> debugging a program changes it's behavior. So which buggy behavior
> >>> should we prefer? SIGSEGV where it is totally clear that the behavior
> >>> has changed or -EPERM and ask the debugged program to handle it.
> >>> I lean towards SIGSEGV because then it is clear the code should not
> >>> handle it.
> >>>
> >>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
> >>> handle unexpected things happening is to terminate the application.
> >>>
> >>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
> >>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
> >>> of CLONE_THREAD.
> >>>
> >>>
> >>> Plus all of these things are only considerations if we are exec'ing a
> >>> program that transitions to a different set of credentials. Something
> >>> that happens but is quite rare itself.
AppArmor's exec rules rely heavily on transitioning to different creds
on exec. For example, an AppArmor policy like
profile example_1 /usr/bin/example_1 {
/usr/bin/example_2 Px -> example_2_profile,
/usr/bin/example_3 Px,
}
will allow binary example_1 to execute binaries example_2 and
example_3, launching those processes under a different confinement
(example_2_profile and a profile that attaches to /usr/bin/example_3,
respectively). We will need to look into how much this patch (or a
corresponding change in behavior) would affect our use case, but
confinement transitions (where the confinement information is stored
as an LSM blob on the cred struct) are extremely common in a system
that uses AppArmor as an LSM.
> >>>
> >>> In practice I don't expect there is anything that depends on the exact
> >>> behavior of what happens when exec'ing a suid executable to gain
> >>> privileges when ptraced. The closes I can imagine is upstart and
> >>> I think upstart ran as root when ptracing other programs so there is no
> >>> gaining of privilege and thus no reason for a security module to
> >>> complain.
> >>>
> >>> Who knows I could be wrong, and someone could actually care. Which is
> >>> hy I think we should document it.>>
> >>
> >>
> >> Well, I dont know for sure, but the security engine could deny the execution
> >> for any reason, not only because of being ptraced.
> >> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
> >>
> >>
> >> Bernd.
> >>
> >
> > Hmm, funny..
> >
> > I installed this patch on top of
> >
> > commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
> > Merge: c966813ea120 7b6216baae75
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date: Thu Nov 20 11:04:37 2025 -0800
> >
> > but it does panic when I try to boot:
> >
> > [ 0.870539] TERM=1inux
> > [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
> > [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
> > [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
> > [ 0.870580] Call Trace:
> > [ 0.870590] <TASK>
> > [ 0.870592] vpanic+0x36d/0x380
> > [ 0.870607] ? __pfx_kernel_init+0x10/0x10
> > [ 0.870615] panic+0x5b/0x60
> > [ 0.870617] kernel_init+0x17d/0x1c0
> > [ 0.870623] ret_from_fork+0x124/0x150
> > [ 0.870625} ? __pfx_kernel_init+0x10/0x10
> > [ 0.870627] ret_from_fork_asm+0x1a/0x30
> > [ 0.870632] </TASK>
> > [ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
> > [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
> >
> >
> > Is that a known problem?
>
> Nope. It looks like the code needs a little bit bug fixing testing.
>
> I will take see about taking a look.
>
> Eric
>
I've also CC'ed the AppArmor mailing list on this patch to facilitate
discussion if, upon further investigation, this patch would require
changes or cause other problems on the AppArmor side.
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Günther Noack @ 2025-11-22 10:16 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-13-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index e22e10edb103..d1a004c2e0f5 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
> }
> }
>
> +static int test_socketpair(int family, int type, int protocol)
> +{
> + int fds[2];
> + int err;
> +
> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
> + if (err)
> + return errno;
> + /*
> + * Mixing error codes from close(2) and socketpair(2) should not lead to
> + * any (access type) confusion for this test.
> + */
> + if (close(fds[0]) != 0)
> + return errno;
> + if (close(fds[1]) != 0)
> + return errno;
Very minor nit: the function leaks an FD if it returns early after the
first close() call failed. (Highly unlikely to happen though.)
> + return 0;
> +}
> +
> +TEST_F(mini, socketpair)
> +{
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_socket_attr unix_socket_create = {
> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> + .family = AF_UNIX,
> + .type = SOCK_STREAM,
> + .protocol = 0,
> + };
> + int ruleset_fd;
> +
> + /* Tries to create socket when ruleset is not established. */
> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> + &unix_socket_create, 0));
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is allowed */
> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
You may want to check that landlock_create_ruleset() succeeded here:
ASSERT_LE(0, ruleset_fd)
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is restricted. */
> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +}
> +
> TEST_HARNESS_MAIN
> --
> 2.34.1
>
Otherwise, looks good.
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Mikhail Ivanov @ 2025-11-22 10:21 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.4795c4c3bb03@gnoack.org>
On 11/22/2025 1:16 PM, Günther Noack wrote:
> On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index e22e10edb103..d1a004c2e0f5 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
>> }
>> }
>>
>> +static int test_socketpair(int family, int type, int protocol)
>> +{
>> + int fds[2];
>> + int err;
>> +
>> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
>> + if (err)
>> + return errno;
>> + /*
>> + * Mixing error codes from close(2) and socketpair(2) should not lead to
>> + * any (access type) confusion for this test.
>> + */
>> + if (close(fds[0]) != 0)
>> + return errno;
>> + if (close(fds[1]) != 0)
>> + return errno;
>
> Very minor nit: the function leaks an FD if it returns early after the
> first close() call failed. (Highly unlikely to happen though.)
Yeah, but AFAIK fd[0] may be leaked anyway if close() fails. Anyway
this shouldn't be an issue for tests.
>
>> + return 0;
>> +}
>> +
>> +TEST_F(mini, socketpair)
>> +{
>> + const struct landlock_ruleset_attr ruleset_attr = {
>> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + };
>> + const struct landlock_socket_attr unix_socket_create = {
>> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + .family = AF_UNIX,
>> + .type = SOCK_STREAM,
>> + .protocol = 0,
>> + };
>> + int ruleset_fd;
>> +
>> + /* Tries to create socket when ruleset is not established. */
>> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> + ruleset_fd =
>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> + ASSERT_LE(0, ruleset_fd);
>> +
>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> + &unix_socket_create, 0));
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is allowed */
>> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +
>> + ruleset_fd =
>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>
> You may want to check that landlock_create_ruleset() succeeded here:
>
> ASSERT_LE(0, ruleset_fd)
thanks, I'll fix it.
>
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is restricted. */
>> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +}
>> +
>> TEST_HARNESS_MAIN
>> --
>> 2.34.1
>>
>
> Otherwise, looks good.
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Günther Noack @ 2025-11-22 10:49 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-2-ivanov.mikhail1@huawei-partners.com>
Hello!
On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
> It is possible to create sockets of the same protocol with different
> protocol number values. For example, TCP sockets can be created using one
> of the following commands:
> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
> protocol of the given protocol family and can be mapped to another
> value.
>
> Socket rules do not perform such mappings to not increase complexity
> of rules definition and their maintenance.
Minor phrasing nit: Maybe we can phrase this constructively, like
"rules operate on the socket(2) parameters as they are passed by the
user, before this mapping happens"?
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f030adc462ee..030c96cb5d25 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
> * flags`_).
> */
> __u64 handled_access_net;
> + /**
> + * @handled_access_socket: Bitmask of handled actions performed on sockets
> + * (cf. `Socket flags`).
> + */
> + __u64 handled_access_socket;
This struct can only be extended at the end, for ABI compatibility reasons.
In the call to landlock_create_ruleset(2), the user passes the __user
pointer to this struct along with its size (as known to the user at
compile time). When we copy this into the kernel, we blank out the
struct and only copy the prefix of the caller-supplied size. The
implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
When you rearrange the order, please also update it in other places
where these fields are mentioned next to each other, for
consistency. I'll try to point it out where I see it in the review,
but I might miss some places.
> /**
> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> * restricting a Landlock domain from accessing outside
> @@ -140,6 +145,11 @@ enum landlock_rule_type {
> * landlock_net_port_attr .
> */
> LANDLOCK_RULE_NET_PORT,
> + /**
> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
> + * landlock_socket_attr.
^
Nit: Adjacent documentation has a space before the dot.
I assume this is needed for kernel doc formatting?
> + */
> + LANDLOCK_RULE_SOCKET,
> };
>
> /**
> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
> __u64 port;
> };
>
> +/**
> + * struct landlock_socket_attr - Socket protocol definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_socket_attr {
> + /**
> + * @allowed_access: Bitmask of allowed access for a socket protocol
> + * (cf. `Socket flags`_).
> + */
> + __u64 allowed_access;
> + /**
> + * @family: Protocol family used for communication
> + * (cf. include/linux/socket.h).
> + */
> + __s32 family;
> + /**
> + * @type: Socket type (cf. include/linux/net.h)
> + */
> + __s32 type;
> + /**
> + * @protocol: Communication protocol specific to protocol family set in
> + * @family field.
This is specific to both the @family and the @type, not just the @family.
From socket(2):
Normally only a single protocol exists to support a particular
socket type within a given protocol family.
For instance, in your commit message above the protocol in the example
is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
with SOCK_DGRAM.
> + */
> + __s32 protocol;
> +} __attribute__((packed));
Since we are in the UAPI header, please also document the wildcard
values for @type and @protocol.
(Remark, should those be exposed as constants?)
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d..03ccd6fbfe83 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> /* Makes sure all network access rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all socket access rights can be stored. */
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
> /* Makes sure all scoped rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> struct access_masks {
> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
> access_mask_t scope : LANDLOCK_NUM_SCOPE;
(Please re-adjust field order for consistency with UAPI)
> };
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index dfcdc19ea268..a34d2dbe3954 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> return new_ruleset;
> }
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t fs_access_mask,
> - const access_mask_t net_access_mask,
> - const access_mask_t scope_mask)
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
(Please re-adjust field order for consistency with UAPI)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask && !net_access_mask && !scope_mask)
> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
> + !scope_mask)
(Please re-adjust field order for consistency with UAPI)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (IS_ERR(new_ruleset))
> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> if (net_access_mask)
> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> + if (socket_access_mask)
> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
> + 0);
(Please re-adjust order of these "if"s for consistency with UAPI)
> if (scope_mask)
> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
> return new_ruleset;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 1a78cba662b2..a60ede2fc2a5 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -189,10 +204,9 @@ struct landlock_ruleset {
> };
> };
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t access_mask_fs,
> - const access_mask_t access_mask_net,
> - const access_mask_t scope_mask);
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
(Please re-adjust field order for consistency with UAPI)
> index 000000000000..28a80dcad629
> --- /dev/null
> +++ b/security/landlock/socket.c
> @@ -0,0 +1,105 @@
> [...]
> +#define TYPE_ALL (-1)
> +#define PROTOCOL_ALL (-1)
Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 33eafb71e4f3..e9f500f97c86 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -101,9 +104,10 @@ static void build_check_abi(void)
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> ruleset_size += sizeof(ruleset_attr.handled_access_net);
> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
> ruleset_size += sizeof(ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
> [...]
> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> LANDLOCK_MASK_ACCESS_NET)
> return -EINVAL;
>
> + /* Checks socket content (and 32-bits cast). */
> + if ((ruleset_attr.handled_access_socket |
> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
> + return -EINVAL;
> +
> /* Checks IPC scoping content (and 32-bits cast). */
> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> return -EINVAL;
> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> /* Checks arguments and transforms to kernel struct. */
> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> ruleset_attr.handled_access_net,
> + ruleset_attr.handled_access_socket,
> ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
> [...]
> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
> * handled accesses)
> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
> + * &landlock_socket_attr.protocol is greater than 65534;
Hmm, this is a bit annoying that these values have such unusual
bounds, even though the input parameters are 32 bit. We are exposing
a little bit that we are internally storing this with only 8 and 16
bits... (I don't know a better solution immediately either, though. I
think we discussed this on a previous version of the patch set as well
and ended up with permitting larger values than the narrower SOCK_MAX
etc bounds.)
> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
> * 0);
> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
> return add_rule_path_beneath(ruleset, rule_attr);
> case LANDLOCK_RULE_NET_PORT:
> return add_rule_net_port(ruleset, rule_attr);
> + case LANDLOCK_RULE_SOCKET:
> + return add_rule_socket(ruleset, rule_attr);
> default:
> return -EINVAL;
> }
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Mikhail Ivanov @ 2025-11-22 11:13 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.e645d2f1b8a1@gnoack.org>
On 11/22/2025 1:49 PM, Günther Noack wrote:
> Hello!
>
> On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
>> It is possible to create sockets of the same protocol with different
>> protocol number values. For example, TCP sockets can be created using one
>> of the following commands:
>> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
>> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
>> protocol of the given protocol family and can be mapped to another
>> value.
>>
>> Socket rules do not perform such mappings to not increase complexity
>> of rules definition and their maintenance.
>
> Minor phrasing nit: Maybe we can phrase this constructively, like
> "rules operate on the socket(2) parameters as they are passed by the
> user, before this mapping happens"?
OK, thats sounds good.
>
>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f030adc462ee..030c96cb5d25 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
>> * flags`_).
>> */
>> __u64 handled_access_net;
>> + /**
>> + * @handled_access_socket: Bitmask of handled actions performed on sockets
>> + * (cf. `Socket flags`).
>> + */
>> + __u64 handled_access_socket;
>
> This struct can only be extended at the end, for ABI compatibility reasons.
>
> In the call to landlock_create_ruleset(2), the user passes the __user
> pointer to this struct along with its size (as known to the user at
> compile time). When we copy this into the kernel, we blank out the
> struct and only copy the prefix of the caller-supplied size. The
> implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
Indeed... Thanks for pointing on this, I'll move this field in the end
of the structure.
>
> When you rearrange the order, please also update it in other places
> where these fields are mentioned next to each other, for
> consistency. I'll try to point it out where I see it in the review,
> but I might miss some places.
ok
>
>> /**
>> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
>> * restricting a Landlock domain from accessing outside
>> @@ -140,6 +145,11 @@ enum landlock_rule_type {
>> * landlock_net_port_attr .
>> */
>> LANDLOCK_RULE_NET_PORT,
>> + /**
>> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
>> + * landlock_socket_attr.
> ^
>
> Nit: Adjacent documentation has a space before the dot.
> I assume this is needed for kernel doc formatting?
Probably, I'll fix this anyway.
>
>> + */
>> + LANDLOCK_RULE_SOCKET,
>> };
>>
>> /**
>> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
>> __u64 port;
>> };
>>
>> +/**
>> + * struct landlock_socket_attr - Socket protocol definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_socket_attr {
>> + /**
>> + * @allowed_access: Bitmask of allowed access for a socket protocol
>> + * (cf. `Socket flags`_).
>> + */
>> + __u64 allowed_access;
>> + /**
>> + * @family: Protocol family used for communication
>> + * (cf. include/linux/socket.h).
>> + */
>> + __s32 family;
>> + /**
>> + * @type: Socket type (cf. include/linux/net.h)
>> + */
>> + __s32 type;
>> + /**
>> + * @protocol: Communication protocol specific to protocol family set in
>> + * @family field.
>
> This is specific to both the @family and the @type, not just the @family.
>
>>From socket(2):
>
> Normally only a single protocol exists to support a particular
> socket type within a given protocol family.
>
> For instance, in your commit message above the protocol in the example
> is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
> with SOCK_DGRAM.
You're right.
>
>> + */
>> + __s32 protocol;
>> +} __attribute__((packed));
>
> Since we are in the UAPI header, please also document the wildcard
> values for @type and @protocol.
I'll add the description, thanks!
>
> (Remark, should those be exposed as constants?)
I thought it could overcomplicate socket rules definition and Landlock
API. Do you think introducing such constants will be better decision?
>
>
>> diff --git a/security/landlock/access.h b/security/landlock/access.h
>> index 7961c6630a2d..03ccd6fbfe83 100644
>> --- a/security/landlock/access.h
>> +++ b/security/landlock/access.h
>> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> /* Makes sure all network access rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>> +/* Makes sure all socket access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
>> /* Makes sure all scoped rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
>> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> struct access_masks {
>> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
>> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
>> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
>> access_mask_t scope : LANDLOCK_NUM_SCOPE;
>
> (Please re-adjust field order for consistency with UAPI)
ok, will be fixed in all such places.
>
>> };
>
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index dfcdc19ea268..a34d2dbe3954 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> return new_ruleset;
>> }
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask,
>> - const access_mask_t net_access_mask,
>> - const access_mask_t scope_mask)
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
>> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> {
>> struct landlock_ruleset *new_ruleset;
>>
>> /* Informs about useless ruleset. */
>> - if (!fs_access_mask && !net_access_mask && !scope_mask)
>> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
>> + !scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> return ERR_PTR(-ENOMSG);
>> new_ruleset = create_ruleset(1);
>> if (IS_ERR(new_ruleset))
>> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>> if (net_access_mask)
>> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> + if (socket_access_mask)
>> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
>> + 0);
>
> (Please re-adjust order of these "if"s for consistency with UAPI)
>
>> if (scope_mask)
>> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
>> return new_ruleset;
>
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 1a78cba662b2..a60ede2fc2a5 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -189,10 +204,9 @@ struct landlock_ruleset {
>> };
>> };
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t access_mask_fs,
>> - const access_mask_t access_mask_net,
>> - const access_mask_t scope_mask);
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
>> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> index 000000000000..28a80dcad629
>> --- /dev/null
>> +++ b/security/landlock/socket.c
>> @@ -0,0 +1,105 @@
>> [...]
>> +#define TYPE_ALL (-1)
>> +#define PROTOCOL_ALL (-1)
>
> Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
answered above.
>
>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 33eafb71e4f3..e9f500f97c86 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -101,9 +104,10 @@ static void build_check_abi(void)
>> */
>> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>> ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>> ruleset_size += sizeof(ruleset_attr.scoped);
> (Please re-adjust field order for consistency with UAPI)
>
>> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
>> [...]
>
>> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> LANDLOCK_MASK_ACCESS_NET)
>> return -EINVAL;
>>
>> + /* Checks socket content (and 32-bits cast). */
>> + if ((ruleset_attr.handled_access_socket |
>> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
>> + return -EINVAL;
>> +
>> /* Checks IPC scoping content (and 32-bits cast). */
>> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
>> return -EINVAL;
>> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> /* Checks arguments and transforms to kernel struct. */
>> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>> ruleset_attr.handled_access_net,
>> + ruleset_attr.handled_access_socket,
>> ruleset_attr.scoped);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> if (IS_ERR(ruleset))
>> return PTR_ERR(ruleset);
>> [...]
>
>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>> * handled accesses)
>> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>> + * &landlock_socket_attr.protocol is greater than 65534;
>
> Hmm, this is a bit annoying that these values have such unusual
> bounds, even though the input parameters are 32 bit. We are exposing
> a little bit that we are internally storing this with only 8 and 16
> bits... (I don't know a better solution immediately either, though. I
> think we discussed this on a previous version of the patch set as well
> and ended up with permitting larger values than the narrower SOCK_MAX
> etc bounds.)
I agree, one of the possible solutions may be to store larger values in
socket keys (eg. s32), but this would require to make a separate
interface for storing socket rules (in order to not change key size for
other type of rules which is currently 32-64 bit depending on virtual
address size).
>
>> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
>> * 0);
>> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
>> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>> return add_rule_path_beneath(ruleset, rule_attr);
>> case LANDLOCK_RULE_NET_PORT:
>> return add_rule_net_port(ruleset, rule_attr);
>> + case LANDLOCK_RULE_SOCKET:
>> + return add_rule_socket(ruleset, rule_attr);
>> default:
>> return -EINVAL;
>> }
>
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 06/19] landlock: Add hook on socket creation
From: Günther Noack @ 2025-11-22 11:41 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-7-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:26PM +0800, Mikhail Ivanov wrote:
> Add hook on security_socket_create(), which checks whether the socket
> of requested protocol is allowed by domain.
>
> Due to support of masked protocols Landlock tries to find one of the
> 4 rules that can allow creation of requested protocol.
>
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> Changes since v3:
> * Changes LSM hook from socket_post_create to socket_create so
> creation would be blocked before socket allocation and initialization.
> * Uses credential instead of domain in hook_socket create.
> * Removes get_raw_handled_socket_accesses.
> * Adds checks for rules with wildcard type and protocol values.
> * Minor refactoring, fixes.
>
> Changes since v2:
> * Adds check in `hook_socket_create()` to not restrict kernel space
> sockets.
> * Inlines `current_check_access_socket()` in the `hook_socket_create()`.
> * Fixes commit message.
>
> Changes since v1:
> * Uses lsm hook arguments instead of struct socket fields as family-type
> values.
> * Packs socket family and type using helper.
> * Fixes commit message.
> * Formats with clang-format.
> ---
> security/landlock/setup.c | 2 +
> security/landlock/socket.c | 78 ++++++++++++++++++++++++++++++++++++++
> security/landlock/socket.h | 2 +
> 3 files changed, 82 insertions(+)
>
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index bd53c7a56ab9..140a53b022f7 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -17,6 +17,7 @@
> #include "fs.h"
> #include "id.h"
> #include "net.h"
> +#include "socket.h"
> #include "setup.h"
> #include "task.h"
>
> @@ -68,6 +69,7 @@ static int __init landlock_init(void)
> landlock_add_task_hooks();
> landlock_add_fs_hooks();
> landlock_add_net_hooks();
> + landlock_add_socket_hooks();
> landlock_init_id();
> landlock_initialized = true;
> pr_info("Up and running.\n");
> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
> index 28a80dcad629..d7e6e7b92b7a 100644
> --- a/security/landlock/socket.c
> +++ b/security/landlock/socket.c
> @@ -103,3 +103,81 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>
> return err;
> }
> +
> +static int check_socket_access(const struct landlock_ruleset *dom,
> + uintptr_t key,
> + layer_mask_t (*const layer_masks)[],
> + access_mask_t handled_access)
> +{
> + const struct landlock_rule *rule;
> + struct landlock_id id = {
> + .type = LANDLOCK_KEY_SOCKET,
> + };
> +
> + id.key.data = key;
This line can be made part of the designated initializer:
struct landlock_id id = {
.type = ...,
.key.data = ...,
};
> + rule = landlock_find_rule(dom, id);
> + if (landlock_unmask_layers(rule, handled_access, layer_masks,
> + LANDLOCK_NUM_ACCESS_SOCKET))
> + return 0;
> + return -EACCES;
> +}
> +
> +static int hook_socket_create(int family, int type, int protocol, int kern)
> +{
> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
> + access_mask_t handled_access;
> + const struct access_masks masks = {
> + .socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_cred_security *const subject =
> + landlock_get_applicable_subject(current_cred(), masks, NULL);
> + uintptr_t key;
> +
> + if (!subject)
> + return 0;
> + /* Checks only user space sockets. */
> + if (kern)
> + return 0;
> +
> + handled_access = landlock_init_layer_masks(
> + subject->domain, LANDLOCK_ACCESS_SOCKET_CREATE, &layer_masks,
> + LANDLOCK_KEY_SOCKET);
Nit: I had to double check to confirm that the same PF_INET/PF_PACKET
transformation (which net/socket.c refers to as the "uglymoron") has
already happened on the arguments before hook_socket_create() gets
called from there. Maybe it's worth a brief mention in a comment
here.
> + /*
> + * Error could happen due to parameters are outside of the allowed range,
Grammar nit: drop the "are"
Suggestion: "If this error happens, the parameters are outside of the
allowed range, so this combination can't have been added to the
ruleset previously."
> + * so this combination couldn't be added in ruleset previously.
> + * Therefore, it's not permitted.
> + */
> + if (pack_socket_key(family, type, protocol, &key) == -EACCES)
> + return -EACCES;
BUG: pack_socket_key() does never return -EACCES!
(Consider whether that function should really return an error? Maybe
a boolean would be better, if you anyway need a different error code
in both locations where it is called.)
Can this code path actually get hit, or do the entry points for
creating sockets refuse these wrong values at an earlier stage with
EINVAL already?
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + /* Ranges were already checked. */
> + (void)pack_socket_key(family, TYPE_ALL, protocol, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, type, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, TYPE_ALL, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + return -EACCES;
> +}
It initially doesn't look very nice to drop the error from
pack_socket_key() repeatedly. The call repeats the bounds checks and
requires more cross-function reasoning to understand.
Since 'key' is an uintptr_t anyway, and the wildcards are all ones,
maybe a simpler way is to define masks for the wildcards?
const uintptr_t any_type_mask = (union key){.data.type = UINT8_MAX}.packed;
const uintptr_t any_protocol_mask = (union key){.data.protocol = UINT16_MAX}.packed;
and then, after calling pack_socket_key() once with error check, use
the combinations
* key
* key | any_type
* key | any_protocol
* key | any_type | any_protocol
to construct the wildcard-enabled keys in the four calls to
check_socket_access()? You could have compile-time assertions or
tests to check that the masking does the same as packing it from
scratch when passing -1.
(That being said, I don't feel strongly about it.)
Remark on the side: I was briefly confused why we don't need to guard
on CONFIG_SECURITY_NETWORK, but this is already required by
CONFIG_LANDLOCK. So that looks good.
–Günther
^ 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