On February 21, 2018 11:19:09 AM Greg Edwards <gedwa...@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 <gedwa...@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.
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)
> --
> 2.14.3
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
