Re: [PATCH v2] audit: do not panic kernel on invalid audit parameter
On Wed, Feb 21, 2018 at 04:08:25PM -0500, Paul Moore wrote:
On February 21, 2018 11:19:09 AM Greg Edwards
<gedwards(a)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.
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").
Yes, I think this would be a good idea, and for what it's worth,
'audit=off' worked until 4.15. One of our CI tests that verifies
upstream kernels picked this up starting with 4.15.
For example, booting a 4.14.20 kernel (defconfig + kvmconfig):
[ 0.000000] Linux version 4.14.20 (gedwards@psuche) (gcc version 7.3.1 20180130 (Red
Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:14:25 M
ST 2018
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: disabled (until reboot)
^^^^^^^^
#2 - If panic("<msg>") doesn't work, does
pr_err("<msg>")? If it
does, I would be curious to understand why.
Yes, pr_err() does work. Booting 4.16-rc2 (defconfig + kvmconfig) with
this patch:
[ 0.000000] Linux version 4.16.0-rc2+ (gedwards@psuche) (gcc version 7.3.1 20180130
(Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:23:10 MST 2018
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: invalid 'audit' parameter value (off)
[ 0.000000] audit: enabled (after initialization)
I suspect what is happening with the current audit_enable() code is the
serial console has not been fully initialized yet by the time we call
panic(), so we never see the early printk messages queued up. I will
try and confirm.
#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?
I haven't looked at those other calls to panic(), but I would bet they
display the same behavior.
#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'm unclear why we would need this, given that #2 above does work. This
is the first time I've ever looked at the audit code, though. I was
just doing a drive-by. ;)
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 :)
Sure, I'm happy to look at the above.
Greg