* [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Thorsten Blum @ 2025-10-31 11:06 UTC (permalink / raw)
To: Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-security-module, linux-kernel
strcpy() is deprecated and sprintf() does not perform bounds checking
either. While the current code works correctly, strscpy() and snprintf()
are safer alternatives that follow secure coding best practices.
Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/device_cgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index dc4df7475081..a41f558f6fdd 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -273,9 +273,9 @@ static char type_to_char(short type)
static void set_majmin(char *str, unsigned m)
{
if (m == ~0)
- strcpy(str, "*");
+ strscpy(str, "*", MAJMINLEN);
else
- sprintf(str, "%u", m);
+ snprintf(str, MAJMINLEN, "%u", m);
}
static int devcgroup_seq_show(struct seq_file *m, void *v)
--
2.51.1
^ permalink raw reply related
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: David Laight @ 2025-10-31 12:59 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031110647.102728-2-thorsten.blum@linux.dev>
On Fri, 31 Oct 2025 12:06:47 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:
> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. While the current code works correctly, strscpy() and snprintf()
> are safer alternatives that follow secure coding best practices.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/device_cgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..a41f558f6fdd 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -273,9 +273,9 @@ static char type_to_char(short type)
> static void set_majmin(char *str, unsigned m)
> {
> if (m == ~0)
> - strcpy(str, "*");
> + strscpy(str, "*", MAJMINLEN);
> else
> - sprintf(str, "%u", m);
> + snprintf(str, MAJMINLEN, "%u", m);
> }
>
> static int devcgroup_seq_show(struct seq_file *m, void *v)
There is no point using sting length limits that aren't passed into the function.
In any case the code seems to be crap (why is 'security' code always bad?)
(See https://elixir.bootlin.com/linux/v6.18-rc3/source/security/device_cgroup.c#L247)
I doubt ex->major or ex->minor can ever be ~0.
So there are two sets of calls, one set passes ~0 and the other doesn't.
The output buffers are then passed into another printf().
Even if ex->major can be ~0 there are much cleaner ways of writing this code.
David
^ permalink raw reply
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Serge E. Hallyn @ 2025-10-31 13:02 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031110647.102728-2-thorsten.blum@linux.dev>
On Fri, Oct 31, 2025 at 12:06:47PM +0100, Thorsten Blum wrote:
> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. While the current code works correctly, strscpy() and snprintf()
> are safer alternatives that follow secure coding best practices.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/device_cgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..a41f558f6fdd 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -273,9 +273,9 @@ static char type_to_char(short type)
> static void set_majmin(char *str, unsigned m)
> {
> if (m == ~0)
> - strcpy(str, "*");
> + strscpy(str, "*", MAJMINLEN);
I dunno, I'm not saying I would say no to this, but it does look
a little ridiculous. If it used some macro version which computes
the length of str instead of typing in MAJMINLEN, that might be
better. But we're just as likely here to get MAJMINLEN out of
sync with the length of strscpy as anything else happening, and
all to copy over two bytes.
> else
> - sprintf(str, "%u", m);
> + snprintf(str, MAJMINLEN, "%u", m);
Here, to me it always looks suspect to use snprintf without
checking its return value, and in this case, if snprintf cuts
off the value it is in fact a potential security issue, one
which did not exist before this.
So make up your mind: is it worth doing the length check or
not? If not, then this switch is uncalled for. If so, then
you should check the return value, else one day I may be able
to whitelist a device which the admin didn't allow.
> }
>
> static int devcgroup_seq_show(struct seq_file *m, void *v)
> --
> 2.51.1
^ permalink raw reply
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Thorsten Blum @ 2025-10-31 15:23 UTC (permalink / raw)
To: David Laight
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20251031125916.3b0c8b22@pumpkin>
On 31. Oct 2025, at 13:59, David Laight wrote:
> Even if ex->major can be ~0 there are much cleaner ways of writing this code.
Thanks for pointing this out. Looking at the bigger picture makes it
clear that most of the code can actually be removed. What do you think
of this change?
Thanks,
Thorsten
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index a41f558f6fdd..cb845b1fad6b 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -244,7 +244,6 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
#define DEVCG_DENY 2
#define DEVCG_LIST 3
-#define MAJMINLEN 13
#define ACCLEN 4
static void set_access(char *acc, short access)
@@ -270,19 +269,11 @@ static char type_to_char(short type)
return 'X';
}
-static void set_majmin(char *str, unsigned m)
-{
- if (m == ~0)
- strscpy(str, "*", MAJMINLEN);
- else
- snprintf(str, MAJMINLEN, "%u", m);
-}
-
static int devcgroup_seq_show(struct seq_file *m, void *v)
{
struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
struct dev_exception_item *ex;
- char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
+ char acc[ACCLEN];
rcu_read_lock();
/*
@@ -293,17 +284,12 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
*/
if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
set_access(acc, DEVCG_ACC_MASK);
- set_majmin(maj, ~0);
- set_majmin(min, ~0);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
- maj, min, acc);
+ seq_printf(m, "%c *:* %s\n", type_to_char(DEVCG_DEV_ALL), acc);
} else {
list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
set_access(acc, ex->access);
- set_majmin(maj, ex->major);
- set_majmin(min, ex->minor);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
- maj, min, acc);
+ seq_printf(m, "%c %u:%u %s\n", type_to_char(ex->type),
+ ex->major, ex->minor, acc);
}
}
rcu_read_unlock();
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Christian Brauner @ 2025-10-31 15:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <CAHk-=wgZ9x+yxUB9sjete2s9KBiHnPm2+rcwiWNXhx-rpcKxcw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4471 bytes --]
On Fri, Oct 24, 2025 at 01:31:11PM -0700, Linus Torvalds wrote:
> [ Adding LSM people. Also Christian, because he did the cred refcount
Sorry, late to the party. I was working on other stuf. Let me see...
> cleanup with override_creds() and friends last year, and I'm
> suggesting taking that one step further ]
>
> On Fri, 24 Oct 2025 at 06:58, Jens Axboe <axboe@kernel.dk> wrote:
> >
> > Ondrej Mosnacek (1):
> > nbd: override creds to kernel when calling sock_{send,recv}msg()
>
> I've pulled this, but looking at the patch, I note that more than half
> the patch - 75% to be exact - is just boilerplate for "I need to
> allocate the kernel cred and deal with error handling there".
>
> It literally has three lines of new actual useful code (two statements
> and one local variable declaration), and then nine lines of the "setup
> dance".
>
> Which isn't wrong, but when the infrastructure boilerplate is three
> times more than the actual code, it makes me think we should maybe
> just get rid of the
>
> my_kernel_cred = prepare_kernel_cred(&init_task);
>
> pattern for this use-case, and just let people use "init_cred"
> directly for things like this.
>
> Because that's essentially what that prepare_kernel_cred() thing
> returns, except it allocates a new copy of said thing, so now you have
> error handling and you have to free it after-the-fact.
>
> And I'm not seeing that the extra error handling and freeing dance
> actually buys us anything at all.
>
> Now, some *other* users actually go on to change the creds: they want
> that prepare_kernel_cred() dance because they then actually do
> something else like using their own keyring or whatever (eg the NFS
> idmap code or some other filesystem stuff).
>
> So it's not like prepare_kernel_cred() is wrong, but in this kind of
> case where people just go "I'm a driver with hardware access, I want
> to do something with kernel privileges not user privileges", it
> actually seems counterproductive to have extra code just to complicate
> things.
>
> Now, my gut feel is that if we just let people use 'init_cred'
> directly, we should also make sure that it's always exposed as a
> 'const struct cred' , but wouldn't that be a whole lot simpler and
> more straightforward?
>
> This is *not* the only use case of that.
>
> We now have at least four use-cases of this "raw kernel cred" pattern:
> core-dumping over unix domain socket, nbd, firmware loading and SCSI
> target all do this exact thing as far as I can tell.
>
> So they all just want that bare kernel cred, and this interface then
> forces it to do extra work instead of just doing
>
> old_cred = override_creds(&init_cred);
> ...
> revert_creds(old_cred);
>
> and it ends up being extra code for allocating and freeing that copy
> of a cred that we already *had* and could just have used directly.
>
> I did just check that making 'init_cred' be const
Hm, two immediate observations before I go off and write the series.
(1) The thing is that init_cred would have to be exposed to modules via
EXPORT_SYMBOL() for this to work. It would be easier to just force
the use of init_task->cred instead.
That pointer deref won't matter in the face of the allocations and
refcounts we wipe out with this. Then we should also move init_cred
to init/init_task.c and make it static const. Nobody really needs it
currently.
(2) I think the plain override_creds() would work but we can do better.
I envision we can leverage CLASS() to completely hide any access to
init_cred and force a scope with kernel creds.
/me goess off to write that up.
Ok, so I have it and it survives the coredump socket tests. They are a
prime example for this sort of thing. Any unprivileged task needs to be
able to connect to the coredump socket when it coredumps so we override
credentials only for the path lookup. With my patchset this becomes:
if (flags & SOCK_COREDUMP) {
struct path root;
task_lock(&init_task);
get_fs_root(init_task.fs, &root);
task_unlock(&init_task);
scoped_with_kernel_creds()
err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
LOOKUP_NO_MAGICLINKS, &path);
path_put(&root);
if (err)
goto fail;
} else {
Patches appended.
[-- Attachment #2: 0000-creds-add-scoped_-with_kernel_creds.eml --]
[-- Type: message/rfc822, Size: 2435 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 0/6] creds: add {scoped_}with_kernel_creds()
Date: Fri, 31 Oct 2025 16:37:35 +0100
Message-ID: <20251031-work-creds-init_cred-v1-0-cbf0400d6e0e@kernel.org>
Don't needlessly duplicate the initial credentials just to briefly use
them. Avoid all that work and add guards that hide all this away.
Note, using init_task in the macros to get at the credentials has the
advantage that we don't actually need to export init_cred to modules
such as nbd.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (6):
cred: add {scoped_}with_kernel_creds
creds: make init_cred static
firmware: don't copy kernel creds
nbd: don't copy kernel creds
target: don't copy kernel creds
unix: don't copy creds
drivers/base/firmware_loader/main.c | 59 +++++++++++++++--------------------
drivers/block/nbd.c | 17 ++--------
drivers/target/target_core_configfs.c | 14 ++-------
include/linux/cred.h | 25 +++++++++++++++
include/linux/init_task.h | 1 -
init/init_task.c | 24 ++++++++++++++
kernel/cred.c | 24 --------------
net/unix/af_unix.c | 17 +++-------
security/keys/process_keys.c | 2 +-
9 files changed, 83 insertions(+), 100 deletions(-)
---
base-commit: d2818517e3486d11c9bd55aca3e14059e4c69886
change-id: 20251031-work-creds-init_cred-db9556a70d67
[-- Attachment #3: 0001-cred-add-scoped_-with_kernel_creds.eml --]
[-- Type: message/rfc822, Size: 2559 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 1/6] cred: add {scoped_}with_kernel_creds
Date: Fri, 31 Oct 2025 16:37:36 +0100
Message-ID: <20251031-work-creds-init_cred-v1-1-cbf0400d6e0e@kernel.org>
Add a new CLASS(with_kernel_creds) allowing code to run with kernel
creds without having to duplicate them first. Only use the CLASS() never
the plain helpers!
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/cred.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 89ae50ad2ace..8a8d6b3fbadb 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -20,6 +20,8 @@
struct cred;
struct inode;
+extern struct task_struct init_task;
+
/*
* COW Supplementary groups list
*/
@@ -180,6 +182,29 @@ static inline const struct cred *revert_creds(const struct cred *revert_cred)
return rcu_replace_pointer(current->cred, revert_cred, 1);
}
+static inline const struct cred *__assume_kernel_creds(void)
+{
+ return rcu_replace_pointer(current->cred, init_task.cred, 1);
+}
+
+static inline void __yield_kernel_creds(const struct cred *revert_cred)
+{
+ WARN_ON_ONCE(current->cred != init_task.cred);
+ rcu_replace_pointer(current->cred, revert_cred, 1);
+}
+
+DEFINE_CLASS(with_kernel_creds,
+ const struct cred *,
+ __yield_kernel_creds(_T),
+ __assume_kernel_creds(), void)
+
+#define with_kernel_creds() \
+ CLASS(with_kernel_creds, __UNIQUE_ID(cred))()
+
+#define scoped_with_kernel_creds() \
+ for (CLASS(with_kernel_creds, __UNIQUE_ID(cred))(), \
+ *__p = (void *)1; __p; __p = NULL)
+
/**
* get_cred_many - Get references on a set of credentials
* @cred: The credentials to reference
--
2.47.3
[-- Attachment #4: 0002-creds-make-init_cred-static.eml --]
[-- Type: message/rfc822, Size: 4417 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 2/6] creds: make init_cred static
Date: Fri, 31 Oct 2025 16:37:37 +0100
Message-ID: <20251031-work-creds-init_cred-v1-2-cbf0400d6e0e@kernel.org>
There's zero need to expose struct init_cred.
The very few places that need direct access can just go through
init_task and be done with it.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/init_task.h | 1 -
init/init_task.c | 24 ++++++++++++++++++++++++
kernel/cred.c | 24 ------------------------
security/keys/process_keys.c | 2 +-
4 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index bccb3f1f6262..a6cb241ea00c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -25,7 +25,6 @@
extern struct files_struct init_files;
extern struct fs_struct init_fs;
extern struct nsproxy init_nsproxy;
-extern struct cred init_cred;
#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
#define INIT_PREV_CPUTIME(x) .prev_cputime = { \
diff --git a/init/init_task.c b/init/init_task.c
index a55e2189206f..68059eac9a1e 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -62,6 +62,30 @@ unsigned long init_shadow_call_stack[SCS_SIZE / sizeof(long)] = {
};
#endif
+/*
+ * The initial credentials for the initial task
+ */
+static const struct cred init_cred = {
+ .usage = ATOMIC_INIT(4),
+ .uid = GLOBAL_ROOT_UID,
+ .gid = GLOBAL_ROOT_GID,
+ .suid = GLOBAL_ROOT_UID,
+ .sgid = GLOBAL_ROOT_GID,
+ .euid = GLOBAL_ROOT_UID,
+ .egid = GLOBAL_ROOT_GID,
+ .fsuid = GLOBAL_ROOT_UID,
+ .fsgid = GLOBAL_ROOT_GID,
+ .securebits = SECUREBITS_DEFAULT,
+ .cap_inheritable = CAP_EMPTY_SET,
+ .cap_permitted = CAP_FULL_SET,
+ .cap_effective = CAP_FULL_SET,
+ .cap_bset = CAP_FULL_SET,
+ .user = INIT_USER,
+ .user_ns = &init_user_ns,
+ .group_info = &init_groups,
+ .ucounts = &init_ucounts,
+};
+
/*
* Set up the first task table, touch at your own risk!. Base=0,
* limit=0x1fffff (=2MB)
diff --git a/kernel/cred.c b/kernel/cred.c
index dbf6b687dc5c..9ff0b349b80b 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -38,30 +38,6 @@ static struct kmem_cache *cred_jar;
/* init to 2 - one for init_task, one to ensure it is never freed */
static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
-/*
- * The initial credentials for the initial task
- */
-struct cred init_cred = {
- .usage = ATOMIC_INIT(4),
- .uid = GLOBAL_ROOT_UID,
- .gid = GLOBAL_ROOT_GID,
- .suid = GLOBAL_ROOT_UID,
- .sgid = GLOBAL_ROOT_GID,
- .euid = GLOBAL_ROOT_UID,
- .egid = GLOBAL_ROOT_GID,
- .fsuid = GLOBAL_ROOT_UID,
- .fsgid = GLOBAL_ROOT_GID,
- .securebits = SECUREBITS_DEFAULT,
- .cap_inheritable = CAP_EMPTY_SET,
- .cap_permitted = CAP_FULL_SET,
- .cap_effective = CAP_FULL_SET,
- .cap_bset = CAP_FULL_SET,
- .user = INIT_USER,
- .user_ns = &init_user_ns,
- .group_info = &init_groups,
- .ucounts = &init_ucounts,
-};
-
/*
* The RCU callback to actually dispose of a set of credentials
*/
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index b5d5333ab330..98ba8a7d3118 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -51,7 +51,7 @@ static struct key *get_user_register(struct user_namespace *user_ns)
if (!reg_keyring) {
reg_keyring = keyring_alloc(".user_reg",
user_ns->owner, INVALID_GID,
- &init_cred,
+ init_task.cred,
KEY_POS_WRITE | KEY_POS_SEARCH |
KEY_USR_VIEW | KEY_USR_READ,
0,
--
2.47.3
[-- Attachment #5: 0003-firmware-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 3963 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 3/6] firmware: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:38 +0100
Message-ID: <20251031-work-creds-init_cred-v1-3-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/base/firmware_loader/main.c | 59 ++++++++++++++++---------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 6942c62fa59d..bee3050a20d9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -829,8 +829,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
size_t offset, u32 opt_flags)
{
struct firmware *fw = NULL;
- struct cred *kern_cred = NULL;
- const struct cred *old_cred;
bool nondirect = false;
int ret;
@@ -871,45 +869,38 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
* called by a driver when serving an unrelated request from userland, we use
* the kernel credentials to read the file.
*/
- kern_cred = prepare_kernel_cred(&init_task);
- if (!kern_cred) {
- ret = -ENOMEM;
- goto out;
- }
- old_cred = override_creds(kern_cred);
+ scoped_with_kernel_creds() {
+ ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
- ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
-
- /* Only full reads can support decompression, platform, and sysfs. */
- if (!(opt_flags & FW_OPT_PARTIAL))
- nondirect = true;
+ /* Only full reads can support decompression, platform, and sysfs. */
+ if (!(opt_flags & FW_OPT_PARTIAL))
+ nondirect = true;
#ifdef CONFIG_FW_LOADER_COMPRESS_ZSTD
- if (ret == -ENOENT && nondirect)
- ret = fw_get_filesystem_firmware(device, fw->priv, ".zst",
- fw_decompress_zstd);
+ if (ret == -ENOENT && nondirect)
+ ret = fw_get_filesystem_firmware(device, fw->priv, ".zst",
+ fw_decompress_zstd);
#endif
#ifdef CONFIG_FW_LOADER_COMPRESS_XZ
- if (ret == -ENOENT && nondirect)
- ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
- fw_decompress_xz);
+ if (ret == -ENOENT && nondirect)
+ ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
+ fw_decompress_xz);
#endif
- if (ret == -ENOENT && nondirect)
- ret = firmware_fallback_platform(fw->priv);
+ if (ret == -ENOENT && nondirect)
+ ret = firmware_fallback_platform(fw->priv);
- if (ret) {
- if (!(opt_flags & FW_OPT_NO_WARN))
- dev_warn(device,
- "Direct firmware load for %s failed with error %d\n",
- name, ret);
- if (nondirect)
- ret = firmware_fallback_sysfs(fw, name, device,
- opt_flags, ret);
- } else
- ret = assign_fw(fw, device);
-
- revert_creds(old_cred);
- put_cred(kern_cred);
+ if (ret) {
+ if (!(opt_flags & FW_OPT_NO_WARN))
+ dev_warn(device,
+ "Direct firmware load for %s failed with error %d\n",
+ name, ret);
+ if (nondirect)
+ ret = firmware_fallback_sysfs(fw, name, device,
+ opt_flags, ret);
+ } else {
+ ret = assign_fw(fw, device);
+ }
+ }
out:
if (ret < 0) {
--
2.47.3
[-- Attachment #6: 0004-nbd-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 3029 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 4/6] nbd: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:39 +0100
Message-ID: <20251031-work-creds-init_cred-v1-4-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/block/nbd.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index a853c65ac65d..1f0d89e21ec8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -52,7 +52,6 @@
static DEFINE_IDR(nbd_index_idr);
static DEFINE_MUTEX(nbd_index_mutex);
static struct workqueue_struct *nbd_del_wq;
-static struct cred *nbd_cred;
static int nbd_total_devices = 0;
struct nbd_sock {
@@ -555,7 +554,6 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
int result;
struct msghdr msg = {} ;
unsigned int noreclaim_flag;
- const struct cred *old_cred;
if (unlikely(!sock)) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
@@ -564,10 +562,10 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
return -EINVAL;
}
- old_cred = override_creds(nbd_cred);
-
msg.msg_iter = *iter;
+ with_kernel_creds();
+
noreclaim_flag = memalloc_noreclaim_save();
do {
sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
@@ -590,8 +588,6 @@ static int __sock_xmit(struct nbd_device *nbd, struct socket *sock, int send,
memalloc_noreclaim_restore(noreclaim_flag);
- revert_creds(old_cred);
-
return result;
}
@@ -2683,15 +2679,7 @@ static int __init nbd_init(void)
return -ENOMEM;
}
- nbd_cred = prepare_kernel_cred(&init_task);
- if (!nbd_cred) {
- destroy_workqueue(nbd_del_wq);
- unregister_blkdev(NBD_MAJOR, "nbd");
- return -ENOMEM;
- }
-
if (genl_register_family(&nbd_genl_family)) {
- put_cred(nbd_cred);
destroy_workqueue(nbd_del_wq);
unregister_blkdev(NBD_MAJOR, "nbd");
return -EINVAL;
@@ -2746,7 +2734,6 @@ static void __exit nbd_cleanup(void)
/* Also wait for nbd_dev_remove_work() completes */
destroy_workqueue(nbd_del_wq);
- put_cred(nbd_cred);
idr_destroy(&nbd_index_idr);
unregister_blkdev(NBD_MAJOR, "nbd");
}
--
2.47.3
[-- Attachment #7: 0005-target-don-t-copy-kernel-creds.eml --]
[-- Type: message/rfc822, Size: 2249 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 5/6] target: don't copy kernel creds
Date: Fri, 31 Oct 2025 16:37:40 +0100
Message-ID: <20251031-work-creds-init_cred-v1-5-cbf0400d6e0e@kernel.org>
Get rid of all the boilerplate and tightly scope when the task runs with
kernel creds.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
drivers/target/target_core_configfs.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index b19acd662726..9e51c535ba8c 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -3670,8 +3670,6 @@ static int __init target_core_init_configfs(void)
{
struct configfs_subsystem *subsys = &target_core_fabrics;
struct t10_alua_lu_gp *lu_gp;
- struct cred *kern_cred;
- const struct cred *old_cred;
int ret;
pr_debug("TARGET_CORE[0]: Loading Generic Kernel Storage"
@@ -3748,16 +3746,8 @@ static int __init target_core_init_configfs(void)
if (ret < 0)
goto out;
- /* We use the kernel credentials to access the target directory */
- kern_cred = prepare_kernel_cred(&init_task);
- if (!kern_cred) {
- ret = -ENOMEM;
- goto out;
- }
- old_cred = override_creds(kern_cred);
- target_init_dbroot();
- revert_creds(old_cred);
- put_cred(kern_cred);
+ scoped_with_kernel_creds()
+ target_init_dbroot();
return 0;
--
2.47.3
[-- Attachment #8: 0006-unix-don-t-copy-creds.eml --]
[-- Type: message/rfc822, Size: 2244 bytes --]
From: Christian Brauner <brauner@kernel.org>
To: Christian Brauner <brauner@kernel.org>
Subject: [PATCH 6/6] unix: don't copy creds
Date: Fri, 31 Oct 2025 16:37:41 +0100
Message-ID: <20251031-work-creds-init_cred-v1-6-cbf0400d6e0e@kernel.org>
No need to copy kernel credentials.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
net/unix/af_unix.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 768098dec231..68c94f49f7b5 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1210,25 +1210,16 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
unix_mkname_bsd(sunaddr, addr_len);
if (flags & SOCK_COREDUMP) {
- const struct cred *cred;
- struct cred *kcred;
struct path root;
- kcred = prepare_kernel_cred(&init_task);
- if (!kcred) {
- err = -ENOMEM;
- goto fail;
- }
-
task_lock(&init_task);
get_fs_root(init_task.fs, &root);
task_unlock(&init_task);
- cred = override_creds(kcred);
- err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
- LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
- LOOKUP_NO_MAGICLINKS, &path);
- put_cred(revert_creds(cred));
+ scoped_with_kernel_creds()
+ err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
+ LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
+ LOOKUP_NO_MAGICLINKS, &path);
path_put(&root);
if (err)
goto fail;
--
2.47.3
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Christian Brauner @ 2025-10-31 15:53 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <20251031-zerkratzen-privileg-77a7fb326e34@brauner>
On Fri, Oct 31, 2025 at 04:43:54PM +0100, Christian Brauner wrote:
> On Fri, Oct 24, 2025 at 01:31:11PM -0700, Linus Torvalds wrote:
> > [ Adding LSM people. Also Christian, because he did the cred refcount
>
> Sorry, late to the party. I was working on other stuf. Let me see...
>
> > cleanup with override_creds() and friends last year, and I'm
> > suggesting taking that one step further ]
> >
> > On Fri, 24 Oct 2025 at 06:58, Jens Axboe <axboe@kernel.dk> wrote:
> > >
> > > Ondrej Mosnacek (1):
> > > nbd: override creds to kernel when calling sock_{send,recv}msg()
> >
> > I've pulled this, but looking at the patch, I note that more than half
> > the patch - 75% to be exact - is just boilerplate for "I need to
> > allocate the kernel cred and deal with error handling there".
> >
> > It literally has three lines of new actual useful code (two statements
> > and one local variable declaration), and then nine lines of the "setup
> > dance".
> >
> > Which isn't wrong, but when the infrastructure boilerplate is three
> > times more than the actual code, it makes me think we should maybe
> > just get rid of the
> >
> > my_kernel_cred = prepare_kernel_cred(&init_task);
> >
> > pattern for this use-case, and just let people use "init_cred"
> > directly for things like this.
> >
> > Because that's essentially what that prepare_kernel_cred() thing
> > returns, except it allocates a new copy of said thing, so now you have
> > error handling and you have to free it after-the-fact.
> >
> > And I'm not seeing that the extra error handling and freeing dance
> > actually buys us anything at all.
> >
> > Now, some *other* users actually go on to change the creds: they want
> > that prepare_kernel_cred() dance because they then actually do
> > something else like using their own keyring or whatever (eg the NFS
> > idmap code or some other filesystem stuff).
> >
> > So it's not like prepare_kernel_cred() is wrong, but in this kind of
> > case where people just go "I'm a driver with hardware access, I want
> > to do something with kernel privileges not user privileges", it
> > actually seems counterproductive to have extra code just to complicate
> > things.
> >
> > Now, my gut feel is that if we just let people use 'init_cred'
> > directly, we should also make sure that it's always exposed as a
> > 'const struct cred' , but wouldn't that be a whole lot simpler and
> > more straightforward?
> >
> > This is *not* the only use case of that.
> >
> > We now have at least four use-cases of this "raw kernel cred" pattern:
> > core-dumping over unix domain socket, nbd, firmware loading and SCSI
> > target all do this exact thing as far as I can tell.
> >
> > So they all just want that bare kernel cred, and this interface then
> > forces it to do extra work instead of just doing
> >
> > old_cred = override_creds(&init_cred);
> > ...
> > revert_creds(old_cred);
> >
> > and it ends up being extra code for allocating and freeing that copy
> > of a cred that we already *had* and could just have used directly.
> >
> > I did just check that making 'init_cred' be const
>
> Hm, two immediate observations before I go off and write the series.
>
> (1) The thing is that init_cred would have to be exposed to modules via
> EXPORT_SYMBOL() for this to work. It would be easier to just force
> the use of init_task->cred instead.
>
> That pointer deref won't matter in the face of the allocations and
> refcounts we wipe out with this. Then we should also move init_cred
> to init/init_task.c and make it static const. Nobody really needs it
> currently.
>
> (2) I think the plain override_creds() would work but we can do better.
> I envision we can leverage CLASS() to completely hide any access to
> init_cred and force a scope with kernel creds.
>
> /me goess off to write that up.
>
> Ok, so I have it and it survives the coredump socket tests. They are a
> prime example for this sort of thing. Any unprivileged task needs to be
> able to connect to the coredump socket when it coredumps so we override
> credentials only for the path lookup. With my patchset this becomes:
>
> if (flags & SOCK_COREDUMP) {
> struct path root;
>
> task_lock(&init_task);
> get_fs_root(init_task.fs, &root);
> task_unlock(&init_task);
>
> scoped_with_kernel_creds()
> err = vfs_path_lookup(root.dentry, root.mnt, sunaddr->sun_path,
> LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> LOOKUP_NO_MAGICLINKS, &path);
> path_put(&root);
> if (err)
> goto fail;
> } else {
>
> Patches appended.
> Date: Fri, 31 Oct 2025 16:37:35 +0100
> From: Christian Brauner <brauner@kernel.org>
> To: Christian Brauner <brauner@kernel.org>
> Subject: [PATCH 0/6] creds: add {scoped_}with_kernel_creds()
> Message-Id: <20251031-work-creds-init_cred-v1-0-cbf0400d6e0e@kernel.org>
Needs one diff I forgot to fold:
diff --git a/init/init_task.c b/init/init_task.c
index 68059eac9a1e..15288e62334f 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -62,6 +62,9 @@ unsigned long init_shadow_call_stack[SCS_SIZE / sizeof(long)] = {
};
#endif
+/* init to 2 - one for init_task, one to ensure it is never freed */
+static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
+
/*
* The initial credentials for the initial task
*/
diff --git a/kernel/cred.c b/kernel/cred.c
index 9ff0b349b80b..ac87ed9d43b1 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -35,9 +35,6 @@ do { \
static struct kmem_cache *cred_jar;
-/* init to 2 - one for init_task, one to ensure it is never freed */
-static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
-
/*
* The RCU callback to actually dispose of a set of credentials
*/
^ permalink raw reply related
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Linus Torvalds @ 2025-10-31 16:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <20251031-zerkratzen-privileg-77a7fb326e34@brauner>
On Fri, 31 Oct 2025 at 08:44, Christian Brauner <brauner@kernel.org> wrote:
>
> Hm, two immediate observations before I go off and write the series.
>
> (1) The thing is that init_cred would have to be exposed to modules via
> EXPORT_SYMBOL() for this to work. It would be easier to just force
> the use of init_task->cred instead.
Yea, I guess we already export that.
> That pointer deref won't matter in the face of the allocations and
> refcounts we wipe out with this. Then we should also move init_cred
> to init/init_task.c and make it static const. Nobody really needs it
> currently.
Well, I did the "does it compile ok" with it marked as 'const', but as
mentioned, those 'struct cred' instances aren't *really* const, they
are only pseudo-const things in that they are *marked* const so that
nobody modifies them by mistake, but then the ref-counting will cast
the constness away in order to update references.
So I don't think we can *actually* mark it "static const", because
that will put the data structure in the const data section, and then
the refcounting will trigger kernel page faults.
End result: I think we can indeed move it to init/init_task.c. And
yes, we can and should make it static to that file, but not plain
'const'.
If we expose it to others - but I think you're right that maybe it's
not a good idea - we should *expose* it as a 'const' data structure.
But we should probably put it in some explicitly writable section (I
was going to suggest marking it "__read_mostly", but it turns out some
architectures #define that to be empty, so a "const __read_mosyly"
data structure could still end up in a read-only section).
> (2) I think the plain override_creds() would work but we can do better.
> I envision we can leverage CLASS() to completely hide any access to
> init_cred and force a scope with kernel creds.
Ack.
Linus
^ permalink raw reply
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: David Laight @ 2025-10-31 16:54 UTC (permalink / raw)
To: Thorsten Blum
Cc: Paul Moore, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <FE3AAB5A-9AB9-49B6-BB67-FCB97CD5AF29@linux.dev>
On Fri, 31 Oct 2025 16:23:02 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:
> On 31. Oct 2025, at 13:59, David Laight wrote:
> > Even if ex->major can be ~0 there are much cleaner ways of writing this code.
>
> Thanks for pointing this out. Looking at the bigger picture makes it
> clear that most of the code can actually be removed. What do you think
> of this change?
That is sort of what I was thinking about, but it doesn't quite work.
>
> Thanks,
> Thorsten
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index a41f558f6fdd..cb845b1fad6b 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -244,7 +244,6 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
> #define DEVCG_DENY 2
> #define DEVCG_LIST 3
>
> -#define MAJMINLEN 13
> #define ACCLEN 4
>
> static void set_access(char *acc, short access)
> @@ -270,19 +269,11 @@ static char type_to_char(short type)
> return 'X';
> }
>
> -static void set_majmin(char *str, unsigned m)
> -{
> - if (m == ~0)
> - strscpy(str, "*", MAJMINLEN);
> - else
> - snprintf(str, MAJMINLEN, "%u", m);
> -}
> -
> static int devcgroup_seq_show(struct seq_file *m, void *v)
> {
> struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
> struct dev_exception_item *ex;
> - char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
> + char acc[ACCLEN];
>
> rcu_read_lock();
> /*
> @@ -293,17 +284,12 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
> */
> if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> set_access(acc, DEVCG_ACC_MASK);
> - set_majmin(maj, ~0);
> - set_majmin(min, ~0);
> - seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
> - maj, min, acc);
> + seq_printf(m, "%c *:* %s\n", type_to_char(DEVCG_DEV_ALL), acc);
type_to_char(DEVCG_DEV_ALL) is 'a' and this is the only place it happens,
also acc is "rwm".
So that could be:
seq_puts(m, "a *:* rwm\n");
> } else {
> list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
> set_access(acc, ex->access);
> - set_majmin(maj, ex->major);
> - set_majmin(min, ex->minor);
> - seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
> - maj, min, acc);
> + seq_printf(m, "%c %u:%u %s\n", type_to_char(ex->type),
> + ex->major, ex->minor, acc);
It looks like both ex->major and ex->minor can be ~0.
(I'm not sure it makes any sense to have major == ~0 and minor != ~0).
However this should be ok:
seq_putc(m, type_to_char(ex->type);
if (ex->major == ~0)
seq_puts(m, " *:");
else
seq_printf(m, " %u:", ex->major);
if (ex->minor == ~0)
seq_puts(m, "* ");
else
seq_printf(m, "%u ", ex->minor);
if (ex->access & DEVCG_ACC_READ)
seq_putc(m, 'r');
if (ex->access & DEVCG_ACC_WRITE)
seq_putc(m, 'w');
if (ex->access & DEV_ACC_MKNOD)
seq_putc(m. 'm');
seq_putc(m, '\n');
A less intrusive change would be to pass 'm' the the set_xxx() functions
and add the separators between the calls.
David
> }
> }
> rcu_read_unlock();
>
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Return early on allocation failure and drop goto
From: Serge E. Hallyn @ 2025-10-31 20:32 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <20251029163157.119000-1-thorsten.blum@linux.dev>
On Wed, Oct 29, 2025 at 05:31:56PM +0100, Thorsten Blum wrote:
> Return ERR_PTR(-ENOMEM) immediately if memory allocation fails, instead
> of using goto and returning a NULL pointer, and remove the now-unused
> 'out' label.
>
> At the call site, check 'ascii_buf' with IS_ERR() and propagate the
> error code returned by datablob_format().
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
It seems like purely personal preference, but I don't see any
error in it, so in that sense
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> security/keys/encrypted-keys/encrypted.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index be1f2118447c..25df00b7dbe9 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -276,7 +276,7 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
>
> ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
> if (!ascii_buf)
> - goto out;
> + return ERR_PTR(-ENOMEM);
>
> ascii_buf[asciiblob_len] = '\0';
>
> @@ -288,7 +288,6 @@ static char *datablob_format(struct encrypted_key_payload *epayload,
> bufp = &ascii_buf[len];
> for (i = 0; i < (asciiblob_len - len) / 2; i++)
> bufp = hex_byte_pack(bufp, iv[i]);
> -out:
> return ascii_buf;
> }
>
> @@ -932,8 +931,8 @@ static long encrypted_read(const struct key *key, char *buffer,
> goto out;
>
> ascii_buf = datablob_format(epayload, asciiblob_len);
> - if (!ascii_buf) {
> - ret = -ENOMEM;
> + if (IS_ERR(ascii_buf)) {
> + ret = PTR_ERR(ascii_buf);
> goto out;
> }
>
> --
> 2.51.0
^ permalink raw reply
* Re: [PATCH] KEYS: Remove the ad-hoc compilation flag CAAM_DEBUG
From: Serge E. Hallyn @ 2025-10-31 20:41 UTC (permalink / raw)
To: Ye Bin
Cc: a.fatoum, kernel, James.Bottomley, jarkko, zohar, dhowells, paul,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
yebin10
In-Reply-To: <20251028132254.841715-1-yebin@huaweicloud.com>
On Tue, Oct 28, 2025 at 09:22:54PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Fix the broken design based on Jarkko Sakkinen's suggestions as follows:
>
> 1. Remove the ad-hoc compilation flag (i.e., CAAM_DEBUG).
> 2. Substitute pr_info calls with pr_debug calls.
>
> Closes: https://patchwork.kernel.org/project/linux-integrity/patch/20251024061153.61470-1-yebin@huaweicloud.com/
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
(I guess this is only in linux-next right now?)
> ---
> security/keys/trusted-keys/trusted_caam.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
> index 601943ce0d60..c903ee7328ca 100644
> --- a/security/keys/trusted-keys/trusted_caam.c
> +++ b/security/keys/trusted-keys/trusted_caam.c
> @@ -28,16 +28,10 @@ static const match_table_t key_tokens = {
> {opt_err, NULL}
> };
>
> -#ifdef CAAM_DEBUG
> static inline void dump_options(const struct caam_pkey_info *pkey_info)
> {
> - pr_info("key encryption algo %d\n", pkey_info->key_enc_algo);
> + pr_debug("key encryption algo %d\n", pkey_info->key_enc_algo);
> }
> -#else
> -static inline void dump_options(const struct caam_pkey_info *pkey_info)
> -{
> -}
> -#endif
>
> static int get_pkey_options(char *c,
> struct caam_pkey_info *pkey_info)
> --
> 2.34.1
^ permalink raw reply
* [PATCH v2] device_cgroup: Refactor devcgroup_seq_show to use seq_put* helpers
From: Thorsten Blum @ 2025-10-31 21:39 UTC (permalink / raw)
To: David Laight, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-security-module, linux-kernel
Replace set_access(), set_majmin(), and type_to_char() with new helpers
seq_putaccess(), seq_puttype(), and seq_putversion() that write directly
to 'seq_file'.
Simplify devcgroup_seq_show() by hard-coding "a *:* rwm", and use the
new seq_put* helper functions to list the exceptions otherwise.
This allows us to remove the intermediate string buffers while
maintaining the same functionality, including wildcard handling.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Changes in v2:
- Add setq_put* helpers to modify seq_file directly (David)
- Update patch subject and description
- Link to v1: https://lore.kernel.org/lkml/20251031110647.102728-2-thorsten.blum@linux.dev/
---
security/device_cgroup.c | 56 ++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index dc4df7475081..7fec575d32d6 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -244,45 +244,40 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
#define DEVCG_DENY 2
#define DEVCG_LIST 3
-#define MAJMINLEN 13
-#define ACCLEN 4
-
-static void set_access(char *acc, short access)
+static void seq_putaccess(struct seq_file *m, short access)
{
- int idx = 0;
- memset(acc, 0, ACCLEN);
if (access & DEVCG_ACC_READ)
- acc[idx++] = 'r';
+ seq_putc(m, 'r');
if (access & DEVCG_ACC_WRITE)
- acc[idx++] = 'w';
+ seq_putc(m, 'w');
if (access & DEVCG_ACC_MKNOD)
- acc[idx++] = 'm';
+ seq_putc(m, 'm');
}
-static char type_to_char(short type)
+static void seq_puttype(struct seq_file *m, short type)
{
if (type == DEVCG_DEV_ALL)
- return 'a';
- if (type == DEVCG_DEV_CHAR)
- return 'c';
- if (type == DEVCG_DEV_BLOCK)
- return 'b';
- return 'X';
+ seq_putc(m, 'a');
+ else if (type == DEVCG_DEV_CHAR)
+ seq_putc(m, 'c');
+ else if (type == DEVCG_DEV_BLOCK)
+ seq_putc(m, 'b');
+ else
+ seq_putc(m, 'X');
}
-static void set_majmin(char *str, unsigned m)
+static void seq_putversion(struct seq_file *m, unsigned int version)
{
- if (m == ~0)
- strcpy(str, "*");
+ if (version == ~0)
+ seq_putc(m, '*');
else
- sprintf(str, "%u", m);
+ seq_printf(m, "%u", version);
}
static int devcgroup_seq_show(struct seq_file *m, void *v)
{
struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
struct dev_exception_item *ex;
- char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
rcu_read_lock();
/*
@@ -292,18 +287,17 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
* This way, the file remains as a "whitelist of devices"
*/
if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
- set_access(acc, DEVCG_ACC_MASK);
- set_majmin(maj, ~0);
- set_majmin(min, ~0);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
- maj, min, acc);
+ seq_puts(m, "a *:* rwm\n");
} else {
list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
- set_access(acc, ex->access);
- set_majmin(maj, ex->major);
- set_majmin(min, ex->minor);
- seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
- maj, min, acc);
+ seq_puttype(m, ex->type);
+ seq_putc(m, ' ');
+ seq_putversion(m, ex->major);
+ seq_putc(m, ':');
+ seq_putversion(m, ex->minor);
+ seq_putc(m, ' ');
+ seq_putaccess(m, ex->access);
+ seq_putc(m, '\n');
}
}
rcu_read_unlock();
--
2.51.1
^ permalink raw reply related
* Re: [PATCH v2] device_cgroup: Refactor devcgroup_seq_show to use seq_put* helpers
From: Serge E. Hallyn @ 2025-10-31 23:38 UTC (permalink / raw)
To: Thorsten Blum
Cc: David Laight, Paul Moore, James Morris, Serge E. Hallyn,
linux-security-module, linux-kernel
In-Reply-To: <20251031213915.211046-1-thorsten.blum@linux.dev>
On Fri, Oct 31, 2025 at 10:39:14PM +0100, Thorsten Blum wrote:
> Replace set_access(), set_majmin(), and type_to_char() with new helpers
> seq_putaccess(), seq_puttype(), and seq_putversion() that write directly
> to 'seq_file'.
>
> Simplify devcgroup_seq_show() by hard-coding "a *:* rwm", and use the
> new seq_put* helper functions to list the exceptions otherwise.
>
> This allows us to remove the intermediate string buffers while
> maintaining the same functionality, including wildcard handling.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Thank you, that is much nicer.
Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> Changes in v2:
> - Add setq_put* helpers to modify seq_file directly (David)
> - Update patch subject and description
> - Link to v1: https://lore.kernel.org/lkml/20251031110647.102728-2-thorsten.blum@linux.dev/
> ---
> security/device_cgroup.c | 56 ++++++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index dc4df7475081..7fec575d32d6 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -244,45 +244,40 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
> #define DEVCG_DENY 2
> #define DEVCG_LIST 3
>
> -#define MAJMINLEN 13
> -#define ACCLEN 4
> -
> -static void set_access(char *acc, short access)
> +static void seq_putaccess(struct seq_file *m, short access)
> {
> - int idx = 0;
> - memset(acc, 0, ACCLEN);
> if (access & DEVCG_ACC_READ)
> - acc[idx++] = 'r';
> + seq_putc(m, 'r');
> if (access & DEVCG_ACC_WRITE)
> - acc[idx++] = 'w';
> + seq_putc(m, 'w');
> if (access & DEVCG_ACC_MKNOD)
> - acc[idx++] = 'm';
> + seq_putc(m, 'm');
> }
>
> -static char type_to_char(short type)
> +static void seq_puttype(struct seq_file *m, short type)
> {
> if (type == DEVCG_DEV_ALL)
I do think a switch would be easier to read here, but that's
personal preference...
> - return 'a';
> - if (type == DEVCG_DEV_CHAR)
> - return 'c';
> - if (type == DEVCG_DEV_BLOCK)
> - return 'b';
> - return 'X';
> + seq_putc(m, 'a');
> + else if (type == DEVCG_DEV_CHAR)
> + seq_putc(m, 'c');
> + else if (type == DEVCG_DEV_BLOCK)
> + seq_putc(m, 'b');
> + else
> + seq_putc(m, 'X');
> }
>
> -static void set_majmin(char *str, unsigned m)
> +static void seq_putversion(struct seq_file *m, unsigned int version)
> {
> - if (m == ~0)
> - strcpy(str, "*");
> + if (version == ~0)
> + seq_putc(m, '*');
> else
> - sprintf(str, "%u", m);
> + seq_printf(m, "%u", version);
> }
>
> static int devcgroup_seq_show(struct seq_file *m, void *v)
> {
> struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
> struct dev_exception_item *ex;
> - char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
>
> rcu_read_lock();
> /*
> @@ -292,18 +287,17 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
> * This way, the file remains as a "whitelist of devices"
> */
> if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> - set_access(acc, DEVCG_ACC_MASK);
> - set_majmin(maj, ~0);
> - set_majmin(min, ~0);
> - seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
> - maj, min, acc);
> + seq_puts(m, "a *:* rwm\n");
> } else {
> list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
> - set_access(acc, ex->access);
> - set_majmin(maj, ex->major);
> - set_majmin(min, ex->minor);
> - seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
> - maj, min, acc);
> + seq_puttype(m, ex->type);
> + seq_putc(m, ' ');
> + seq_putversion(m, ex->major);
> + seq_putc(m, ':');
> + seq_putversion(m, ex->minor);
> + seq_putc(m, ' ');
> + seq_putaccess(m, ex->access);
> + seq_putc(m, '\n');
> }
> }
> rcu_read_unlock();
> --
> 2.51.1
^ permalink raw reply
* Re: [PATCH v2 2/2] ipe: Update documentation for script enforcement
From: Fan Wu @ 2025-10-31 23:50 UTC (permalink / raw)
To: Yanzhu Huang
Cc: wufan, paul, mic, jmorris, serge, corbet, linux-security-module,
linux-doc, linux-kernel
In-Reply-To: <20251031101700.694964-3-yanzhuhuang@linux.microsoft.com>
On Fri, Oct 31, 2025 at 3:17 AM Yanzhu Huang
<yanzhuhuang@linux.microsoft.com> wrote:
>
> This patch adds explanation of script enforcement mechanism in admin
> guide documentation. Describes how IPE supports integrity enforcement
> for indirectly executed scripts through the AT_EXECVE_CHECK flag, and
> how this differs from kernel enforcement for compiled executables.
>
> Signed-off-by: Yanzhu Huang <yanzhuhuang@linux.microsoft.com>
> ---
> Documentation/admin-guide/LSM/ipe.rst | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst
> index dc7088451f9d..3f205d7dd533 100644
> --- a/Documentation/admin-guide/LSM/ipe.rst
> +++ b/Documentation/admin-guide/LSM/ipe.rst
> @@ -95,7 +95,20 @@ languages when these scripts are invoked by passing these program files
> to the interpreter. This is because the way interpreters execute these
> files; the scripts themselves are not evaluated as executable code
> through one of IPE's hooks, but they are merely text files that are read
> -(as opposed to compiled executables) [#interpreters]_.
> +(as opposed to compiled executables). However, with the introduction of the
> +``AT_EXECVE_CHECK`` flag (see `AT_EXECVE_CHECK <https://docs.kernel.org/userspace-api/check_exec.html#at-execve-check>`__),
Using url here might not be ideal, perhaps change it to
:doc:`AT_EXECVE_CHECK </userspace-api/check_exec>`
-Fan
^ permalink raw reply
* Re: [GIT PULL] Block fixes for 6.18-rc3
From: Christian Brauner @ 2025-11-01 13:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jens Axboe, Paul Moore, Serge Hallyn, linux-block@vger.kernel.org,
LSM List
In-Reply-To: <CAHk-=wg6+ngpnQsp0ic_5Ebhv5g=cVKVWjrJyRBQWBp4MDiJNQ@mail.gmail.com>
On Fri, Oct 31, 2025 at 09:30:11AM -0700, Linus Torvalds wrote:
> On Fri, 31 Oct 2025 at 08:44, Christian Brauner <brauner@kernel.org> wrote:
> >
> > Hm, two immediate observations before I go off and write the series.
> >
> > (1) The thing is that init_cred would have to be exposed to modules via
> > EXPORT_SYMBOL() for this to work. It would be easier to just force
> > the use of init_task->cred instead.
>
> Yea, I guess we already export that.
>
> > That pointer deref won't matter in the face of the allocations and
> > refcounts we wipe out with this. Then we should also move init_cred
> > to init/init_task.c and make it static const. Nobody really needs it
> > currently.
>
> Well, I did the "does it compile ok" with it marked as 'const', but as
> mentioned, those 'struct cred' instances aren't *really* const, they
> are only pseudo-const things in that they are *marked* const so that
> nobody modifies them by mistake, but then the ref-counting will cast
> the constness away in order to update references.
>
> So I don't think we can *actually* mark it "static const", because
> that will put the data structure in the const data section, and then
> the refcounting will trigger kernel page faults.
>
> End result: I think we can indeed move it to init/init_task.c. And
> yes, we can and should make it static to that file, but not plain
> 'const'.
>
> If we expose it to others - but I think you're right that maybe it's
> not a good idea - we should *expose* it as a 'const' data structure.
>
> But we should probably put it in some explicitly writable section (I
> was going to suggest marking it "__read_mostly", but it turns out some
> architectures #define that to be empty, so a "const __read_mosyly"
> data structure could still end up in a read-only section).
For some init data structures that are heavily used such as:
init_pid_ns
it often makes sense to just skip the refcounting completely because we
know they are always around. Take the pid namespace as an example:
static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
{
if (ns != &init_pid_ns)
ns_ref_inc(ns);
return ns;
}
void put_pid_ns(struct pid_namespace *ns)
{
if (ns && ns != &init_pid_ns && ns_ref_put(ns))
schedule_work(&ns->work);
}
While it has the obvious disadvantage that it introduces a special-case
into the refcounting and it would obviously be more elegant if we just
did:
void put_pid_ns(struct pid_namespace *ns)
{
if (ns_ref_put(ns))
schedule_work(&ns->work);
}
it does elide a ton of refcount increments and decrements during task
creation.
While that's not true for init_creds it would still be easy to just not
refcount them at all if it's worth it.
Now that I think about it: given that I reworked all the namespace
reference counting completely it should be easy to make all initial
namespaces not get or put reference counts at all, like:
static __always_inline bool is_initial_namespace(struct ns_common *ns)
{
VFS_WARN_ON_ONCE(ns->ns_id == 0);
/* initial namespaces have fixed ids and the ids aren't recycled */
return ns->ns_id <= NS_LAST_INIT_ID;
}
diff --git a/include/linux/ns_common.h b/include/linux/ns_common.h
index 241eb1e98e1d..fe9c81963786 100644
--- a/include/linux/ns_common.h
+++ b/include/linux/ns_common.h
@@ -136,9 +136,8 @@ struct ns_common *__must_check ns_owner(struct ns_common *ns);
#define to_ns_common(__ns) \
@@ -225,6 +224,8 @@ static __always_inline __must_check int __ns_ref_active_read(const struct ns_com
static __always_inline __must_check bool __ns_ref_put(struct ns_common *ns)
{
+ if (is_initial_namespace(ns))
+ return false;
if (refcount_dec_and_test(&ns->__ns_ref)) {
VFS_WARN_ON_ONCE(__ns_ref_active_read(ns));
return true;
@@ -234,6 +235,8 @@ static __always_inline __must_check bool __ns_ref_put(struct ns_common *ns)
static __always_inline __must_check bool __ns_ref_get(struct ns_common *ns)
{
+ if (is_initial_namespace(ns))
+ return true;
if (refcount_inc_not_zero(&ns->__ns_ref))
return true;
VFS_WARN_ON_ONCE(__ns_ref_active_read(ns));
@@ -246,7 +249,8 @@ static __always_inline __must_check int __ns_ref_read(const struct ns_common *ns
}
#define ns_ref_read(__ns) __ns_ref_read(to_ns_common((__ns)))
-#define ns_ref_inc(__ns) refcount_inc(&to_ns_common((__ns))->__ns_ref)
+#define ns_ref_inc(__ns) \
+ do { if (!is_initial_namespace(to_ns_common(__ns))) refcount_inc(&to_ns_common((__ns))->__ns_ref); } while (0)
#define ns_ref_get(__ns) __ns_ref_get(to_ns_common((__ns)))
#define ns_ref_put(__ns) __ns_ref_put(to_ns_common((__ns)))
#define ns_ref_put_and_lock(__ns, __lock) \
This effectively means we can drop all the special-casing in the
namespace helpers like:
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 445517a72ad0..ef06c3d3fb52 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -61,9 +61,7 @@ static inline struct pid_namespace *to_pid_ns(struct ns_common *ns)
static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
{
- if (ns != &init_pid_ns)
- ns_ref_inc(ns);
- return ns;
+ ns_ref_inc(ns);
}
#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 650be58d8d18..e48f5de41361 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -184,7 +184,7 @@ struct pid_namespace *copy_pid_ns(u64 flags,
void put_pid_ns(struct pid_namespace *ns)
{
- if (ns && ns != &init_pid_ns && ns_ref_put(ns))
+ if (ns && ns_ref_put(ns))
schedule_work(&ns->work);
}
EXPORT_SYMBOL_GPL(put_pid_ns);
And all the other ones - without having looked into any potential
pitfalls - would get the same behavior as the pidns for free. Worth it?
I think especially for the network namespace that might potentially
avoid a bunch of cacheline ping-pong. But idk, it's just a theory. But
it's easy enough to implement.
>
> > (2) I think the plain override_creds() would work but we can do better.
> > I envision we can leverage CLASS() to completely hide any access to
> > init_cred and force a scope with kernel creds.
>
> Ack.
>
> Linus
^ permalink raw reply related
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-01 16:50 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, linux-security-module, Mimi Zohar, Karel Srot,
James Morris, Serge E. Hallyn, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, open list, open list:MODULE SUPPORT
In-Reply-To: <20251031074016.1975356-1-coxu@redhat.com>
On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
>
> Currently, when in-kernel module decompression (CONFIG_MODULE_DECOMPRESS)
> is enabled, IMA has no way to verify the appended module signature as it
> can't decompress the module.
>
> Define a new LSM hook security_kernel_module_read_file which will be
> called after kernel module decompression is done so IMA can access the
> decompressed kernel module to verify the appended signature.
>
> Since IMA can access both xattr and appended kernel module signature
> with the new LSM hook, it no longer uses the security_kernel_post_read_file
> LSM hook for kernel module loading.
>
> Before enabling in-kernel module decompression, a kernel module in
> initramfs can still be loaded with ima_policy=secure_boot. So adjust the
> kernel module rule in secure_boot policy to allow either an IMA
> signature OR an appended signature i.e. to use
> "appraise func=MODULE_CHECK appraise_type=imasig|modsig".
>
> Reported-by: Karel Srot <ksrot@redhat.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
> v1: https://lore.kernel.org/linux-integrity/20250928030358.3873311-1-coxu@redhat.com/
>
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 7 +++++++
> kernel/module/main.c | 10 +++++++++-
> security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
> security/integrity/ima/ima_policy.c | 2 +-
> security/security.c | 17 +++++++++++++++++
> 6 files changed, 62 insertions(+), 2 deletions(-)
We don't really need a new LSM hook for this do we? Can't we just
define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
another call to security_kernel_post_read_file() after the module is
unpacked? Something like the snippet below ...
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..f127000d2e0a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
ar __user * uargs, int
mod_stat_add_long(len, &invalid_decompress_bytes);
return err;
}
+
+ err = security_kernel_post_read_file(f,
+ (char *)info.hdr, info.len,
+ READING_MODULE_DECOMPRESS);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ return err;
+ }
} else {
info.hdr = buf;
info.len = len;
--
paul-moore.com
^ permalink raw reply related
* Re: [PATCH] device_cgroup: Replace strcpy/sprintf in set_majmin
From: Paul Moore @ 2025-11-01 17:00 UTC (permalink / raw)
To: David Laight
Cc: Thorsten Blum, James Morris, Serge E. Hallyn,
linux-security-module, linux-kernel
In-Reply-To: <20251031165417.4490941a@pumpkin>
On Fri, Oct 31, 2025 at 12:54 PM David Laight
<david.laight.linux@gmail.com> wrote:
> On Fri, 31 Oct 2025 16:23:02 +0100
> Thorsten Blum <thorsten.blum@linux.dev> wrote:
>
> > On 31. Oct 2025, at 13:59, David Laight wrote:
> > > Even if ex->major can be ~0 there are much cleaner ways of writing this code.
> >
> > Thanks for pointing this out. Looking at the bigger picture makes it
> > clear that most of the code can actually be removed. What do you think
> > of this change?
>
> That is sort of what I was thinking about, but it doesn't quite work.
>
> >
> > Thanks,
> > Thorsten
> >
> > diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> > index a41f558f6fdd..cb845b1fad6b 100644
> > --- a/security/device_cgroup.c
> > +++ b/security/device_cgroup.c
> > @@ -244,7 +244,6 @@ static void devcgroup_css_free(struct cgroup_subsys_state *css)
> > #define DEVCG_DENY 2
> > #define DEVCG_LIST 3
> >
> > -#define MAJMINLEN 13
> > #define ACCLEN 4
> >
> > static void set_access(char *acc, short access)
> > @@ -270,19 +269,11 @@ static char type_to_char(short type)
> > return 'X';
> > }
> >
> > -static void set_majmin(char *str, unsigned m)
> > -{
> > - if (m == ~0)
> > - strscpy(str, "*", MAJMINLEN);
> > - else
> > - snprintf(str, MAJMINLEN, "%u", m);
> > -}
> > -
> > static int devcgroup_seq_show(struct seq_file *m, void *v)
> > {
> > struct dev_cgroup *devcgroup = css_to_devcgroup(seq_css(m));
> > struct dev_exception_item *ex;
> > - char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
> > + char acc[ACCLEN];
> >
> > rcu_read_lock();
> > /*
> > @@ -293,17 +284,12 @@ static int devcgroup_seq_show(struct seq_file *m, void *v)
> > */
> > if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> > set_access(acc, DEVCG_ACC_MASK);
> > - set_majmin(maj, ~0);
> > - set_majmin(min, ~0);
> > - seq_printf(m, "%c %s:%s %s\n", type_to_char(DEVCG_DEV_ALL),
> > - maj, min, acc);
> > + seq_printf(m, "%c *:* %s\n", type_to_char(DEVCG_DEV_ALL), acc);
>
> type_to_char(DEVCG_DEV_ALL) is 'a' and this is the only place it happens,
> also acc is "rwm".
> So that could be:
> seq_puts(m, "a *:* rwm\n");
>
> > } else {
> > list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
> > set_access(acc, ex->access);
> > - set_majmin(maj, ex->major);
> > - set_majmin(min, ex->minor);
> > - seq_printf(m, "%c %s:%s %s\n", type_to_char(ex->type),
> > - maj, min, acc);
> > + seq_printf(m, "%c %u:%u %s\n", type_to_char(ex->type),
> > + ex->major, ex->minor, acc);
>
> It looks like both ex->major and ex->minor can be ~0.
> (I'm not sure it makes any sense to have major == ~0 and minor != ~0).
> However this should be ok:
> seq_putc(m, type_to_char(ex->type);
> if (ex->major == ~0)
> seq_puts(m, " *:");
> else
> seq_printf(m, " %u:", ex->major);
> if (ex->minor == ~0)
> seq_puts(m, "* ");
> else
> seq_printf(m, "%u ", ex->minor);
> if (ex->access & DEVCG_ACC_READ)
> seq_putc(m, 'r');
> if (ex->access & DEVCG_ACC_WRITE)
> seq_putc(m, 'w');
> if (ex->access & DEV_ACC_MKNOD)
> seq_putc(m. 'm');
> seq_putc(m, '\n');
>
> A less intrusive change would be to pass 'm' the the set_xxx() functions
> and add the separators between the calls.
Yes, just pass the seq_file pointer up to set_majmin() and have it do
the writes/puts directly. Might as well rename if from set_majmin()
to put_majmin() while you are at it.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-02 15:05 UTC (permalink / raw)
To: Paul Moore, Coiby Xu
Cc: linux-integrity, linux-security-module, Karel Srot, James Morris,
Serge E. Hallyn, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
open list, open list:MODULE SUPPORT
In-Reply-To: <CAHC9VhRBXkW+XuqhxJvEOYR_VMxFh4TRWUtXzZky=AG_nyBYEQ@mail.gmail.com>
On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
> On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
> >
> > Currently, when in-kernel module decompression (CONFIG_MODULE_DECOMPRESS)
> > is enabled, IMA has no way to verify the appended module signature as it
> > can't decompress the module.
> >
> > Define a new LSM hook security_kernel_module_read_file which will be
> > called after kernel module decompression is done so IMA can access the
> > decompressed kernel module to verify the appended signature.
> >
> > Since IMA can access both xattr and appended kernel module signature
> > with the new LSM hook, it no longer uses the security_kernel_post_read_file
> > LSM hook for kernel module loading.
> >
> > Before enabling in-kernel module decompression, a kernel module in
> > initramfs can still be loaded with ima_policy=secure_boot. So adjust the
> > kernel module rule in secure_boot policy to allow either an IMA
> > signature OR an appended signature i.e. to use
> > "appraise func=MODULE_CHECK appraise_type=imasig|modsig".
> >
> > Reported-by: Karel Srot <ksrot@redhat.com>
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > Signed-off-by: Coiby Xu <coxu@redhat.com>
> > ---
> > v1: https://lore.kernel.org/linux-integrity/20250928030358.3873311-1-coxu@redhat.com/
> >
> > include/linux/lsm_hook_defs.h | 2 ++
> > include/linux/security.h | 7 +++++++
> > kernel/module/main.c | 10 +++++++++-
> > security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
> > security/integrity/ima/ima_policy.c | 2 +-
> > security/security.c | 17 +++++++++++++++++
> > 6 files changed, 62 insertions(+), 2 deletions(-)
>
> We don't really need a new LSM hook for this do we? Can't we just
> define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
> another call to security_kernel_post_read_file() after the module is
> unpacked? Something like the snippet below ...
Yes, this is similar to my suggestion based on defining multiple enumerations:
READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
With this solution, IMA would need to make an exception in the post kernel
module read for the READING_COMPRESSED_MODULE case, since the kernel module has
not yet been decompressed.
Coiby suggested further simplification by moving the call later. At which point
either there is or isn't an appended signature for non-compressed and
decompressed kernel modules.
As long as you don't have a problem calling the security_kernel_post_read_file()
hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..f127000d2e0a 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
> ar __user * uargs, int
> mod_stat_add_long(len, &invalid_decompress_bytes);
> return err;
> }
> +
> + err = security_kernel_post_read_file(f,
> + (char *)info.hdr, info.len,
> + READING_MODULE_DECOMPRESS);
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + return err;
> + }
> } else {
> info.hdr = buf;
> info.len = len;
== defer security_kernel_post_read_file() call to here ==
Mimi
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Paul Moore @ 2025-11-02 15:43 UTC (permalink / raw)
To: Mimi Zohar
Cc: Coiby Xu, linux-integrity, linux-security-module, Karel Srot,
James Morris, Serge E. Hallyn, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, open list, open list:MODULE SUPPORT
In-Reply-To: <baa39fcd1b6b485f14b8f06dcd96b81359e6e491.camel@linux.ibm.com>
On Sun, Nov 2, 2025 at 10:06 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Sat, 2025-11-01 at 12:50 -0400, Paul Moore wrote:
> > On Fri, Oct 31, 2025 at 3:41 AM Coiby Xu <coxu@redhat.com> wrote:
> > >
> > > Currently, when in-kernel module decompression (CONFIG_MODULE_DECOMPRESS)
> > > is enabled, IMA has no way to verify the appended module signature as it
> > > can't decompress the module.
> > >
> > > Define a new LSM hook security_kernel_module_read_file which will be
> > > called after kernel module decompression is done so IMA can access the
> > > decompressed kernel module to verify the appended signature.
> > >
> > > Since IMA can access both xattr and appended kernel module signature
> > > with the new LSM hook, it no longer uses the security_kernel_post_read_file
> > > LSM hook for kernel module loading.
> > >
> > > Before enabling in-kernel module decompression, a kernel module in
> > > initramfs can still be loaded with ima_policy=secure_boot. So adjust the
> > > kernel module rule in secure_boot policy to allow either an IMA
> > > signature OR an appended signature i.e. to use
> > > "appraise func=MODULE_CHECK appraise_type=imasig|modsig".
> > >
> > > Reported-by: Karel Srot <ksrot@redhat.com>
> > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > > Signed-off-by: Coiby Xu <coxu@redhat.com>
> > > ---
> > > v1: https://lore.kernel.org/linux-integrity/20250928030358.3873311-1-coxu@redhat.com/
> > >
> > > include/linux/lsm_hook_defs.h | 2 ++
> > > include/linux/security.h | 7 +++++++
> > > kernel/module/main.c | 10 +++++++++-
> > > security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
> > > security/integrity/ima/ima_policy.c | 2 +-
> > > security/security.c | 17 +++++++++++++++++
> > > 6 files changed, 62 insertions(+), 2 deletions(-)
> >
> > We don't really need a new LSM hook for this do we? Can't we just
> > define a new file read type, e.g. READING_MODULE_DECOMPRESS, and do
> > another call to security_kernel_post_read_file() after the module is
> > unpacked? Something like the snippet below ...
>
> Yes, this is similar to my suggestion based on defining multiple enumerations:
> READING_MODULE, READING_COMPRESSED_MODULE, and READING_DECOMPRESSED_MODULE.
> With this solution, IMA would need to make an exception in the post kernel
> module read for the READING_COMPRESSED_MODULE case, since the kernel module has
> not yet been decompressed.
>
> Coiby suggested further simplification by moving the call later. At which point
> either there is or isn't an appended signature for non-compressed and
> decompressed kernel modules.
>
> As long as you don't have a problem calling the security_kernel_post_read_file()
> hook again, could we move the call later and pass READING_MODULE_UNCOMPRESSED?
It isn't clear from these comments if you are talking about moving
only the second security_kernel_post_read_file() call that was
proposed for init_module_from_file() to later in the function, leaving
the call in kernel_read_file() intact, or something else?
I think we want to leave the hook calls in kernel_read_file() intact,
in which case I'm not certain what advantage there is in moving the
security_kernel_post_read_file() call to a location where it is called
in init_module_from_file() regardless of if the module is compressed
or not. In the uncompressed case you are calling the hook twice for
no real benefit? It may be helpful to submit a patch with your
proposal as a patch can be worth a thousand words ;)
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index c66b26184936..f127000d2e0a 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -3693,6 +3693,14 @@ static int init_module_from_file(struct file *f, const ch
> > ar __user * uargs, int
> > mod_stat_add_long(len, &invalid_decompress_bytes);
> > return err;
> > }
> > +
> > + err = security_kernel_post_read_file(f,
> > + (char *)info.hdr, info.len,
> > + READING_MODULE_DECOMPRESS);
> > + if (err) {
> > + mod_stat_inc(&failed_kreads);
> > + return err;
> > + }
> > } else {
> > info.hdr = buf;
> > info.len = len;
>
> == defer security_kernel_post_read_file() call to here ==
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Oleg Nesterov @ 2025-11-02 16:17 UTC (permalink / raw)
To: Bernd Edlinger
Cc: 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: <GV2PPF74270EBEE9EF78827D73D3D7212F7E432A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
On 08/21, Bernd Edlinger wrote:
>
> v16: rebased to 6.17-rc2, fixed some minor merge conflicts.
>
> v17: avoid use of task->in_execve in ptrace_attach.
So I guess this version doesn't really differ from v14 I tried to review...
(yes, iirc my review wasn't really good, sorry).
Perhaps I am wrong, but I still think we need another approach. de_thread()
should not wait until all sub-threads are reaped, it should drop cred_guard_mutex
earlier.
I mean, something like
[PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held
https://lore.kernel.org/lkml/20170213180454.GA2858@redhat.com/
which is hopelessly outdated now.
Again, perhaps I am wrong. I'll try to take another look next week.
Oleg.
^ permalink raw reply
* [PATCH RESEND] apparmor: Replace sprintf/strcpy with scnprintf/strscpy in aa_policy_init
From: Thorsten Blum @ 2025-11-03 9:06 UTC (permalink / raw)
To: John Johansen, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, apparmor, linux-security-module, linux-kernel
strcpy() is deprecated and sprintf() does not perform bounds checking
either. Although an overflow is unlikely, it's better to proactively
avoid it by using the safer strscpy() and scnprintf(), respectively.
Additionally, unify memory allocation for 'hname' to simplify and
improve aa_policy_init().
Link: https://github.com/KSPP/linux/issues/88
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/lib.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index 82dbb97ad406..acf7f5189bec 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -478,19 +478,17 @@ bool aa_policy_init(struct aa_policy *policy, const char *prefix,
const char *name, gfp_t gfp)
{
char *hname;
+ size_t hname_sz;
+ hname_sz = (prefix ? strlen(prefix) + 2 : 0) + strlen(name) + 1;
/* freed by policy_free */
- if (prefix) {
- hname = aa_str_alloc(strlen(prefix) + strlen(name) + 3, gfp);
- if (hname)
- sprintf(hname, "%s//%s", prefix, name);
- } else {
- hname = aa_str_alloc(strlen(name) + 1, gfp);
- if (hname)
- strcpy(hname, name);
- }
+ hname = aa_str_alloc(hname_sz, gfp);
if (!hname)
return false;
+ if (prefix)
+ scnprintf(hname, hname_sz, "%s//%s", prefix, name);
+ else
+ strscpy(hname, name, hname_sz);
policy->hname = hname;
/* base.name is a substring of fqname */
policy->name = basename(policy->hname);
--
2.51.1
^ permalink raw reply related
* [PATCH] security: sctp: Format type and permission checks tables
From: Bagas Sanjaya @ 2025-11-03 11:39 UTC (permalink / raw)
To: Linux Kernel Mailing List, Linux Documentation,
Linux Security Module
Cc: Jonathan Corbet, Jarkko Sakkinen, Christian Brauner,
Bagas Sanjaya, Jeff Layton, Kees Cook, Mickaël Salaün,
Stuart Yoder
Use reST grid tables for both type and permission checks tables.
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
This patch is based on lsm tree.
Documentation/security/SCTP.rst | 48 +++++++++++++++++++++------------
1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst
index 6d80d464ab6e7c..321bf6c8738970 100644
--- a/Documentation/security/SCTP.rst
+++ b/Documentation/security/SCTP.rst
@@ -46,24 +46,31 @@ Returns 0 on success, error on failure.
ipv4 or ipv6 address using sizeof(struct sockaddr_in) or
sizeof(struct sockaddr_in6).
- ------------------------------------------------------------------
- | BIND Type Checks |
+.. table:: BIND Type Checks
+
+ +----------------------------+-----------------------------------+
| @optname | @address contains |
- |----------------------------|-----------------------------------|
+ +============================+===================================+
| SCTP_SOCKOPT_BINDX_ADD | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_PRIMARY_ADDR | Single ipv4 or ipv6 address |
+ +----------------------------+-----------------------------------+
| SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address |
- ------------------------------------------------------------------
+ +----------------------------+-----------------------------------+
+
+.. table:: CONNECT Type Checks
- ------------------------------------------------------------------
- | CONNECT Type Checks |
+ +----------------------------+-----------------------------------+
| @optname | @address contains |
- |----------------------------|-----------------------------------|
+ +============================+===================================+
| SCTP_SOCKOPT_CONNECTX | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_PARAM_ADD_IP | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_SENDMSG_CONNECT | Single ipv4 or ipv6 address |
+ +----------------------------+-----------------------------------+
| SCTP_PARAM_SET_PRIMARY | Single ipv4 or ipv6 address |
- ------------------------------------------------------------------
+ +----------------------------+-----------------------------------+
A summary of the ``@optname`` entries is as follows::
@@ -228,26 +235,33 @@ The security module performs the following operations:
security_sctp_bind_connect()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Checks permissions required for ipv4/ipv6 addresses based on the ``@optname``
-as follows::
+as follows:
- ------------------------------------------------------------------
- | BIND Permission Checks |
+.. table:: BIND Permission Checks
+
+ +----------------------------+-----------------------------------+
| @optname | @address contains |
- |----------------------------|-----------------------------------|
+ +============================+===================================+
| SCTP_SOCKOPT_BINDX_ADD | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_PRIMARY_ADDR | Single ipv4 or ipv6 address |
+ +----------------------------+-----------------------------------+
| SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address |
- ------------------------------------------------------------------
+ +----------------------------+-----------------------------------+
+
+.. table:: CONNECT Permission Checks
- ------------------------------------------------------------------
- | CONNECT Permission Checks |
+ +----------------------------+-----------------------------------+
| @optname | @address contains |
- |----------------------------|-----------------------------------|
+ +============================+===================================+
| SCTP_SOCKOPT_CONNECTX | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_PARAM_ADD_IP | One or more ipv4 / ipv6 addresses |
+ +----------------------------+-----------------------------------+
| SCTP_SENDMSG_CONNECT | Single ipv4 or ipv6 address |
+ +----------------------------+-----------------------------------+
| SCTP_PARAM_SET_PRIMARY | Single ipv4 or ipv6 address |
- ------------------------------------------------------------------
+ +----------------------------+-----------------------------------+
`SCTP LSM Support`_ gives a summary of the ``@optname``
base-commit: dfa024bc3f67a97e1a975dd66b83af8b3845eb19
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply related
* Re: [PATCH v6 5/5] Smack: add support for lsm_config_self_policy and lsm_config_system_policy
From: Casey Schaufler @ 2025-11-04 14:41 UTC (permalink / raw)
To: Maxime Bélair, linux-security-module
Cc: john.johansen, paul, jmorris, serge, mic, kees,
stephen.smalley.work, takedakn, penguin-kernel, song, rdunlap,
linux-api, apparmor, linux-kernel, Casey Schaufler
In-Reply-To: <20251010132610.12001-6-maxime.belair@canonical.com>
On 10/10/2025 6:25 AM, Maxime Bélair wrote:
> Enable users to manage Smack policies through the new hooks
> lsm_config_self_policy and lsm_config_system_policy.
>
> lsm_config_self_policy allows adding Smack policies for the current cred.
> For now it remains restricted to CAP_MAC_ADMIN.
>
> lsm_config_system_policy allows adding globabl Smack policies. This is
> restricted to CAP_MAC_ADMIN.
>
> Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Apologies for the late review. I see that Paul has suggested the set
wait until the LSM namespace discussions have moved forward.
> ---
> security/smack/smack.h | 8 +++++
> security/smack/smack_lsm.c | 73 ++++++++++++++++++++++++++++++++++++++
> security/smack/smackfs.c | 2 +-
> 3 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index bf6a6ed3946c..3e3d30dfdcf7 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -275,6 +275,14 @@ struct smk_audit_info {
> #endif
> };
>
> +/*
> + * This function is in smackfs.c
> + */
> +ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos,
> + struct list_head *rule_list,
> + struct mutex *rule_lock, int format);
> +
> /*
> * These functions are in smack_access.c
> */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 99833168604e..bf4bb2242768 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -5027,6 +5027,76 @@ static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
>
> #endif /* CONFIG_IO_URING */
>
> +/**
> + * smack_lsm_config_system_policy - Configure a system smack policy
Smack prefers to say "rule set" instead of "policy". Smack policy
doesn't change, but the allowed exceptions to the policy (rules)
are mutable.
> + * @op: operation to perform. Currently, only LSM_POLICY_LOAD is supported
> + * @buf: User-supplied buffer in the form "<fmt><policy>"
> + * <fmt> is the 1-byte format of <policy>
> + * <policy> is the policy to load
> + * @size: size of @buf
> + * @flags: reserved for future use; must be zero
> + *
> + * Returns: number of written rules on success, negative value on error
> + */
> +static int smack_lsm_config_system_policy(u32 op, void __user *buf, size_t size,
> + u32 flags)
> +{
> + loff_t pos = 0;
> + u8 fmt;
> +
> + if (op != LSM_POLICY_LOAD || flags)
> + return -EOPNOTSUPP;
> +
> + if (size < 2)
> + return -EINVAL;
There should be a max check as well.
> +
> + if (get_user(fmt, (uint8_t *)buf))
> + return -EFAULT;
> +
> + return smk_write_rules_list(NULL, buf + 1, size - 1, &pos, NULL, NULL, fmt);
> +}
> +
> +/**
> + * smack_lsm_config_self_policy - Configure a smack policy for the current cred
> + * @op: operation to perform. Currently, only LSM_POLICY_LOAD is supported
> + * @buf: User-supplied buffer in the form "<fmt><policy>"
> + * <fmt> is the 1-byte format of <policy>
> + * <policy> is the policy to load
> + * @size: size of @buf
> + * @flags: reserved for future use; must be zero
> + *
> + * Returns: number of written rules on success, negative value on error
> + */
> +static int smack_lsm_config_self_policy(u32 op, void __user *buf, size_t size,
> + u32 flags)
> +{
> + loff_t pos = 0;
> + u8 fmt;
> + struct task_smack *tsp;
> +
> + if (op != LSM_POLICY_LOAD || flags)
> + return -EOPNOTSUPP;
> +
> + if (size < 2)
> + return -EINVAL;
> +
> + if (get_user(fmt, (uint8_t *)buf))
> + return -EFAULT;
> + /**
> + * smk_write_rules_list could be used to gain privileges.
> + * This function is thus restricted to CAP_MAC_ADMIN.
> + * TODO: Ensure that the new rule does not give extra privileges
> + * before dropping this CAP_MAC_ADMIN check.
> + */
> + if (!capable(CAP_MAC_ADMIN))
> + return -EPERM;
> +
> +
> + tsp = smack_cred(current_cred());
> + return smk_write_rules_list(NULL, buf + 1, size - 1, &pos, &tsp->smk_rules,
> + &tsp->smk_rules_lock, fmt);
> +}
> +
> struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
> .lbs_cred = sizeof(struct task_smack),
> .lbs_file = sizeof(struct smack_known *),
> @@ -5203,6 +5273,9 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(uring_sqpoll, smack_uring_sqpoll),
> LSM_HOOK_INIT(uring_cmd, smack_uring_cmd),
> #endif
> + LSM_HOOK_INIT(lsm_config_self_policy, smack_lsm_config_self_policy),
> + LSM_HOOK_INIT(lsm_config_system_policy, smack_lsm_config_system_policy),
> +
> };
>
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 90a67e410808..ed1814588d56 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -441,7 +441,7 @@ static ssize_t smk_parse_long_rule(char *data, struct smack_parsed_rule *rule,
> * "subject<whitespace>object<whitespace>
> * acc_enable<whitespace>acc_disable[<whitespace>...]"
> */
> -static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
> +ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos,
> struct list_head *rule_list,
> struct mutex *rule_lock, int format)
^ permalink raw reply
* Re: [PATCH RFC 5/15] LSM: Single calls in secid hooks
From: Casey Schaufler @ 2025-11-04 16:00 UTC (permalink / raw)
To: Paul Moore, eparis, linux-security-module, audit
Cc: jmorris, serge, keescook, john.johansen, penguin-kernel,
stephen.smalley.work, linux-kernel, selinux, Casey Schaufler
In-Reply-To: <ee015074a9019ef4725f7e613fd76f86@paul-moore.com>
On 10/14/2025 4:12 PM, Paul Moore wrote:
> On Jun 21, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote:
>> security_socket_getpeersec_stream(), security_socket_getpeersec_dgram()
>> and security_secctx_to_secid() can only provide a single security context
>> or secid to their callers. Open code these hooks to return the first
>> hook provided. Because only one "major" LSM is allowed there will only
>> be one hook in the list, with the excepton being BPF. BPF is not expected
>> to be using these interfaces.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> security/security.c | 24 ++++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/security/security.c b/security/security.c
>> index db85006d2fd5..2286285f8aea 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -3806,8 +3806,13 @@ EXPORT_SYMBOL(security_lsmprop_to_secctx);
>> */
>> int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>> {
>> + struct lsm_static_call *scall;
>> +
>> *secid = 0;
>> - return call_int_hook(secctx_to_secid, secdata, seclen, secid);
>> + lsm_for_each_hook(scall, secctx_to_secid) {
>> + return scall->hl->hook.secctx_to_secid(secdata, seclen, secid);
>> + }
>> + return LSM_RET_DEFAULT(secctx_to_secid);
>> }
>> EXPORT_SYMBOL(security_secctx_to_secid);
> Two thoughts come to mind:
>
> If we are relying on BPF not using these hooks we should remove the BPF
> callback. It looks like the secctx_to_secid and socket_getpeersec_stream
> callbacks are already absent from the BPF LSM, so it's just a matter of
> working with the BPF folks to see if socket_getpeersec_dgram can be
> removed. If it can't be removed, you'll need to find another solution.
That should be doable. If BPF decides they want to use lsm_prop data
they already have a passel of work to do, and I see that they have
already suggested removing the BPF data from lsm_prop.
The socket_getpeersec_dgram interface uses secids, not lsm_prop, but
that's an artifact of networking attitude, not what's "right" for it.
> Instead of opening up the call_int_hook() wrapper here, what would it
> look like if we enforced the single callback rule at LSM registration
> time?
I have considered that approach in the past. It would require that
security_add_hooks() know which hooks are single callback and only
call lsm_static_call_init() if no LSM had requested the hook before.
This would be fairly straight forward and have the advantage of allowing
the infrastructure to report which single callback hooks have been
chosen and which disallowed. It does raise the question of whether the
LSM that requested the hook should be notified in the case it was
discarded. That's messy, as there are multiple single callback hooks,
and you could have a case where some are chosen and others disallowed.
I would go without notification, as it's hard to say what an LSM would
do with that information.
I'll give it a go in the next version.
>> @@ -4268,8 +4273,13 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>> int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
>> sockptr_t optlen, unsigned int len)
>> {
>> - return call_int_hook(socket_getpeersec_stream, sock, optval, optlen,
>> - len);
>> + struct lsm_static_call *scall;
>> +
>> + lsm_for_each_hook(scall, socket_getpeersec_stream) {
>> + return scall->hl->hook.socket_getpeersec_stream(sock, optval,
>> + optlen, len);
>> + }
>> + return LSM_RET_DEFAULT(socket_getpeersec_stream);
>> }
>>
>> /**
>> @@ -4289,7 +4299,13 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
>> int security_socket_getpeersec_dgram(struct socket *sock,
>> struct sk_buff *skb, u32 *secid)
>> {
>> - return call_int_hook(socket_getpeersec_dgram, sock, skb, secid);
>> + struct lsm_static_call *scall;
>> +
>> + lsm_for_each_hook(scall, socket_getpeersec_dgram) {
>> + return scall->hl->hook.socket_getpeersec_dgram(sock, skb,
>> + secid);
>> + }
>> + return LSM_RET_DEFAULT(socket_getpeersec_dgram);
>> }
>> EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>>
>> --
>> 2.47.0
> --
> paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/2] LSM: Exclusive secmark usage
From: Casey Schaufler @ 2025-11-04 16:41 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, jmorris, serge, keescook, john.johansen,
penguin-kernel, stephen.smalley.work, linux-kernel, selinux,
Casey Schaufler
In-Reply-To: <CAHC9VhTJBSejFr78csXudG4xKW5hXVy3undDP-m8YdjhJLYrYA@mail.gmail.com>
On 10/13/2025 2:57 PM, Paul Moore wrote:
> On Wed, Oct 1, 2025 at 5:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> The network secmark can only be used by one security module
>> at a time. Establish mechanism to identify to security modules
>> whether they have access to the secmark. SELinux already
>> incorparates mechanism, but it has to be added to Smack and
>> AppArmor.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> include/linux/lsm_hooks.h | 1 +
>> security/apparmor/include/net.h | 5 +++++
>> security/apparmor/lsm.c | 7 ++++---
>> security/security.c | 6 ++++++
>> security/selinux/hooks.c | 4 +++-
>> security/smack/smack.h | 5 +++++
>> security/smack/smack_lsm.c | 3 ++-
>> security/smack/smack_netfilter.c | 10 ++++++++--
>> 8 files changed, 34 insertions(+), 7 deletions(-)
> ..
>
>> /* Prepare LSM for initialization. */
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index c95a5874bf7d..5b6db7d8effb 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -164,7 +164,8 @@ __setup("checkreqprot=", checkreqprot_setup);
>> */
>> static int selinux_secmark_enabled(void)
>> {
>> - return (selinux_policycap_alwaysnetwork() ||
>> + return selinux_blob_sizes.lbs_secmark &&
>> + (selinux_policycap_alwaysnetwork() ||
>> atomic_read(&selinux_secmark_refcount));
>> }
> This is an odd way to approach secmark enablement in SELinux, and not
> something I think I want to see. Ignoring the
> selinux_policycap_alwaysnetwork "abomination" (a joke I think only
> about four people in the world might understand), the
> selinux_secmark_enabled() function is really there simply as a
> performance optimization since the majority of SELinux users don't
> utilize the per-packet access controls. Using it as a mechanism to
> effectively turn off SELinux's secmark functionality could result in a
> confusing situation for users who are setting SELinux secmarks on
> packets and not seeing the system's policy properly enforced.
One could argue that a user who creates a system that would have this
problem has configured it incorrectly. A system with Smack before SELinux
( https://lwn.net/Articles/645245/ ) would, by the "first LSM gets it"
rule, give the secmark to Smack. If the user wants SELinux to use secmarks
SELinux must precede Smack. The SELinux policy, as well as the Smack rule
set, are going to have to be correct for the configuration, as are any
netfilter rules. Yes, that's likely to make some sysadmin's heads explode.
Complex configurations are admittedly difficult to get right. When you
start with a system that isn't simple and add to it you can help but run
into situations that are baffling.
You can create a correctly behaving system with the "first LSM" behavior.
You can also create a system that goes completely wonky. Just as you want
a well trained developer creating your SELinux policy, you want someone
who knows what they're doing composing LSM stacks.
This is going to be an issue for other features, including audit rules and IMA.
^ permalink raw reply
* Re: [PATCH 1/2] LSM: Exclusive secmark usage
From: Casey Schaufler @ 2025-11-04 16:58 UTC (permalink / raw)
To: Paul Moore
Cc: Stephen Smalley, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, linux-kernel, selinux,
Casey Schaufler
In-Reply-To: <CAHC9VhTr_0XtFfp017vXmaQVm77kkN+4ZqxNqNnBOW6MpFQqkg@mail.gmail.com>
On 10/13/2025 3:11 PM, Paul Moore wrote:
> On Fri, Oct 10, 2025 at 11:03 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/9/2025 11:49 AM, Stephen Smalley wrote:
>>> On Wed, Oct 1, 2025 at 5:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> The network secmark can only be used by one security module
>>>> at a time. Establish mechanism to identify to security modules
>>> a mechanism to inform security modules?
>>>
>>>> whether they have access to the secmark. SELinux already
>>>> incorparates mechanism, but it has to be added to Smack and
>>> incorporates
>>>
>>>> AppArmor.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>> include/linux/lsm_hooks.h | 1 +
>>>> security/apparmor/include/net.h | 5 +++++
>>>> security/apparmor/lsm.c | 7 ++++---
>>>> security/security.c | 6 ++++++
>>>> security/selinux/hooks.c | 4 +++-
>>>> security/smack/smack.h | 5 +++++
>>>> security/smack/smack_lsm.c | 3 ++-
>>>> security/smack/smack_netfilter.c | 10 ++++++++--
>>>> 8 files changed, 34 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/security/security.c b/security/security.c
>>>> index ad163f06bf7a..e59e3d403de6 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -283,6 +283,12 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>>>> lsm_set_blob_size(&needed->lbs_xattr_count,
>>>> &blob_sizes.lbs_xattr_count);
>>>> lsm_set_blob_size(&needed->lbs_bdev, &blob_sizes.lbs_bdev);
>>>> + if (needed->lbs_secmark) {
>>>> + if (blob_sizes.lbs_secmark)
>>>> + needed->lbs_secmark = false;
>>>> + else
>>>> + blob_sizes.lbs_secmark = true;
>>>> + }
>>> So if I understand correctly, the first LSM to register with
>>> lbs_secmark set wins.
>>> Not sure that's a great idea - seemingly some LSMs may want to insist
>>> that they get to use secmark regardless of registration order?
>> But what if two LSMs insist on getting the secmark? The whole point
>> is to make it possible to use multiple LSMs that what the feature at
>> the same time.
> My current thinking is that if two LSMs *insist* on access to the
> secmark, one of them has to fail to load/initialize, even if that
> means a panic on boot (we should flag that as an invalid config in
> Kconfig).
That's sensible, but why should an LSM be allowed to insist on access
to the secmark? Best I can tell, SELinux rarely uses it in real life.
Smack currently always uses it, but that's fixed in this patch set.
I would be perplexed by a "dog in the manger" attitude on the part of
any maintainers.
>
> Perhaps the solution is to have lbs_secmark as a tristate value: don't
> use secmarks, would like access to secmarks, must have access to
> secmarks. Upon registration a LSM that requested "would like" could
> check to see if they have been granted access and could adjust
> accordingly. A LSM that requested "must have" would fail to register
> if the secmarks were already taken by a prior LSM.
I would be unhappy if any existing LSM decided it "must have" secmarks.
I can imagine a LSM that really required the secmark, but it would have
a tough time getting accepted.
>> The limitation on a secmark being a u32 is a huge problem,
>> and Paul has battled with the netdev people over it for years.
> I suspect the only way forward at this point is to convert the secmark
> field into an IDR* that we could use to point to a LSM security blob
> that could store LSM specific structs for both secmarks and general
> LSM state associated with a skb. This would also allow us to do some
> cool things in the forward path that we can't properly do now and
> would make it much easier to handle a wider variety of packet level
> access control scenarios.
>
> It's on my todo list for <hand_waving>someday</hand_waving>, but if
> somebody wanted to do it that would be awesome. Just a word of
> warning, this is not a quick task and it is probably only suited for
> someone who already has a few netdev inflicted scars.
I expect to be dead, or at least suffering serious memory loss,
by the time that work can be done. :(
>
> *I see that IDR is now deprecated in favor of XArray, I haven't looked
> that closely at XArray but it looks workable too.
>
^ 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