* Re: [PATCH v2] lsm: use unrcu_pointer() for current->cred in security_init()
From: Paul Moore @ 2025-11-20 21:27 UTC (permalink / raw)
To: Xiu Jianfeng; +Cc: linux-security-module
In-Reply-To: <f3631b3b-4461-4176-9178-27fa1dc66226@huawei.com>
On Thu, Nov 20, 2025 at 2:48 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote:
> On 11/19/2025 11:36 PM, Paul Moore wrote:
> > We need to directly allocate the cred's LSM state for the initial task
> > when we initialize the LSM framework. Unfortunately, this results in a
> > RCU related type mismatch, use the unrcu_pointer() macro to handle this
> > a bit more elegantly.
> >
> > The explicit type casting still remains as we need to work around the
> > constification of current->cred in this particular case.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Reviewed-by: Xiu Jianfeng <xiujianfeng@huawei.com>
Thanks for the review, I just merged this into lsm/dev.
--
paul-moore.com
^ permalink raw reply
* [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-20 20:57 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: <87wm3ky5n9.fsf@email.froward.int.ebiederm.org>
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.
We gain a lot.
We stop holding the cred_guard_mutex over places where the code sleeps
and waits for userspace. These places include the waiting for the
tracer in PTRACE_EVENT_EXIT, "put_user(0, tsk->clear_child_tid)" in
mm_release, and "get_user(futex_offset, ...") in exit_robust_mutex.
We can remove fs_struct->in_exec. The case where it was used simply
never comes up, when we compute the cred after de_thread completes.
We remove the possibility of a hang between a tracer calling
PTRACE_ATTACH/PTRACE_SIEZE and the kernel waiting for the tracer
in PTRACE_EVENT_EXIT.
---
Oleg, Kees, Bernd, Can you see anything I am missing?
The code compiles but I haven't test it yet.
I thought I was going to move commit_creds before de_thread, but that
would have taken commit_cred out of exec_update_lock (which introduces
races).
However I can't see any drawbacks of going the other direction.
fs/exec.c | 88 ++++++++++++++----------------------
fs/fs_struct.c | 1 -
fs/proc/base.c | 4 +-
include/linux/fs_struct.h | 1 -
include/linux/sched/signal.h | 6 ---
init/init_task.c | 1 -
kernel/cred.c | 2 +-
kernel/fork.c | 8 +---
kernel/ptrace.c | 4 +-
kernel/seccomp.c | 12 ++---
10 files changed, 45 insertions(+), 82 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 4298e7e08d5d..5ae96584dab0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1090,6 +1090,9 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
perf_event_comm(tsk, exec);
}
+static int prepare_bprm_creds(struct linux_binprm *bprm);
+static void check_unsafe_exec(struct linux_binprm *bprm);
+
/*
* Calling this is the point of no return. None of the failures will be
* seen by userspace since either the process is already taking a fatal
@@ -1101,10 +1104,6 @@ int begin_new_exec(struct linux_binprm * bprm)
struct task_struct *me = current;
int retval;
- /* Once we are committed compute the creds */
- retval = bprm_creds_from_file(bprm);
- if (retval)
- return retval;
/*
* This tracepoint marks the point before flushing the old exec where
@@ -1123,8 +1122,6 @@ int begin_new_exec(struct linux_binprm * bprm)
retval = de_thread(me);
if (retval)
goto out;
- /* see the comment in check_unsafe_exec() */
- current->fs->in_exec = 0;
/*
* Cancel any io_uring activity across execve
*/
@@ -1251,6 +1248,25 @@ int begin_new_exec(struct linux_binprm * bprm)
WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1);
flush_signal_handlers(me, 0);
+ retval = prepare_bprm_creds(bprm);
+ if (retval)
+ goto out_unlock;
+
+ /*
+ * Check for unsafe execution states before exec_binprm(), which
+ * will call back into begin_new_exec(), into bprm_creds_from_file(),
+ * where setuid-ness is evaluated.
+ */
+ check_unsafe_exec(bprm);
+
+ /* Set the unchanging part of bprm->cred */
+ retval = security_bprm_creds_for_exec(bprm);
+
+ /* Once we are committed compute the creds */
+ retval = bprm_creds_from_file(bprm);
+ if (retval)
+ goto out_unlock;
+
retval = set_cred_ucounts(bprm->cred);
if (retval < 0)
goto out_unlock;
@@ -1272,9 +1288,9 @@ int begin_new_exec(struct linux_binprm * bprm)
if (get_dumpable(me->mm) != SUID_DUMP_USER)
perf_event_exit_task(me);
/*
- * cred_guard_mutex must be held at least to this point to prevent
+ * exec_update_lock must be held at least to this point to prevent
* ptrace_attach() from altering our determination of the task's
- * credentials; any time after this it may be unlocked.
+ * credentials.
*/
security_bprm_committed_creds(bprm);
@@ -1291,8 +1307,6 @@ int begin_new_exec(struct linux_binprm * bprm)
out_unlock:
up_write(&me->signal->exec_update_lock);
- if (!bprm->cred)
- mutex_unlock(&me->signal->cred_guard_mutex);
out:
return retval;
@@ -1336,7 +1350,6 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
me->mm->task_size = TASK_SIZE;
up_write(&me->signal->exec_update_lock);
- mutex_unlock(&me->signal->cred_guard_mutex);
}
EXPORT_SYMBOL(setup_new_exec);
@@ -1351,21 +1364,15 @@ void finalize_exec(struct linux_binprm *bprm)
EXPORT_SYMBOL(finalize_exec);
/*
- * Prepare credentials and lock ->cred_guard_mutex.
- * setup_new_exec() commits the new creds and drops the lock.
- * Or, if exec fails before, free_bprm() should release ->cred
- * and unlock.
+ * Prepare credentials. begin_new_exec() commits the new creds.
+ * Or, if exec fails before, free_bprm() should release ->cred.
*/
static int prepare_bprm_creds(struct linux_binprm *bprm)
{
- if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
- return -ERESTARTNOINTR;
-
bprm->cred = prepare_exec_creds();
if (likely(bprm->cred))
return 0;
- mutex_unlock(¤t->signal->cred_guard_mutex);
return -ENOMEM;
}
@@ -1386,9 +1393,7 @@ static void free_bprm(struct linux_binprm *bprm)
}
free_arg_pages(bprm);
if (bprm->cred) {
- /* in case exec fails before de_thread() succeeds */
- current->fs->in_exec = 0;
- mutex_unlock(¤t->signal->cred_guard_mutex);
+ /* in case exec fails before commit_creds succeeds */
abort_creds(bprm->cred);
}
do_close_execat(bprm->file);
@@ -1486,13 +1491,12 @@ EXPORT_SYMBOL(bprm_change_interp);
/*
* determine how safe it is to execute the proposed program
- * - the caller must hold ->cred_guard_mutex to protect against
+ * - the caller must hold ->exec_update_lock to protect against
* PTRACE_ATTACH or seccomp thread-sync
*/
static void check_unsafe_exec(struct linux_binprm *bprm)
{
- struct task_struct *p = current, *t;
- unsigned n_fs;
+ struct task_struct *p = current;
if (p->ptrace)
bprm->unsafe |= LSM_UNSAFE_PTRACE;
@@ -1509,25 +1513,9 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
* suid exec because the differently privileged task
* will be able to manipulate the current directory, etc.
* It would be nice to force an unshare instead...
- *
- * Otherwise we set fs->in_exec = 1 to deny clone(CLONE_FS)
- * from another sub-thread until de_thread() succeeds, this
- * state is protected by cred_guard_mutex we hold.
*/
- n_fs = 1;
- read_seqlock_excl(&p->fs->seq);
- rcu_read_lock();
- for_other_threads(p, t) {
- if (t->fs == p->fs)
- n_fs++;
- }
- rcu_read_unlock();
-
- /* "users" and "in_exec" locked for copy_fs() */
- if (p->fs->users > n_fs)
+ if (p->fs->users > 1)
bprm->unsafe |= LSM_UNSAFE_SHARE;
- else
- p->fs->in_exec = 1;
read_sequnlock_excl(&p->fs->seq);
}
@@ -1731,25 +1719,15 @@ static int bprm_execve(struct linux_binprm *bprm)
{
int retval;
- retval = prepare_bprm_creds(bprm);
- if (retval)
- return retval;
+ if (bprm->is_check)
+ return 0;
- /*
- * Check for unsafe execution states before exec_binprm(), which
- * will call back into begin_new_exec(), into bprm_creds_from_file(),
- * where setuid-ness is evaluated.
- */
- check_unsafe_exec(bprm);
current->in_execve = 1;
sched_mm_cid_before_execve(current);
sched_exec();
- /* Set the unchanging part of bprm->cred */
- retval = security_bprm_creds_for_exec(bprm);
- if (retval || bprm->is_check)
- goto out;
+
retval = exec_binprm(bprm);
if (retval < 0)
diff --git a/fs/fs_struct.c b/fs/fs_struct.c
index 28be762ac1c6..945bc0916f65 100644
--- a/fs/fs_struct.c
+++ b/fs/fs_struct.c
@@ -109,7 +109,6 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)
/* We don't need to lock fs - think why ;-) */
if (fs) {
fs->users = 1;
- fs->in_exec = 0;
seqlock_init(&fs->seq);
fs->umask = old->umask;
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6299878e3d97..7041fb4d1689 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2834,14 +2834,14 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
}
/* Guard against adverse ptrace interaction */
- rv = mutex_lock_interruptible(¤t->signal->cred_guard_mutex);
+ rv = down_write_killable(¤t->signal->exec_update_lock);
if (rv < 0)
goto out_free;
rv = security_setprocattr(PROC_I(inode)->op.lsmid,
file->f_path.dentry->d_name.name, page,
count);
- mutex_unlock(¤t->signal->cred_guard_mutex);
+ up_write(¤t->signal->exec_update_lock);
out_free:
kfree(page);
out:
diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
index baf200ab5c77..29d0f7d57743 100644
--- a/include/linux/fs_struct.h
+++ b/include/linux/fs_struct.h
@@ -10,7 +10,6 @@ struct fs_struct {
int users;
seqlock_t seq;
int umask;
- int in_exec;
struct path root, pwd;
} __randomize_layout;
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 7d6449982822..7e9259c8fb2b 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -241,12 +241,6 @@ struct signal_struct {
struct mm_struct *oom_mm; /* recorded mm when the thread group got
* killed by the oom killer */
- struct mutex cred_guard_mutex; /* guard against foreign influences on
- * credential calculations
- * (notably. ptrace)
- * Deprecated do not use in new code.
- * Use exec_update_lock instead.
- */
struct rw_semaphore exec_update_lock; /* Held while task_struct is
* being updated during exec,
* and may have inconsistent
diff --git a/init/init_task.c b/init/init_task.c
index a55e2189206f..4813bffe217e 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -30,7 +30,6 @@ static struct signal_struct init_signals = {
#ifdef CONFIG_CGROUPS
.cgroup_threadgroup_rwsem = __RWSEM_INITIALIZER(init_signals.cgroup_threadgroup_rwsem),
#endif
- .cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
.exec_update_lock = __RWSEM_INITIALIZER(init_signals.exec_update_lock),
#ifdef CONFIG_POSIX_TIMERS
.posix_timers = HLIST_HEAD_INIT,
diff --git a/kernel/cred.c b/kernel/cred.c
index dbf6b687dc5c..80e376ce005f 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -252,7 +252,7 @@ EXPORT_SYMBOL(prepare_creds);
/*
* Prepare credentials for current to perform an execve()
- * - The caller must hold ->cred_guard_mutex
+ * - The caller must hold ->exec_update_lock
*/
struct cred *prepare_exec_creds(void)
{
diff --git a/kernel/fork.c b/kernel/fork.c
index 3da0f08615a9..996c649b9a4c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1555,11 +1555,6 @@ static int copy_fs(u64 clone_flags, struct task_struct *tsk)
if (clone_flags & CLONE_FS) {
/* tsk->fs is already what we want */
read_seqlock_excl(&fs->seq);
- /* "users" and "in_exec" locked for check_unsafe_exec() */
- if (fs->in_exec) {
- read_sequnlock_excl(&fs->seq);
- return -EAGAIN;
- }
fs->users++;
read_sequnlock_excl(&fs->seq);
return 0;
@@ -1699,7 +1694,6 @@ static int copy_signal(u64 clone_flags, struct task_struct *tsk)
sig->oom_score_adj = current->signal->oom_score_adj;
sig->oom_score_adj_min = current->signal->oom_score_adj_min;
- mutex_init(&sig->cred_guard_mutex);
init_rwsem(&sig->exec_update_lock);
return 0;
@@ -1710,7 +1704,7 @@ static void copy_seccomp(struct task_struct *p)
#ifdef CONFIG_SECCOMP
/*
* Must be called with sighand->lock held, which is common to
- * all threads in the group. Holding cred_guard_mutex is not
+ * all threads in the group. Holding exec_update_lock is not
* needed because this new task is not yet running and cannot
* be racing exec.
*/
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 75a84efad40f..8140d4bfc279 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -444,8 +444,8 @@ static int ptrace_attach(struct task_struct *task, long request,
* SUID, SGID and LSM creds get determined differently
* under ptrace.
*/
- scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
- &task->signal->cred_guard_mutex) {
+ scoped_cond_guard (rwsem_read_intr, return -ERESTARTNOINTR,
+ &task->signal->exec_update_lock) {
scoped_guard (task_lock, task) {
retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 25f62867a16d..87de8d47d876 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -479,7 +479,7 @@ static int is_ancestor(struct seccomp_filter *parent,
/**
* seccomp_can_sync_threads: checks if all threads can be synchronized
*
- * Expects sighand and cred_guard_mutex locks to be held.
+ * Expects sighand and exec_update_lock locks to be held.
*
* Returns 0 on success, -ve on error, or the pid of a thread which was
* either not in the correct seccomp mode or did not have an ancestral
@@ -489,7 +489,7 @@ static inline pid_t seccomp_can_sync_threads(void)
{
struct task_struct *thread, *caller;
- BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
+ BUG_ON(!rwsem_is_locked(¤t->signal->exec_update_lock));
assert_spin_locked(¤t->sighand->siglock);
/* Validate all threads being eligible for synchronization. */
@@ -590,7 +590,7 @@ void seccomp_filter_release(struct task_struct *tsk)
*
* @flags: SECCOMP_FILTER_FLAG_* flags to set during sync.
*
- * Expects sighand and cred_guard_mutex locks to be held, and for
+ * Expects sighand and exec_update_lock locks to be held, and for
* seccomp_can_sync_threads() to have returned success already
* without dropping the locks.
*
@@ -599,7 +599,7 @@ static inline void seccomp_sync_threads(unsigned long flags)
{
struct task_struct *thread, *caller;
- BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
+ BUG_ON(!rwsem_is_locked(¤t->signal->exec_update_lock));
assert_spin_locked(¤t->sighand->siglock);
/*
@@ -2011,7 +2011,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
* while another thread is in the middle of calling exec.
*/
if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
- mutex_lock_killable(¤t->signal->cred_guard_mutex))
+ down_read_killable(¤t->signal->exec_update_lock))
goto out_put_fd;
spin_lock_irq(¤t->sighand->siglock);
@@ -2034,7 +2034,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
out:
spin_unlock_irq(¤t->sighand->siglock);
if (flags & SECCOMP_FILTER_FLAG_TSYNC)
- mutex_unlock(¤t->signal->cred_guard_mutex);
+ up_read(¤t->signal->exec_update_lock);
out_put_fd:
if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
if (ret) {
--
2.41.0
^ permalink raw reply related
* [PATCH v2 0/6] Implement 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
Hi,
This series introduces a new Landlock rule flag,
LANDLOCK_ADD_RULE_NO_INHERIT, which disables inheriting access rights
from parent objects within the same Landlock layer.
This is useful for policies where a parent directory requires broader
access than its children. For example, a sandbox may allow read-write
access to /home/user but prohibit writes to ~/.bashrc or ~/.ssh even
though they are nested inside the parent directory. Today, this is not
possible because access rights always propagate from parent to child
within a layer.
When a rule is added with LANDLOCK_ADD_RULE_NO_INHERIT:
* access rights on parent inodes are ignored for that inode and its
descendants; and
* operations that change the direct parent subtree of such an object
(rename, rmdir, link) are denied up to the mountpoint.
These parent-directory restrictions mitigate sandbox-restart attacks.
Without them, a sandboxed program could move a protected directory
before exit, causing the next sandbox instance to apply policies on the
wrong path. Denying such operations prevents these attacks.
This v2 series differs from v1 in the following ways:
1. Adds four new selftests for the new flag.
2. Adds a KUnit test for access layer propagation with flags.
2. Extends parent-directory protections (rename/unlink/link) to prevent
sandbox-restart attacks.
3. Rebased on v4 of Tingmao Wang's amazing "quiet flag" series.
For convenience, the original v1 is here:
https://lore.kernel.org/linux-security-module/20251105180019.1432367-1-utilityemal77@gmail.com/T/#t
And the quiet-flag v4 series is here:
https://lore.kernel.org/linux-security-module/cover.1763330228.git.m@maowtm.org/T/#t
A short usage example:
# LL_FS_RO="" LL_FS_RW="/" LL_FS_RO_NO_INHERIT="/a/b/c" landlock-sandboxer sh
# touch /a/b/c/fi # denied (does not inherit / RW)
# rmdir /a/b/c # denied due to parent-directory protections
# mv /a /bad # denied
# mkdir /a/good; touch /a/good/fi # allowed (unrelated to /a/b/c)
All kunit and selftests related to landlock, including the new ones, pass on my test system.
Feedback is very welcome. Thank you for your time and consideration.
Regards,
Justin Suess
Justin Suess (6):
landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT
landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
samples/landlock: Add LANDLOCK_ADD_RULE_NO_INHERIT to
landlock-sandboxer
selftests/landlock: Implement selftests for
LANDLOCK_ADD_RULE_NO_INHERIT
landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is
disabled.
landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
include/uapi/linux/landlock.h | 9 +
samples/landlock/sandboxer.c | 39 +-
security/landlock/audit.h | 3 +-
security/landlock/fs.c | 433 ++++++++++++++++++++-
security/landlock/net.h | 2 +-
security/landlock/object.h | 2 +
security/landlock/ruleset.c | 171 +++++++-
security/landlock/ruleset.h | 39 ++
security/landlock/syscalls.c | 5 +-
tools/testing/selftests/landlock/fs_test.c | 222 ++++++++++-
10 files changed, 883 insertions(+), 42 deletions(-)
base-commit: d74278e895485ffe1c3708f7f2bc5c5b364aab0a
--
2.51.2
^ permalink raw reply
* [PATCH 1/6] landlock: Implement 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>
Implements a flag to prevent access grant inheritance within the filesystem hierarchy
for landlock rules.
If a landlock rule on an inode has this flag, any access grants on parent inodes will be
ignored. Moreover, operations that involve altering the direct parent tree of the subject with
LANDLOCK_ADD_RULE_NO_INHERIT will be denied up to the mountpoint.
For example, if /a/b/c/ = read only + LANDLOCK_ADD_RULE_NO_INHERIT and / = read write, writes to
files in /a/b/c will be denied. Moreover, moving /a to /bad, removing /a/b/c, or creating links to
/a will be prohibited.
The parent directory restrictions mitigate sandbox-restart attacks. For example, if a sandboxed program
is able to move a LANDLOCK_ADD_RULE_NO_INHERIT restricted directory, upon sandbox restart, the policy
applied naively on the same filenames would be invalid. Preventing these operations mitigates these attacks.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
security/landlock/fs.c | 431 ++++++++++++++++++++++++++++++++++--
security/landlock/object.h | 2 +
security/landlock/ruleset.c | 84 ++++++-
security/landlock/ruleset.h | 39 ++++
4 files changed, 539 insertions(+), 17 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 5fb2c9810e6b..ebeee080ea7a 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -14,6 +14,7 @@
#include <linux/atomic.h>
#include <linux/bitops.h>
#include <linux/bits.h>
+#include <linux/compiler.h>
#include <linux/compiler_types.h>
#include <linux/dcache.h>
#include <linux/err.h>
@@ -23,6 +24,7 @@
#include <linux/kernel.h>
#include <linux/limits.h>
#include <linux/list.h>
+#include <linux/lockdep.h>
#include <linux/lsm_audit.h>
#include <linux/lsm_hooks.h>
#include <linux/mount.h>
@@ -36,6 +38,7 @@
#include <linux/types.h>
#include <linux/wait_bit.h>
#include <linux/workqueue.h>
+#include <linux/xarray.h>
#include <uapi/linux/fiemap.h>
#include <uapi/linux/landlock.h>
@@ -317,21 +320,152 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
LANDLOCK_ACCESS_FS_IOCTL_DEV)
/* clang-format on */
+static inline layer_mask_t landlock_layer_bit(
+ const struct landlock_layer *layer, u32 layer_index)
+{
+ u32 level = layer->level;
+
+ if (!level)
+ level = layer_index + 1;
+ return BIT_ULL(level - 1);
+}
+
+static const struct landlock_rule *find_rule(
+ const struct landlock_ruleset *const domain,
+ const struct dentry *const dentry);
+
+static layer_mask_t get_ruleset_cached_no_inherit_layers(
+ const struct landlock_ruleset *const ruleset,
+ struct landlock_object *object)
+{
+ struct landlock_no_inherit_desc *desc;
+
+ if (!ruleset || !object)
+ return 0;
+
+ /* xa_load doesn't accept const, but we're only reading */
+ desc = xa_load(&((struct landlock_ruleset *)ruleset)->no_inherit_desc,
+ (unsigned long)object);
+ if (!desc)
+ return 0;
+ return desc->desc_layers;
+}
+
+static layer_mask_t get_no_inherit_desc_layers_for_dentry(
+ const struct landlock_ruleset *const ruleset,
+ struct dentry *const dentry)
+{
+ const struct landlock_rule *rule;
+ struct landlock_object *object;
+ layer_mask_t layers = 0;
+ u32 layer_index;
+
+ if (!ruleset || !dentry || d_is_negative(dentry))
+ return 0;
+
+ rcu_read_lock();
+ object = rcu_dereference(landlock_inode(d_backing_inode(dentry))->object);
+ if (object)
+ layers |= get_ruleset_cached_no_inherit_layers(ruleset, object);
+ rcu_read_unlock();
+
+ rule = find_rule(ruleset, dentry);
+ if (!rule)
+ return layers;
+
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *layer = &rule->layers[layer_index];
+
+ if (!layer->flags.no_inherit &&
+ !layer->flags.has_no_inherit_descendant)
+ continue;
+ layers |= landlock_layer_bit(layer, layer_index);
+ }
+
+ return layers;
+}
+
+static void mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
+ struct dentry *dentry,
+ layer_mask_t descendant_layers);
+
+static bool mask_no_inherit_descendant_layers(
+ const struct landlock_ruleset *const domain,
+ struct dentry *const dentry,
+ layer_mask_t child_layers,
+ const access_mask_t access_request,
+ layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
+ struct collected_rule_flags *const rule_flags)
+{
+ layer_mask_t descendant_layers;
+ const unsigned long access_req = access_request;
+ unsigned long access_bit;
+ bool changed = false;
+
+ if (!access_request || !layer_masks || !rule_flags || !dentry)
+ return false;
+ if (d_is_negative(dentry))
+ return false;
+
+ descendant_layers = get_no_inherit_desc_layers_for_dentry(domain, dentry);
+ {
+ layer_mask_t shared_layers = descendant_layers & child_layers;
+
+ if (shared_layers) {
+ rule_flags->no_inherit_masks |= shared_layers;
+ rule_flags->no_inherit_desc_masks |= shared_layers;
+ }
+ }
+ descendant_layers &= ~child_layers;
+ descendant_layers &= ~rule_flags->no_inherit_masks;
+ if (!descendant_layers)
+ return false;
+
+ for_each_set_bit(access_bit, &access_req,
+ ARRAY_SIZE(*layer_masks)) {
+ layer_mask_t *const layer_mask = &(*layer_masks)[access_bit];
+
+ if (*layer_mask & descendant_layers) {
+ *layer_mask &= ~descendant_layers;
+ changed = true;
+ }
+ }
+
+ if (!changed)
+ return false;
+
+ rule_flags->no_inherit_masks |= descendant_layers;
+ rule_flags->no_inherit_desc_masks |= descendant_layers;
+
+ return true;
+}
+
/*
* @path: Should have been checked by get_path_from_fd().
*/
int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
- const struct path *const path,
- access_mask_t access_rights, const int flags)
+ const struct path *const path,
+ access_mask_t access_rights, const int flags)
{
int err;
+ const bool is_dir = d_is_dir(path->dentry);
struct landlock_id id = {
.type = LANDLOCK_KEY_INODE,
};
/* Files only get access rights that make sense. */
- if (!d_is_dir(path->dentry) &&
- (access_rights | ACCESS_FILE) != ACCESS_FILE)
+ if (!is_dir) {
+ const access_mask_t file_rights = access_rights & ACCESS_FILE;
+
+ /*
+ * Reject purely directory-only access on files, but allow
+ * metadata-only quiet rules.
+ */
+ if (!file_rights && !(flags & LANDLOCK_ADD_RULE_QUIET))
+ return -EINVAL;
+ access_rights = file_rights;
+ }
+ if ((flags & LANDLOCK_ADD_RULE_NO_INHERIT) && !is_dir)
return -EINVAL;
if (WARN_ON_ONCE(ruleset->num_layers != 1))
return -EINVAL;
@@ -344,6 +478,39 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
return PTR_ERR(id.key.object);
mutex_lock(&ruleset->lock);
err = landlock_insert_rule(ruleset, id, access_rights, flags);
+ if (!err && (flags & LANDLOCK_ADD_RULE_NO_INHERIT)) {
+ const struct landlock_rule *rule;
+ layer_mask_t descendant_layers = 0;
+ u32 layer_index;
+
+ rule = find_rule(ruleset, path->dentry);
+ if (rule) {
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *layer =
+ &rule->layers[layer_index];
+
+ if (layer->flags.no_inherit ||
+ layer->flags.has_no_inherit_descendant)
+ descendant_layers |=
+ landlock_layer_bit(layer, layer_index);
+ }
+ if (descendant_layers)
+ mark_no_inherit_ancestors(ruleset, path->dentry,
+ descendant_layers);
+ }
+ }
+
+ /*
+ * Register this object in the xarray even if it doesn't currently have
+ * descendant no-inherit layers, so collect_no_inherit_layers() can walk
+ * ancestors reliably when checking topology changes.
+ */
+ if (!err) {
+ const struct landlock_rule *rule = find_rule(ruleset, path->dentry);
+
+ if (rule)
+ landlock_set_no_inherit_desc_layers(ruleset, id.key.object, 0);
+ }
mutex_unlock(&ruleset->lock);
/*
* No need to check for an error because landlock_insert_rule()
@@ -382,6 +549,102 @@ find_rule(const struct landlock_ruleset *const domain,
return rule;
}
+
+static void mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
+ struct dentry *dentry,
+ layer_mask_t descendant_layers)
+{
+ struct dentry *cursor;
+ u32 layer_index;
+
+ if (!ruleset || !dentry || !descendant_layers)
+ return;
+
+ lockdep_assert_held(&ruleset->lock);
+
+ cursor = dget(dentry);
+ while (cursor) {
+ struct dentry *parent;
+
+ if (IS_ROOT(cursor)) {
+ dput(cursor);
+ break;
+ }
+
+ parent = dget_parent(cursor);
+ dput(cursor);
+ if (!parent)
+ break;
+
+ if (!d_is_negative(parent)) {
+ const struct landlock_rule *rule;
+ struct landlock_object *parent_object;
+
+ rule = find_rule(ruleset, parent);
+ if (rule) {
+ struct landlock_rule *mutable_rule =
+ (struct landlock_rule *)rule;
+
+ for (layer_index = 0;
+ layer_index < mutable_rule->num_layers;
+ layer_index++) {
+ struct landlock_layer *layer =
+ &mutable_rule->layers[layer_index];
+ layer_mask_t layer_bit =
+ landlock_layer_bit(layer, layer_index);
+
+ if (descendant_layers & layer_bit)
+ layer->flags.has_no_inherit_descendant = true;
+ }
+ }
+
+ parent_object = get_inode_object(d_backing_inode(parent));
+ if (!IS_ERR(parent_object)) {
+ landlock_set_no_inherit_desc_layers(ruleset, parent_object,
+ descendant_layers);
+ landlock_put_object(parent_object);
+ }
+ }
+
+ cursor = parent;
+ }
+}
+
+static layer_mask_t get_no_inherit_layers_for_target(
+ const struct landlock_ruleset *const ruleset,
+ struct dentry *const dentry,
+ const bool include_descendants)
+{
+ struct landlock_object *object;
+ const struct landlock_rule *rule;
+ layer_mask_t layers = 0;
+ u32 layer_index;
+
+ if (!ruleset || !dentry || d_is_negative(dentry))
+ return 0;
+
+ rcu_read_lock();
+ object = rcu_dereference(landlock_inode(d_backing_inode(dentry))->object);
+ if (object && include_descendants)
+ layers |= get_ruleset_cached_no_inherit_layers(ruleset, object);
+ rcu_read_unlock();
+
+ rule = find_rule(ruleset, dentry);
+ if (!rule)
+ return layers;
+
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *layer = &rule->layers[layer_index];
+
+ if (layer->flags.no_inherit ||
+ (include_descendants &&
+ layer->flags.has_no_inherit_descendant))
+ layers |= landlock_layer_bit(layer, layer_index);
+ }
+
+ return layers;
+}
+
/*
* Allows access to pseudo filesystems that will never be mountable (e.g.
* sockfs, pipefs), but can still be reachable through
@@ -778,6 +1041,13 @@ static bool is_access_to_paths_allowed(
layer_mask_t(*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS] = NULL,
(*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
struct collected_rule_flags _rule_flag_parent1_bkp, _rule_flag_parent2_bkp;
+ layer_mask_t child1_layers = 0;
+ layer_mask_t child2_layers = 0;
+
+ if (dentry_child1)
+ child1_layers = get_no_inherit_desc_layers_for_dentry(domain, dentry_child1);
+ if (dentry_child2)
+ child2_layers = get_no_inherit_desc_layers_for_dentry(domain, dentry_child2);
if (!access_request_parent1 && !access_request_parent2)
return true;
@@ -976,8 +1246,12 @@ static bool is_access_to_paths_allowed(
memcpy(&_rule_flag_parent2_bkp,
rule_flags_parent2,
sizeof(_rule_flag_parent2_bkp));
- is_dom_check_bkp = is_dom_check;
}
+ is_dom_check_bkp = is_dom_check;
+ child1_layers = get_no_inherit_desc_layers_for_dentry(
+ domain, walker_path.dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
/* Ignores hidden mount points. */
goto jump_up;
@@ -1001,15 +1275,48 @@ static bool is_access_to_paths_allowed(
break;
}
- /*
- * We reached a disconnected root directory from a bind mount, and
- * we need to reset the walk to the current mount root.
- */
- goto reset_to_mount_root;
- }
- parent_dentry = dget_parent(walker_path.dentry);
- dput(walker_path.dentry);
- walker_path.dentry = parent_dentry;
+ /*
+ * We reached a disconnected root directory from a bind mount, and
+ * we need to reset the walk to the current mount root.
+ */
+ goto reset_to_mount_root;
+ }
+ if (likely(!d_is_negative(walker_path.dentry))) {
+ child1_layers = get_no_inherit_desc_layers_for_dentry(
+ domain, walker_path.dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
+ } else {
+ child1_layers = 0;
+ if (layer_masks_parent2)
+ child2_layers = 0;
+ }
+ parent_dentry = dget_parent(walker_path.dentry);
+ dput(walker_path.dentry);
+ walker_path.dentry = parent_dentry;
+ /*
+ * Apply descendant no-inherit masking now that we've moved to the
+ * parent. This ensures the parent respects any no-inherit rules from
+ * the child we just left. Only applies to refer operations (rename/link).
+ */
+ if (unlikely(layer_masks_parent2)) {
+ if (mask_no_inherit_descendant_layers(
+ domain, walker_path.dentry, child1_layers,
+ access_masked_parent1,
+ layer_masks_parent1, rule_flags_parent1))
+ allowed_parent1 =
+ allowed_parent1 ||
+ is_layer_masks_allowed(layer_masks_parent1);
+
+ if (rule_flags_parent2 &&
+ mask_no_inherit_descendant_layers(
+ domain, walker_path.dentry, child2_layers,
+ access_masked_parent2,
+ layer_masks_parent2, rule_flags_parent2))
+ allowed_parent2 =
+ allowed_parent2 ||
+ is_layer_masks_allowed(layer_masks_parent2);
+ }
continue;
reset_to_mount_root:
@@ -1057,6 +1364,10 @@ static bool is_access_to_paths_allowed(
dput(walker_path.dentry);
walker_path.dentry = walker_path.mnt->mnt_root;
dget(walker_path.dentry);
+ child1_layers = get_no_inherit_desc_layers_for_dentry(
+ domain, walker_path.dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
}
path_put(&walker_path);
@@ -1083,7 +1394,7 @@ static bool is_access_to_paths_allowed(
}
static int current_check_access_path(const struct path *const path,
- access_mask_t access_request)
+ access_mask_t access_request)
{
const struct access_masks masks = {
.fs = access_request,
@@ -1102,7 +1413,7 @@ static int current_check_access_path(const struct path *const path,
LANDLOCK_KEY_INODE);
if (is_access_to_paths_allowed(subject->domain, path, access_request,
&layer_masks, &rule_flags, &request,
- NULL, 0, NULL, NULL, NULL, NULL))
+ NULL, 0, NULL, NULL, NULL, NULL))
return 0;
landlock_log_denial(subject, &request, rule_flags);
@@ -1233,6 +1544,65 @@ static bool collect_domain_accesses(
return ret;
}
+static layer_mask_t collect_no_inherit_layers(
+ const struct landlock_ruleset *domain, struct dentry *dentry)
+{
+ struct dentry *cursor, *parent;
+ layer_mask_t layers = 0;
+ bool include_descendants = true;
+
+ if (!domain || !dentry)
+ return 0;
+
+ cursor = dget(dentry);
+ if (!cursor)
+ return 0;
+
+ while (true) {
+ layers = get_no_inherit_layers_for_target(domain, cursor,
+ include_descendants);
+ if (layers || IS_ROOT(cursor) || d_is_negative(cursor))
+ break;
+
+ parent = dget_parent(cursor);
+ dput(cursor);
+ if (!parent)
+ return layers;
+ cursor = parent;
+ include_descendants = false;
+ }
+
+ dput(cursor);
+ return layers;
+}
+
+static int deny_no_inherit_topology_change(
+ const struct landlock_cred_security *subject,
+ struct dentry *dentry)
+{
+ layer_mask_t sealed_layers;
+ unsigned long layer_index;
+
+ if (!subject || !dentry || d_is_negative(dentry))
+ return 0;
+
+ sealed_layers = collect_no_inherit_layers(subject->domain, dentry);
+ if (!sealed_layers)
+ return 0;
+
+ layer_index = __ffs((unsigned long)sealed_layers);
+ landlock_log_denial(subject, &(struct landlock_request) {
+ .type = LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY,
+ .audit = {
+ .type = LSM_AUDIT_DATA_DENTRY,
+ .u.dentry = dentry,
+ },
+ .layer_plus_one = layer_index + 1,
+ }, no_rule_flags);
+
+ return -EACCES;
+}
+
/**
* current_check_refer_path - Check if a rename or link action is allowed
*
@@ -1319,6 +1689,16 @@ static int current_check_refer_path(struct dentry *const old_dentry,
access_request_parent2 =
get_mode_access(d_backing_inode(old_dentry)->i_mode);
if (removable) {
+ int err;
+
+ err = deny_no_inherit_topology_change(subject, old_dentry);
+ if (err)
+ return err;
+ if (exchange) {
+ err = deny_no_inherit_topology_change(subject, new_dentry);
+ if (err)
+ return err;
+ }
access_request_parent1 |= maybe_remove(old_dentry);
access_request_parent2 |= maybe_remove(new_dentry);
}
@@ -1711,12 +2091,31 @@ static int hook_path_symlink(const struct path *const dir,
static int hook_path_unlink(const struct path *const dir,
struct dentry *const dentry)
{
+ 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)
+ return err;
+ }
return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE);
}
static int hook_path_rmdir(const struct path *const dir,
struct dentry *const dentry)
{
+ 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)
+ return err;
+ }
+
return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
}
diff --git a/security/landlock/object.h b/security/landlock/object.h
index 5f28c35e8aa8..2520955825bb 100644
--- a/security/landlock/object.h
+++ b/security/landlock/object.h
@@ -13,6 +13,8 @@
#include <linux/refcount.h>
#include <linux/spinlock.h>
+#include "access.h"
+
struct landlock_object;
/**
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index fd633aee0599..63aa420ab593 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -48,6 +48,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
#endif /* IS_ENABLED(CONFIG_INET) */
new_ruleset->num_layers = num_layers;
+ xa_init(&new_ruleset->no_inherit_desc);
/*
* hierarchy = NULL
* num_rules = 0
@@ -108,6 +109,45 @@ static bool is_object_pointer(const enum landlock_key_type key_type)
}
}
+void landlock_set_no_inherit_desc_layers(struct landlock_ruleset *ruleset,
+ struct landlock_object *object,
+ layer_mask_t layers)
+{
+ struct landlock_no_inherit_desc *desc;
+ int err;
+
+ if (!ruleset || !object || !layers)
+ return;
+
+ desc = xa_load(&ruleset->no_inherit_desc, (unsigned long)object);
+ if (desc) {
+ desc->desc_layers |= layers;
+ return;
+ }
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
+ if (!desc)
+ return;
+
+ desc->object = object;
+ desc->desc_layers = layers;
+ landlock_get_object(object);
+ err = xa_insert(&ruleset->no_inherit_desc, (unsigned long)object, desc,
+ GFP_KERNEL_ACCOUNT);
+ if (err) {
+ struct landlock_no_inherit_desc *existing;
+
+ if (err == -EBUSY) {
+ existing = xa_load(&ruleset->no_inherit_desc,
+ (unsigned long)object);
+ if (existing)
+ existing->desc_layers |= layers;
+ }
+ landlock_put_object(object);
+ kfree(desc);
+ }
+}
+
static struct landlock_rule *
create_rule(const struct landlock_id id,
const struct landlock_layer (*const layers)[], const u32 num_layers,
@@ -257,6 +297,10 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
return -EINVAL;
this->layers[0].access |= (*layers)[0].access;
this->layers[0].flags.quiet |= (*layers)[0].flags.quiet;
+ this->layers[0].flags.no_inherit |=
+ (*layers)[0].flags.no_inherit;
+ this->layers[0].flags.has_no_inherit_descendant |=
+ (*layers)[0].flags.has_no_inherit_descendant;
return 0;
}
@@ -315,7 +359,10 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
.level = 0,
.flags = {
.quiet = !!(flags & LANDLOCK_ADD_RULE_QUIET),
- },
+ .no_inherit = !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
+ .has_no_inherit_descendant =
+ !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
+ }
} };
build_check_layer();
@@ -402,6 +449,15 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
goto out_unlock;
#endif /* IS_ENABLED(CONFIG_INET) */
+ {
+ unsigned long index;
+ struct landlock_no_inherit_desc *desc;
+
+ xa_for_each(&src->no_inherit_desc, index, desc)
+ landlock_set_no_inherit_desc_layers(dst, desc->object,
+ desc->desc_layers);
+ }
+
out_unlock:
mutex_unlock(&src->lock);
mutex_unlock(&dst->lock);
@@ -465,6 +521,15 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
goto out_unlock;
#endif /* IS_ENABLED(CONFIG_INET) */
+ {
+ unsigned long index;
+ struct landlock_no_inherit_desc *desc;
+
+ xa_for_each(&parent->no_inherit_desc, index, desc)
+ landlock_set_no_inherit_desc_layers(child, desc->object,
+ desc->desc_layers);
+ }
+
if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
err = -EINVAL;
goto out_unlock;
@@ -489,6 +554,8 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
static void free_ruleset(struct landlock_ruleset *const ruleset)
{
struct landlock_rule *freeme, *next;
+ struct landlock_no_inherit_desc *desc;
+ unsigned long index;
might_sleep();
rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
@@ -501,6 +568,12 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
free_rule(freeme, LANDLOCK_KEY_NET_PORT);
#endif /* IS_ENABLED(CONFIG_INET) */
+ xa_for_each(&ruleset->no_inherit_desc, index, desc) {
+ landlock_put_object(desc->object);
+ kfree(desc);
+ }
+ xa_destroy(&ruleset->no_inherit_desc);
+
landlock_put_hierarchy(ruleset->hierarchy);
kfree(ruleset);
}
@@ -660,9 +733,18 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
unsigned long access_bit;
bool is_empty;
+ /* Skip layers that already have no inherit flags. */
+ if (rule_flags &&
+ (rule_flags->no_inherit_masks & layer_bit))
+ continue;
+
/* Collect rule flags for each layer. */
if (rule_flags && layer->flags.quiet)
rule_flags->quiet_masks |= layer_bit;
+ if (rule_flags && layer->flags.no_inherit)
+ rule_flags->no_inherit_masks |= layer_bit;
+ if (rule_flags && layer->flags.has_no_inherit_descendant)
+ rule_flags->no_inherit_desc_masks |= layer_bit;
/*
* Records in @layer_masks which layer grants access to each requested
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index cd0434d8dc63..4e0484c1575d 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -15,6 +15,7 @@
#include <linux/rbtree.h>
#include <linux/refcount.h>
#include <linux/workqueue.h>
+#include <linux/xarray.h>
#include "access.h"
#include "limits.h"
@@ -40,6 +41,18 @@ struct landlock_layer {
* down the file hierarchy.
*/
bool quiet:1;
+ /**
+ * @no_inherit: When set, this layer's rule does not inherit
+ * allowed accesses from parent objects within the same layer.
+ * (currently only applies to filesystem objects)
+ */
+ bool no_inherit:1;
+ /**
+ * @has_no_inherit_descendant: Marks that a descendant rule within
+ * this layer carries the no-inherit flag and therefore seals
+ * topology changes on the path.
+ */
+ bool has_no_inherit_descendant:1;
} flags;
/**
* @access: Bitfield of allowed actions on the kernel object. They are
@@ -56,6 +69,15 @@ struct collected_rule_flags {
* @quiet_masks: Layers for which the quiet flag is effective.
*/
layer_mask_t quiet_masks;
+ /**
+ * @no_inherit_masks: Layers for which the no_inherit flag is effective.
+ */
+ layer_mask_t no_inherit_masks;
+ /**
+ * @no_inherit_desc_masks: Layers for which a descendant rule carries
+ * the no_inherit flag.
+ */
+ layer_mask_t no_inherit_desc_masks;
};
/**
@@ -136,6 +158,11 @@ struct landlock_rule {
struct landlock_layer layers[] __counted_by(num_layers);
};
+struct landlock_no_inherit_desc {
+ struct landlock_object *object;
+ layer_mask_t desc_layers;
+};
+
/**
* struct landlock_ruleset - Landlock ruleset
*
@@ -190,6 +217,12 @@ struct landlock_ruleset {
* @num_rules: Number of non-overlapping (i.e. not for
* the same object) rules in this ruleset.
*/
+ /**
+ * @no_inherit_desc: XArray containing objects
+ * with no_inherit descendants in this ruleset.
+ * This is used to quickly merge no_inherit flags,
+ */
+ struct xarray no_inherit_desc;
u32 num_rules;
/**
* @num_layers: Number of layers that are used in this
@@ -345,4 +378,10 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
layer_mask_t (*const layer_masks)[],
const enum landlock_key_type key_type);
+
+
+void landlock_set_no_inherit_desc_layers(struct landlock_ruleset *ruleset,
+ struct landlock_object *object,
+ layer_mask_t layers);
+
#endif /* _SECURITY_LANDLOCK_RULESET_H */
--
2.51.2
^ permalink raw reply related
* [PATCH 2/6] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
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>
Implements the syscall side flag handling and kernel api headers for the
LANDLOCK_ADD_RULE_NO_INHERIT flag.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
include/uapi/linux/landlock.h | 9 +++++++++
security/landlock/syscalls.c | 5 ++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 50f0806b7e33..d9daef551d96 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -127,10 +127,19 @@ struct landlock_ruleset_attr {
* 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.
+ * %LANDLOCK_ADD_RULE_NO_INHERIT
+ * When this flag is set while adding a rule to a ruleset, the rule
+ * will not inherit allowed accesses from rules on parent objects
+ * within the same layer. (currently only applies to filesystem objects)
+ * By default, Landlock rules added to a ruleset inherit allowed accesses
+ * from parent objects, meaning that if a parent directory has been granted
+ * certain access rights, those rights will also apply to its child objects.
+ * This flag prevents such inheritance for the specific rule being added.
*/
/* clang-format off */
#define LANDLOCK_ADD_RULE_QUIET (1U << 0)
+#define LANDLOCK_ADD_RULE_NO_INHERIT (1U << 1)
/* clang-format on */
/**
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 93396bfc1500..200287a34895 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -463,7 +463,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
if (!is_initialized())
return -EOPNOTSUPP;
- if (flags && flags != LANDLOCK_ADD_RULE_QUIET)
+ if (flags && flags & ~(LANDLOCK_ADD_RULE_QUIET | LANDLOCK_ADD_RULE_NO_INHERIT))
+ return -EINVAL;
+ if ((flags & LANDLOCK_ADD_RULE_NO_INHERIT) &&
+ rule_type != LANDLOCK_RULE_PATH_BENEATH)
return -EINVAL;
/* Gets and checks the ruleset. */
--
2.51.2
^ permalink raw reply related
* [PATCH 3/6] samples/landlock: Add LANDLOCK_ADD_RULE_NO_INHERIT to landlock-sandboxer
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>
Adds support to landlock-sandboxer with environment variables LL_FS_RO_NO_INHERIT
and LL_FS_RW_NO_INHERIT. These create the same rulesets as their non-no inherit varients,
plus the LANDLOCK_ADD_RULE_NO_INHERIT flag.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
samples/landlock/sandboxer.c | 39 +++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 2d8e3e94b77b..2b40b2df83b4 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -58,6 +58,8 @@ static inline int landlock_restrict_self(const int ruleset_fd,
#define ENV_FS_RO_NAME "LL_FS_RO"
#define ENV_FS_RW_NAME "LL_FS_RW"
+#define ENV_FS_RO_NO_INHERIT_NAME "LL_FS_RO_NO_INHERIT"
+#define ENV_FS_RW_NO_INHERIT_NAME "LL_FS_RW_NO_INHERIT"
#define ENV_FS_QUIET_NAME "LL_FS_QUIET"
#define ENV_FS_QUIET_ACCESS_NAME "LL_FS_QUIET_ACCESS"
#define ENV_TCP_BIND_NAME "LL_TCP_BIND"
@@ -121,7 +123,8 @@ static int parse_path(char *env_path, const char ***const path_list)
/* clang-format on */
static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
- const __u64 allowed_access, bool quiet)
+ const __u64 allowed_access,
+ __u32 add_rule_flags, bool mandatory)
{
int num_paths, i, ret = 1;
char *env_path_name;
@@ -132,9 +135,13 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
env_path_name = getenv(env_var);
if (!env_path_name) {
- /* Prevents users to forget a setting. */
- fprintf(stderr, "Missing environment variable %s\n", env_var);
- return 1;
+ if (mandatory) {
+ /* Prevents users to forget a setting. */
+ fprintf(stderr, "Missing environment variable %s\n",
+ env_var);
+ return 1;
+ }
+ return 0;
}
env_path_name = strdup(env_path_name);
unsetenv(env_var);
@@ -171,8 +178,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
if (!S_ISDIR(statbuf.st_mode))
path_beneath.allowed_access &= ACCESS_FILE;
if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
- &path_beneath,
- quiet ? LANDLOCK_ADD_RULE_QUIET : 0)) {
+ &path_beneath, add_rule_flags)) {
fprintf(stderr,
"Failed to update the ruleset with \"%s\": %s\n",
path_list[i], strerror(errno));
@@ -375,6 +381,8 @@ static const char help[] =
"Optional settings (when not set, their associated access check "
"is always allowed, which is different from an empty string which "
"means an empty list):\n"
+ "* " ENV_FS_RO_NO_INHERIT_NAME ": read-only paths without rule inheritance\n"
+ "* " ENV_FS_RW_NO_INHERIT_NAME ": read-write paths without rule inheritance\n"
"* " ENV_TCP_BIND_NAME ": ports allowed to bind (server)\n"
"* " ENV_TCP_CONNECT_NAME ": ports allowed to connect (client)\n"
"* " ENV_SCOPED_NAME ": actions denied on the outside of the landlock domain\n"
@@ -596,17 +604,28 @@ int main(const int argc, char *const argv[], char *const *const envp)
}
if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro,
- false)) {
+ 0, true)) {
goto err_close_ruleset;
}
if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw,
- false)) {
+ 0, true)) {
+ goto err_close_ruleset;
+ }
+ /* Optional no-inherit rules mirror the regular read-only/read-write sets. */
+ if (populate_ruleset_fs(ENV_FS_RO_NO_INHERIT_NAME, ruleset_fd,
+ access_fs_ro, LANDLOCK_ADD_RULE_NO_INHERIT,
+ false)) {
+ goto err_close_ruleset;
+ }
+ if (populate_ruleset_fs(ENV_FS_RW_NO_INHERIT_NAME, ruleset_fd,
+ access_fs_rw, LANDLOCK_ADD_RULE_NO_INHERIT,
+ false)) {
goto err_close_ruleset;
}
/* Don't require this env to be present. */
- if (quiet_supported && getenv(ENV_FS_QUIET_NAME)) {
+ if (quiet_supported) {
if (populate_ruleset_fs(ENV_FS_QUIET_NAME, ruleset_fd, 0,
- true)) {
+ LANDLOCK_ADD_RULE_QUIET, false)) {
goto err_close_ruleset;
}
}
--
2.51.2
^ permalink raw reply related
* [PATCH 4/6] selftests/landlock: Implement selftests 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>
Implements 5 selftests for the flag, covering allowed and disallowed operations on parent
and child directories when this flag is set, as well as multi-layer configurations.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
tools/testing/selftests/landlock/fs_test.c | 222 +++++++++++++++++++--
1 file changed, 210 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d4819ff44230..1cdded3f67e6 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -717,16 +717,12 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
}
static void add_path_beneath(struct __test_metadata *const _metadata,
- const int ruleset_fd, const __u64 allowed_access,
- const char *const path, bool quiet)
+ const int ruleset_fd, const __u64 allowed_access,
+ const char *const path, __u32 flags)
{
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = allowed_access,
};
- __u32 flags = 0;
-
- if (quiet)
- flags |= LANDLOCK_ADD_RULE_QUIET;
path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
ASSERT_LE(0, path_beneath.parent_fd)
@@ -790,7 +786,7 @@ static int create_ruleset(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
- rules[i].path, false);
+ rules[i].path, 0);
}
return ruleset_fd;
}
@@ -1368,7 +1364,7 @@ TEST_F_FORK(layout1, inherit_subset)
* ANDed with the previous ones.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d2, false);
+ dir_s1d2, 0);
/*
* According to ruleset_fd, dir_s1d2 should now have the
* LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
@@ -1400,7 +1396,7 @@ TEST_F_FORK(layout1, inherit_subset)
* Try to get more privileges by adding new access rights to the parent
* directory: dir_s1d1.
*/
- add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, false);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, 0);
enforce_ruleset(_metadata, ruleset_fd);
/* Same tests and results as above. */
@@ -1423,7 +1419,7 @@ TEST_F_FORK(layout1, inherit_subset)
* that there was no rule tied to it before.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d3, false);
+ dir_s1d3, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1476,7 +1472,7 @@ TEST_F_FORK(layout1, inherit_superset)
add_path_beneath(_metadata, ruleset_fd,
LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_READ_DIR,
- dir_s1d2, false);
+ dir_s1d2, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1488,6 +1484,39 @@ TEST_F_FORK(layout1, inherit_superset)
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
}
+TEST_F_FORK(layout1, inherit_no_inherit_flag)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW,
+ };
+ int ruleset_fd;
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, 0);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Parent directory still grants write access to its direct children. */
+ EXPECT_EQ(0, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d1, O_WRONLY));
+
+ /* dir_s1d2 gets only its explicit read-only access rights. */
+ EXPECT_EQ(0, test_open(dir_s1d2, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d2, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
+
+ /* Descendants of dir_s1d2 inherit the reduced access mask. */
+ EXPECT_EQ(0, test_open(dir_s1d3, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s1d3, O_WRONLY));
+}
+
TEST_F_FORK(layout0, max_layers)
{
int i, err;
@@ -4412,6 +4441,175 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
ASSERT_EQ(0, close(cli_fd));
}
+TEST_F_FORK(layout1, inherit_no_inherit_topology_dir)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ int ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds a no-inherit rule on a leaf directory. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Topology modifications of the rule path and its parents are denied.
+ */
+
+ /* Target directory s1d3 */
+ ASSERT_EQ(-1, rmdir(dir_s1d3));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d3, dir_s2d3));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Parent directory s1d2 */
+ ASSERT_EQ(-1, rmdir(dir_s1d2));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d2));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Grandparent directory s1d1 */
+ ASSERT_EQ(-1, rmdir(dir_s1d1));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d1, dir_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * Sibling operations are allowed.
+ */
+ /* Sibling of s1d3 */
+ ASSERT_EQ(0, unlink(file1_s1d2));
+ /* Sibling of s1d2 */
+ ASSERT_EQ(0, unlink(file1_s1d1));
+
+ /*
+ * Content of the no-inherit directory is restricted by the rule (RO).
+ */
+ ASSERT_EQ(-1, unlink(file1_s1d3));
+ ASSERT_EQ(EACCES, errno);
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_topology_unrelated)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ static const char unrelated_dir[] = TMP_DIR "/s2d1/unrelated";
+ static const char unrelated_file[] = TMP_DIR "/s2d1/unrelated/f1";
+ int ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds a no-inherit rule on a leaf directory unrelated to s2. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Ensure we can still create and delete files outside the sealed branch. */
+ ASSERT_EQ(0, mkdir(unrelated_dir, 0700));
+ ASSERT_EQ(0, mknod(unrelated_file, S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(unrelated_file));
+ ASSERT_EQ(0, rmdir(unrelated_dir));
+
+ /* Existing siblings in s2 remain modifiable. */
+ ASSERT_EQ(0, unlink(file1_s2d1));
+ ASSERT_EQ(0, mknod(file1_s2d1, S_IFREG | 0700, 0));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_topology_file)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ int ruleset_fd;
+ struct landlock_path_beneath_attr path_beneath = {
+ .allowed_access = ACCESS_RO,
+ };
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, path_beneath.parent_fd);
+ ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &path_beneath,
+ LANDLOCK_ADD_RULE_NO_INHERIT));
+ ASSERT_EQ(EINVAL, errno);
+ ASSERT_EQ(0, close(path_beneath.parent_fd));
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_layered)
+{
+ const struct rule layer1[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ int ruleset_fd;
+ static const char unrelated_dir[] = TMP_DIR "/s2d1/unrelated";
+ static const char unrelated_file[] = TMP_DIR "/s2d1/unrelated/f1";
+
+ /* Layer 1: RW on TMP_DIR */
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Layer 2: Add no-inherit RO rule on s1d2 */
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1);
+ ASSERT_LE(0, ruleset_fd);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Operations in unrelated areas should still work */
+ ASSERT_EQ(0, mkdir(unrelated_dir, 0700));
+ ASSERT_EQ(0, mknod(unrelated_file, S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(unrelated_file));
+ ASSERT_EQ(0, rmdir(unrelated_dir));
+
+ /* Creating in s1d1 should be allowed (parent still has RW) */
+ ASSERT_EQ(0, mknod(TMP_DIR "/s1d1/newfile", S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(TMP_DIR "/s1d1/newfile"));
+
+ /* Content of s1d2 should be read-only */
+ ASSERT_EQ(-1, unlink(file1_s1d2));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Topology changes to s1d2 should be denied */
+ ASSERT_EQ(-1, rename(dir_s1d2, TMP_DIR "/s2d1/renamed"));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Renaming s1d1 should also be denied (it's an ancestor) */
+ ASSERT_EQ(-1, rename(dir_s1d1, TMP_DIR "/s2d1/renamed"));
+ ASSERT_EQ(EACCES, errno);
+}
+
/* clang-format off */
FIXTURE(ioctl) {};
@@ -7647,7 +7845,7 @@ static int apply_a_layer(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, rs_fd, r->access, r->path,
- r->quiet);
+ r->quiet ? LANDLOCK_ADD_RULE_QUIET : 0);
}
ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
--
2.51.2
^ permalink raw reply related
* [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
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