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&gt; 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