lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHC9VhTScG513+-_GDN+nzBQjASW31LrE8juU3c03=0fJ_csGw@mail.gmail.com> Date: Tue, 18 Oct 2022 17:50:09 -0400 From: Paul Moore <paul@...l-moore.com> To: Kees Cook <keescook@...omium.org> Cc: James Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH] LSM: Better reporting of actual LSMs at boot On Tue, Oct 18, 2022 at 2:48 AM Kees Cook <keescook@...omium.org> wrote: > > Enhance the details reported by "lsm.debug" in several ways: > > - report contents of "security=" > - report contents of "CONFIG_LSM" > - report contents of "lsm=" > - report any early LSM details > - whitespace-align the output of similar phases for easier visual parsing > - change "disabled" to more accurate "skipped" > - explain what "skipped" and "ignored" mean in a parenthetical > > Upgrade the "security= is ignored" warning from pr_info to pr_warn, > and include full arguments list to make the cause even more clear. > > Replace static "Security Framework initializing" pr_info with specific > list of the resulting order of enabled LSMs. > > Cc: Paul Moore <paul@...l-moore.com> > Cc: James Morris <jmorris@...ei.org> > Cc: "Serge E. Hallyn" <serge@...lyn.com> > Cc: linux-security-module@...r.kernel.org > Signed-off-by: Kees Cook <keescook@...omium.org> > --- > security/security.c | 61 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 54 insertions(+), 7 deletions(-) I don't know about you, but when I'm reading commit descriptions about how a patch changes the user visible output of something, e.g. console messages, I always enjoy seeing an example of the new output, both in normal and debug mode (hint, hint) ;) More comments below ... > diff --git a/security/security.c b/security/security.c > index 9696dd64095e..6f6079dec270 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -159,7 +159,7 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from) > lsm->enabled = &lsm_enabled_true; > ordered_lsms[last_lsm++] = lsm; > > - init_debug("%s ordering: %s (%sabled)\n", from, lsm->name, > + init_debug("%s ordered: %s (%sabled)\n", from, lsm->name, > is_enabled(lsm) ? "en" : "dis"); This isn't your fault, but since you're changing this line let's get rid of the "en"/"dis" and do a proper "enabled"/"disabled" string to make it slightly easier to find string while grep'ing through the sources. Example: init_debug("... %s\n", (is_enabled(lsm) ? "enabled" : "disabled")); > @@ -307,7 +308,8 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) > if (exists_ordered_lsm(lsm)) > continue; > set_enabled(lsm, false); > - init_debug("%s disabled: %s\n", origin, lsm->name); > + init_debug("%s skipped: %s (not in requested order)\n", > + origin, lsm->name); I'm not sure the distinction between "disabled" and "skipped" above is that significant, and in fact I tend to think "disabled" is more appropriate. There is also some (minor) advantage to keeping the user visible log messages consistent. However, I do think the parenthetical explanations are a nice addition. (If we did go the "skipped" route, I think we should also change the "security=%s disabled: %s\n" further up the function for the sake of consistent language.) > @@ -318,6 +320,44 @@ static void __init lsm_early_task(struct task_struct *task); > > static int lsm_append(const char *new, char **result); > > +static void __init report_lsm_order(void) > +{ > + struct lsm_info **lsm, *early; > + size_t size = 0; > + char *effective, *step, *end; > + > + /* Count the length of each enabled LSM name. */ > + for (early = __start_early_lsm_info; early < __end_early_lsm_info; early++) > + if (is_enabled(early)) > + size += strlen(early->name) + 1; > + for (lsm = ordered_lsms; *lsm; lsm++) > + if (is_enabled(*lsm)) > + size += strlen((*lsm)->name) + 1; > + > + /* Allocate space with trailing %NUL or give up. */ > + size += 1; > + effective = kzalloc(size, GFP_KERNEL); > + if (!effective) > + return; > + end = effective + size; > + step = effective; > + > + /* Append each enabled LSM name. */ > + for (early = __start_early_lsm_info; early < __end_early_lsm_info; early++) > + if (is_enabled(early)) > + step += scnprintf(step, end - step, "%s%s", > + step == effective ? "" : ",", > + early->name); > + for (lsm = ordered_lsms; *lsm; lsm++) > + if (is_enabled(*lsm)) > + step += scnprintf(step, end - step, "%s%s", > + step == effective ? "" : ",", > + (*lsm)->name); > + > + pr_info("initializing lsm=%s\n", effective); Instead of going through all the trouble of determining the string size and formatting the string via a series of scnprintf() calls, why not cut out the intermediate string buffer and use pr_info()/pr_cont()? What am I missing? > @@ -393,13 +436,17 @@ int __init security_init(void) > { > struct lsm_info *lsm; > > - pr_info("Security Framework initializing\n"); > + init_debug("legacy security=%s\n", chosen_major_lsm ?: " *unspecified*"); > + init_debug(" CONFIG_LSM=%s\n", builtin_lsm_order); > + init_debug("boot arg lsm=%s\n", chosen_lsm_order ?: " *unspecified*"); > > /* > * Append the names of the early LSM modules now that kmalloc() is > * available > */ > for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { > + init_debug(" early started: %s (%sabled)\n", lsm->name, > + is_enabled(lsm) ? "en" : "dis"); See the earlier comment about "en"/"dis" versus "enabled"/"disabled". However, I wonder how useful the above debug message is when you consider report_lsm_order(). Since report_lsm_order() dumps both the early and normal LSMs, perhaps it makes more sense to annotate the output there to indicate early vs normal LSM loading? (This is one of the reasons why it can be nice to see an example of the output in the commit description.) -- paul-moore.com
Powered by blists - more mailing lists