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

[Greg Edwards <gedwards@ddn.com>]

On Wed, Feb 21, 2018 at 04:08:25PM -0500, 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.
>
> 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