On Tue, Feb 27, 2018 at 5:52 PM, Greg Edwards <gedwa...@ddn.com> wrote: > On Tue, Feb 27, 2018 at 05:28:09PM -0500, Paul Moore wrote: >> On Tue, Feb 27, 2018 at 10:59 AM, Greg Edwards <gedwa...@ddn.com> wrote: >>> 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. >> >> I thought the issue was that we couldn't reliably write to the console >> in audit_enable() as it required early printks to be enabled? > > You can't reliably panic from audit_enable() unless earlyprintk is > enabled, since the boot stops at the panic and the regular console isn't > initialized yet. pr_err/printk etc work fine, as those messages just > get queued up and output once the regular console is initialized (since > the boot continues on).
Thanks for the more detailed explanation, I was operating under the assumption that the printks were happening immediately and not getting queued; my mistake. > So, if you want to keep the panic behavior on bad audit parameters, your > delayed processing should do the trick. If it instead, you are fine > with just pr_err and leaving audit enabled for that error case, then we > are almost back to my original patch, with the exceptions you previously > noted: > > * leave audit enabled on parsing error > * change panic on audit_set_enabled() failure to pr_err > * handle on/off as well > > My apologies if my commit message was misleading! No need to apologize, I was a bit confused, but I think I've got a handle on it now. If we get rid of the need to panic(), which I think we are all okay with, I think we can resolve everything with something like this, yes? diff --git a/kernel/audit.c b/kernel/audit.c index 1a3e75d9a66c..d41d09e84163 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1618,16 +1618,20 @@ 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; - - 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 { + pr_err("audit: invalid 'audit' parameter value (%s)\n", str); + audit_default = AUDIT_ON; + } if (audit_default == AUDIT_OFF) audit_initialized = AUDIT_DISABLED; if (audit_set_enabled(audit_default)) - panic("audit: error setting audit state (%d)\n", audit_default); + pr_err("audit: error setting audit state (%d)\n", + audit_default); pr_info("%s\n", audit_default ? "enabled (after initialization)" : "disabled (until reboot)"); -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit