[PATCH v2] audit: do not panic kernel on invalid audit parameter
Steve Grubb
sgrubb at redhat.com
Wed Feb 21 22:52:53 UTC 2018
On Wednesday, February 21, 2018 4:08:25 PM EST Paul Moore wrote:
> On February 21, 2018 11:19:09 AM Greg Edwards <gedwards at 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 at 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
More information about the Linux-audit
mailing list