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

[Greg Edwards <gedwards@ddn.com>]

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@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).

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!

Greg