* 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
* Re: [PATCH 2/2] LSM: Allow reservation of netlabel
From: Casey Schaufler @ 2025-11-04 17:07 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: <CAHC9VhSBxhiTvxPpHHqZJygDTTuMWOPFpQcoMSsvZD6Bueg0ZQ@mail.gmail.com>
On 10/13/2025 3:21 PM, Paul Moore wrote:
> On Fri, Oct 10, 2025 at 5:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/10/2025 12:53 PM, Stephen Smalley wrote:
>>> On Fri, Oct 10, 2025 at 11:09 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 10/9/2025 11:53 AM, Stephen Smalley wrote:
>>>>> On Wed, Oct 1, 2025 at 5:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> ..
>
>>> But some security modules may not function correctly (or at all) if
>>> secmark and/or netlabel are silently disabled on them, and the end
>>> user needs a better way to express intent.
> This is the point I was trying to make in patch 1/2 with secmarks, but
> Stephen has captured the idea much better in the sentence above. To
> be clear, the argument applies to both secmarks and NetLabel.
>
>> I'm open to suggestions. Would boot options lsm.secmark and lsm.netlabel
>> be sufficient to address your concern?
> No. Please no. We already have two LSM initialization related
> command line parameters, and one of them is pretty broken and very
> confusing in the new world of multiple LSMs (as an aside, does someone
> want to kick off the work to deprecate "security=?"). Maybe we have
> to go this route eventually, but let's keep it simple for right now; I
> don't want to add a lot of user-visible APIs for something that is
> pretty niche.
>
> If you absolutely can't live with the "first one gets it" approach,
> look at the no/wants/must idea in my patch 1/2 comments. It would
> require work in the individual LSMs to support it, but I'd rather try
> that route first.
I'm fine (for now, at least) with the "first LSM" approach, which is
what I have implemented. What I *am* afraid of is SELinux deciding that
it can only ever possibly work if it is the "first LSM". Best I can tell,
there's no reason for it beyond "configuration is hard". Which it is,
but we're already there.
^ permalink raw reply
* Re: [PATCH 2/2] LSM: Allow reservation of netlabel
From: Casey Schaufler @ 2025-11-04 17:01 UTC (permalink / raw)
To: Stephen Smalley
Cc: paul, linux-security-module, jmorris, serge, keescook,
john.johansen, penguin-kernel, linux-kernel, selinux,
Casey Schaufler
In-Reply-To: <CAEjxPJ48PiZ5ZOZbZjka5YeiBxaWFsCufoGcY_jEztM+wtEUCA@mail.gmail.com>
On 10/9/2025 11:53 AM, Stephen Smalley wrote:
> On Wed, Oct 1, 2025 at 5:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Allow LSMs to request exclusive access to the netlabel facility.
>> Provide mechanism for LSMs to determine if they have access to
>> netlabel. Update the current users of netlabel, SELinux and Smack,
>> to use and respect the exclusive use of netlabel.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> diff --git a/security/security.c b/security/security.c
>> index e59e3d403de6..9eca10844b56 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -289,6 +289,12 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>> else
>> blob_sizes.lbs_secmark = true;
>> }
>> + if (needed->lbs_netlabel) {
>> + if (blob_sizes.lbs_netlabel)
>> + needed->lbs_netlabel = false;
>> + else
>> + blob_sizes.lbs_netlabel = true;
>> +
> Same principle here - if a LSM wants to use netlabel, it may want to
> guarantee that it truly has exclusive access to it no matter what the
> LSM order is.
Again, SELinux doesn't actually use this very often. Declaring that SELinux
always wants it to the exclusion of others would be obstructionist.
^ permalink raw reply
* Re: [PATCH v2 2/2] LSM: Infrastructure management of the mnt_opts security blob
From: Casey Schaufler @ 2025-11-04 17:46 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley
Cc: Ondrej Mosnacek, eparis, linux-security-module, jmorris, serge,
keescook, john.johansen, penguin-kernel, linux-kernel, selinux,
Casey Schaufler
In-Reply-To: <CAHC9VhSRGyMuTYxP0nDpXv_MwvNqVsrBXcak84AGHj7ycDtu3A@mail.gmail.com>
On 10/13/2025 1:55 PM, Paul Moore wrote:
> On Thu, Oct 9, 2025 at 2:38 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Thu, Sep 25, 2025 at 1:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> Move management of the mnt_opts->security blob out of the individual
>>> security modules and into the security infrastructure. The modules
>>> tell the infrastructure how much space is required, and the space is
>>> allocated as required in the interfaces that use the blob.
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> ---
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 4bba9d119713..1ccf880e4894 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -656,19 +651,13 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>>> mutex_lock(&sbsec->lock);
>>>
>>> if (!selinux_initialized()) {
>>> - if (!opts) {
>>> - /* Defer initialization until selinux_complete_init,
>>> - after the initial policy is loaded and the security
>>> - server is ready to handle calls. */
>>> - if (kern_flags & SECURITY_LSM_NATIVE_LABELS) {
>>> - sbsec->flags |= SE_SBNATIVE;
>>> - *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>>> - }
>>> - goto out;
>>> + /* Defer initialization until selinux_complete_init,
>>> + after the initial policy is loaded and the security
>>> + server is ready to handle calls. */
>>> + if (kern_flags & SECURITY_LSM_NATIVE_LABELS) {
>>> + sbsec->flags |= SE_SBNATIVE;
>>> + *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>> This seemingly would produce a change in behavior for SELinux.
Except that it doesn't, at least from the tests I've been able to find.
If multiple LSMs use mount options you can't use the !opts test, because
there may be options for another LSM. Deferring initialization is harmless
when there are options, as it's all checked again later.
>> Previously we would only do this if there were no SELinux mount
>> options specified.
> What Stephen said. I think this is good work that needs to be done
> (thank you for doing it!), but we have to preserve existing behaviors.
>
^ permalink raw reply
* Re: [PATCH v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Coiby Xu @ 2025-11-05 0:18 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar
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: <CAHC9VhToe-VNqbh6TY2iYnRvqTHRfQjnHYSRWYgt8K7NcLKMdg@mail.gmail.com>
On Sun, Nov 02, 2025 at 10:43:04AM -0500, Paul Moore wrote:
>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?
Hi Paul and Mimi,
Thanks for sharing your feedback! Yes, you are right, there is no need
for a new LSM hook. Actually by not introducing a new LSM hook, we can
have a much simpler solution!
>
>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 ==
By moving security_kernel_post_read_file, I think what Mimi means is to
move security_kernel_post_read_file in init_module_from_file() to later
in the function,
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b261849362a..66725e53fef0c1 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3678,6 +3678,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
struct load_info info = { };
void *buf = NULL;
int len;
+ int err;
len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
if (len < 0) {
@@ -3686,7 +3687,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
}
if (flags & MODULE_INIT_COMPRESSED_FILE) {
- int err = module_decompress(&info, buf, len);
+ err = module_decompress(&info, buf, len);
vfree(buf); /* compressed data is no longer needed */
if (err) {
mod_stat_inc(&failed_decompress);
@@ -3698,6 +3699,14 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
info.len = len;
}
+ err = security_kernel_post_read_file(f, (char *)info.hdr, info.len,
+ READING_MODULE);
+ if (err) {
+ mod_stat_inc(&failed_kreads);
+ free_copy(&info, flags);
+ return err;
+ }
+
return load_module(&info, uargs, flags);
}
If we only call security_kernel_post_read_file the 2nd time for a
decompressed kernel module, IMA won't be sure what to do when
security_kernel_post_read_file is called for the 1st time because it
can't distinguish between a compressed module with appended signature or
a uncompressed module without appended signature. If it permits 1st
calling security_kernel_post_read_file, a uncompressed module without
appended signature can be loaded. If it doesn't permit 1st calling
security_kernel_post_read_file, there is no change to call
security_kernel_post_read_file again for decompressed module.
And you are right, there is no need to call
security_kernel_post_read_file twice. And from the perspective of IMA,
it simplifies reasoning if it is guaranteed that IMA will always access
uncompressed kernel module regardless regardless of its original
compression state.
So I think a better solution is to stop calling
security_kernel_post_read_file in kernel_read_file for READING_MODULE.
This can also avoiding introducing an unnecessary
READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration and
can make the solution even simpler,
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index de32c95d823dbd..7c78e84def6ec7 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -107,7 +107,12 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf,
goto out_free;
}
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ /*
+ * security_kernel_post_read_file will be called later after
+ * a read kernel module is truly decompressed
+ */
+ if (id != READING_MODULE)
+ ret = security_kernel_post_read_file(file, *buf, i_size, id);
}
Btw, I notice IMA is the only user of security_kernel_post_read_file so
this change won't affect other LSMs. For a full patch, please visit
https://github.com/coiby/linux/commit/558d85779ab5d794874749ecfae0e48b890bf3e0.patch
If there are concerns that I'm unaware of and a new
READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration is
necessary, here's another patch
https://github.com/coiby/linux/commit/cdd40317b6070f48ec871c6a89428084f38ca083.patch
>
>--
>paul-moore.com
>
--
Best regards,
Coiby
^ 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-05 2:47 UTC (permalink / raw)
To: Coiby Xu
Cc: Mimi Zohar, 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: <fftfj4o3kqxmfu3hb655xczqcddoeqjv55llsnwkrdu5isdm4z@6sqe3k24a6kk>
On Tue, Nov 4, 2025 at 7:19 PM Coiby Xu <coxu@redhat.com> wrote:
> On Sun, Nov 02, 2025 at 10:43:04AM -0500, Paul Moore wrote:
> >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?
>
> Hi Paul and Mimi,
>
> Thanks for sharing your feedback! Yes, you are right, there is no need
> for a new LSM hook. Actually by not introducing a new LSM hook, we can
> have a much simpler solution!
>
> >
> >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 ==
>
> By moving security_kernel_post_read_file, I think what Mimi means is to
> move security_kernel_post_read_file in init_module_from_file() to later
> in the function,
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362a..66725e53fef0c1 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3678,6 +3678,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
> struct load_info info = { };
> void *buf = NULL;
> int len;
> + int err;
>
> len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> if (len < 0) {
> @@ -3686,7 +3687,7 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
> }
>
> if (flags & MODULE_INIT_COMPRESSED_FILE) {
> - int err = module_decompress(&info, buf, len);
> + err = module_decompress(&info, buf, len);
> vfree(buf); /* compressed data is no longer needed */
> if (err) {
> mod_stat_inc(&failed_decompress);
> @@ -3698,6 +3699,14 @@ static int init_module_from_file(struct file *f, const char __user * uargs, int
> info.len = len;
> }
>
> + err = security_kernel_post_read_file(f, (char *)info.hdr, info.len,
> + READING_MODULE);
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + free_copy(&info, flags);
> + return err;
> + }
> +
> return load_module(&info, uargs, flags);
> }
>
> If we only call security_kernel_post_read_file the 2nd time for a
> decompressed kernel module, IMA won't be sure what to do when
> security_kernel_post_read_file is called for the 1st time because it
> can't distinguish between a compressed module with appended signature or
> a uncompressed module without appended signature. If it permits 1st
> calling security_kernel_post_read_file, a uncompressed module without
> appended signature can be loaded. If it doesn't permit 1st calling
> security_kernel_post_read_file, there is no change to call
> security_kernel_post_read_file again for decompressed module.
>
> And you are right, there is no need to call
> security_kernel_post_read_file twice. And from the perspective of IMA,
> it simplifies reasoning if it is guaranteed that IMA will always access
> uncompressed kernel module regardless regardless of its original
> compression state.
>
> So I think a better solution is to stop calling
> security_kernel_post_read_file in kernel_read_file for READING_MODULE.
> This can also avoiding introducing an unnecessary
> READING_MODULE_UNCOMPRESSED/READING_COMPRESSED_MODULE enumeration and
> can make the solution even simpler,
>
> diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
> index de32c95d823dbd..7c78e84def6ec7 100644
> --- a/fs/kernel_read_file.c
> +++ b/fs/kernel_read_file.c
> @@ -107,7 +107,12 @@ ssize_t kernel_read_file(struct file *file, loff_t offset, void **buf,
> goto out_free;
> }
>
> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
> + /*
> + * security_kernel_post_read_file will be called later after
> + * a read kernel module is truly decompressed
> + */
> + if (id != READING_MODULE)
> + ret = security_kernel_post_read_file(file, *buf, i_size, id);
> }
Assuming I'm understanding the problem correctly, I think you're
making this harder than it needs to be. I believe something like this
should solve the problem without having to add more conditionals
around the hooks in kernel_read_file(), and limiting the multiple
security_kernel_post_read_file() calls to just the compressed case ...
and honestly in each of the _post_read_file() calls in the compressed
case, the buffer contents have changed so it somewhat makes sense.
Given the code below, IMA could simply ignore the
READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
in that case) and focus on the READING_MODULE case as it does today.
I expect the associated IMA patch would be both trivial and small.
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b26184936..b435c498ec01 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
ent *u)
static int init_module_from_file(struct file *f, const char __user * uargs, int
flags)
{
+ bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
struct load_info info = { };
void *buf = NULL;
int len;
- len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
+ len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
+ compressed ? READING_MODULE_COMPRESSED : READING_
MODULE);
if (len < 0) {
mod_stat_inc(&failed_kreads);
return len;
}
- if (flags & MODULE_INIT_COMPRESSED_FILE) {
+ if (compressed) {
int err = module_decompress(&info, buf, len);
vfree(buf); /* compressed data is no longer needed */
if (err) {
@@ -3693,6 +3695,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);
+ 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 v2] lsm,ima: new LSM hook security_kernel_module_read_file to access decompressed kernel module
From: Mimi Zohar @ 2025-11-05 14:07 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: <CAHC9VhRGwXvhU64Nk5jdmtPfrt9bbkzpLVqS0LRbtN3Q3HhnCw@mail.gmail.com>
On Tue, 2025-11-04 at 21:47 -0500, Paul Moore wrote:
> Assuming I'm understanding the problem correctly, I think you're
> making this harder than it needs to be. I believe something like this
> should solve the problem without having to add more conditionals
> around the hooks in kernel_read_file(), and limiting the multiple
> security_kernel_post_read_file() calls to just the compressed case ...
> and honestly in each of the _post_read_file() calls in the compressed
> case, the buffer contents have changed so it somewhat makes sense.
> Given the code below, IMA could simply ignore the
> READING_MODULE_COMPRESSED case (or whatever it is the IMA needs to do
> in that case) and focus on the READING_MODULE case as it does today.
> I expect the associated IMA patch would be both trivial and small.
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..b435c498ec01 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3675,17 +3675,19 @@ static int idempotent_wait_for_completion(struct idempot
> ent *u)
>
> static int init_module_from_file(struct file *f, const char __user * uargs, int
> flags)
> {
> + bool compressed = !!(flags & MODULE_INIT_COMPRESSED_FILE);
> struct load_info info = { };
> void *buf = NULL;
> int len;
>
> - len = kernel_read_file(f, 0, &buf, INT_MAX, NULL, READING_MODULE);
> + len = kernel_read_file(f, 0, &buf, INT_MAX, NULL,
> + compressed ? READING_MODULE_COMPRESSED : READING_
> MODULE);
> if (len < 0) {
> mod_stat_inc(&failed_kreads);
> return len;
> }
>
> - if (flags & MODULE_INIT_COMPRESSED_FILE) {
> + if (compressed) {
> int err = module_decompress(&info, buf, len);
> vfree(buf); /* compressed data is no longer needed */
> if (err) {
> @@ -3693,6 +3695,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);
Without changing the enumeration here, IMA would not be able to differentiate
the first call to security_kernel_post_read_file() and this one. The first call
would result in unnecessary error messages.
Adding an additional call to security_kernel_post_read_file() here, would
require defining 2 additional enumerations: READING_MODULE_COMPRESSED,
READING_MODULE_DECOMPRESSED.
> + if (err) {
> + mod_stat_inc(&failed_kreads);
> + return err;
> + }
> } else {
> info.hdr = buf;
> info.len = len;
Deferring the security_kernel_post_read_file() call to here, eliminates the need
for defining additional enumerations. (Coiby's first link.)
Adding an additional call to security_kernel_post_read_file() here, requires 1
additional enumeration. (Coiby's 2nd link.)
Mimi
^ 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