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.
        audit_buffer_cache =
kmem_cache_create("audit_buffer",
                                               sizeof(struct audit_buffer),
                                               0, SLAB_PANIC, NULL);
 @@ -1618,19 +1626,23 @@ 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;
 +       /* NOTE: we can't reliably send any messages to the console here */
 -       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
 +               audit_default = AUDIT_ARGERR; 
Just pr_err() here and set audit_default = AUDIT_ON for the error case.
 -       if (audit_default == AUDIT_OFF)
 +       if (audit_default) {
 +               audit_enabled = AUDIT_ON;
 +               audit_ever_enabled = AUDIT_ON;
 +       } else {
 +               audit_enabled = AUDIT_OFF;
 +               audit_ever_enabled = AUDIT_OFF;
                audit_initialized = AUDIT_DISABLED;
 -       if (audit_set_enabled(audit_default))
 -               panic("audit: error setting audit state (%d)\n",
audit_default); 
You could leave this here if you did error
reporting/audit_default=AUDIT_ON in audit_enable() directly.
 -
 -       pr_info("%s\n", audit_default ?
 -               "enabled (after initialization)" : "disabled (until
reboot)");
 +       }
        return 1;
 } 
Another idea I had was switching those original panic() calls to
audit_panic(), and then making audit_failure another __setup option,
i.e. audit_failure={silent,printk,panic} corresponding to
AUDIT_FAIL_{SILENT,PRINTK,PANIC}.  You could default it to
AUDIT_FAIL_PRINTK as it is today.  Users that really cared could boot
with audit_failure=panic.  I don't know if that would be overloading
audit_panic() outside of its intended purpose, though.
Greg