Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init()

On Tue, Feb 27, 2018 at 12:53 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2018-02-26 19:00, Paul Moore wrote: >> On Thu, Feb 22, 2018 at 7:22 PM, Greg Edwards <gedwards(a)ddn.com&gt; wrote: >> > The processing of the "audit" boot parameter is handled before the >> > console has been initialized. We therefore miss any panic messages if >> > we fail to verify the boot parameter or set the audit state, unless we >> > also enable earlyprintk. >> > >> > Instead, have the boot parameter function just save the parameter value >> > and process it later from audit_init(), which is a postcore_initcall() >> > function. >> > >> > Signed-off-by: Greg Edwards <gedwards(a)ddn.com&gt; >> > --- >> > kernel/audit.c | 48 +++++++++++++++++++++++++++++++----------------- >> > 1 file changed, 31 insertions(+), 17 deletions(-) >> >> In the process of trying to explain things a bit further (see the >> discussion thread in 0/2), I realized that some example code might >> speak better than I could. Below is what I was thinking for a fix; I >> haven't tested it, so it may blow up badly, but hopefully it makes >> things a bit more clear. >> >> One thing of note, I did away with the kstrtol() altogether, when we >> are only looking for zero and one it seems easier to just compare the >> strings. > > It is easier, but might break stuff that previously worked, such as any > non-zero integer to turn the feature on. This isn't documented to work > but then neither was "off" and "on" which are now being accomodated. That behavior is preserved in the example code I provided. If the argument is not off, on, zero, or one then audit will be enabled and a message will be displayed on the console indicating an invalid argument.

> Also, keeping a pointer to the offending string would be helpful in the > error reporting text to show exactly what the parser thinks it sees. That would have required add yet another global variable (which I generally despise) for something that is easily determined by either /proc/cmdline, the bootloader config, or the admins memory. Yes, I suppose that if the core kernel code is munging the command line then we might get erratic behavior, but that is so remote that I'm not going to worry about it.

>> diff --git a/kernel/audit.c b/kernel/audit.c >> index 1a3e75d9a66c..5dd63f60ef90 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -61,6 +61,7 @@ >> #include <linux/gfp.h> >> #include <linux/pid.h> >> #include <linux/slab.h> >> +#include <linux/string.h> >> >> #include <linux/audit.h> >> >> @@ -86,6 +87,7 @@ static int audit_initialized; >> #define AUDIT_OFF 0 >> #define AUDIT_ON 1 >> #define AUDIT_LOCKED 2 >> +#define AUDIT_ARGERR 3 /* indicate a "audit=X" syntax error at boot */ >> u32 audit_enabled = AUDIT_OFF; >> bool audit_ever_enabled = !!AUDIT_OFF; >> >> @@ -1581,6 +1583,12 @@ static int __init audit_init(void) >> if (audit_initialized == AUDIT_DISABLED) >> return 0; >> >> + /* handle any delayed error reporting from audit_enable() */ >> + if (audit_default == AUDIT_ARGERR) { >> + pr_err("invalid 'audit' parameter value, use 0 or 1\n"); >> + audit_default = AUDIT_ON; >> + } >> + >> audit_buffer_cache = kmem_cache_create("audit_buffer", >> sizeof(struct audit_buffer), >> 0, SLAB_PANIC, NULL); >> @@ -1618,19 +1626,23 @@ postcore_initcall(audit_init); >> /* Process kernel command-line parameter at boot time. audit=0 or audit=1. */ >> static int __init audit_enable(char *str) >> { >> - long val; >> + /* NOTE: we can't reliably send any messages to the console here */ >> >> - if (kstrtol(str, 0, &val)) >> - panic("audit: invalid 'audit' parameter value (%s)\n", str); >> - audit_default = (val ? AUDIT_ON : AUDIT_OFF); >> + if (!strcasecmp(str, "off") || !strcmp(str, "0")) >> + audit_default = AUDIT_OFF; >> + else if (!strcasecmp(str, "on") || !strcmp(str, "1")) >> + audit_default = AUDIT_ON; >> + else >> + audit_default = AUDIT_ARGERR; >> >> - if (audit_default == AUDIT_OFF) >> + if (audit_default) { >> + audit_enabled = AUDIT_ON; >> + audit_ever_enabled = AUDIT_ON; >> + } else { >> + audit_enabled = AUDIT_OFF; >> + audit_ever_enabled = AUDIT_OFF; >> audit_initialized = AUDIT_DISABLED; >> - if (audit_set_enabled(audit_default)) >> - panic("audit: error setting audit state (%d)\n", audit_default); >> - >> - pr_info("%s\n", audit_default ? >> - "enabled (after initialization)" : "disabled (until reboot)"); >> + } >> >> return 1; >> } >> >> -- >> paul moore >> www.paul-moore.com >> >> -- >> Linux-audit mailing list >> Linux-audit(a)redhat.com >> https://www.redhat.com/mailman/listinfo/linux-audit > > - RGB > > -- > Richard Guy Briggs <rgb(a)redhat.com&gt; > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 -- paul moore www.paul-moore.com