| Message ID | 20250409185019.238841-39-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rework the LSM initialization | expand |
On Wed, Apr 09, 2025 at 02:49:53PM -0400, Paul Moore wrote: > The LSM currently has a lot of code to maintain a list of the > currently active LSMs in a human readable string, with the only > user being the "/sys/kernel/security/lsm" code. Let's drop all > of that code and generate the string on an as-needed basis when > userspace reads "/sys/kernel/security/lsm". > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > include/linux/lsm_hooks.h | 1 - > security/inode.c | 27 +++++++++++++++++++-- > security/lsm_init.c | 49 --------------------------------------- > 3 files changed, 25 insertions(+), 52 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 4cd17c9a229f..bc477fb20d02 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -169,7 +169,6 @@ struct lsm_info { > > > /* DO NOT tamper with these variables outside of the LSM framework */ > -extern char *lsm_names; > extern struct lsm_static_calls_table static_calls_table __ro_after_init; > > /** > diff --git a/security/inode.c b/security/inode.c > index da3ab44c8e57..49bc3578bd23 100644 > --- a/security/inode.c > +++ b/security/inode.c > @@ -22,6 +22,8 @@ > #include <linux/lsm_hooks.h> > #include <linux/magic.h> > > +#include "lsm.h" > + > static struct vfsmount *mount; > static int mount_count; > > @@ -343,8 +345,29 @@ static struct dentry *lsm_dentry; > static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, > loff_t *ppos) > { > - return simple_read_from_buffer(buf, count, ppos, lsm_names, > - strlen(lsm_names)); > + int i; > + char *str; > + ssize_t rc, len = 0; > + > + for (i = 0; i < lsm_count; i++) > + /* the '+ 1' accounts for either a comma or a NUL terminator */ > + len += strlen(lsm_order[i]->id->name) + 1; > + > + str = kmalloc(len, GFP_KERNEL); > + if (!str) > + return -ENOMEM; > + str[0] = '\0'; > + > + i = 0; > + while (i < lsm_count) { > + strcat(str, lsm_order[i]->id->name); > + if (++i < lsm_count) > + strcat(str, ","); > + } > + > + rc = simple_read_from_buffer(buf, count, ppos, str, len); > + kfree(str); > + return rc; Hrm, at least cache it? static char *lsm_names; static ssize_t lsm_names_len; static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, loff_t *ppos) { if (!lsm_names) { int i; char *str; ssize_t len = 0; ... lsm_names = str; } return simple_read_from_buffer(buf, count, ppos, lsm_names, lsm_names_len); } Better yet, do this whole thing in a initcall after LSMs are loaded, and both can gain __ro_after_init... > > static const struct file_operations lsm_ops = { > diff --git a/security/lsm_init.c b/security/lsm_init.c > index 981ddb20f48e..978bb81b58fa 100644 > --- a/security/lsm_init.c > +++ b/security/lsm_init.c > @@ -10,8 +10,6 @@ > > #include "lsm.h" > > -char *lsm_names; > - > /* Pointers to LSM sections defined in include/asm-generic/vmlinux.lds.h */ > extern struct lsm_info __start_lsm_info[], __end_lsm_info[]; > extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; > @@ -363,42 +361,6 @@ static void __init lsm_init_ordered(void) > } > } > > -static bool match_last_lsm(const char *list, const char *lsm) > -{ > - const char *last; > - > - if (WARN_ON(!list || !lsm)) > - return false; > - last = strrchr(list, ','); > - if (last) > - /* Pass the comma, strcmp() will check for '\0' */ > - last++; > - else > - last = list; > - return !strcmp(last, lsm); > -} > - > -static int lsm_append(const char *new, char **result) > -{ > - char *cp; > - > - if (*result == NULL) { > - *result = kstrdup(new, GFP_KERNEL); > - if (*result == NULL) > - return -ENOMEM; > - } else { > - /* Check if it is the last registered name */ > - if (match_last_lsm(*result, new)) > - return 0; > - cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new); > - if (cp == NULL) > - return -ENOMEM; > - kfree(*result); > - *result = cp; > - } > - return 0; > -} > - > static void __init lsm_static_call_init(struct security_hook_list *hl) > { > struct lsm_static_call *scall = hl->scalls; > @@ -435,15 +397,6 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, > hooks[i].lsmid = lsmid; > lsm_static_call_init(&hooks[i]); > } > - > - /* > - * Don't try to append during early_security_init(), we'll come back > - * and fix this up afterwards. > - */ > - if (slab_is_available()) { > - if (lsm_append(lsmid->name, &lsm_names) < 0) > - panic("%s - Cannot get early memory.\n", __func__); > - } > } > > int __init early_security_init(void) > @@ -480,8 +433,6 @@ int __init security_init(void) > lsm_early_for_each_raw(lsm) { > init_debug(" early started: %s (%s)\n", lsm->id->name, > is_enabled(lsm) ? "enabled" : "disabled"); > - if (lsm->enabled) > - lsm_append(lsm->id->name, &lsm_names); > } > > /* Load LSMs in specified order. */ > -- > 2.49.0 >
On Wed, Apr 9, 2025 at 7:13 PM Kees Cook <kees@kernel.org> wrote: > > On Wed, Apr 09, 2025 at 02:49:53PM -0400, Paul Moore wrote: > > The LSM currently has a lot of code to maintain a list of the > > currently active LSMs in a human readable string, with the only > > user being the "/sys/kernel/security/lsm" code. Let's drop all > > of that code and generate the string on an as-needed basis when > > userspace reads "/sys/kernel/security/lsm". > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > include/linux/lsm_hooks.h | 1 - > > security/inode.c | 27 +++++++++++++++++++-- > > security/lsm_init.c | 49 --------------------------------------- > > 3 files changed, 25 insertions(+), 52 deletions(-) ... > > @@ -343,8 +345,29 @@ static struct dentry *lsm_dentry; > > static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, > > loff_t *ppos) > > { > > - return simple_read_from_buffer(buf, count, ppos, lsm_names, > > - strlen(lsm_names)); > > + int i; > > + char *str; > > + ssize_t rc, len = 0; > > + > > + for (i = 0; i < lsm_count; i++) > > + /* the '+ 1' accounts for either a comma or a NUL terminator */ > > + len += strlen(lsm_order[i]->id->name) + 1; > > + > > + str = kmalloc(len, GFP_KERNEL); > > + if (!str) > > + return -ENOMEM; > > + str[0] = '\0'; > > + > > + i = 0; > > + while (i < lsm_count) { > > + strcat(str, lsm_order[i]->id->name); > > + if (++i < lsm_count) > > + strcat(str, ","); > > + } > > + > > + rc = simple_read_from_buffer(buf, count, ppos, str, len); > > + kfree(str); > > + return rc; > > Hrm, at least cache it? Are you aware of a performance critical use of this? > Better yet, do this whole thing in a initcall after LSMs are loaded, and > both can gain __ro_after_init... I *really* disliked all the stuff we were having to do during boot, and all the redundant global state we were keeping around. I'll go ahead and cache the lsm_read() result local to the function but that's probably all I'm going to accept at this point in time.
On Thu, Apr 10, 2025 at 06:47:12PM -0400, Paul Moore wrote: > On Wed, Apr 9, 2025 at 7:13 PM Kees Cook <kees@kernel.org> wrote: > > Better yet, do this whole thing in a initcall after LSMs are loaded, and > > both can gain __ro_after_init... > > I *really* disliked all the stuff we were having to do during boot, > and all the redundant global state we were keeping around. I'll go > ahead and cache the lsm_read() result local to the function but that's > probably all I'm going to accept at this point in time. Oh, for sure. I love that all that can get thrown away. I mean literally copy/paste what you have in lsm_read() and stick it immediately before the "lsms are done loading" notifier. Then it only needs to be done once, it's impossible to race, etc.
On Thu, Apr 10, 2025 at 10:15 PM Kees Cook <kees@kernel.org> wrote: > On Thu, Apr 10, 2025 at 06:47:12PM -0400, Paul Moore wrote: > > On Wed, Apr 9, 2025 at 7:13 PM Kees Cook <kees@kernel.org> wrote: > > > Better yet, do this whole thing in a initcall after LSMs are loaded, and > > > both can gain __ro_after_init... > > > > I *really* disliked all the stuff we were having to do during boot, > > and all the redundant global state we were keeping around. I'll go > > ahead and cache the lsm_read() result local to the function but that's > > probably all I'm going to accept at this point in time. > > Oh, for sure. I love that all that can get thrown away. I mean literally > copy/paste what you have in lsm_read() and stick it immediately before > the "lsms are done loading" notifier. Then it only needs to be done > once, it's impossible to race, etc. Maybe I'll change my mind at some point, but right now I'm feeling pretty strongly against generating the list string at boot. I've added a basic cache protected by a dumb spinlock in lsm_read which should work.
On 4/10/25 15:47, Paul Moore wrote: > On Wed, Apr 9, 2025 at 7:13 PM Kees Cook <kees@kernel.org> wrote: >> >> On Wed, Apr 09, 2025 at 02:49:53PM -0400, Paul Moore wrote: >>> The LSM currently has a lot of code to maintain a list of the >>> currently active LSMs in a human readable string, with the only >>> user being the "/sys/kernel/security/lsm" code. Let's drop all >>> of that code and generate the string on an as-needed basis when >>> userspace reads "/sys/kernel/security/lsm". >>> >>> Signed-off-by: Paul Moore <paul@paul-moore.com> >>> --- >>> include/linux/lsm_hooks.h | 1 - >>> security/inode.c | 27 +++++++++++++++++++-- >>> security/lsm_init.c | 49 --------------------------------------- >>> 3 files changed, 25 insertions(+), 52 deletions(-) > > ... > >>> @@ -343,8 +345,29 @@ static struct dentry *lsm_dentry; >>> static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, >>> loff_t *ppos) >>> { >>> - return simple_read_from_buffer(buf, count, ppos, lsm_names, >>> - strlen(lsm_names)); >>> + int i; >>> + char *str; >>> + ssize_t rc, len = 0; >>> + >>> + for (i = 0; i < lsm_count; i++) >>> + /* the '+ 1' accounts for either a comma or a NUL terminator */ >>> + len += strlen(lsm_order[i]->id->name) + 1; >>> + >>> + str = kmalloc(len, GFP_KERNEL); >>> + if (!str) >>> + return -ENOMEM; >>> + str[0] = '\0'; >>> + >>> + i = 0; >>> + while (i < lsm_count) { >>> + strcat(str, lsm_order[i]->id->name); >>> + if (++i < lsm_count) >>> + strcat(str, ","); >>> + } >>> + >>> + rc = simple_read_from_buffer(buf, count, ppos, str, len); >>> + kfree(str); >>> + return rc; >> >> Hrm, at least cache it? > > Are you aware of a performance critical use of this? > no I can't see anything performance critical, I think it just is cleaner to only generate once if after init the list doesn't change. >> Better yet, do this whole thing in a initcall after LSMs are loaded, and >> both can gain __ro_after_init... > > I *really* disliked all the stuff we were having to do during boot, > and all the redundant global state we were keeping around. I'll go > ahead and cache the lsm_read() result local to the function but that's > probably all I'm going to accept at this point in time. > fair, I don't even think this needs to be changed, I think kees's suggestion is more of a nice to have
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4cd17c9a229f..bc477fb20d02 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -169,7 +169,6 @@ struct lsm_info { /* DO NOT tamper with these variables outside of the LSM framework */ -extern char *lsm_names; extern struct lsm_static_calls_table static_calls_table __ro_after_init; /** diff --git a/security/inode.c b/security/inode.c index da3ab44c8e57..49bc3578bd23 100644 --- a/security/inode.c +++ b/security/inode.c @@ -22,6 +22,8 @@ #include <linux/lsm_hooks.h> #include <linux/magic.h> +#include "lsm.h" + static struct vfsmount *mount; static int mount_count; @@ -343,8 +345,29 @@ static struct dentry *lsm_dentry; static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count, loff_t *ppos) { - return simple_read_from_buffer(buf, count, ppos, lsm_names, - strlen(lsm_names)); + int i; + char *str; + ssize_t rc, len = 0; + + for (i = 0; i < lsm_count; i++) + /* the '+ 1' accounts for either a comma or a NUL terminator */ + len += strlen(lsm_order[i]->id->name) + 1; + + str = kmalloc(len, GFP_KERNEL); + if (!str) + return -ENOMEM; + str[0] = '\0'; + + i = 0; + while (i < lsm_count) { + strcat(str, lsm_order[i]->id->name); + if (++i < lsm_count) + strcat(str, ","); + } + + rc = simple_read_from_buffer(buf, count, ppos, str, len); + kfree(str); + return rc; } static const struct file_operations lsm_ops = { diff --git a/security/lsm_init.c b/security/lsm_init.c index 981ddb20f48e..978bb81b58fa 100644 --- a/security/lsm_init.c +++ b/security/lsm_init.c @@ -10,8 +10,6 @@ #include "lsm.h" -char *lsm_names; - /* Pointers to LSM sections defined in include/asm-generic/vmlinux.lds.h */ extern struct lsm_info __start_lsm_info[], __end_lsm_info[]; extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; @@ -363,42 +361,6 @@ static void __init lsm_init_ordered(void) } } -static bool match_last_lsm(const char *list, const char *lsm) -{ - const char *last; - - if (WARN_ON(!list || !lsm)) - return false; - last = strrchr(list, ','); - if (last) - /* Pass the comma, strcmp() will check for '\0' */ - last++; - else - last = list; - return !strcmp(last, lsm); -} - -static int lsm_append(const char *new, char **result) -{ - char *cp; - - if (*result == NULL) { - *result = kstrdup(new, GFP_KERNEL); - if (*result == NULL) - return -ENOMEM; - } else { - /* Check if it is the last registered name */ - if (match_last_lsm(*result, new)) - return 0; - cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new); - if (cp == NULL) - return -ENOMEM; - kfree(*result); - *result = cp; - } - return 0; -} - static void __init lsm_static_call_init(struct security_hook_list *hl) { struct lsm_static_call *scall = hl->scalls; @@ -435,15 +397,6 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, hooks[i].lsmid = lsmid; lsm_static_call_init(&hooks[i]); } - - /* - * Don't try to append during early_security_init(), we'll come back - * and fix this up afterwards. - */ - if (slab_is_available()) { - if (lsm_append(lsmid->name, &lsm_names) < 0) - panic("%s - Cannot get early memory.\n", __func__); - } } int __init early_security_init(void) @@ -480,8 +433,6 @@ int __init security_init(void) lsm_early_for_each_raw(lsm) { init_debug(" early started: %s (%s)\n", lsm->id->name, is_enabled(lsm) ? "enabled" : "disabled"); - if (lsm->enabled) - lsm_append(lsm->id->name, &lsm_names); } /* Load LSMs in specified order. */
The LSM currently has a lot of code to maintain a list of the currently active LSMs in a human readable string, with the only user being the "/sys/kernel/security/lsm" code. Let's drop all of that code and generate the string on an as-needed basis when userspace reads "/sys/kernel/security/lsm". Signed-off-by: Paul Moore <paul@paul-moore.com> --- include/linux/lsm_hooks.h | 1 - security/inode.c | 27 +++++++++++++++++++-- security/lsm_init.c | 49 --------------------------------------- 3 files changed, 25 insertions(+), 52 deletions(-)