[PATCH 1/2] audit: move processing of "audit" boot param to audit_init()

[Paul Moore]

On Tue, Feb 27, 2018 at 5:52 PM, Greg Edwards <gedwards at ddn.com> wrote:
> On Tue, Feb 27, 2018 at 05:28:09PM -0500, Paul Moore wrote:
>> On Tue, Feb 27, 2018 at 10:59 AM, Greg Edwards <gedwards at ddn.com> wrote:
>>> On Mon, Feb 26, 2018 at 07:00:51PM -0500, Paul Moore wrote:
>>>>
>>>> In the process of trying to explain things a bit further (see the
>>>> discussion thread in 0/2), I realized that some example code might
>>>> speak better than I could.  Below is what I was thinking for a fix; I
>>>> haven't tested it, so it may blow up badly, but hopefully it makes
>>>> things a bit more clear.
>>>>
>>>> One thing of note, I did away with the kstrtol() altogether, when we
>>>> are only looking for zero and one it seems easier to just compare the
>>>> strings.
>>>>
>>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>>> index 1a3e75d9a66c..5dd63f60ef90 100644
>>>> --- a/kernel/audit.c
>>>> +++ b/kernel/audit.c
>>>> @@ -61,6 +61,7 @@
>>>> #include <linux/gfp.h>
>>>> #include <linux/pid.h>
>>>> #include <linux/slab.h>
>>>> +#include <linux/string.h>
>>>>
>>>> #include <linux/audit.h>
>>>>
>>>> @@ -86,6 +87,7 @@ static int    audit_initialized;
>>>> #define AUDIT_OFF      0
>>>> #define AUDIT_ON       1
>>>> #define AUDIT_LOCKED   2
>>>> +#define AUDIT_ARGERR   3       /* indicate a "audit=X" syntax error at boot */
>>>> u32            audit_enabled = AUDIT_OFF;
>>>> bool           audit_ever_enabled = !!AUDIT_OFF;
>>>>
>>>> @@ -1581,6 +1583,12 @@ static int __init audit_init(void)
>>>>        if (audit_initialized == AUDIT_DISABLED)
>>>>                return 0;
>>>>
>>>> +       /* handle any delayed error reporting from audit_enable() */
>>>> +       if (audit_default == AUDIT_ARGERR) {
>>>> +               pr_err("invalid 'audit' parameter value, use 0 or 1\n");
>>>> +               audit_default = AUDIT_ON;
>>>> +       }
>>>> +
>>>
>>> If you are just going to pr_err() on invalid audit parameter instead of
>>> panic, you don't need AUDIT_ARGERR at all or the delayed error reporting
>>> of it here.  You can just use pr_err() in audit_enable() directly.
>>
>> I thought the issue was that we couldn't reliably write to the console
>> in audit_enable() as it required early printks to be enabled?
>
> You can't reliably panic from audit_enable() unless earlyprintk is
> enabled, since the boot stops at the panic and the regular console isn't
> initialized yet.  pr_err/printk etc work fine, as those messages just
> get queued up and output once the regular console is initialized (since
> the boot continues on).

Thanks for the more detailed explanation, I was operating under the
assumption that the printks were happening immediately and not getting
queued; my mistake.

> So, if you want to keep the panic behavior on bad audit parameters, your
> delayed processing should do the trick.  If it instead, you are fine
> with just pr_err and leaving audit enabled for that error case, then we
> are almost back to my original patch, with the exceptions you previously
> noted:
>
>   * leave audit enabled on parsing error
>   * change panic on audit_set_enabled() failure to pr_err
>   * handle on/off as well
>
> My apologies if my commit message was misleading!

No need to apologize, I was a bit confused, but I think I've got a
handle on it now.

If we get rid of the need to panic(), which I think we are all okay
with, I think we can resolve everything with something like this, yes?

diff --git a/kernel/audit.c b/kernel/audit.c
index 1a3e75d9a66c..d41d09e84163 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1618,16 +1618,20 @@ postcore_initcall(audit_init);
/* Process kernel command-line parameter at boot time.  audit=0 or audit=1. */
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);

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

-- 
paul moore
www.paul-moore.com