On Mon, Feb 26, 2018 at 07:00:51PM -0500, Paul Moore wrote: > > 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. > > 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; > + } > +
If you are just going to pr_err() on invalid audit parameter instead of panic, you don't need AUDIT_ARGERR at all or the delayed error reporting of it here. You can just use pr_err() in audit_enable() directly. > 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; Just pr_err() here and set audit_default = AUDIT_ON for the error case. > > - 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); You could leave this here if you did error reporting/audit_default=AUDIT_ON in audit_enable() directly. > - > - pr_info("%s\n", audit_default ? > - "enabled (after initialization)" : "disabled (until reboot)"); > + } > > return 1; > } Another idea I had was switching those original panic() calls to audit_panic(), and then making audit_failure another __setup option, i.e. audit_failure={silent,printk,panic} corresponding to AUDIT_FAIL_{SILENT,PRINTK,PANIC}. You could default it to AUDIT_FAIL_PRINTK as it is today. Users that really cared could boot with audit_failure=panic. I don't know if that would be overloading audit_panic() outside of its intended purpose, though. Greg -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit