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

[Greg Edwards <gedwards@ddn.com>]

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