Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter

On February 21, 2018 11:19:09 AM 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. > > 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(-) Thanks for the quick follow-up, it's actually a little *too* quick if I'm honest, I still haven't fully thought through all the different options here :) However, in the interest in capitalizing on your enthusiasm and willingness to help, here are some of the things I was thinking about, in no particular order: #1 - We might want to consider accepting both "0" and "off" as acceptable inputs. It should be a trivial change, and if we are going to default to on/enabled it seems like we should make a reasonable effort to do the right thing when people attempt to disable audit (unfortunately the kernel command line parameters seem to use both "0" and "off" so we can't blame people too much when they use "off"). #2 - If panic("<msg>") doesn't work, does pr_err("<msg>")? If it does, I would be curious to understand why. #3 - Related to #2 above, but there are other calls to panic() and pr_*() in audit_enable() that should probably be re-evaluated and changed. If we can't notify users/admins here, why are we trying? #4 - Related to #2 and #3, if we can't emit messages in audit_enable() we need to find a way to "remember" that the user specified a bogus audit setting and let them know as soon as we can. One possibility might be to overload the audit_default variable (most places seem to treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value (make it non-zero, say "3"?) and we could display a message in audit_init() or similar. Full disclosure, this *should* work ... I think ... but I might be missing some crucial detail.

I realize this is probably much more than you bargained for when you first submitted your patch, and if you're not interested in taking this any further I understand .... however, if you are willing to play a bit more I would be very grateful :) > 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) -- paul moore www.paul-moore.com