On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards <gedwards(a)ddn.com&gt; wrote: > If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off', > the kernel panics very early in boot with no output on the console > indicating the problem. I'm guessing the problem is that there was too much info dumped to the console and the error message was lost (there is one, to say there is "no output" isn't completely correct), is that what happened? Or was there honestly *no* output on the console? > This seems overly harsh. Instead, print the error indicating an invalid > audit parameter value and leave auditing disabled. There are some audit requirements which appear rather bizarre at times, e.g. the need to panic the kernel instead of losing an audit event. Steve is the one who follows most of these audit requirements so I'm going to wait until he has a chance to look at this. There is also another issue in this patch, on error you have the audit subsystem default to off, we may want to change this to default to on in case of error (fail safely).

> Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore") > Signed-off-by: Greg Edwards <gedwards(a)ddn.com&gt; > --- > kernel/audit.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 227db99b0f19..d8af7682d6a3 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str) > { > long val; > > - if (kstrtol(str, 0, &val)) > - panic("audit: invalid 'audit' parameter value (%s)\n", str); > + if (kstrtol(str, 0, &val)) { > + pr_err("invalid 'audit' parameter value (%s)\n", str); > + val = AUDIT_OFF; > + } > audit_default = (val ? AUDIT_ON : AUDIT_OFF); > > if (audit_default == AUDIT_OFF) > -- > 2.14.3 > -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off', the kernel panics very early in boot with no output on the console indicating the problem. Instead, print the error indicating an invalid audit parameter value, but leave auditing enabled. Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore") Signed-off-by: Greg Edwards <gedwards(a)ddn.com&gt; --- Changes v1 -> v2: - default to auditing enabled for the error case kernel/audit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c index 227db99b0f19..9b80e9895107 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str) { long val; - if (kstrtol(str, 0, &val)) - panic("audit: invalid 'audit' parameter value (%s)\n", str); + if (kstrtol(str, 0, &val)) { + pr_err("invalid 'audit' parameter value (%s)\n", str); + val = AUDIT_ON; + } audit_default = (val ? AUDIT_ON : AUDIT_OFF); if (audit_default == AUDIT_OFF) -- 2.14.3

On 2018-03-05 15:05, Greg Edwards wrote: > If you pass in an invalid audit boot parameter value, e.g. "audit=off", > the kernel panics very early in boot before the regular console is > initialized. Unless you have earlyprintk enabled, there is no > indication of what the problem is on the console. > > Convert the panic() calls to pr_err(), and leave auditing enabled if an > invalid parameter value was passed in. > > Modify the parameter to also accept "on" or "off" as valid values, and > update the documentation accordingly. > > Signed-off-by: Greg Edwards <gedwards(a)ddn.com&gt; > --- > Changes v2 -> v3: > - convert panic() calls to pr_err() > - add handling of "on"/"off" as valid values > - update documentation > > Changes v1 -> v2: > - default to auditing enabled for the error case > > Documentation/admin-guide/kernel-parameters.txt | 14 +++++++------- > kernel/audit.c | 21 ++++++++++++++------- > 2 files changed, 21 insertions(+), 14 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 1d1d53f85ddd..0b926779315c 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -389,15 +389,15 @@ > Use software keyboard repeat > > audit= [KNL] Enable the audit sub-system > - Format: { "0" | "1" } (0 = disabled, 1 = enabled) > - 0 - kernel audit is disabled and can not be enabled > - until the next reboot > + Format: { "0" | "1" | "off" | "on" } > + 0 | off - kernel audit is disabled and can not be > + enabled until the next reboot > unset - kernel audit is initialized but disabled and > will be fully enabled by the userspace auditd. > - 1 - kernel audit is initialized and partially enabled, > - storing at most audit_backlog_limit messages in > - RAM until it is fully enabled by the userspace > - auditd. > + 1 | on - kernel audit is initialized and partially > + enabled, storing at most audit_backlog_limit > + messages in RAM until it is fully enabled by the > + userspace auditd. > Default: unset > > audit_backlog_limit= [KNL] Set the audit queue size limit. > diff --git a/kernel/audit.c b/kernel/audit.c > index 227db99b0f19..8fccea5ded71 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1567,19 +1567,26 @@ static int __init audit_init(void) > } > postcore_initcall(audit_init); > > -/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */ > +/* > + * Process kernel command-line parameter at boot time. > + * audit={0|off} or audit={1|on}. > + */ > 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); This patch looks good.

However, I wonder if this second panic should be left alone, since it isn't related to the two audit_default options above.

audit_set_enabled() can't be sent AUDIT_LOCKED from here, there must be an error returned from looking up the security context when trying to log the config change.

There is already an audit_panic when that is detected, but this is so early that audit_panic won't be configured to panic yet and defaults to printk. If it is also so early that no LSMs have been loaded yet then this concern is moot. There is still the question of just how useful it is to panic this early.

> pr_info("%s\n", audit_default ? > "enabled (after initialization)" : "disabled (until reboot)"); > -- > 2.14.3 > - 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

On Tue, Mar 6, 2018 at 9:38 AM, Paul Moore <paul(a)paul-moore.com&gt; wrote: > On Mon, Mar 5, 2018 at 10:24 PM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: >> On 2018-03-05 15:05, Greg Edwards wrote: >>> If you pass in an invalid audit boot parameter value, e.g. "audit=off", >>> the kernel panics very early in boot before the regular console is >>> initialized. Unless you have earlyprintk enabled, there is no >>> indication of what the problem is on the console. >>> >>> Convert the panic() calls to pr_err(), and leave auditing enabled if an >>> invalid parameter value was passed in. >>> >>> Modify the parameter to also accept "on" or "off" as valid values, and >>> update the documentation accordingly. >>> >>> Signed-off-by: Greg Edwards <gedwards(a)ddn.com&gt; >>> --- >>> Changes v2 -> v3: >>> - convert panic() calls to pr_err() >>> - add handling of "on"/"off" as valid values >>> - update documentation >>> >>> Changes v1 -> v2: >>> - default to auditing enabled for the error case >>> >>> Documentation/admin-guide/kernel-parameters.txt | 14 +++++++------- >>> kernel/audit.c | 21 ++++++++++++++------- >>> 2 files changed, 21 insertions(+), 14 deletions(-) >>> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>> index 1d1d53f85ddd..0b926779315c 100644 >>> --- a/Documentation/admin-guide/kernel-parameters.txt >>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>> @@ -389,15 +389,15 @@ >>> Use software keyboard repeat >>> >>> audit= [KNL] Enable the audit sub-system >>> - Format: { "0" | "1" } (0 = disabled, 1 = enabled) >>> - 0 - kernel audit is disabled and can not be enabled >>> - until the next reboot >>> + Format: { "0" | "1" | "off" | "on" } >>> + 0 | off - kernel audit is disabled and can not be >>> + enabled until the next reboot >>> unset - kernel audit is initialized but disabled and >>> will be fully enabled by the userspace auditd. >>> - 1 - kernel audit is initialized and partially enabled, >>> - storing at most audit_backlog_limit messages in >>> - RAM until it is fully enabled by the userspace >>> - auditd. >>> + 1 | on - kernel audit is initialized and partially >>> + enabled, storing at most audit_backlog_limit >>> + messages in RAM until it is fully enabled by the >>> + userspace auditd. >>> Default: unset >>> >>> audit_backlog_limit= [KNL] Set the audit queue size limit. >>> diff --git a/kernel/audit.c b/kernel/audit.c >>> index 227db99b0f19..8fccea5ded71 100644 >>> --- a/kernel/audit.c >>> +++ b/kernel/audit.c >>> @@ -1567,19 +1567,26 @@ static int __init audit_init(void) >>> } >>> postcore_initcall(audit_init); >>> >>> -/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */ >>> +/* >>> + * Process kernel command-line parameter at boot time. >>> + * audit={0|off} or audit={1|on}. >>> + */ >>> 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); >> >> This patch looks good. > > On quick glance, I agree. I'll look at it a bit closer later today > and likely merge it. > > Thanks Greg. It's merge now. Thanks again everyone!

paul moore