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

[Steve Grubb <sgrubb@redhat.com>]

On Wednesday, February 21, 2018 4:08:25 PM EST Paul Moore wrote:
> On February 21, 2018 11:19:09 AM Greg Edwards <gedwards@ddn.com> 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@ddn.com>
> > ---
> > 
> > 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.

Well, auditd will probably have a big problem starting up and that should be 
a big clue. Also, this could be remembered in a way that a fault indication 
is returned by auditctl -s? Loading audit rules leads to checking audit 
status which journald keeps around.

-Steve

> 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