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

[Paul Moore]

On Tue, Mar 6, 2018 at 9:38 AM, Paul Moore <paul at paul-moore.com> wrote:
> 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.

It's merge now.  Thanks again everyone!

-- 
paul moore
www.paul-moore.com