[PATCH] audit: do not panic on invalid boot parameter

[Richard Guy Briggs]

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 at 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.
However, I wonder if this second panic should be left alone, since it
isn't related to the two audit_default options above.
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.  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.

>  	pr_info("%s\n", audit_default ?
>  		"enabled (after initialization)" : "disabled (until reboot)");
> -- 
> 2.14.3
> 

- 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