8:38 a.m.
On Mon, Mar 5, 2018 at 10:24 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2018-03-05 15:05, Greg Edwards wrote:
> If you pass in an invalid audit boot parameter value, e.g. "audit=off",
> the kernel panics very early in boot before the regular console is
> initialized. Unless you have earlyprintk enabled, there is no
> indication of what the problem is on the console.
>
> Convert the panic() calls to pr_err(), and leave auditing enabled if an
> invalid parameter value was passed in.
>
> Modify the parameter to also accept "on" or "off" as valid
values, and
> update the documentation accordingly.
>
> Signed-off-by: Greg Edwards <gedwards(a)ddn.com>
> ---
> Changes v2 -> v3:
> - convert panic() calls to pr_err()
> - add handling of "on"/"off" as valid values
> - update documentation
>
> Changes v1 -> v2:
> - default to auditing enabled for the error case
>
> Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
> kernel/audit.c | 21 ++++++++++++++-------
> 2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
> index 1d1d53f85ddd..0b926779315c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -389,15 +389,15 @@
> Use software keyboard repeat
>
> audit= [KNL] Enable the audit sub-system
> - Format: { "0" | "1" } (0 = disabled, 1 =
enabled)
> - 0 - kernel audit is disabled and can not be enabled
> - until the next reboot
> + Format: { "0" | "1" | "off" |
"on" }
> + 0 | off - kernel audit is disabled and can not be
> + enabled until the next reboot
> unset - kernel audit is initialized but disabled and
> will be fully enabled by the userspace auditd.
> - 1 - kernel audit is initialized and partially enabled,
> - storing at most audit_backlog_limit messages in
> - RAM until it is fully enabled by the userspace
> - auditd.
> + 1 | on - kernel audit is initialized and partially
> + enabled, storing at most audit_backlog_limit
> + messages in RAM until it is fully enabled by the
> + userspace auditd.
> Default: unset
>
> audit_backlog_limit= [KNL] Set the audit queue size limit.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99b0f19..8fccea5ded71 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1567,19 +1567,26 @@ static int __init audit_init(void)
> }
> postcore_initcall(audit_init);
>
> -/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
> +/*
> + * Process kernel command-line parameter at boot time.
> + * audit={0|off} or audit={1|on}.
> + */
> static int __init audit_enable(char *str)
> {
> - long val;
> -
> - if (kstrtol(str, 0, &val))
> - panic("audit: invalid 'audit' parameter value
(%s)\n", str);
> - audit_default = (val ? AUDIT_ON : AUDIT_OFF);
> + if (!strcasecmp(str, "off") || !strcmp(str, "0"))
> + audit_default = AUDIT_OFF;
> + else if (!strcasecmp(str, "on") || !strcmp(str, "1"))
> + audit_default = AUDIT_ON;
> + else {
> + pr_err("audit: invalid 'audit' parameter value
(%s)\n", str);
> + audit_default = AUDIT_ON;
> + }
>
> if (audit_default == AUDIT_OFF)
> audit_initialized = AUDIT_DISABLED;
> if (audit_set_enabled(audit_default))
> - panic("audit: error setting audit state (%d)\n",
audit_default);
> + pr_err("audit: error setting audit state (%d)\n",
> + audit_default);
This patch looks good.
On quick glance, I agree. I'll look at it a bit closer later today
and likely merge it.
Thanks Greg.
However, I wonder if this second panic should be left alone, since
it
isn't related to the two audit_default options above.
There is still the silent-panic problem that started this entire
discussion/patch. If we really need to panic() here (and I currently
don't think we need to panic), then we need to devise another solution
(see the previous patches that we discussed).
audit_set_enabled() can't be sent AUDIT_LOCKED from here, there
must be
an error returned from looking up the security context when trying to
log the config change.
Keep in mind that we aren't actually logging anything here as
audit_initialized isn't set to AUDIT_INITIALIZED yet. We're using
audit_set_enabled() simply so we consolidate all of the enable/disable
code in one place (see the original commit where I switched it over to
use audit_set_enabled()).
There is already an audit_panic when that is
detected, but this is so early that audit_panic won't be configured to
panic yet and defaults to printk. If it is also so early that no LSMs
have been loaded yet then this concern is moot. There is still the
question of just how useful it is to panic this early.
Not an issue, we are never going to get past audit_log_start() as we
haven't initialized the audit subsystem yet.
> pr_info("%s\n", audit_default ?
> "enabled (after initialization)" : "disabled (until
reboot)");
> --
> 2.14.3
>
- RGB
--
Richard Guy Briggs <rgb(a)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
--
paul moore
www.paul-moore.com