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

[Richard Guy Briggs]

On 2018-02-21 15:51, Greg Edwards wrote:
> On Wed, Feb 21, 2018 at 04:08:25PM -0500, 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.
> >
> > 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.

Huh, at first I wondered if the earlier audit init was at play here, but
now I'm suspecting
	80ab4df62706b882922c3bb0b05ce2c8ab10828a
	("audit: don't use simple_strtol() anymore")
is the primary culprit, exacerbated by earlier init from the same
patchset.

> For example, booting a 4.14.20 kernel (defconfig + kvmconfig):
> 
> [    0.000000] Linux version 4.14.20 (gedwards at 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 at 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

- RGB

--
Richard Guy Briggs <rgb at redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635