* Re: [syzbot] [lsm?] WARNING in put_cred_rcu
From: Christian Brauner @ 2025-11-10 8:45 UTC (permalink / raw)
To: syzbot
Cc: linux-kernel, linux-security-module, luto, peterz, syzkaller-bugs,
tglx
In-Reply-To: <690dfca7.a70a0220.22f260.004d.GAE@google.com>
On Fri, Nov 07, 2025 at 06:05:27AM -0800, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 17490bd0527f Add linux-next specific files for 20251104
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=16c09bcd980000
> kernel config: https://syzkaller.appspot.com/x/.config?x=9995c0d2611ab121
> dashboard link: https://syzkaller.appspot.com/bug?extid=553c4078ab14e3cf3358
> compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1500a532580000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=12e0a114580000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/a4d318147846/disk-17490bd0.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/86641a470170/vmlinux-17490bd0.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/35c008a540c8/bzImage-17490bd0.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+553c4078ab14e3cf3358@syzkaller.appspotmail.com
#syz test: https://github.com/brauner/linux.git work.namespace-6.19.fixes
^ permalink raw reply
* Re: [syzbot] [lsm?] WARNING in put_cred_rcu
From: syzbot @ 2025-11-10 10:09 UTC (permalink / raw)
To: brauner, linux-kernel, linux-security-module, luto, peterz,
syzkaller-bugs, tglx
In-Reply-To: <20251110-hufen-klang-9cf8ec9dda59@brauner>
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-by: syzbot+553c4078ab14e3cf3358@syzkaller.appspotmail.com
Tested-by: syzbot+553c4078ab14e3cf3358@syzkaller.appspotmail.com
Tested on:
commit: 80156be0 ns: fixes for namespace iteration and active ..
git tree: https://github.com/brauner/linux.git work.namespace-6.19.fixes
console output: https://syzkaller.appspot.com/x/log.txt?x=17fc5412580000
kernel config: https://syzkaller.appspot.com/x/.config?x=e31f5f45f87b6763
dashboard link: https://syzkaller.appspot.com/bug?extid=553c4078ab14e3cf3358
compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply
* Re: [RFC PATCH 2/3] exec: don't wait for zombie threads with cred_guard_mutex held
From: Cyrill Gorcunov @ 2025-11-10 10:58 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module
In-Reply-To: <aRDMNWx-69fL_gf-@redhat.com>
On Sun, Nov 09, 2025 at 06:15:33PM +0100, Oleg Nesterov wrote:
..
> static int kill_sub_threads(struct task_struct *tsk)
> {
> struct signal_struct *sig = tsk->signal;
> int err = -EINTR;
>
> read_lock(&tasklist_lock);
> spin_lock_irq(&tsk->sighand->siglock);
> if (!((sig->flags & SIGNAL_GROUP_EXIT) || sig->group_exec_task)) {
> sig->group_exec_task = tsk;
> sig->notify_count = -zap_other_threads(tsk);
Hi Oleg! I somehow manage to miss a moment -- why negative result here?
> err = 0;
> }
> spin_unlock_irq(&tsk->sighand->siglock);
> read_unlock(&tasklist_lock);
>
> return err;
> }
p.s. i've dropped long CC but left ML intact)
Cyrill
^ permalink raw reply
* [PATCH] security: Add KUnit tests for rootid_owns_currentns()
From: Ryan Foster @ 2025-11-10 14:37 UTC (permalink / raw)
To: selinux; +Cc: linux-security-module, linux-kernel, serge, paul, Ryan Foster
The rootid_owns_currentns() function in security/commoncap.c is a
security-critical function used to determine if a root ID from a
parent namespace owns the current namespace. This function is used
in multiple security-critical paths:
1. cap_inode_getsecurity() - file capability retrieval
2. get_vfs_caps_from_disk() - file capability loading during exec
Despite its security-critical nature, this function had no test
coverage. This patch adds KUnit tests to verify the function's
behavior in various scenarios:
- Root ID in init namespace (positive case)
- Invalid vfsuid (negative case)
- Non-root UID (negative case)
The function is made testable by exporting it when
CONFIG_SECURITY_COMMONCAP_KUNIT_TEST is enabled, while maintaining
static visibility in production builds.
This follows the pattern established by other security subsystems
(IPE, Landlock) for KUnit testing.
Signed-off-by: Ryan Foster <foster.ryan.r@gmail.com>
---
security/Kconfig | 17 ++++++
security/Makefile | 1 +
security/commoncap.c | 5 ++
security/commoncap_test.c | 109 ++++++++++++++++++++++++++++++++++++++
4 files changed, 132 insertions(+)
create mode 100644 security/commoncap_test.c
diff --git a/security/Kconfig b/security/Kconfig
index 285f284dfcac..c7b3f42ef875 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -284,6 +284,23 @@ config LSM
If unsure, leave this as the default.
+config SECURITY_COMMONCAP_KUNIT_TEST
+ bool "Build KUnit tests for commoncap" if !KUNIT_ALL_TESTS
+ depends on KUNIT=y
+ default KUNIT_ALL_TESTS
+ help
+ This builds the commoncap KUnit tests.
+
+ KUnit tests run during boot and output the results to the debug log
+ in TAP format (https://testanything.org/). Only useful for kernel devs
+ running KUnit test harness and are not for inclusion into a
+ production build.
+
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
source "security/Kconfig.hardening"
endmenu
diff --git a/security/Makefile b/security/Makefile
index 22ff4c8bd8ce..5b1285fde552 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_KEYS) += keys/
# always enable default capabilities
obj-y += commoncap.o
+obj-$(CONFIG_SECURITY_COMMONCAP_KUNIT_TEST) += commoncap_test.o
obj-$(CONFIG_SECURITY) += lsm_syscalls.o
obj-$(CONFIG_MMU) += min_addr.o
diff --git a/security/commoncap.c b/security/commoncap.c
index 6bd4adeb4795..4d0e014ce7cd 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -358,7 +358,12 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry)
return error;
}
+#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
+bool rootid_owns_currentns(vfsuid_t rootvfsuid);
+bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+#else
static bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+#endif
{
struct user_namespace *ns;
kuid_t kroot;
diff --git a/security/commoncap_test.c b/security/commoncap_test.c
new file mode 100644
index 000000000000..9757d433d314
--- /dev/null
+++ b/security/commoncap_test.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * KUnit tests for commoncap.c security functions
+ *
+ * Tests for security-critical functions in the capability subsystem,
+ * particularly namespace-related capability checks.
+ */
+
+#include <kunit/test.h>
+#include <linux/user_namespace.h>
+#include <linux/uidgid.h>
+#include <linux/module.h>
+
+/* We need to include the actual vfsuid_t definition but avoid the problematic
+ * inline functions in mnt_idmapping.h. Include just the type definitions.
+ */
+#include <linux/types.h>
+
+/* Forward declare the minimal types we need - match the actual kernel definitions */
+struct mnt_idmap;
+typedef struct {
+ uid_t val;
+} vfsuid_t;
+
+/* Minimal macros we need - match kernel definitions from mnt_idmapping.h */
+static inline uid_t __vfsuid_val(vfsuid_t uid)
+{
+ return uid.val;
+}
+
+#define VFSUIDT_INIT(val) ((vfsuid_t){ __kuid_val(val) })
+#define INVALID_VFSUID VFSUIDT_INIT(INVALID_UID)
+
+#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
+
+/* Forward declaration - function is exported when KUNIT_TEST is enabled */
+extern bool rootid_owns_currentns(vfsuid_t rootvfsuid);
+
+/**
+ * test_rootid_owns_currentns_init_ns - Test rootid_owns_currentns with init ns
+ *
+ * Verifies that a root ID in the init namespace correctly owns the current
+ * namespace when running in init_user_ns.
+ */
+static void test_rootid_owns_currentns_init_ns(struct kunit *test)
+{
+ vfsuid_t root_vfsuid;
+ kuid_t root_kuid;
+
+ /* Create a root UID in init namespace */
+ root_kuid = KUIDT_INIT(0);
+ root_vfsuid = VFSUIDT_INIT(root_kuid);
+
+ /* In init namespace, root should own current namespace */
+ KUNIT_EXPECT_TRUE(test, rootid_owns_currentns(root_vfsuid));
+}
+
+/**
+ * test_rootid_owns_currentns_invalid - Test rootid_owns_currentns with invalid vfsuid
+ *
+ * Verifies that an invalid vfsuid correctly returns false.
+ */
+static void test_rootid_owns_currentns_invalid(struct kunit *test)
+{
+ vfsuid_t invalid_vfsuid;
+
+ /* Use the predefined invalid vfsuid */
+ invalid_vfsuid = INVALID_VFSUID;
+
+ /* Invalid vfsuid should return false */
+ KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(invalid_vfsuid));
+}
+
+/**
+ * test_rootid_owns_currentns_nonroot - Test rootid_owns_currentns with non-root UID
+ *
+ * Verifies that a non-root UID correctly returns false.
+ */
+static void test_rootid_owns_currentns_nonroot(struct kunit *test)
+{
+ vfsuid_t nonroot_vfsuid;
+ kuid_t nonroot_kuid;
+
+ /* Create a non-root UID */
+ nonroot_kuid = KUIDT_INIT(1000);
+ nonroot_vfsuid = VFSUIDT_INIT(nonroot_kuid);
+
+ /* Non-root UID should return false */
+ KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(nonroot_vfsuid));
+}
+
+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),
+ {}
+};
+
+static struct kunit_suite commoncap_test_suite = {
+ .name = "commoncap",
+ .test_cases = commoncap_test_cases,
+};
+
+kunit_test_suite(commoncap_test_suite);
+
+MODULE_LICENSE("GPL");
+
+#endif /* CONFIG_SECURITY_COMMONCAP_KUNIT_TEST */
+
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH 0/3] mt-exec: fix deadlock with ptrace_attach()
From: Oleg Nesterov @ 2025-11-10 14:47 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Linus Torvalds, Dmitry Levin, Alexander Viro, Alexey Dobriyan,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Eric W. Biederman, 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: <GV2PPF74270EBEE83C2CA09B945BC954FA3E4CEA@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
Hi Bernd,
On 11/10, Bernd Edlinger wrote:
>
> When the debugger wants to attach the de_thread the debug-user access rights are
> checked against the current user and additionally against the new user credentials.
> This I did by quickly switching the user credenitals to the next user and back again,
> under the cred_guard_mutex, which should make that safe.
Let me repeat, I can't really comment this part, I don't know if it is
actually safe. But the very fact your patch changes ->mm and ->cred of
the execing task in ptrace_attach() makes me worry... At least I think
you should update or remove this comment in begin_new_exec:
/*
* cred_guard_mutex 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.
*/
security_bprm_committed_creds(bprm);
> So at this time I have only one request for you.
> Could you please try out how the test case in my patch behaves with your fix?
The new TEST(attach2) added by your patch fails as expected, see 3/3.
128 static long thread2_tid;
129 static void *thread2(void *arg)
130 {
131 thread2_tid = syscall(__NR_gettid);
132 sleep(2);
133 execlp("false", "false", NULL);
134 return NULL;
135 }
136
137 TEST(attach2)
138 {
139 int s, k, pid = fork();
140
141 if (!pid) {
142 pthread_t pt;
143
144 pthread_create(&pt, NULL, thread2, NULL);
145 pthread_join(pt, NULL);
146 return;
147 }
148
149 sleep(1);
150 k = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
151 ASSERT_EQ(k, 0);
152 k = waitpid(-1, &s, 0);
153 ASSERT_EQ(k, pid);
154 ASSERT_EQ(WIFSTOPPED(s), 1);
155 ASSERT_EQ(WSTOPSIG(s), SIGSTOP);
156 k = ptrace(PTRACE_SETOPTIONS, pid, 0L, PTRACE_O_TRACEEXIT);
157 ASSERT_EQ(k, 0);
158 thread2_tid = ptrace(PTRACE_PEEKDATA, pid, &thread2_tid, 0L);
159 ASSERT_NE(thread2_tid, -1);
160 ASSERT_NE(thread2_tid, 0);
161 ASSERT_NE(thread2_tid, pid);
162 k = waitpid(-1, &s, WNOHANG);
163 ASSERT_EQ(k, 0);
164 sleep(2);
165 /* deadlock may happen here */
166 k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L);
PTRACE_ATTACH fails.
thread2() kills the old leader, takes it pid, execlp() succeeds.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH 2/3] exec: don't wait for zombie threads with cred_guard_mutex held
From: Oleg Nesterov @ 2025-11-10 15:09 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module
In-Reply-To: <aRHFSrTxYSOkFic7@grain>
Hi Cyrill,
On 11/10, Cyrill Gorcunov wrote:
>
> On Sun, Nov 09, 2025 at 06:15:33PM +0100, Oleg Nesterov wrote:
> ..
> > static int kill_sub_threads(struct task_struct *tsk)
> > {
> > struct signal_struct *sig = tsk->signal;
> > int err = -EINTR;
> >
> > read_lock(&tasklist_lock);
> > spin_lock_irq(&tsk->sighand->siglock);
> > if (!((sig->flags & SIGNAL_GROUP_EXIT) || sig->group_exec_task)) {
> > sig->group_exec_task = tsk;
> > sig->notify_count = -zap_other_threads(tsk);
>
> Hi Oleg! I somehow manage to miss a moment -- why negative result here?
You know, initially I wrote
sig->notify_count = 0 - zap_other_threads(tsk);
to make it clear that this is not a typo ;)
This is for exit_notify() which does
/* mt-exec, de_thread() -> wait_for_notify_count() */
if (tsk->signal->notify_count < 0 && !++tsk->signal->notify_count)
wake_up_process(tsk->signal->group_exec_task);
Then setup_new_exec() sets notify_count > 0 for __exit_signal() which does
/* mt-exec, setup_new_exec() -> wait_for_notify_count() */
if (sig->notify_count > 0 && !--sig->notify_count)
wake_up_process(sig->group_exec_task);
Yes this needs more comments and (with or without this patch) cleanups.
Note that exit_notify() and __exit_signal() already (before this patch)
use ->notify_count almost the same way, just exit_notify() assumes that
notify_count < 0 means the !thread_group_leader() case in de_thread().
Oleg.
^ permalink raw reply
* Re: [PATCH v5 11/14] Add start_renaming_two_dentries()
From: Stephen Smalley @ 2025-11-10 16:08 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Amir Goldstein, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <20251106005333.956321-12-neilb@ownmail.net>
On Wed, Nov 5, 2025 at 7:56 PM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> A few callers want to lock for a rename and already have both dentries.
> Also debugfs does want to perform a lookup but doesn't want permission
> checking, so start_renaming_dentry() cannot be used.
>
> This patch introduces start_renaming_two_dentries() which is given both
> dentries. debugfs performs one lookup itself. As it will only continue
> with a negative dentry and as those cannot be renamed or unlinked, it is
> safe to do the lookup before getting the rename locks.
>
> overlayfs uses start_renaming_two_dentries() in three places and selinux
> uses it twice in sel_make_policy_nodes().
>
> In sel_make_policy_nodes() we now lock for rename twice instead of just
> once so the combined operation is no longer atomic w.r.t the parent
> directory locks. As selinux_state.policy_mutex is held across the whole
> operation this does open up any interesting races.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
>
> ---
> changes since v3:
> added missing assignment to rd.mnt_idmap in ovl_cleanup_and_whiteout
> ---
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 232e087bce3e..a224ef9bb831 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
> if (ret)
> goto out;
>
> - lock_rename(tmp_parent, fsi->sb->s_root);
> + rd.old_parent = tmp_parent;
> + rd.new_parent = fsi->sb->s_root;
>
> /* booleans */
> - d_exchange(tmp_bool_dir, fsi->bool_dir);
> + ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> + if (!ret) {
> + d_exchange(tmp_bool_dir, fsi->bool_dir);
I would recommend an immediate goto out if ret != 0; we don't want to
silently fall through and possibly reset ret on the next
start_renaming_two_dentries() call, thereby ultimately returning 0 to
the caller and acting as if nothing bad happened.
>
> - swap(fsi->bool_num, bool_num);
> - swap(fsi->bool_pending_names, bool_names);
> - swap(fsi->bool_pending_values, bool_values);
> + swap(fsi->bool_num, bool_num);
> + swap(fsi->bool_pending_names, bool_names);
> + swap(fsi->bool_pending_values, bool_values);
>
> - fsi->bool_dir = tmp_bool_dir;
> + fsi->bool_dir = tmp_bool_dir;
> + end_renaming(&rd);
> + }
>
> /* classes */
> - d_exchange(tmp_class_dir, fsi->class_dir);
> - fsi->class_dir = tmp_class_dir;
> + ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> + if (ret == 0) {
> + d_exchange(tmp_class_dir, fsi->class_dir);
> + fsi->class_dir = tmp_class_dir;
>
> - unlock_rename(tmp_parent, fsi->sb->s_root);
> + end_renaming(&rd);
> + }
>
> out:
> sel_remove_old_bool_data(bool_num, bool_names, bool_values);
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [PATCH v5 11/14] Add start_renaming_two_dentries()
From: Stephen Smalley @ 2025-11-10 17:30 UTC (permalink / raw)
To: NeilBrown
Cc: Alexander Viro, Christian Brauner, Amir Goldstein, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <CAEjxPJ528Ou4dvRwHo+kXjWreGicda8BOXkQRvq3vMED6JQKOQ@mail.gmail.com>
On Mon, Nov 10, 2025 at 11:08 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Nov 5, 2025 at 7:56 PM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > A few callers want to lock for a rename and already have both dentries.
> > Also debugfs does want to perform a lookup but doesn't want permission
> > checking, so start_renaming_dentry() cannot be used.
> >
> > This patch introduces start_renaming_two_dentries() which is given both
> > dentries. debugfs performs one lookup itself. As it will only continue
> > with a negative dentry and as those cannot be renamed or unlinked, it is
> > safe to do the lookup before getting the rename locks.
> >
> > overlayfs uses start_renaming_two_dentries() in three places and selinux
> > uses it twice in sel_make_policy_nodes().
> >
> > In sel_make_policy_nodes() we now lock for rename twice instead of just
> > once so the combined operation is no longer atomic w.r.t the parent
> > directory locks. As selinux_state.policy_mutex is held across the whole
> > operation this does open up any interesting races.
Also, I assume you mean "does NOT open up any interesting races" above.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: NeilBrown <neil@brown.name>
> >
> > ---
> > changes since v3:
> > added missing assignment to rd.mnt_idmap in ovl_cleanup_and_whiteout
> > ---
>
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 232e087bce3e..a224ef9bb831 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
> > if (ret)
> > goto out;
> >
> > - lock_rename(tmp_parent, fsi->sb->s_root);
> > + rd.old_parent = tmp_parent;
> > + rd.new_parent = fsi->sb->s_root;
> >
> > /* booleans */
> > - d_exchange(tmp_bool_dir, fsi->bool_dir);
> > + ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> > + if (!ret) {
> > + d_exchange(tmp_bool_dir, fsi->bool_dir);
>
> I would recommend an immediate goto out if ret != 0; we don't want to
> silently fall through and possibly reset ret on the next
> start_renaming_two_dentries() call, thereby ultimately returning 0 to
> the caller and acting as if nothing bad happened.
>
> >
> > - swap(fsi->bool_num, bool_num);
> > - swap(fsi->bool_pending_names, bool_names);
> > - swap(fsi->bool_pending_values, bool_values);
> > + swap(fsi->bool_num, bool_num);
> > + swap(fsi->bool_pending_names, bool_names);
> > + swap(fsi->bool_pending_values, bool_values);
> >
> > - fsi->bool_dir = tmp_bool_dir;
> > + fsi->bool_dir = tmp_bool_dir;
> > + end_renaming(&rd);
> > + }
> >
> > /* classes */
> > - d_exchange(tmp_class_dir, fsi->class_dir);
> > - fsi->class_dir = tmp_class_dir;
> > + ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> > + if (ret == 0) {
> > + d_exchange(tmp_class_dir, fsi->class_dir);
> > + fsi->class_dir = tmp_class_dir;
> >
> > - unlock_rename(tmp_parent, fsi->sb->s_root);
> > + end_renaming(&rd);
> > + }
> >
> > out:
> > sel_remove_old_bool_data(bool_num, bool_names, bool_values);
> > --
> > 2.50.0.107.gf914562f5916.dirty
> >
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Tahera Fahimi @ 2025-11-10 19:06 UTC (permalink / raw)
To: Anirudh Venkataramanan, zohar, roberto.sassu, dmitry.kasatkin,
eric.snowberg, paul, jmorris, serge, linux-integrity,
linux-security-module, linux-kernel, code
In-Reply-To: <b36a6508-1b2a-4c87-b3b5-9af0b402dc0b@linux.microsoft.com>
On 11/6/2025 12:32 PM, Anirudh Venkataramanan wrote:
> On 11/6/2025 10:14 AM, Tahera Fahimi wrote:
>> Prevent redundant IMA policy rules by checking for duplicates before insertion. This ensures that
>> rules are not re-added when userspace is restarted (using systemd-soft-reboot) without a full system
>> reboot. ima_rule_exists() detects duplicates in both temporary and active rule lists.
>
> I have run into this too. Thanks for proposing a patch!
>
> FWIW - I am fairly new to the IMA subsystem, so feedback below is mostly structural, with some IMA specific comments.
Hi Ahirudh, Thanks for your feedback.
>>
>> Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
>> ---
>> security/integrity/ima/ima_policy.c | 157 +++++++++++++++++++++++++++-
>> 1 file changed, 156 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 164d62832f8ec..3dd902101dbda 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -1953,6 +1953,153 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>> return result;
>> }
>> +static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
>> +{
>> + int j;
>
> j is declared in the loop header below too, which is more correct because it keeps the scope of j to be within the loop. So I'd say get rid of the above declaration.
The declaration of j is at the beginning to adhere proper kernel style and ancient compile support.
>> +
>> + for (int j = 0; j < template2->num_fields; j++)
>> + if (strcmp(field_id, template2->fields[j]->field_id) == 0)
>> + return true;
> I believe the preferred kernel style is to use if (!strcmp(...)).
>
>> +
>> + return false;
>> +}
>> +
>> +static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
>> +{
>> + int j;
>> +
>> + for (j = 0; j < keyrings->count; j++) {
>> + if (strcmp(item, keyrings->items[j]) == 0)
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
>> +{
>> + int j;
>> +
>> + for (j = 0; j < labels->count; j++) {
>> + if (strcmp(item, labels->items[j]) == 0)
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
>> +{
>> + int i;
>
> i is used further down in this function, and even in all those cases, the scope of i can be limited to the loop body where it's used.
>
> If you didn't know this already - you can use cppcheck to identify and reduce the scope of variables.
>
>> +
>> + if (rule1->flags != rule2->flags)
>> + return false;
>> +
>> + if (rule1->action != rule2->action)
>> + return false;
>> +
>> + if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
>> + ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
>> + ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
>> + ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
>> + ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
>> + ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
>> + ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
>> + ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
>> + ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
>> + ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
>> + ((rule1->flags & IMA_VALIDATE_ALGOS) &&
>> + rule1->allowed_algos != rule2->allowed_algos) ||
>> + ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
>> + ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
>> + return false;
>
> So the goal is to prevent the exact same policy rule from being added, not to update an existing rule, correct? IOW, you could end up with two very similar rules, because the new rule has one thing that's different compared to the existing rule?
The purpose of this patch is to prohibit two exact same rule.
We can have other approaches like merging the new rule to the previously existing rule, ignore
new rule if a similar rule exists. However, this approaches would add more complexity to the code
and are not the purpose of this patch.
> I feel that a little bit of commentary around what makes two rules the same would be useful.
>
>> +
>> + if (!rule1->template && !rule2->template) {
>> + ;
> You're trying to do nothing and continue on. A goto statement would communicate intent better. There are other places below with the same noop structure.
>
> To be fair, I also don't completely understand what you're trying to achieve here, Regardless, this "do nothing inside a conditional" looks weird and I feel like there should be a way to structure your logic without resorting to this.
>
>> + } else if (!rule1->template || !rule2->template) {
>> + return false;
>> + } else if (rule1->template->num_fields != rule2->template->num_fields) {
>> + return false;
>> + } else if (rule1->template->num_fields != 0) {
>> + for (i = 0; i < rule1->template->num_fields; i++) {
>> + if (!template_has_field(rule1->template->fields[i]->field_id,
>> + rule2->template))
>> + return false;
>> + }
>> + }
>
> if + return will achieve the same end goals as else if + return, with lesser clutter. I have seen some static analyzers flag this pattern, but I can't remember which one at the moment.
>
> So something like this:
>
> if (!rule1->template && !rule2->template)
> goto some_target;
>
> if (!rule1->template || !rule2->template)
> return false;
>
> if (rule1->template->num_fields != rule2->template->num_fields)
> return false;
>
> if (rule1->template->num_fields != 0) {
> for (i = 0; i < rule1->template->num_fields; i++) {
> if (!template_has_field(rule1->template->fields[i]->field_id,
> rule2->template))
> return false;
> }
> }> some_target:
> ...
> ...
I don't think having two goto in the code will improve its readability.
>> +
>> + if (rule1->flags & IMA_KEYRINGS) {
>> + if (!rule1->keyrings && !rule2->keyrings) {
>> + ;
>
> Another if block no-op
>
>> + } else if (!rule1->keyrings || !rule2->keyrings) {
>> + return false;
>> + } else if (rule1->keyrings->count != rule2->keyrings->count) {
>> + return false;
>> + } else if (rule1->keyrings->count != 0) {
>
> if (rule1->keyrings->count)
>
>> + for (i = 0; i < rule1->keyrings->count; i++) {
>
> for (int i,
>
>> + if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
>> + return false;
>> + }
>> + }
>> + }
>> +
>> + if (rule1->flags & IMA_LABEL) {
>> + if (!rule1->label && !rule2->label) {
>> + ;
>
> Another if block no-op
>
>> + } else if (!rule1->label || !rule2->label) {
>> + return false;
>> + } else if (rule1->label->count != rule2->label->count) {
>> + return false;
>> + } else if (rule1->label->count != 0) {
>> + for (i = 0; i < rule1->label->count; i++) {
>> + if (!labels_has_item(rule1->label->items[i], rule2->label))
>> + return false;
>> + }
>> + }
>> + }
>> +
>> + for (i = 0; i < MAX_LSM_RULES; i++) {
>
> for (int i,
>
>> + if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
>> + continue;
>> +
>> + if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
>> + return false;
>> +
>> + if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +/**
>> + * ima_rule_exists - check if a rule already exists in the policy
>> + *
>> + * Checking both the active policy and the temporary rules list.
>> + */
>> +static bool ima_rule_exists(struct ima_rule_entry *new_rule)
>> +{
>> + struct ima_rule_entry *entry;
>> + struct list_head *ima_rules_tmp;
>> +
>> + if (!list_empty(&ima_temp_rules)) {
>> + list_for_each_entry(entry, &ima_temp_rules, list) {
>> + if (ima_rules_equal(entry, new_rule))
>> + return true;
>> + }
>> + }
>> +
>> + rcu_read_lock();
>> + ima_rules_tmp = rcu_dereference(ima_rules);
>> + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
>> + if (ima_rules_equal(entry, new_rule)) {
>> + rcu_read_unlock();
>> + return true;
>> + }
>> + }
>> + rcu_read_unlock();
>> +
>> + return false;
>> +}
>> +
>> /**
>> * ima_parse_add_rule - add a rule to ima_policy_rules
>> * @rule: ima measurement policy rule
>> @@ -1993,7 +2140,15 @@ ssize_t ima_parse_add_rule(char *rule)
>> return result;
>> }
>> - list_add_tail(&entry->list, &ima_temp_rules);
>> + if (!ima_rule_exists(entry)) {
>> + list_add_tail(&entry->list, &ima_temp_rules);
>> + } else {
>> + result = -EEXIST;
> Is it necessary to set result? Or can you just pass -EEXIST to the audit call below?
>
>> + ima_free_rule(entry);
>> + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
>> + NULL, op, "duplicate-policy", result,
>> + audit_info);
>> + }
>> return len;
>> }
I
^ permalink raw reply
* Re: [RFC PATCH 2/3] exec: don't wait for zombie threads with cred_guard_mutex held
From: Cyrill Gorcunov @ 2025-11-10 21:49 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module
In-Reply-To: <aRIAEYH2iLLN-Fjg@redhat.com>
On Mon, Nov 10, 2025 at 04:09:05PM +0100, Oleg Nesterov wrote:
...
> > > if (!((sig->flags & SIGNAL_GROUP_EXIT) || sig->group_exec_task)) {
> > > sig->group_exec_task = tsk;
> > > sig->notify_count = -zap_other_threads(tsk);
> >
> > Hi Oleg! I somehow manage to miss a moment -- why negative result here?
>
> You know, initially I wrote
>
> sig->notify_count = 0 - zap_other_threads(tsk);
>
> to make it clear that this is not a typo ;)
Aha! Thanks a huge for explanation :)
>
> This is for exit_notify() which does
>
> /* mt-exec, de_thread() -> wait_for_notify_count() */
> if (tsk->signal->notify_count < 0 && !++tsk->signal->notify_count)
> wake_up_process(tsk->signal->group_exec_task);
>
> Then setup_new_exec() sets notify_count > 0 for __exit_signal() which does
>
> /* mt-exec, setup_new_exec() -> wait_for_notify_count() */
> if (sig->notify_count > 0 && !--sig->notify_count)
> wake_up_process(sig->group_exec_task);
>
> Yes this needs more comments and (with or without this patch) cleanups.
> Note that exit_notify() and __exit_signal() already (before this patch)
> use ->notify_count almost the same way, just exit_notify() assumes that
> notify_count < 0 means the !thread_group_leader() case in de_thread().
Yeah, just realized. It's been a long time since I looked into this signals
and tasks related code so to be honest don't think I would be helpful here)
Anyway while looking into patch I got wonder why
+static int wait_for_notify_count(struct task_struct *tsk)
+{
+ for (;;) {
+ return -EINTR;
+ set_current_state(TASK_KILLABLE);
+ if (!tsk->signal->notify_count)
+ break;
We have no any barrier here in fetching @notify_count? I mean updating
this value is done under locks (spin or read/write) in turn condition
test is a raw one. Not a big deal since set_current_state() and schedule()
are buffer flushers by themselves and after all not immediate update of
notify_count simply force us to yield one more schedule() call but I've
been a bit confused that we don't use some read_once here or something.
Another (more likely) that I've just said something stupid)
+ schedule();
}
+ __set_current_state(TASK_RUNNING);
+ return 0;
+}
Cyrill
^ permalink raw reply
* Re: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
From: Paul Moore @ 2025-11-11 0:12 UTC (permalink / raw)
To: ryan foster; +Cc: kunit-dev, serge, linux-kernel, linux-security-module
In-Reply-To: <CAHC9VhQw2yXuwhVSZERXiOREXzuO7aU=LTUiL1go6v5m5sx3Mg@mail.gmail.com>
This time with the proper LSM list address.
On Mon, Nov 10, 2025 at 6:17 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sun, Nov 9, 2025 at 8:13 PM ryan foster <foster.ryan.r@gmail.com> wrote:
> >
> > Please review this patch that adds KUnit tests for
> > rootid_owns_currentns() function in security/commoncap.c
>
> Hi Ryan,
>
> In case you haven't already seen this, it would be a good idea to read
> the doc below on how to submit patches to the upstream Linux kernel
> lists. For example, sending patches as attachment is frowned upon as
> it is difficult to properly review patches that way.
>
> * https://docs.kernel.org/process/submitting-patches.html
>
> If you are having difficulty configuring git to send email, you may
> want to look at the "b4" tool, doc link below. One of its primary
> goals is to make it easier for people to post kernel patches for
> review.
>
> https://b4.docs.kernel.org/en/latest
>
> Good luck!
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
From: Serge E. Hallyn @ 2025-11-11 0:33 UTC (permalink / raw)
To: Ryan Foster; +Cc: selinux, linux-security-module, linux-kernel, serge, paul
In-Reply-To: <20251110143748.4144288-1-foster.ryan.r@gmail.com>
On Mon, Nov 10, 2025 at 06:37:48AM -0800, Ryan Foster wrote:
> The rootid_owns_currentns() function in security/commoncap.c is a
> security-critical function used to determine if a root ID from a
> parent namespace owns the current namespace. This function is used
> in multiple security-critical paths:
>
> 1. cap_inode_getsecurity() - file capability retrieval
> 2. get_vfs_caps_from_disk() - file capability loading during exec
>
> Despite its security-critical nature, this function had no test
> coverage. This patch adds KUnit tests to verify the function's
> behavior in various scenarios:
>
> - Root ID in init namespace (positive case)
> - Invalid vfsuid (negative case)
> - Non-root UID (negative case)
I'd be more excited about this if it tested the actual
functionality.
The rootid_owns_currentns() could be split so that it calls
a (static if not testing) inline rootid_owns_userns(), and
then you test the latter. Then create a few user namespaces
with different values for the namespace's uid 0, and make
sure rootid_owns_userns(testns, testuid) behaves correctly.
Actually, this function should probably be renamed.
"rootid_owns_currentns()" is not correct. It should just be
"uid_owns_currentns()".
> The function is made testable by exporting it when
> CONFIG_SECURITY_COMMONCAP_KUNIT_TEST is enabled, while maintaining
> static visibility in production builds.
>
> This follows the pattern established by other security subsystems
> (IPE, Landlock) for KUnit testing.
>
> Signed-off-by: Ryan Foster <foster.ryan.r@gmail.com>
> ---
> security/Kconfig | 17 ++++++
> security/Makefile | 1 +
> security/commoncap.c | 5 ++
> security/commoncap_test.c | 109 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 132 insertions(+)
> create mode 100644 security/commoncap_test.c
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 285f284dfcac..c7b3f42ef875 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -284,6 +284,23 @@ config LSM
>
> If unsure, leave this as the default.
>
> +config SECURITY_COMMONCAP_KUNIT_TEST
> + bool "Build KUnit tests for commoncap" if !KUNIT_ALL_TESTS
> + depends on KUNIT=y
> + default KUNIT_ALL_TESTS
> + help
> + This builds the commoncap KUnit tests.
> +
> + KUnit tests run during boot and output the results to the debug log
> + in TAP format (https://testanything.org/). Only useful for kernel devs
> + running KUnit test harness and are not for inclusion into a
> + production build.
> +
> + For more information on KUnit and unit tests in general please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> source "security/Kconfig.hardening"
>
> endmenu
> diff --git a/security/Makefile b/security/Makefile
> index 22ff4c8bd8ce..5b1285fde552 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_KEYS) += keys/
>
> # always enable default capabilities
> obj-y += commoncap.o
> +obj-$(CONFIG_SECURITY_COMMONCAP_KUNIT_TEST) += commoncap_test.o
> obj-$(CONFIG_SECURITY) += lsm_syscalls.o
> obj-$(CONFIG_MMU) += min_addr.o
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 6bd4adeb4795..4d0e014ce7cd 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -358,7 +358,12 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry)
> return error;
> }
>
> +#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
> +bool rootid_owns_currentns(vfsuid_t rootvfsuid);
> +bool rootid_owns_currentns(vfsuid_t rootvfsuid)
> +#else
> static bool rootid_owns_currentns(vfsuid_t rootvfsuid)
> +#endif
> {
> struct user_namespace *ns;
> kuid_t kroot;
> diff --git a/security/commoncap_test.c b/security/commoncap_test.c
> new file mode 100644
> index 000000000000..9757d433d314
> --- /dev/null
> +++ b/security/commoncap_test.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * KUnit tests for commoncap.c security functions
> + *
> + * Tests for security-critical functions in the capability subsystem,
> + * particularly namespace-related capability checks.
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/user_namespace.h>
> +#include <linux/uidgid.h>
> +#include <linux/module.h>
> +
> +/* We need to include the actual vfsuid_t definition but avoid the problematic
> + * inline functions in mnt_idmapping.h. Include just the type definitions.
> + */
> +#include <linux/types.h>
> +
> +/* Forward declare the minimal types we need - match the actual kernel definitions */
> +struct mnt_idmap;
> +typedef struct {
> + uid_t val;
> +} vfsuid_t;
> +
> +/* Minimal macros we need - match kernel definitions from mnt_idmapping.h */
> +static inline uid_t __vfsuid_val(vfsuid_t uid)
> +{
> + return uid.val;
> +}
> +
> +#define VFSUIDT_INIT(val) ((vfsuid_t){ __kuid_val(val) })
> +#define INVALID_VFSUID VFSUIDT_INIT(INVALID_UID)
> +
> +#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
> +
> +/* Forward declaration - function is exported when KUNIT_TEST is enabled */
> +extern bool rootid_owns_currentns(vfsuid_t rootvfsuid);
> +
> +/**
> + * test_rootid_owns_currentns_init_ns - Test rootid_owns_currentns with init ns
> + *
> + * Verifies that a root ID in the init namespace correctly owns the current
> + * namespace when running in init_user_ns.
> + */
> +static void test_rootid_owns_currentns_init_ns(struct kunit *test)
> +{
> + vfsuid_t root_vfsuid;
> + kuid_t root_kuid;
> +
> + /* Create a root UID in init namespace */
> + root_kuid = KUIDT_INIT(0);
> + root_vfsuid = VFSUIDT_INIT(root_kuid);
> +
> + /* In init namespace, root should own current namespace */
> + KUNIT_EXPECT_TRUE(test, rootid_owns_currentns(root_vfsuid));
> +}
> +
> +/**
> + * test_rootid_owns_currentns_invalid - Test rootid_owns_currentns with invalid vfsuid
> + *
> + * Verifies that an invalid vfsuid correctly returns false.
> + */
> +static void test_rootid_owns_currentns_invalid(struct kunit *test)
> +{
> + vfsuid_t invalid_vfsuid;
> +
> + /* Use the predefined invalid vfsuid */
> + invalid_vfsuid = INVALID_VFSUID;
> +
> + /* Invalid vfsuid should return false */
> + KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(invalid_vfsuid));
> +}
> +
> +/**
> + * test_rootid_owns_currentns_nonroot - Test rootid_owns_currentns with non-root UID
> + *
> + * Verifies that a non-root UID correctly returns false.
> + */
> +static void test_rootid_owns_currentns_nonroot(struct kunit *test)
> +{
> + vfsuid_t nonroot_vfsuid;
> + kuid_t nonroot_kuid;
> +
> + /* Create a non-root UID */
> + nonroot_kuid = KUIDT_INIT(1000);
> + nonroot_vfsuid = VFSUIDT_INIT(nonroot_kuid);
> +
> + /* Non-root UID should return false */
> + KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(nonroot_vfsuid));
> +}
> +
> +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),
> + {}
> +};
> +
> +static struct kunit_suite commoncap_test_suite = {
> + .name = "commoncap",
> + .test_cases = commoncap_test_cases,
> +};
> +
> +kunit_test_suite(commoncap_test_suite);
> +
> +MODULE_LICENSE("GPL");
> +
> +#endif /* CONFIG_SECURITY_COMMONCAP_KUNIT_TEST */
> +
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH] security: sctp: Format type and permission checks tables
From: Bagas Sanjaya @ 2025-11-11 2:34 UTC (permalink / raw)
To: Linux Kernel Mailing List, Linux Documentation,
Linux Security Module
Cc: Jonathan Corbet, Jarkko Sakkinen, Christian Brauner, Jeff Layton,
Kees Cook, Mickaël Salaün, Stuart Yoder, Randy Dunlap
In-Reply-To: <20251103113922.61232-2-bagasdotme@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 143 bytes --]
On Mon, Nov 03, 2025 at 06:39:23PM +0700, Bagas Sanjaya wrote:
> Use reST grid tables for both type and permission checks tables.
review ping
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
From: kernel test robot @ 2025-11-11 6:52 UTC (permalink / raw)
To: Ryan Foster, selinux
Cc: oe-kbuild-all, linux-security-module, linux-kernel, serge, paul,
Ryan Foster
In-Reply-To: <20251110143748.4144288-1-foster.ryan.r@gmail.com>
Hi Ryan,
kernel test robot noticed the following build errors:
[auto build test ERROR on pcmoore-selinux/next]
[also build test ERROR on linus/master v6.18-rc5 next-20251110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ryan-Foster/security-Add-KUnit-tests-for-rootid_owns_currentns/20251110-223824
base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link: https://lore.kernel.org/r/20251110143748.4144288-1-foster.ryan.r%40gmail.com
patch subject: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
config: mips-randconfig-r053-20251111 (https://download.01.org/0day-ci/archive/20251111/202511111459.1ToyUP5p-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251111/202511111459.1ToyUP5p-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511111459.1ToyUP5p-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
>> security/commoncap_test.c:23:3: error: conflicting types for 'vfsuid_t'
} vfsuid_t;
^~~~~~~~
In file included from include/linux/fs.h:45,
from arch/mips/include/asm/elf.h:12,
from include/linux/elf.h:6,
from include/linux/module.h:20,
from include/kunit/test.h:24,
from security/commoncap_test.c:9:
include/linux/mnt_idmapping.h:17:3: note: previous declaration of 'vfsuid_t' was here
} vfsuid_t;
^~~~~~~~
>> security/commoncap_test.c:26:21: error: conflicting types for '__vfsuid_val'
static inline uid_t __vfsuid_val(vfsuid_t uid)
^~~~~~~~~~~~
In file included from include/linux/fs.h:45,
from arch/mips/include/asm/elf.h:12,
from include/linux/elf.h:6,
from include/linux/module.h:20,
from include/kunit/test.h:24,
from security/commoncap_test.c:9:
include/linux/mnt_idmapping.h:34:21: note: previous definition of '__vfsuid_val' was here
static inline uid_t __vfsuid_val(vfsuid_t uid)
^~~~~~~~~~~~
>> security/commoncap_test.c:31: warning: "VFSUIDT_INIT" redefined
#define VFSUIDT_INIT(val) ((vfsuid_t){ __kuid_val(val) })
In file included from include/linux/fs.h:45,
from arch/mips/include/asm/elf.h:12,
from include/linux/elf.h:6,
from include/linux/module.h:20,
from include/kunit/test.h:24,
from security/commoncap_test.c:9:
include/linux/mnt_idmapping.h:109: note: this is the location of the previous definition
#define VFSUIDT_INIT(val) (vfsuid_t){ __kuid_val(val) }
vim +/vfsuid_t +23 security/commoncap_test.c
18
19 /* Forward declare the minimal types we need - match the actual kernel definitions */
20 struct mnt_idmap;
21 typedef struct {
22 uid_t val;
> 23 } vfsuid_t;
24
25 /* Minimal macros we need - match kernel definitions from mnt_idmapping.h */
> 26 static inline uid_t __vfsuid_val(vfsuid_t uid)
27 {
28 return uid.val;
29 }
30
> 31 #define VFSUIDT_INIT(val) ((vfsuid_t){ __kuid_val(val) })
32 #define INVALID_VFSUID VFSUIDT_INIT(INVALID_UID)
33
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
From: kernel test robot @ 2025-11-11 7:03 UTC (permalink / raw)
To: Ryan Foster, selinux
Cc: oe-kbuild-all, linux-security-module, linux-kernel, serge, paul,
Ryan Foster
In-Reply-To: <20251110143748.4144288-1-foster.ryan.r@gmail.com>
Hi Ryan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on linus/master v6.18-rc5 next-20251110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ryan-Foster/security-Add-KUnit-tests-for-rootid_owns_currentns/20251110-223824
base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link: https://lore.kernel.org/r/20251110143748.4144288-1-foster.ryan.r%40gmail.com
patch subject: [PATCH] security: Add KUnit tests for rootid_owns_currentns()
config: x86_64-rhel-9.4-kunit (https://download.01.org/0day-ci/archive/20251111/202511111453.e1clsyyc-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251111/202511111453.e1clsyyc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511111453.e1clsyyc-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Warning: security/commoncap_test.c:45 function parameter 'test' not described in 'test_rootid_owns_currentns_init_ns'
>> Warning: security/commoncap_test.c:63 function parameter 'test' not described in 'test_rootid_owns_currentns_invalid'
>> Warning: security/commoncap_test.c:79 function parameter 'test' not described in 'test_rootid_owns_currentns_nonroot'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [linux-next:master] [ns] 3a18f80918: WARNING:at_include/linux/ns_common.h:#put_cred_rcu
From: kernel test robot @ 2025-11-11 7:08 UTC (permalink / raw)
To: Christian Brauner
Cc: oe-lkp, lkp, linux-fsdevel, linux-kernel, linux-security-module,
oliver.sang
Hello,
kernel test robot noticed "WARNING:at_include/linux/ns_common.h:#put_cred_rcu" on:
commit: 3a18f809184bc5a1cfad7cde5b8b026e2ff61587 ("ns: add active reference count")
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
in testcase: trinity
version:
with following parameters:
runtime: 300s
group: group-01
nr_groups: 5
config: x86_64-randconfig-r053-20251109
compiler: gcc-14
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 32G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202511111547.de480df9-lkp@intel.com
[ 41.172047][ C1] ------------[ cut here ]------------
[ 41.172821][ C1] WARNING: CPU: 1 PID: 0 at include/linux/ns_common.h:227 put_cred_rcu (include/linux/ns_common.h:227 include/linux/user_namespace.h:189 kernel/cred.c:88)
[ 41.173907][ C1] Modules linked in: serio_raw(F) floppy(F) tiny_power_button(F) button(F)
[ 41.174959][ C1] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: GF T 6.18.0-rc2-00014-g3a18f809184b #1 PREEMPTLAZY 9f2dc8152166a7dcc87d7d6a6b2b12a17475cded
[ 41.176815][ C1] Tainted: [F]=FORCED_MODULE, [T]=RANDSTRUCT
[ 41.177517][ C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 41.178764][ C1] RIP: 0010:put_cred_rcu (include/linux/ns_common.h:227 include/linux/user_namespace.h:189 kernel/cred.c:88)
[ 41.179419][ C1] Code: 02 48 89 e8 83 e0 07 83 c0 03 38 d0 7c 0c 84 d2 74 08 48 89 ef e8 0d 9f 55 00 8b 83 30 03 00 00 85 c0 74 09 e8 ae 24 1c 00 90 <0f> 0b 90 e8 a5 24 1c 00 48 89 df e8 fd a9 1b 00 e8 98 24 1c 00 4c
All code
========
0: 02 48 89 add -0x77(%rax),%cl
3: e8 83 e0 07 83 call 0xffffffff8307e08b
8: c0 03 38 rolb $0x38,(%rbx)
b: d0 7c 0c 84 sarb $1,-0x7c(%rsp,%rcx,1)
f: d2 74 08 48 shlb %cl,0x48(%rax,%rcx,1)
13: 89 ef mov %ebp,%edi
15: e8 0d 9f 55 00 call 0x559f27
1a: 8b 83 30 03 00 00 mov 0x330(%rbx),%eax
20: 85 c0 test %eax,%eax
22: 74 09 je 0x2d
24: e8 ae 24 1c 00 call 0x1c24d7
29: 90 nop
2a:* 0f 0b ud2 <-- trapping instruction
2c: 90 nop
2d: e8 a5 24 1c 00 call 0x1c24d7
32: 48 89 df mov %rbx,%rdi
35: e8 fd a9 1b 00 call 0x1baa37
3a: e8 98 24 1c 00 call 0x1c24d7
3f: 4c rex.WR
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 90 nop
3: e8 a5 24 1c 00 call 0x1c24ad
8: 48 89 df mov %rbx,%rdi
b: e8 fd a9 1b 00 call 0x1baa0d
10: e8 98 24 1c 00 call 0x1c24ad
15: 4c rex.WR
[ 41.181507][ C1] RSP: 0018:ffffc900001c8e58 EFLAGS: 00010246
[ 41.182352][ C1] RAX: 0000000000000000 RBX: ffff8881649b8780 RCX: 0000000000000000
[ 41.183326][ C1] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 41.184311][ C1] RBP: ffff8881649b8ab0 R08: 0000000000000000 R09: 0000000000000000
[ 41.185324][ C1] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88815d490c80
[ 41.186287][ C1] R13: ffffffff83d2b7c0 R14: 0000000000000004 R15: ffffffff813e6e70
[ 41.187244][ C1] FS: 0000000000000000(0000) GS:ffff888799e76000(0000) knlGS:0000000000000000
[ 41.188285][ C1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 41.191457][ C1] CR2: 00000000004003c0 CR3: 0000000003c88000 CR4: 00000000000406b0
[ 41.192491][ C1] Call Trace:
[ 41.192939][ C1] <IRQ>
[ 41.193359][ C1] rcu_do_batch (include/linux/rcupdate.h:341 kernel/rcu/tree.c:2607)
[ 41.193952][ C1] ? rcu_pending (kernel/rcu/tree.c:2529)
[ 41.194567][ C1] ? rcu_disable_urgency_upon_qs (kernel/rcu/tree.c:725 (discriminator 1))
[ 41.195408][ C1] ? trace_irq_enable+0xac/0xe0
[ 41.196177][ C1] rcu_core (kernel/rcu/tree.c:2863)
[ 41.196782][ C1] handle_softirqs (arch/x86/include/asm/jump_label.h:36 include/trace/events/irq.h:142 kernel/softirq.c:623)
[ 41.197442][ C1] __irq_exit_rcu (kernel/softirq.c:496 kernel/softirq.c:723)
[ 41.198071][ C1] irq_exit_rcu (kernel/softirq.c:741 (discriminator 38))
[ 41.198779][ C1] sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1052 (discriminator 35) arch/x86/kernel/apic/apic.c:1052 (discriminator 35))
[ 41.199466][ C1] </IRQ>
[ 41.199885][ C1] <TASK>
[ 41.200311][ C1] asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:569)
[ 41.201027][ C1] RIP: 0010:pv_native_safe_halt (arch/x86/kernel/paravirt.c:82)
[ 41.201736][ C1] Code: 48 8b 3d 28 51 54 02 e8 23 00 00 00 48 2b 05 fc cf af 00 31 ff c3 cc cc cc cc cc cc cc cc cc eb 07 0f 00 2d 97 fc 0e 00 fb f4 <c3> cc cc cc cc 41 57 41 56 41 55 41 54 55 48 89 fd 53 44 8b 6d 00
All code
========
0: 48 8b 3d 28 51 54 02 mov 0x2545128(%rip),%rdi # 0x254512f
7: e8 23 00 00 00 call 0x2f
c: 48 2b 05 fc cf af 00 sub 0xafcffc(%rip),%rax # 0xafd00f
13: 31 ff xor %edi,%edi
15: c3 ret
16: cc int3
17: cc int3
18: cc int3
19: cc int3
1a: cc int3
1b: cc int3
1c: cc int3
1d: cc int3
1e: cc int3
1f: eb 07 jmp 0x28
21: 0f 00 2d 97 fc 0e 00 verw 0xefc97(%rip) # 0xefcbf
28: fb sti
29: f4 hlt
2a:* c3 ret <-- trapping instruction
2b: cc int3
2c: cc int3
2d: cc int3
2e: cc int3
2f: 41 57 push %r15
31: 41 56 push %r14
33: 41 55 push %r13
35: 41 54 push %r12
37: 55 push %rbp
38: 48 89 fd mov %rdi,%rbp
3b: 53 push %rbx
3c: 44 8b 6d 00 mov 0x0(%rbp),%r13d
Code starting with the faulting instruction
===========================================
0: c3 ret
1: cc int3
2: cc int3
3: cc int3
4: cc int3
5: 41 57 push %r15
7: 41 56 push %r14
9: 41 55 push %r13
b: 41 54 push %r12
d: 55 push %rbp
e: 48 89 fd mov %rdi,%rbp
11: 53 push %rbx
12: 44 8b 6d 00 mov 0x0(%rbp),%r13d
[ 41.203822][ C1] RSP: 0018:ffffc9000014fe38 EFLAGS: 00000246
[ 41.204551][ C1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[ 41.205618][ C1] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 41.206601][ C1] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 41.207613][ C1] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8881008322c0
[ 41.208650][ C1] R13: 1ffff92000029fca R14: dffffc0000000000 R15: 0000000000000000
[ 41.209689][ C1] default_idle (arch/x86/include/asm/paravirt.h:107 arch/x86/kernel/process.c:767)
[ 41.210257][ C1] default_idle_call (include/linux/cpuidle.h:143 (discriminator 1) kernel/sched/idle.c:123 (discriminator 1))
[ 41.210879][ C1] cpuidle_idle_call (kernel/sched/idle.c:191)
[ 41.211506][ C1] ? arch_cpu_idle_exit+0x30/0x30
[ 41.215390][ C1] ? tick_nohz_start_idle (kernel/time/tick-sched.c:753)
[ 41.216069][ C1] ? tsc_verify_tsc_adjust (arch/x86/kernel/tsc_sync.c:81)
[ 41.216763][ C1] do_idle (kernel/sched/idle.c:332)
[ 41.217295][ C1] cpu_startup_entry (kernel/sched/idle.c:427)
[ 41.217929][ C1] start_secondary (arch/x86/kernel/smpboot.c:315)
[ 41.218650][ C1] ? set_cpu_sibling_map (arch/x86/kernel/smpboot.c:233)
[ 41.219100][ C1] common_startup_64 (arch/x86/kernel/head_64.S:419)
[ 41.219506][ C1] </TASK>
[ 41.219783][ C1] irq event stamp: 42022
[ 41.220131][ C1] hardirqs last enabled at (42021): tick_nohz_idle_exit (arch/x86/include/asm/irqflags.h:42 arch/x86/include/asm/irqflags.h:119 kernel/time/tick-sched.c:1472)
[ 41.220844][ C1] hardirqs last disabled at (42022): __schedule (kernel/sched/core.c:6814)
[ 41.221520][ C1] softirqs last enabled at (42010): handle_softirqs (kernel/softirq.c:469 (discriminator 2) kernel/softirq.c:650 (discriminator 2))
[ 41.222380][ C1] softirqs last disabled at (42001): __irq_exit_rcu (kernel/softirq.c:496 kernel/softirq.c:723)
[ 41.223139][ C1] ---[ end trace 0000000000000000 ]---
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20251111/202511111547.de480df9-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Christian Brauner @ 2025-11-11 9:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Kees Cook,
Andy Lutomirski, Will Drewry, Andrew Morton, Michal Hocko,
Serge Hallyn, James Morris, Randy Dunlap, Suren Baghdasaryan,
Yafang Shao, Helge Deller, Eric W. Biederman, 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: <20251105143210.GA25535@redhat.com>
On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
> I am still thinking about another approach, will write another email.
> But let me take a closer look at your patch.
>
> First of all, can you split it? See below.
>
> On 08/21, Bernd Edlinger wrote:
> >
> > -static int de_thread(struct task_struct *tsk)
> > +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm)
> > {
> > struct signal_struct *sig = tsk->signal;
> > struct sighand_struct *oldsighand = tsk->sighand;
> > spinlock_t *lock = &oldsighand->siglock;
> > + struct task_struct *t;
> > + bool unsafe_execve_in_progress = false;
> >
> > if (thread_group_empty(tsk))
> > goto no_thread_group;
> > @@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk)
> > if (!thread_group_leader(tsk))
> > sig->notify_count--;
> >
> > + for_other_threads(tsk, t) {
> > + if (unlikely(t->ptrace)
> > + && (t != tsk->group_leader || !t->exit_state))
> > + unsafe_execve_in_progress = true;
>
> you can add "break" into the "if ()" block...
>
> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
> unconditionally.
>
> If you really think it makes sense, please make another patch with the
> changelog.
>
> I'd certainly prefer to avoid this boolean at least for the start. If nothing
> else to catch the potential problems earlier.
>
> > + if (unlikely(unsafe_execve_in_progress)) {
> > + spin_unlock_irq(lock);
> > + sig->exec_bprm = bprm;
> > + mutex_unlock(&sig->cred_guard_mutex);
> > + spin_lock_irq(lock);
>
> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
>
> > @@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm)
> > */
> > trace_sched_prepare_exec(current, bprm);
> >
> > + /* If the binary is not readable then enforce mm->dumpable=0 */
> > + would_dump(bprm, bprm->file);
> > + if (bprm->have_execfd)
> > + would_dump(bprm, bprm->executable);
> > +
> > + /*
> > + * Figure out dumpability. Note that this checking only of current
> > + * is wrong, but userspace depends on it. This should be testing
> > + * bprm->secureexec instead.
> > + */
> > + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
> > + is_dumpability_changed(current_cred(), bprm->cred) ||
> > + !(uid_eq(current_euid(), current_uid()) &&
> > + gid_eq(current_egid(), current_gid())))
> > + set_dumpable(bprm->mm, suid_dumpable);
> > + else
> > + set_dumpable(bprm->mm, SUID_DUMP_USER);
> > +
>
> OK, we need to do this before de_thread() drops cred_guard_mutex.
> But imo this too should be done in a separate patch, the changelog should
> explain this change.
>
> > @@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm)
> > if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
> > return -ERESTARTNOINTR;
> >
> > + if (unlikely(current->signal->exec_bprm)) {
> > + mutex_unlock(¤t->signal->cred_guard_mutex);
> > + return -ERESTARTNOINTR;
> > + }
>
> OK, if signal->exec_bprm != NULL, then current is already killed. But
> proc_pid_attr_write() and ptrace_traceme() do the same. So how about
> something like
>
> int lock_current_cgm(void)
> {
> if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
> return -ERESTARTNOINTR;
>
> if (!current->signal->group_exec_task)
> return 0;
>
> WARN_ON(!fatal_signal_pending(current));
> mutex_unlock(¤t->signal->cred_guard_mutex);
> return -ERESTARTNOINTR;
> }
>
> ?
>
> Note that it checks ->group_exec_task, not ->exec_bprm. So this change can
> come in a separate patch too, but I won't insist.
>
> > @@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request,
> > return retval;
> > }
> >
> > + if (unlikely(task == task->signal->group_exec_task)) {
> > + retval = down_write_killable(&task->signal->exec_update_lock);
> > + if (retval)
> > + return retval;
> > +
> > + scoped_guard (task_lock, task) {
> > + struct linux_binprm *bprm = task->signal->exec_bprm;
> > + const struct cred __rcu *old_cred = task->real_cred;
> > + struct mm_struct *old_mm = task->mm;
> > +
> > + rcu_assign_pointer(task->real_cred, bprm->cred);
> > + task->mm = bprm->mm;
> > + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> > + rcu_assign_pointer(task->real_cred, old_cred);
> > + task->mm = old_mm;
> > + }
>
> This is the most problematic change which I can't review...
>
> Firstly, it changes task->mm/real_cred for __ptrace_may_access() and this
> looks dangerous to me.
Yeah, that is not ok. This is effectively override_creds for real_cred
and that is not a pattern I want to see us establish at all! Temporary
credential overrides for the subjective credentials is already terrible
but at least we have the explicit split between real_cred and cred
expressely for that. So no, that's not an acceptable solution.
>
> Say, current_is_single_threaded() called by another CLONE_VM process can
> miss group_exec_task and falsely return true. Probably not that bad, in
> this case old_mm should go away soon, but still...
>
> And I don't know if this can fool the users of task_cred_xxx/__task_cred
> somehow.
>
> Or. check_unsafe_exec() sets LSM_UNSAFE_PTRACE if ptrace. Is it safe to
> ptrace the execing task after that? I have no idea what the security hooks
> can do...
>
> Again, can't review this part.
>
> Oleg.
>
^ permalink raw reply
* Re: [syzbot] [fs?] WARNING in nsproxy_ns_active_put
From: Christian Brauner @ 2025-11-11 9:24 UTC (permalink / raw)
To: syzbot
Cc: Liam.Howlett, akpm, bpf, bsegall, david, dietmar.eggemann, jack,
jsavitz, juri.lelli, kartikey406, kees, linux-fsdevel,
linux-kernel, linux-mm, linux-security-module, lorenzo.stoakes,
mgorman, mhocko, mingo, mjguzik, oleg, paul, peterz, rostedt,
rppt, sergeh, surenb, syzkaller-bugs, vbabka, vincent.guittot,
viro, vschneid
In-Reply-To: <69104fa2.a70a0220.22f260.00a5.GAE@google.com>
On Sun, Nov 09, 2025 at 12:24:02AM -0800, syzbot wrote:
> syzbot has bisected this issue to:
>
> commit 3a18f809184bc5a1cfad7cde5b8b026e2ff61587
> Author: Christian Brauner <brauner@kernel.org>
> Date: Wed Oct 29 12:20:24 2025 +0000
>
> ns: add active reference count
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11a350b4580000
> start commit: 9c0826a5d9aa Add linux-next specific files for 20251107
> git tree: linux-next
> final oops: https://syzkaller.appspot.com/x/report.txt?x=13a350b4580000
> console output: https://syzkaller.appspot.com/x/log.txt?x=15a350b4580000
> kernel config: https://syzkaller.appspot.com/x/.config?x=f2ebeee52bf052b8
> dashboard link: https://syzkaller.appspot.com/bug?extid=0b2e79f91ff6579bfa5b
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1639d084580000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1625aa92580000
>
> Reported-by: syzbot+0b2e79f91ff6579bfa5b@syzkaller.appspotmail.com
> Fixes: 3a18f809184b ("ns: add active reference count")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
#syz test: https://github.com/brauner/linux.git namespace-6.19
^ permalink raw reply
* Re: [linux-next:master] [ns] 3a18f80918: WARNING:at_include/linux/ns_common.h:#put_cred_rcu
From: Christian Brauner @ 2025-11-11 9:36 UTC (permalink / raw)
To: kernel test robot
Cc: oe-lkp, lkp, linux-fsdevel, linux-kernel, linux-security-module
In-Reply-To: <202511111547.de480df9-lkp@intel.com>
On Tue, Nov 11, 2025 at 03:08:10PM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed "WARNING:at_include/linux/ns_common.h:#put_cred_rcu" on:
>
> commit: 3a18f809184bc5a1cfad7cde5b8b026e2ff61587 ("ns: add active reference count")
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
This is fixed in namespace-6.19 which should've made it into -next about
five hours ago.
Note that "linux-next.git master" is pretty useless. By the time I read
this mail linux-next's master branch will have already been updated. So
you should give a commit hash as well or a tag.
>
> in testcase: trinity
> version:
> with following parameters:
>
> runtime: 300s
> group: group-01
> nr_groups: 5
>
>
>
> config: x86_64-randconfig-r053-20251109
> compiler: gcc-14
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 32G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202511111547.de480df9-lkp@intel.com
>
>
> [ 41.172047][ C1] ------------[ cut here ]------------
> [ 41.172821][ C1] WARNING: CPU: 1 PID: 0 at include/linux/ns_common.h:227 put_cred_rcu (include/linux/ns_common.h:227 include/linux/user_namespace.h:189 kernel/cred.c:88)
> [ 41.173907][ C1] Modules linked in: serio_raw(F) floppy(F) tiny_power_button(F) button(F)
> [ 41.174959][ C1] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: GF T 6.18.0-rc2-00014-g3a18f809184b #1 PREEMPTLAZY 9f2dc8152166a7dcc87d7d6a6b2b12a17475cded
> [ 41.176815][ C1] Tainted: [F]=FORCED_MODULE, [T]=RANDSTRUCT
> [ 41.177517][ C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [ 41.178764][ C1] RIP: 0010:put_cred_rcu (include/linux/ns_common.h:227 include/linux/user_namespace.h:189 kernel/cred.c:88)
> [ 41.179419][ C1] Code: 02 48 89 e8 83 e0 07 83 c0 03 38 d0 7c 0c 84 d2 74 08 48 89 ef e8 0d 9f 55 00 8b 83 30 03 00 00 85 c0 74 09 e8 ae 24 1c 00 90 <0f> 0b 90 e8 a5 24 1c 00 48 89 df e8 fd a9 1b 00 e8 98 24 1c 00 4c
> All code
> ========
> 0: 02 48 89 add -0x77(%rax),%cl
> 3: e8 83 e0 07 83 call 0xffffffff8307e08b
> 8: c0 03 38 rolb $0x38,(%rbx)
> b: d0 7c 0c 84 sarb $1,-0x7c(%rsp,%rcx,1)
> f: d2 74 08 48 shlb %cl,0x48(%rax,%rcx,1)
> 13: 89 ef mov %ebp,%edi
> 15: e8 0d 9f 55 00 call 0x559f27
> 1a: 8b 83 30 03 00 00 mov 0x330(%rbx),%eax
> 20: 85 c0 test %eax,%eax
> 22: 74 09 je 0x2d
> 24: e8 ae 24 1c 00 call 0x1c24d7
> 29: 90 nop
> 2a:* 0f 0b ud2 <-- trapping instruction
> 2c: 90 nop
> 2d: e8 a5 24 1c 00 call 0x1c24d7
> 32: 48 89 df mov %rbx,%rdi
> 35: e8 fd a9 1b 00 call 0x1baa37
> 3a: e8 98 24 1c 00 call 0x1c24d7
> 3f: 4c rex.WR
>
> Code starting with the faulting instruction
> ===========================================
> 0: 0f 0b ud2
> 2: 90 nop
> 3: e8 a5 24 1c 00 call 0x1c24ad
> 8: 48 89 df mov %rbx,%rdi
> b: e8 fd a9 1b 00 call 0x1baa0d
> 10: e8 98 24 1c 00 call 0x1c24ad
> 15: 4c rex.WR
> [ 41.181507][ C1] RSP: 0018:ffffc900001c8e58 EFLAGS: 00010246
> [ 41.182352][ C1] RAX: 0000000000000000 RBX: ffff8881649b8780 RCX: 0000000000000000
> [ 41.183326][ C1] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [ 41.184311][ C1] RBP: ffff8881649b8ab0 R08: 0000000000000000 R09: 0000000000000000
> [ 41.185324][ C1] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88815d490c80
> [ 41.186287][ C1] R13: ffffffff83d2b7c0 R14: 0000000000000004 R15: ffffffff813e6e70
> [ 41.187244][ C1] FS: 0000000000000000(0000) GS:ffff888799e76000(0000) knlGS:0000000000000000
> [ 41.188285][ C1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 41.191457][ C1] CR2: 00000000004003c0 CR3: 0000000003c88000 CR4: 00000000000406b0
> [ 41.192491][ C1] Call Trace:
> [ 41.192939][ C1] <IRQ>
> [ 41.193359][ C1] rcu_do_batch (include/linux/rcupdate.h:341 kernel/rcu/tree.c:2607)
> [ 41.193952][ C1] ? rcu_pending (kernel/rcu/tree.c:2529)
> [ 41.194567][ C1] ? rcu_disable_urgency_upon_qs (kernel/rcu/tree.c:725 (discriminator 1))
> [ 41.195408][ C1] ? trace_irq_enable+0xac/0xe0
> [ 41.196177][ C1] rcu_core (kernel/rcu/tree.c:2863)
> [ 41.196782][ C1] handle_softirqs (arch/x86/include/asm/jump_label.h:36 include/trace/events/irq.h:142 kernel/softirq.c:623)
> [ 41.197442][ C1] __irq_exit_rcu (kernel/softirq.c:496 kernel/softirq.c:723)
> [ 41.198071][ C1] irq_exit_rcu (kernel/softirq.c:741 (discriminator 38))
> [ 41.198779][ C1] sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1052 (discriminator 35) arch/x86/kernel/apic/apic.c:1052 (discriminator 35))
> [ 41.199466][ C1] </IRQ>
> [ 41.199885][ C1] <TASK>
> [ 41.200311][ C1] asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:569)
> [ 41.201027][ C1] RIP: 0010:pv_native_safe_halt (arch/x86/kernel/paravirt.c:82)
> [ 41.201736][ C1] Code: 48 8b 3d 28 51 54 02 e8 23 00 00 00 48 2b 05 fc cf af 00 31 ff c3 cc cc cc cc cc cc cc cc cc eb 07 0f 00 2d 97 fc 0e 00 fb f4 <c3> cc cc cc cc 41 57 41 56 41 55 41 54 55 48 89 fd 53 44 8b 6d 00
> All code
> ========
> 0: 48 8b 3d 28 51 54 02 mov 0x2545128(%rip),%rdi # 0x254512f
> 7: e8 23 00 00 00 call 0x2f
> c: 48 2b 05 fc cf af 00 sub 0xafcffc(%rip),%rax # 0xafd00f
> 13: 31 ff xor %edi,%edi
> 15: c3 ret
> 16: cc int3
> 17: cc int3
> 18: cc int3
> 19: cc int3
> 1a: cc int3
> 1b: cc int3
> 1c: cc int3
> 1d: cc int3
> 1e: cc int3
> 1f: eb 07 jmp 0x28
> 21: 0f 00 2d 97 fc 0e 00 verw 0xefc97(%rip) # 0xefcbf
> 28: fb sti
> 29: f4 hlt
> 2a:* c3 ret <-- trapping instruction
> 2b: cc int3
> 2c: cc int3
> 2d: cc int3
> 2e: cc int3
> 2f: 41 57 push %r15
> 31: 41 56 push %r14
> 33: 41 55 push %r13
> 35: 41 54 push %r12
> 37: 55 push %rbp
> 38: 48 89 fd mov %rdi,%rbp
> 3b: 53 push %rbx
> 3c: 44 8b 6d 00 mov 0x0(%rbp),%r13d
>
> Code starting with the faulting instruction
> ===========================================
> 0: c3 ret
> 1: cc int3
> 2: cc int3
> 3: cc int3
> 4: cc int3
> 5: 41 57 push %r15
> 7: 41 56 push %r14
> 9: 41 55 push %r13
> b: 41 54 push %r12
> d: 55 push %rbp
> e: 48 89 fd mov %rdi,%rbp
> 11: 53 push %rbx
> 12: 44 8b 6d 00 mov 0x0(%rbp),%r13d
> [ 41.203822][ C1] RSP: 0018:ffffc9000014fe38 EFLAGS: 00000246
> [ 41.204551][ C1] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [ 41.205618][ C1] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [ 41.206601][ C1] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> [ 41.207613][ C1] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8881008322c0
> [ 41.208650][ C1] R13: 1ffff92000029fca R14: dffffc0000000000 R15: 0000000000000000
> [ 41.209689][ C1] default_idle (arch/x86/include/asm/paravirt.h:107 arch/x86/kernel/process.c:767)
> [ 41.210257][ C1] default_idle_call (include/linux/cpuidle.h:143 (discriminator 1) kernel/sched/idle.c:123 (discriminator 1))
> [ 41.210879][ C1] cpuidle_idle_call (kernel/sched/idle.c:191)
> [ 41.211506][ C1] ? arch_cpu_idle_exit+0x30/0x30
> [ 41.215390][ C1] ? tick_nohz_start_idle (kernel/time/tick-sched.c:753)
> [ 41.216069][ C1] ? tsc_verify_tsc_adjust (arch/x86/kernel/tsc_sync.c:81)
> [ 41.216763][ C1] do_idle (kernel/sched/idle.c:332)
> [ 41.217295][ C1] cpu_startup_entry (kernel/sched/idle.c:427)
> [ 41.217929][ C1] start_secondary (arch/x86/kernel/smpboot.c:315)
> [ 41.218650][ C1] ? set_cpu_sibling_map (arch/x86/kernel/smpboot.c:233)
> [ 41.219100][ C1] common_startup_64 (arch/x86/kernel/head_64.S:419)
> [ 41.219506][ C1] </TASK>
> [ 41.219783][ C1] irq event stamp: 42022
> [ 41.220131][ C1] hardirqs last enabled at (42021): tick_nohz_idle_exit (arch/x86/include/asm/irqflags.h:42 arch/x86/include/asm/irqflags.h:119 kernel/time/tick-sched.c:1472)
> [ 41.220844][ C1] hardirqs last disabled at (42022): __schedule (kernel/sched/core.c:6814)
> [ 41.221520][ C1] softirqs last enabled at (42010): handle_softirqs (kernel/softirq.c:469 (discriminator 2) kernel/softirq.c:650 (discriminator 2))
> [ 41.222380][ C1] softirqs last disabled at (42001): __irq_exit_rcu (kernel/softirq.c:496 kernel/softirq.c:723)
> [ 41.223139][ C1] ---[ end trace 0000000000000000 ]---
>
>
> The kernel config and materials to reproduce are available at:
> https://download.01.org/0day-ci/archive/20251111/202511111547.de480df9-lkp@intel.com
>
>
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply
* Re: [syzbot] [fs?] WARNING in nsproxy_ns_active_put
From: syzbot @ 2025-11-11 9:46 UTC (permalink / raw)
To: akpm, bpf, brauner, bsegall, david, dietmar.eggemann, jack,
jsavitz, juri.lelli, kartikey406, kees, liam.howlett,
linux-fsdevel, linux-kernel, linux-mm, linux-security-module,
lorenzo.stoakes, mgorman, mhocko, mingo, mjguzik, oleg, paul,
peterz, rostedt, rppt, sergeh, surenb, syzkaller-bugs, vbabka,
vincent.guittot, viro, vschneid
In-Reply-To: <20251111-lausbub-wieweit-76ec521875b2@brauner>
Hello,
syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in __ns_ref_active_put
------------[ cut here ]------------
WARNING: CPU: 0 PID: 6489 at kernel/nscommon.c:171 __ns_ref_active_put+0x3d7/0x450 kernel/nscommon.c:171
Modules linked in:
CPU: 0 UID: 0 PID: 6489 Comm: syz.0.18 Not tainted syzkaller #0 PREEMPT(full)
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/02/2025
RIP: 0010:__ns_ref_active_put+0x3d7/0x450 kernel/nscommon.c:171
Code: 4d 8b 3e e9 1b fd ff ff e8 b6 61 32 00 90 0f 0b 90 e9 29 fd ff ff e8 a8 61 32 00 90 0f 0b 90 e9 59 fd ff ff e8 9a 61 32 00 90 <0f> 0b 90 e9 72 ff ff ff e8 8c 61 32 00 90 0f 0b 90 e9 64 ff ff ff
RSP: 0018:ffffc90003457d50 EFLAGS: 00010293
RAX: ffffffff818e5b86 RBX: 00000000ffffffff RCX: ffff88802cc69e40
RDX: 0000000000000000 RSI: 00000000ffffffff RDI: 0000000000000000
RBP: ffffc90003457e00 R08: ffff8880320be42b R09: 1ffff11006417c85
R10: dffffc0000000000 R11: ffffed1006417c86 R12: dffffc0000000000
R13: 1ffff11006417c84 R14: ffff8880320be420 R15: ffff8880320be428
FS: 00007fe11c3746c0(0000) GS:ffff888125cf3000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b2d863fff CR3: 000000007798c000 CR4: 00000000003526f0
Call Trace:
<TASK>
nsproxy_ns_active_put+0x4a/0x200 fs/nsfs.c:701
free_nsproxy+0x21/0x140 kernel/nsproxy.c:190
put_nsset kernel/nsproxy.c:341 [inline]
__do_sys_setns kernel/nsproxy.c:594 [inline]
__se_sys_setns+0x1459/0x1c60 kernel/nsproxy.c:559
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fe11b590ef7
Code: 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 34 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fe11c373fd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000134
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fe11b590ef7
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000000000c9
RBP: 00007fe11b611f91 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fe11b7e6038 R14: 00007fe11b7e5fa0 R15: 00007ffcd9b83d18
</TASK>
Tested on:
commit: 18b5c400 Merge patch series "ns: header cleanups and i..
git tree: https://github.com/brauner/linux.git namespace-6.19
console output: https://syzkaller.appspot.com/x/log.txt?x=12c08658580000
kernel config: https://syzkaller.appspot.com/x/.config?x=59952e73920025e4
dashboard link: https://syzkaller.appspot.com/bug?extid=0b2e79f91ff6579bfa5b
compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
Note: no patches were applied.
^ permalink raw reply
* Re: [Patch V1] ima: avoid duplicate policy rules insertions
From: Roberto Sassu @ 2025-11-11 9:46 UTC (permalink / raw)
To: Tahera Fahimi, Anirudh Venkataramanan, zohar, roberto.sassu,
dmitry.kasatkin, eric.snowberg, paul, jmorris, serge,
linux-integrity, linux-security-module, linux-kernel, code
In-Reply-To: <14c61ba5-437f-496d-8356-5712ddb37d47@linux.microsoft.com>
On Mon, 2025-11-10 at 11:06 -0800, Tahera Fahimi wrote:
> On 11/6/2025 12:32 PM, Anirudh Venkataramanan wrote:
> > On 11/6/2025 10:14 AM, Tahera Fahimi wrote:
> > > Prevent redundant IMA policy rules by checking for duplicates before insertion. This ensures that
> > > rules are not re-added when userspace is restarted (using systemd-soft-reboot) without a full system
> > > reboot. ima_rule_exists() detects duplicates in both temporary and active rule lists.
> >
> > I have run into this too. Thanks for proposing a patch!
> >
> > FWIW - I am fairly new to the IMA subsystem, so feedback below is mostly structural, with some IMA specific comments.
> Hi Ahirudh, Thanks for your feedback.
> > >
> > > Signed-off-by: Tahera Fahimi <taherafahimi@linux.microsoft.com>
> > > ---
> > > security/integrity/ima/ima_policy.c | 157 +++++++++++++++++++++++++++-
> > > 1 file changed, 156 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 164d62832f8ec..3dd902101dbda 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -1953,6 +1953,153 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > > return result;
> > > }
> > > +static bool template_has_field(const char *field_id, const struct ima_template_desc *template2)
> > > +{
> > > + int j;
> >
> > j is declared in the loop header below too, which is more correct because it keeps the scope of j to be within the loop. So I'd say get rid of the above declaration.
> The declaration of j is at the beginning to adhere proper kernel style and ancient compile support.
> > > +
> > > + for (int j = 0; j < template2->num_fields; j++)
> > > + if (strcmp(field_id, template2->fields[j]->field_id) == 0)
> > > + return true;
> > I believe the preferred kernel style is to use if (!strcmp(...)).
> >
> > > +
> > > + return false;
> > > +}
> > > +
> > > +static bool keyring_has_item(const char *item, const struct ima_rule_opt_list *keyrings)
> > > +{
> > > + int j;
> > > +
> > > + for (j = 0; j < keyrings->count; j++) {
> > > + if (strcmp(item, keyrings->items[j]) == 0)
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +static bool labels_has_item(const char *item, const struct ima_rule_opt_list *labels)
> > > +{
> > > + int j;
> > > +
> > > + for (j = 0; j < labels->count; j++) {
> > > + if (strcmp(item, labels->items[j]) == 0)
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +static bool ima_rules_equal(const struct ima_rule_entry *rule1, const struct ima_rule_entry *rule2)
> > > +{
> > > + int i;
> >
> > i is used further down in this function, and even in all those cases, the scope of i can be limited to the loop body where it's used.
> >
> > If you didn't know this already - you can use cppcheck to identify and reduce the scope of variables.
> >
> > > +
> > > + if (rule1->flags != rule2->flags)
> > > + return false;
> > > +
> > > + if (rule1->action != rule2->action)
> > > + return false;
> > > +
> > > + if (((rule1->flags & IMA_FUNC) && rule1->func != rule2->func) ||
> > > + ((rule1->flags & (IMA_MASK | IMA_INMASK)) && rule1->mask != rule2->mask) ||
> > > + ((rule1->flags & IMA_FSMAGIC) && rule1->fsmagic != rule2->fsmagic) ||
> > > + ((rule1->flags & IMA_FSUUID) && !uuid_equal(&rule1->fsuuid, &rule2->fsuuid)) ||
> > > + ((rule1->flags & IMA_UID) && !uid_eq(rule1->uid, rule2->uid)) ||
> > > + ((rule1->flags & IMA_GID) && !gid_eq(rule1->gid, rule2->gid)) ||
> > > + ((rule1->flags & IMA_FOWNER) && !uid_eq(rule1->fowner, rule2->fowner)) ||
> > > + ((rule1->flags & IMA_FGROUP) && !gid_eq(rule1->fgroup, rule2->fgroup)) ||
> > > + ((rule1->flags & IMA_FSNAME) && (strcmp(rule1->fsname, rule2->fsname) != 0)) ||
> > > + ((rule1->flags & IMA_PCR) && rule1->pcr != rule2->pcr) ||
> > > + ((rule1->flags & IMA_VALIDATE_ALGOS) &&
> > > + rule1->allowed_algos != rule2->allowed_algos) ||
> > > + ((rule1->flags & IMA_EUID) && !uid_eq(rule1->uid, rule2->uid)) ||
> > > + ((rule1->flags & IMA_EGID) && !gid_eq(rule1->gid, rule2->gid)))
> > > + return false;
> >
> > So the goal is to prevent the exact same policy rule from being added, not to update an existing rule, correct? IOW, you could end up with two very similar rules, because the new rule has one thing that's different compared to the existing rule?
>
> The purpose of this patch is to prohibit two exact same rule.
Why would an administrator attempt to load the same rule twice?
How likely is the case where one would like to combine multiple signed
IMA policies with common rules?
Unless there is a realistic use case, it would be better to patch user
space first.
Thanks
Roberto
> We can have other approaches like merging the new rule to the previously existing rule, ignore
> new rule if a similar rule exists. However, this approaches would add more complexity to the code
> and are not the purpose of this patch.
>
> > I feel that a little bit of commentary around what makes two rules the same would be useful.
> >
> > > +
> > > + if (!rule1->template && !rule2->template) {
> > > + ;
> > You're trying to do nothing and continue on. A goto statement would communicate intent better. There are other places below with the same noop structure.
> >
> > To be fair, I also don't completely understand what you're trying to achieve here, Regardless, this "do nothing inside a conditional" looks weird and I feel like there should be a way to structure your logic without resorting to this.
> >
> > > + } else if (!rule1->template || !rule2->template) {
> > > + return false;
> > > + } else if (rule1->template->num_fields != rule2->template->num_fields) {
> > > + return false;
> > > + } else if (rule1->template->num_fields != 0) {
> > > + for (i = 0; i < rule1->template->num_fields; i++) {
> > > + if (!template_has_field(rule1->template->fields[i]->field_id,
> > > + rule2->template))
> > > + return false;
> > > + }
> > > + }
> >
> > if + return will achieve the same end goals as else if + return, with lesser clutter. I have seen some static analyzers flag this pattern, but I can't remember which one at the moment.
> >
> > So something like this:
> >
> > if (!rule1->template && !rule2->template)
> > goto some_target;
> >
> > if (!rule1->template || !rule2->template)
> > return false;
> >
> > if (rule1->template->num_fields != rule2->template->num_fields)
> > return false;
> >
> > if (rule1->template->num_fields != 0) {
> > for (i = 0; i < rule1->template->num_fields; i++) {
> > if (!template_has_field(rule1->template->fields[i]->field_id,
> > rule2->template))
> > return false;
> > }
> > }> some_target:
> > ...
> > ...
> I don't think having two goto in the code will improve its readability.
>
> > > +
> > > + if (rule1->flags & IMA_KEYRINGS) {
> > > + if (!rule1->keyrings && !rule2->keyrings) {
> > > + ;
> >
> > Another if block no-op
> >
> > > + } else if (!rule1->keyrings || !rule2->keyrings) {
> > > + return false;
> > > + } else if (rule1->keyrings->count != rule2->keyrings->count) {
> > > + return false;
> > > + } else if (rule1->keyrings->count != 0) {
> >
> > if (rule1->keyrings->count)
> >
> > > + for (i = 0; i < rule1->keyrings->count; i++) {
> >
> > for (int i,
> >
> > > + if (!keyring_has_item(rule1->keyrings->items[i], rule2->keyrings))
> > > + return false;
> > > + }
> > > + }
> > > + }
> > > +
> > > + if (rule1->flags & IMA_LABEL) {
> > > + if (!rule1->label && !rule2->label) {
> > > + ;
> >
> > Another if block no-op
> >
> > > + } else if (!rule1->label || !rule2->label) {
> > > + return false;
> > > + } else if (rule1->label->count != rule2->label->count) {
> > > + return false;
> > > + } else if (rule1->label->count != 0) {
> > > + for (i = 0; i < rule1->label->count; i++) {
> > > + if (!labels_has_item(rule1->label->items[i], rule2->label))
> > > + return false;
> > > + }
> > > + }
> > > + }
> > > +
> > > + for (i = 0; i < MAX_LSM_RULES; i++) {
> >
> > for (int i,
> >
> > > + if (!rule1->lsm[i].rule && !rule2->lsm[i].rule)
> > > + continue;
> > > +
> > > + if (!rule1->lsm[i].rule || !rule2->lsm[i].rule)
> > > + return false;
> > > +
> > > + if (strcmp(rule1->lsm[i].args_p, rule2->lsm[i].args_p) != 0)
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +/**
> > > + * ima_rule_exists - check if a rule already exists in the policy
> > > + *
> > > + * Checking both the active policy and the temporary rules list.
> > > + */
> > > +static bool ima_rule_exists(struct ima_rule_entry *new_rule)
> > > +{
> > > + struct ima_rule_entry *entry;
> > > + struct list_head *ima_rules_tmp;
> > > +
> > > + if (!list_empty(&ima_temp_rules)) {
> > > + list_for_each_entry(entry, &ima_temp_rules, list) {
> > > + if (ima_rules_equal(entry, new_rule))
> > > + return true;
> > > + }
> > > + }
> > > +
> > > + rcu_read_lock();
> > > + ima_rules_tmp = rcu_dereference(ima_rules);
> > > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> > > + if (ima_rules_equal(entry, new_rule)) {
> > > + rcu_read_unlock();
> > > + return true;
> > > + }
> > > + }
> > > + rcu_read_unlock();
> > > +
> > > + return false;
> > > +}
> > > +
> > > /**
> > > * ima_parse_add_rule - add a rule to ima_policy_rules
> > > * @rule: ima measurement policy rule
> > > @@ -1993,7 +2140,15 @@ ssize_t ima_parse_add_rule(char *rule)
> > > return result;
> > > }
> > > - list_add_tail(&entry->list, &ima_temp_rules);
> > > + if (!ima_rule_exists(entry)) {
> > > + list_add_tail(&entry->list, &ima_temp_rules);
> > > + } else {
> > > + result = -EEXIST;
> > Is it necessary to set result? Or can you just pass -EEXIST to the audit call below?
> >
> > > + ima_free_rule(entry);
> > > + integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
> > > + NULL, op, "duplicate-policy", result,
> > > + audit_info);
> > > + }
> > > return len;
> > > }
> I
^ permalink raw reply
* Re: [syzbot] [fs?] WARNING in nsproxy_ns_active_put
From: Christian Brauner @ 2025-11-11 10:26 UTC (permalink / raw)
To: syzbot
Cc: akpm, bpf, bsegall, david, dietmar.eggemann, jack, jsavitz,
juri.lelli, kartikey406, kees, liam.howlett, linux-fsdevel,
linux-kernel, linux-mm, linux-security-module, lorenzo.stoakes,
mgorman, mhocko, mingo, mjguzik, oleg, paul, peterz, rostedt,
rppt, sergeh, surenb, syzkaller-bugs, vbabka, vincent.guittot,
viro, vschneid
In-Reply-To: <691305db.a70a0220.22f260.0130.GAE@google.com>
On Tue, Nov 11, 2025 at 01:46:03AM -0800, syzbot wrote:
> Hello,
>
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in __ns_ref_active_put
#syz test: https://github.com/brauner/linux.git namespace-6.19.fixes
^ permalink raw reply
* Re: [syzbot] [fs?] WARNING in nsproxy_ns_active_put
From: syzbot @ 2025-11-11 11:02 UTC (permalink / raw)
To: akpm, bpf, brauner, bsegall, david, dietmar.eggemann, jack,
jsavitz, juri.lelli, kartikey406, kees, liam.howlett,
linux-fsdevel, linux-kernel, linux-mm, linux-security-module,
lorenzo.stoakes, mgorman, mhocko, mingo, mjguzik, oleg, paul,
peterz, rostedt, rppt, sergeh, surenb, syzkaller-bugs, vbabka,
vincent.guittot, viro, vschneid
In-Reply-To: <20251111-anbraten-suggerieren-da8ca707af2c@brauner>
Hello,
syzbot tried to test the proposed patch but the build/boot failed:
SYZFAIL: failed to recv rpc
SYZFAIL: failed to recv rpc
Warning: Permanently added '10.128.1.29' (ED25519) to the list of known hosts.
2025/11/11 11:01:12 parsed 1 programs
[ 92.366829][ T894] cfg80211: failed to load regulatory.db
[ 94.101317][ T5831] cgroup: Unknown subsys name 'net'
[ 94.208868][ T5831] cgroup: Unknown subsys name 'cpuset'
[ 94.218695][ T5831] cgroup: Unknown subsys name 'rlimit'
Setting up swapspace version 1, size = 127995904 bytes
[ 95.913996][ T5831] Adding 124996k swap on ./swap-file. Priority:0 extents:1 across:124996k
[ 99.210494][ T5845] soft_limit_in_bytes is deprecated and will be removed. Please report your usecase to linux-mm@kvack.org if you depend on this functionality.
[ 99.368014][ T52] Bluetooth: hci0: unexpected cc 0x0c03 length: 249 > 1
[ 99.376801][ T52] Bluetooth: hci0: unexpected cc 0x1003 length: 249 > 9
[ 99.385991][ T52] Bluetooth: hci0: unexpected cc 0x1001 length: 249 > 9
[ 99.394090][ T52] Bluetooth: hci0: unexpected cc 0x0c23 length: 249 > 4
[ 99.403295][ T52] Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2
[ 99.760048][ T67] wlan0: Created IBSS using preconfigured BSSID 50:50:50:50:50:50
[ 99.769465][ T67] wlan0: Creating new IBSS network, BSSID 50:50:50:50:50:50
[ 99.812956][ T13] wlan1: Created IBSS using preconfigured BSSID 50:50:50:50:50:50
[ 99.820978][ T13] wlan1: Creating new IBSS network, BSSID 50:50:50:50:50:50
[ 101.094305][ T5880] chnl_net:caif_netlink_parms(): no params data found
[ 101.244059][ T5880] bridge0: port 1(bridge_slave_0) entered blocking state
[ 101.252746][ T5880] bridge0: port 1(bridge_slave_0) entered disabled state
[ 101.261818][ T5880] bridge_slave_0: entered allmulticast mode
[ 101.270392][ T5880] bridge_slave_0: entered promiscuous mode
[ 101.283473][ T5880] bridge0: port 2(bridge_slave_1) entered blocking state
[ 101.291198][ T5880] bridge0: port 2(bridge_slave_1) entered disabled state
[ 101.298667][ T5880] bridge_slave_1: entered allmulticast mode
[ 101.307108][ T5880] bridge_slave_1: entered promiscuous mode
[ 101.360560][ T5880] bond0: (slave bond_slave_0): Enslaving as an active interface with an up link
[ 101.373330][ T5880] bond0: (slave bond_slave_1): Enslaving as an active interface with an up link
[ 101.413565][ T5880] team0: Port device team_slave_0 added
[ 101.422832][ T5880] team0: Port device team_slave_1 added
[ 101.463069][ T5880] batman_adv: batadv0: Adding interface: batadv_slave_0
[ 101.470261][ T5880] batman_adv: batadv0: The MTU of interface batadv_slave_0 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1532 would solve the problem.
[ 101.497282][ T5880] batman_adv: batadv0: Not using interface batadv_slave_0 (retrying later): interface not active
[ 101.511788][ T5880] batman_adv: batadv0: Adding interface: batadv_slave_1
[ 101.518889][ T5880] batman_adv: batadv0: The MTU of interface batadv_slave_1 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1532 would solve the problem.
[ 101.545058][ T5880] batman_adv: batadv0: Not using interface batadv_slave_1 (retrying later): interface not active
[ 101.590841][ T5880] hsr_slave_0: entered promiscuous mode
[ 101.597434][ T5880] hsr_slave_1: entered promiscuous mode
[ 101.741572][ T5880] netdevsim netdevsim0 netdevsim0: renamed from eth0
[ 101.754163][ T5880] netdevsim netdevsim0 netdevsim1: renamed from eth1
[ 101.764799][ T5880] netdevsim netdevsim0 netdevsim2: renamed from eth2
[ 101.774770][ T5880] netdevsim netdevsim0 netdevsim3: renamed from eth3
[ 101.805511][ T5880] bridge0: port 2(bridge_slave_1) entered blocking state
[ 101.812788][ T5880] bridge0: port 2(bridge_slave_1) entered forwarding state
[ 101.820983][ T5880] bridge0: port 1(bridge_slave_0) entered blocking state
[ 101.828371][ T5880] bridge0: port 1(bridge_slave_0) entered forwarding state
[ 101.843110][ T13] bridge0: port 1(bridge_slave_0) entered disabled state
[ 101.851795][ T13] bridge0: port 2(bridge_slave_1) entered disabled state
[ 101.904027][ T5880] 8021q: adding VLAN 0 to HW filter on device bond0
[ 101.928006][ T5880] 8021q: adding VLAN 0 to HW filter on device team0
[ 101.942529][ T3448] bridge0: port 1(bridge_slave_0) entered blocking state
[ 101.950392][ T3448] bridge0: port 1(bridge_slave_0) entered forwarding state
[ 101.964563][ T13] bridge0: port 2(bridge_slave_1) entered blocking state
[ 101.971799][ T13] bridge0: port 2(bridge_slave_1) entered forwarding state
[ 102.152983][ T5880] 8021q: adding VLAN 0 to HW filter on device batadv0
[ 102.197805][ T5880] veth0_vlan: entered promiscuous mode
[ 102.210102][ T5880] veth1_vlan: entered promiscuous mode
[ 102.244663][ T5880] veth0_macvtap: entered promiscuous mode
[ 102.254634][ T5880] veth1_macvtap: entered promiscuous mode
[ 102.273656][ T5880] batman_adv: batadv0: Interface activated: batadv_slave_0
[ 102.289496][ T5880] batman_adv: batadv0: Interface activated: batadv_slave_1
[ 102.304731][ T67] netdevsim netdevsim0 netdevsim0: set [1, 0] type 2 family 0 port 6081 - 0
[ 102.314238][ T67] netdevsim netdevsim0 netdevsim1: set [1, 0] type 2 family 0 port 6081 - 0
[ 102.324278][ T67] netdevsim netdevsim0 netdevsim2: set [1, 0] type 2 family 0 port 6081 - 0
[ 102.334159][ T67] netdevsim netdevsim0 netdevsim3: set [1, 0] type 2 family 0 port 6081 - 0
[ 102.469673][ T67] netdevsim netdevsim0 netdevsim3 (unregistering): unset [1, 0] type 2 family 0 port 6081 - 0
[ 102.543054][ T67] netdevsim netdevsim0 netdevsim2 (unregistering): unset [1, 0] type 2 family 0 port 6081 - 0
[ 102.622429][ T67] netdevsim netdevsim0 netdevsim1 (unregistering): unset [1, 0] type 2 family 0 port 6081 - 0
[ 102.698368][ T67] netdevsim netdevsim0 netdevsim0 (unregistering): unset [1, 0] type 2 family 0 port 6081 - 0
2025/11/11 11:01:26 executed programs: 0
[ 104.788606][ T52] Bluetooth: hci0: unexpected cc 0x0c03 length: 249 > 1
[ 104.799432][ T52] Bluetooth: hci0: unexpected cc 0x1003 length: 249 > 9
[ 104.807512][ T52] Bluetooth: hci0: unexpected cc 0x1001 length: 249 > 9
[ 104.816410][ T52] Bluetooth: hci0: unexpected cc 0x0c23 length: 249 > 4
[ 104.824560][ T52] Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2
[ 104.982601][ T5940] chnl_net:caif_netlink_parms(): no params data found
[ 105.059249][ T5940] bridge0: port 1(bridge_slave_0) entered blocking state
[ 105.066542][ T5940] bridge0: port 1(bridge_slave_0) entered disabled state
[ 105.073685][ T5940] bridge_slave_0: entered allmulticast mode
[ 105.081124][ T5940] bridge_slave_0: entered promiscuous mode
[ 105.089124][ T5940] bridge0: port 2(bridge_slave_1) entered blocking state
[ 105.096583][ T5940] bridge0: port 2(bridge_slave_1) entered disabled state
[ 105.104018][ T5940] bridge_slave_1: entered allmulticast mode
[ 105.111771][ T5940] bridge_slave_1: entered promiscuous mode
[ 105.143334][ T5940] bond0: (slave bond_slave_0): Enslaving as an active interface with an up link
[ 105.155734][ T5940] bond0: (slave bond_slave_1): Enslaving as an active interface with an up link
[ 105.191407][ T5940] team0: Port device team_slave_0 added
[ 105.201031][ T5940] team0: Port device team_slave_1 added
[ 105.235802][ T5940] batman_adv: batadv0: Adding interface: batadv_slave_0
[ 105.242802][ T5940] batman_adv: batadv0: The MTU of interface batadv_slave_0 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1532 would solve the problem.
[ 105.269608][ T5940] batman_adv: batadv0: Not using interface batadv_slave_0 (retrying later): interface not active
[ 105.296300][ T5940] batman_adv: batadv0: Adding interface: batadv_slave_1
[ 105.303516][ T5940] batman_adv: batadv0: The MTU of interface batadv_slave_1 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1532 would solve the problem.
[ 105.331738][ T5940] batman_adv: batadv0: Not using interface batadv_slave_1 (retrying later): interface not active
[ 105.417552][ T5940] hsr_slave_0: entered promiscuous mode
[ 105.424204][ T5940] hsr_slave_1: entered promiscuous mode
[ 105.430828][ T5940] debugfs: 'hsr0' already exists in 'hsr'
[ 105.437317][ T5940] Cannot create hsr debugfs directory
[ 105.454873][ T67] bridge_slave_1: left allmulticast mode
[ 105.460813][ T67] bridge_slave_1: left promiscuous mode
[ 105.467853][ T67] bridge0: port 2(bridge_slave_1) entered disabled state
[ 105.479304][ T67] bridge_slave_0: left allmulticast mode
[ 105.485065][ T67] bridge_slave_0: left promiscuous mode
[ 105.491001][ T67] bridge0: port 1(bridge_slave_0) entered disabled state
[ 105.729562][ T67] bond0 (unregistering): (slave bond_slave_0): Releasing backup interface
[ 105.741503][ T67] bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
[ 105.752583][ T67] bond0 (unregistering): Released all slaves
[ 105.835828][ T67] hsr_slave_0: left promiscuous mode
[ 105.842862][ T67] hsr_slave_1: left promiscuous mode
[ 105.849433][ T67] batman_adv: batadv0: Interface deactivated: batadv_slave_0
[ 105.857469][ T67] batman_adv: batadv0: Removing interface: batadv_slave_0
[ 105.865850][ T67] batman_adv: batadv0: Interface deactivated: batadv_slave_1
[ 105.873344][ T67] batman_adv: batadv0: Removing interface: batadv_slave_1
[ 105.890846][ T67] veth1_macvtap: left promiscuous mode
[ 105.897610][ T67] veth0_macvtap: left promiscuous mode
[ 105.903553][ T67] veth1_vlan: left promiscuous mode
[ 105.910171][ T67] veth0_vlan: left promiscuous mode
[ 106.222498][ T67] team0 (unregistering): Port device team_slave_1 removed
[ 106.255035][ T67] team0 (unregistering): Port device team_slave_0 removed
[ 106.849861][ T52] Bluetooth: hci0: command tx timeout
[ 107.366951][ T5940] netdevsim netdevsim0 netdevsim0: renamed from eth0
[ 107.390747][ T5940] netdevsim netdevsim0 netdevsim1: renamed from eth1
[ 107.409101][ T5940] netdevsim netdevsim0 netdevsim2: renamed from eth2
[ 107.429220][ T5940] netdevsim netdevsim0 netdevsim3: renamed from eth3
[ 107.687917][ T5940] 8021q: adding VLAN 0 to HW filter on device bond0
[ 107.729157][ T5940] 8021q: adding VLAN 0 to HW filter on device team0
[ 107.757652][ T1309] bridge0: port 1(bridge_slave_0) entered blocking state
[ 107.764863][ T1309] bridge0: port 1(bridge_slave_0) entered forwarding state
[ 107.814393][ T1309] bridge0: port 2(bridge_slave_1) entered blocking state
[ 107.821819][ T1309] bridge0: port 2(bridge_slave_1) entered forwarding state
[ 108.188295][ T5940] 8021q: adding VLAN 0 to HW filter on device batadv0
[ 108.234481][ T5940] veth0_vlan: entered promiscuous mode
[ 108.246943][ T5940] veth1_vlan: entered promiscuous mode
[ 108.277479][ T5940] veth0_macvtap: entered promiscuous mode
[ 108.288108][ T5940] veth1_macvtap: entered promiscuous mode
[ 108.306578][ T5940] batman_adv: batadv0: Interface activated: batadv_slave_0
[ 108.321859][ T5940] batman_adv: batadv0: Interface activated: batadv_slave_1
[ 108.336901][ T1322] netdevsim netdevsim0 netdevsim0: set [1, 0] type 2 family 0 port 6081 - 0
[ 108.346834][ T1322] netdevsim netdevsim0 netdevsim1: set [1, 0] type 2 family 0 port 6081 - 0
[ 108.358941][ T1322] netdevsim netdevsim0 netdevsim2: set [1, 0] type 2 family 0 port 6081 - 0
[ 108.368475][ T1322] netdevsim netdevsim0 netdevsim3: set [1, 0] type 2 family 0 port 6081 - 0
[ 108.430497][ T1309] wlan0: Created IBSS using preconfigured BSSID 50:50:50:50:50:50
[ 108.438794][ T1309] wlan0: Creating new IBSS network, BSSID 50:50:50:50:50:50
[ 108.474331][ T67] wlan1: Created IBSS using preconfigured BSSID 50:50:50:50:50:50
[ 108.484170][ T67] wlan1: Creating new IBSS network, BSSID 50:50:50:50:50:50
SYZFAIL: failed to recv rpc
syzkaller build log:
go env (err=<nil>)
AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE='auto'
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/syzkaller/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/syzkaller/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3388558029=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/syzkaller/jobs/linux/gopath/src/github.com/google/syzkaller/go.mod'
GOMODCACHE='/syzkaller/jobs/linux/gopath/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/syzkaller/jobs/linux/gopath'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/syzkaller/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.4'
GOWORK=''
PKG_CONFIG='pkg-config'
git status (err=<nil>)
HEAD detached at 4e1406b4def
nothing to commit, working tree clean
tput: No value for $TERM and no -T specified
tput: No value for $TERM and no -T specified
Makefile:31: run command via tools/syz-env for best compatibility, see:
Makefile:32: https://github.com/google/syzkaller/blob/master/docs/contributing.md#using-syz-env
go list -f '{{.Stale}}' -ldflags="-s -w -X github.com/google/syzkaller/prog.GitRevision=4e1406b4defac0e2a9d9424c70706f79a7750cf3 -X github.com/google/syzkaller/prog.gitRevisionDate=20251106-151142" ./sys/syz-sysgen | grep -q false || go install -ldflags="-s -w -X github.com/google/syzkaller/prog.GitRevision=4e1406b4defac0e2a9d9424c70706f79a7750cf3 -X github.com/google/syzkaller/prog.gitRevisionDate=20251106-151142" ./sys/syz-sysgen
make .descriptions
tput: No value for $TERM and no -T specified
tput: No value for $TERM and no -T specified
Makefile:31: run command via tools/syz-env for best compatibility, see:
Makefile:32: https://github.com/google/syzkaller/blob/master/docs/contributing.md#using-syz-env
bin/syz-sysgen
touch .descriptions
GOOS=linux GOARCH=amd64 go build -ldflags="-s -w -X github.com/google/syzkaller/prog.GitRevision=4e1406b4defac0e2a9d9424c70706f79a7750cf3 -X github.com/google/syzkaller/prog.gitRevisionDate=20251106-151142" -o ./bin/linux_amd64/syz-execprog github.com/google/syzkaller/tools/syz-execprog
mkdir -p ./bin/linux_amd64
g++ -o ./bin/linux_amd64/syz-executor executor/executor.cc \
-m64 -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno-stringop-overflow -Wno-array-bounds -Wno-format-overflow -Wno-unused-but-set-variable -Wno-unused-command-line-argument -static-pie -std=c++17 -I. -Iexecutor/_include -DGOOS_linux=1 -DGOARCH_amd64=1 \
-DHOSTGOOS_linux=1 -DGIT_REVISION=\"4e1406b4defac0e2a9d9424c70706f79a7750cf3\"
/usr/bin/ld: /tmp/ccimHo7N.o: in function `Connection::Connect(char const*, char const*)':
executor.cc:(.text._ZN10Connection7ConnectEPKcS1_[_ZN10Connection7ConnectEPKcS1_]+0x104): warning: Using 'gethostbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
./tools/check-syzos.sh 2>/dev/null
Tested on:
commit: ae901e5e Merge patch series "ns: fixes for namespace i..
git tree: https://github.com/brauner/linux.git namespace-6.19.fixes
kernel config: https://syzkaller.appspot.com/x/.config?x=7b0bf36f88602817
dashboard link: https://syzkaller.appspot.com/bug?extid=0b2e79f91ff6579bfa5b
compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
Note: no patches were applied.
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Bernd Edlinger @ 2025-11-11 11:07 UTC (permalink / raw)
To: Christian Brauner, Oleg Nesterov
Cc: Alexander Viro, Alexey Dobriyan, Kees Cook, Andy Lutomirski,
Will Drewry, Andrew Morton, Michal Hocko, Serge Hallyn,
James Morris, Randy Dunlap, Suren Baghdasaryan, Yafang Shao,
Helge Deller, Eric W. Biederman, 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: <20251111-ankreiden-augen-eadcf9bbdfaa@brauner>
On 11/11/25 10:21, Christian Brauner wrote:
> On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
>> I am still thinking about another approach, will write another email.
>> But let me take a closer look at your patch.
>>
>> First of all, can you split it? See below.
>>
>> On 08/21, Bernd Edlinger wrote:
>>>
>>> -static int de_thread(struct task_struct *tsk)
>>> +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm)
>>> {
>>> struct signal_struct *sig = tsk->signal;
>>> struct sighand_struct *oldsighand = tsk->sighand;
>>> spinlock_t *lock = &oldsighand->siglock;
>>> + struct task_struct *t;
>>> + bool unsafe_execve_in_progress = false;
>>>
>>> if (thread_group_empty(tsk))
>>> goto no_thread_group;
>>> @@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk)
>>> if (!thread_group_leader(tsk))
>>> sig->notify_count--;
>>>
>>> + for_other_threads(tsk, t) {
>>> + if (unlikely(t->ptrace)
>>> + && (t != tsk->group_leader || !t->exit_state))
>>> + unsafe_execve_in_progress = true;
>>
>> you can add "break" into the "if ()" block...
>>
>> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
>> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
>> unconditionally.
>>
>> If you really think it makes sense, please make another patch with the
>> changelog.
>>
>> I'd certainly prefer to avoid this boolean at least for the start. If nothing
>> else to catch the potential problems earlier.
>>
>>> + if (unlikely(unsafe_execve_in_progress)) {
>>> + spin_unlock_irq(lock);
>>> + sig->exec_bprm = bprm;
>>> + mutex_unlock(&sig->cred_guard_mutex);
>>> + spin_lock_irq(lock);
>>
>> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
>>
>>> @@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm)
>>> */
>>> trace_sched_prepare_exec(current, bprm);
>>>
>>> + /* If the binary is not readable then enforce mm->dumpable=0 */
>>> + would_dump(bprm, bprm->file);
>>> + if (bprm->have_execfd)
>>> + would_dump(bprm, bprm->executable);
>>> +
>>> + /*
>>> + * Figure out dumpability. Note that this checking only of current
>>> + * is wrong, but userspace depends on it. This should be testing
>>> + * bprm->secureexec instead.
>>> + */
>>> + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
>>> + is_dumpability_changed(current_cred(), bprm->cred) ||
>>> + !(uid_eq(current_euid(), current_uid()) &&
>>> + gid_eq(current_egid(), current_gid())))
>>> + set_dumpable(bprm->mm, suid_dumpable);
>>> + else
>>> + set_dumpable(bprm->mm, SUID_DUMP_USER);
>>> +
>>
>> OK, we need to do this before de_thread() drops cred_guard_mutex.
>> But imo this too should be done in a separate patch, the changelog should
>> explain this change.
>>
>>> @@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm)
>>> if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
>>> return -ERESTARTNOINTR;
>>>
>>> + if (unlikely(current->signal->exec_bprm)) {
>>> + mutex_unlock(¤t->signal->cred_guard_mutex);
>>> + return -ERESTARTNOINTR;
>>> + }
>>
>> OK, if signal->exec_bprm != NULL, then current is already killed. But
>> proc_pid_attr_write() and ptrace_traceme() do the same. So how about
>> something like
>>
>> int lock_current_cgm(void)
>> {
>> if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex))
>> return -ERESTARTNOINTR;
>>
>> if (!current->signal->group_exec_task)
>> return 0;
>>
>> WARN_ON(!fatal_signal_pending(current));
>> mutex_unlock(¤t->signal->cred_guard_mutex);
>> return -ERESTARTNOINTR;
>> }
>>
>> ?
>>
>> Note that it checks ->group_exec_task, not ->exec_bprm. So this change can
>> come in a separate patch too, but I won't insist.
>>
>>> @@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request,
>>> return retval;
>>> }
>>>
>>> + if (unlikely(task == task->signal->group_exec_task)) {
>>> + retval = down_write_killable(&task->signal->exec_update_lock);
>>> + if (retval)
>>> + return retval;
>>> +
>>> + scoped_guard (task_lock, task) {
>>> + struct linux_binprm *bprm = task->signal->exec_bprm;
>>> + const struct cred __rcu *old_cred = task->real_cred;
>>> + struct mm_struct *old_mm = task->mm;
>>> +
>>> + rcu_assign_pointer(task->real_cred, bprm->cred);
>>> + task->mm = bprm->mm;
>>> + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
>>> + rcu_assign_pointer(task->real_cred, old_cred);
>>> + task->mm = old_mm;
>>> + }
>>
>> This is the most problematic change which I can't review...
>>
>> Firstly, it changes task->mm/real_cred for __ptrace_may_access() and this
>> looks dangerous to me.
>
> Yeah, that is not ok. This is effectively override_creds for real_cred
> and that is not a pattern I want to see us establish at all! Temporary
> credential overrides for the subjective credentials is already terrible
> but at least we have the explicit split between real_cred and cred
> expressely for that. So no, that's not an acceptable solution.
>
Well when this is absolutely not acceptable then I would have to change
all security engines to be aware of the current and the new credentials.
That may be as well be possible but would be a rather big change.
Of course that was only meant as a big exception, and somehow safe
as long as it is protected under the right mutexes: cred_guard_mutex,
exec_update_lock and task_lock at least.
>>
>> Say, current_is_single_threaded() called by another CLONE_VM process can
>> miss group_exec_task and falsely return true. Probably not that bad, in
>> this case old_mm should go away soon, but still...
>>
>> And I don't know if this can fool the users of task_cred_xxx/__task_cred
>> somehow.
>>
>> Or. check_unsafe_exec() sets LSM_UNSAFE_PTRACE if ptrace. Is it safe to
>> ptrace the execing task after that? I have no idea what the security hooks
>> can do...
>>
>> Again, can't review this part.
>>
Never mind, your review was really helpful. At the very least it pointed
out some places where better comments are needed.
Thanks
Bernd.
>> Oleg.
>>
^ permalink raw reply
* Re: [syzbot] [fs?] WARNING in nsproxy_ns_active_put
From: Christian Brauner @ 2025-11-11 11:23 UTC (permalink / raw)
To: syzbot
Cc: akpm, bpf, bsegall, david, dietmar.eggemann, jack, jsavitz,
juri.lelli, kartikey406, kees, liam.howlett, linux-fsdevel,
linux-kernel, linux-mm, linux-security-module, lorenzo.stoakes,
mgorman, mhocko, mingo, mjguzik, oleg, paul, peterz, rostedt,
rppt, sergeh, surenb, syzkaller-bugs, vbabka, vincent.guittot,
viro, vschneid
In-Reply-To: <691317ab.a70a0220.22f260.0135.GAE@google.com>
On Tue, Nov 11, 2025 at 03:02:03AM -0800, syzbot wrote:
> Hello,
>
> syzbot tried to test the proposed patch but the build/boot failed:
I think that's unrelated. Anyway, I managed to point this to the wrong
branch. I'll send another test request in a bit.
^ 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