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

[Paul Moore]

On Mon, Mar 5, 2018 at 10:24 PM, Richard Guy Briggs <rgb at 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 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.

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 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

-- 
paul moore
www.paul-moore.com