[PATCH 0/2] audit boot parameter cleanups

[Greg Edwards]

One of our CI tests was booting upstream kernels with the "audit=off" kernel
parameter. This was our error; it should have been "audit=0". However,
in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
("audit: don't use simple_strtol() anymore"), and our errant boot parameter
value starting panic'ing the system.

The problem is this happens so early in boot, the console isn't initialized yet
and you don't see the panic message. You have no idea what the problem is
unless you add an "earlyprintk" boot option, e.g.
earlyprintk=serial,ttyS0,115200n8.

Fix this by having the boot parameter setup function just save the boot
parameter value, and process it later from a call in audit_init(). The console
is initialized by this point, and you can see any panic messages without having
to use an earlyprintk option.

Additionally, add "on" and "off" as valid audit boot parameter values.

Greg Edwards (2):
audit: move processing of "audit" boot param to audit_init()
audit: add "on"/"off" as valid boot parameter values

Documentation/admin-guide/kernel-parameters.txt | 14 +++----
kernel/audit.c | 49 ++++++++++++++++---------
2 files changed, 39 insertions(+), 24 deletions(-)

[Greg Edwards]

The processing of the "audit" boot parameter is handled before the
console has been initialized. We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.

Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.

Signed-off-by: Greg Edwards <***@ddn.com>
---
kernel/audit.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..3fb11bcb4408 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -99,6 +99,9 @@ static u32 audit_failure = AUDIT_FAIL_PRINTK;
/* private audit network namespace index */
static unsigned int audit_net_id;

+/* 'audit' boot parameter value */
+static char *audit_boot;
+
/**
* struct audit_net - audit private network namespace data
* @sk: communication socket
@@ -1528,11 +1531,35 @@ static struct pernet_operations audit_net_ops __net_initdata = {
.size = sizeof(struct audit_net),
};

+/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+static void __init audit_enable(void)
+{
+ long val;
+
+ if (!audit_boot)
+ return;
+
+ if (kstrtol(audit_boot, 0, &val))
+ panic("audit: invalid 'audit' parameter value (%s)\n",
+ audit_boot);
+ audit_default = (val ? AUDIT_ON : AUDIT_OFF);
+
+ 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_info("%s\n", audit_default ?
+ "enabled (after initialization)" : "disabled (until reboot)");
+}
+
/* Initialize audit support at boot time. */
static int __init audit_init(void)
{
int i;

+ audit_enable();
+
if (audit_initialized == AUDIT_DISABLED)
return 0;

@@ -1567,26 +1594,13 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);

-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
-static int __init audit_enable(char *str)
+/* Store kernel command-line parameter at boot time. audit=0 or audit=1. */
+static int __init audit_set(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 (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_info("%s\n", audit_default ?
- "enabled (after initialization)" : "disabled (until reboot)");
-
+ audit_boot = str;
return 1;
}
-__setup("audit=", audit_enable);
+__setup("audit=", audit_set);

/* Process kernel command-line parameter at boot time.
* audit_backlog_limit=<n> */

[Paul Moore]

Post by Greg Edwards
The processing of the "audit" boot parameter is handled before the
console has been initialized. We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.
Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.
---
kernel/audit.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

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;
+ }
+
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;

- 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);
-
- pr_info("%s\n", audit_default ?
- "enabled (after initialization)" : "disabled (until reboot)");
+ }

return 1;
}

[Richard Guy Briggs]

Post by Paul Moore

Post by Greg Edwards
The processing of the "audit" boot parameter is handled before the
console has been initialized. We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.
Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.
---
kernel/audit.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

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.

It is easier, but might break stuff that previously worked, such as any
non-zero integer to turn the feature on. This isn't documented to work
but then neither was "off" and "on" which are now being accomodated.

Also, keeping a pointer to the offending string would be helpful in the
error reporting text to show exactly what the parser thinks it sees.

Post by Paul Moore
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;
+ }
+
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;
- 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);
-
- pr_info("%s\n", audit_default ?
- "enabled (after initialization)" : "disabled (until reboot)");
+ }
return 1;
}
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Greg Edwards
The processing of the "audit" boot parameter is handled before the
console has been initialized. We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.
Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.
---
kernel/audit.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

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.

It is easier, but might break stuff that previously worked, such as any
non-zero integer to turn the feature on. This isn't documented to work
but then neither was "off" and "on" which are now being accomodated.

That behavior is preserved in the example code I provided. If the
argument is not off, on, zero, or one then audit will be enabled and a
message will be displayed on the console indicating an invalid
argument.

Post by Richard Guy Briggs
Also, keeping a pointer to the offending string would be helpful in the
error reporting text to show exactly what the parser thinks it sees.

That would have required add yet another global variable (which I
generally despise) for something that is easily determined by either
/proc/cmdline, the bootloader config, or the admins memory. Yes, I
suppose that if the core kernel code is munging the command line then
we might get erratic behavior, but that is so remote that I'm not
going to worry about it.

Post by Richard Guy Briggs

Post by Paul Moore
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;
+ }
+
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;
- 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);
-
- pr_info("%s\n", audit_default ?
- "enabled (after initialization)" : "disabled (until reboot)");
+ }
return 1;
}
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Greg Edwards
The processing of the "audit" boot parameter is handled before the
console has been initialized. We therefore miss any panic messages if
we fail to verify the boot parameter or set the audit state, unless we
also enable earlyprintk.
Instead, have the boot parameter function just save the parameter value
and process it later from audit_init(), which is a postcore_initcall()
function.
---
kernel/audit.c | 48 +++++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

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.

It is easier, but might break stuff that previously worked, such as any
non-zero integer to turn the feature on. This isn't documented to work
but then neither was "off" and "on" which are now being accomodated.

That behavior is preserved in the example code I provided. If the
argument is not off, on, zero, or one then audit will be enabled and a
message will be displayed on the console indicating an invalid
argument.

Fair enough.

Post by Paul Moore

Post by Richard Guy Briggs
Also, keeping a pointer to the offending string would be helpful in the
error reporting text to show exactly what the parser thinks it sees.

That would have required add yet another global variable (which I
generally despise) for something that is easily determined by either
/proc/cmdline, the bootloader config, or the admins memory. Yes, I
suppose that if the core kernel code is munging the command line then
we might get erratic behavior, but that is so remote that I'm not
going to worry about it.

I was thinking more of unprintable characters in text files that would
be rendered in octal in the error message. I've had to deal with that
in the past and that information saved me a lot of time and frustration.

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore
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;
+ }
+
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;
- 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);
-
- pr_info("%s\n", audit_default ?
- "enabled (after initialization)" : "disabled (until reboot)");
+ }
return 1;
}
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Greg Edwards]

Post by Paul Moore
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.

Post by Paul Moore
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.

Post by Paul Moore
- 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.

Post by Paul Moore
-
- 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

[Paul Moore]

Post by Greg Edwards

Post by Paul Moore
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?

At least that was my understanding based on your previous comments,
help set me straight :)

Post by Greg Edwards
Another idea I had was switching those original panic() calls to
audit_panic() ...

Arguably that probably would have originally been the better solution,
especially since the default value of AUDIT_FAIL_PRINTK would have
avoided the panic().

Post by Greg Edwards
... 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.

I'd like to avoid another command line option if we can at this point
(and I think we can). Eventually we will probably need to make the
command line a bit "richer" to support more configuration options
(requested by the embedded/Android folks), but that's a way off.

[Greg Edwards]

Post by Paul Moore

Post by Greg Edwards

Post by Paul Moore
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

[Paul Moore]

Post by Greg Edwards

Post by Paul Moore

Post by Greg Edwards

Post by Paul Moore
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.

Post by Greg Edwards
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
* 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]

Post by Paul Moore

Post by Greg Edwards
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
* leave audit enabled on parsing error
* change panic on audit_set_enabled() failure to pr_err
* handle on/off as well

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, yes this works great, and exactly what I was thinking.

Great, sorry it took so long to get to this point, but I'm glad we've
finally synced up.

Would you the honor, and the glory (oh the glory!) of submitting a
formal patch? ;)

[Greg Edwards]

Modify the "audit" boot parameter to also accept "on" or "off" as valid
parameter values. Update the documentation accordingly.

Signed-off-by: Greg Edwards <***@ddn.com>
---
Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 9 +++++----
2 files changed, 12 insertions(+), 11 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 3fb11bcb4408..8c8304a3ea8f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1534,15 +1534,16 @@ static struct pernet_operations audit_net_ops __net_initdata = {
/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
static void __init audit_enable(void)
{
- long val;
-
if (!audit_boot)
return;

- if (kstrtol(audit_boot, 0, &val))
+ if (!strcmp(audit_boot, "1") || !strcmp(audit_boot, "on"))
+ audit_default = AUDIT_ON;
+ else if (!strcmp(audit_boot, "0") || !strcmp(audit_boot, "off"))
+ audit_default = AUDIT_OFF;
+ else
panic("audit: invalid 'audit' parameter value (%s)\n",
audit_boot);
- audit_default = (val ? AUDIT_ON : AUDIT_OFF);

if (audit_default == AUDIT_OFF)
audit_initialized = AUDIT_DISABLED;

[Richard Guy Briggs]

Post by Greg Edwards
One of our CI tests was booting upstream kernels with the "audit=off" kernel
parameter. This was our error; it should have been "audit=0". However,
in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
("audit: don't use simple_strtol() anymore"), and our errant boot parameter
value starting panic'ing the system.
The problem is this happens so early in boot, the console isn't initialized yet
and you don't see the panic message. You have no idea what the problem is
unless you add an "earlyprintk" boot option, e.g.
earlyprintk=serial,ttyS0,115200n8.
Fix this by having the boot parameter setup function just save the boot
parameter value, and process it later from a call in audit_init(). The console
is initialized by this point, and you can see any panic messages without having
to use an earlyprintk option.

This part all looks good.

Post by Greg Edwards
Additionally, add "on" and "off" as valid audit boot parameter values.

This part is a step in the right direction, but I've got minor concerns
about variations on "0" and "1" that will no longer work, since any
non-zero integer worked previously and will no longer do so.

I would have still used the integer conversion but checked explicitly
for "on" and "off" prior to testing for an integer.

Post by Greg Edwards
audit: move processing of "audit" boot param to audit_init()
audit: add "on"/"off" as valid boot parameter values
Documentation/admin-guide/kernel-parameters.txt | 14 +++----
kernel/audit.c | 49 ++++++++++++++++---------
2 files changed, 39 insertions(+), 24 deletions(-)

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[Paul Moore]

Post by Richard Guy Briggs

Post by Greg Edwards
One of our CI tests was booting upstream kernels with the "audit=off" kernel
parameter. This was our error; it should have been "audit=0". However,
in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
("audit: don't use simple_strtol() anymore"), and our errant boot parameter
value starting panic'ing the system.
The problem is this happens so early in boot, the console isn't initialized yet
and you don't see the panic message. You have no idea what the problem is
unless you add an "earlyprintk" boot option, e.g.
earlyprintk=serial,ttyS0,115200n8.

Ah ha, that helps explain things - thank you!

Post by Richard Guy Briggs

Post by Greg Edwards
Fix this by having the boot parameter setup function just save the boot
parameter value, and process it later from a call in audit_init(). The console
is initialized by this point, and you can see any panic messages without having
to use an earlyprintk option.

This part all looks good.

I had forgotten how tricky this code can be ... I believe there are a
few issues with patch 1/2 and how it initializes audit (it breaks
auditing for PID 1), but I need to double check a few things first.

Post by Richard Guy Briggs

Post by Greg Edwards
Additionally, add "on" and "off" as valid audit boot parameter values.

This part is a step in the right direction, but I've got minor concerns
about variations on "0" and "1" that will no longer work, since any
non-zero integer worked previously and will no longer do so.
I would have still used the integer conversion but checked explicitly
for "on" and "off" prior to testing for an integer.

I agree with Richard that testing for "on"/"off" independently, and
first, would be a good idea.

I also just realized that we probably can't default to enabling audit,
at least not how I was thinking anyway.

Anyway, a bit of an apology, I had hoped to review this before I ended
my day today, but I didn't leave myself enough time ... I'll try to
provide proper feedback this weekend, if that doesn't happen you
should see something early next week.

Thanks again for your help thus far, additional hands are always welcome!

Post by Richard Guy Briggs

Post by Greg Edwards
audit: move processing of "audit" boot param to audit_init()
audit: add "on"/"off" as valid boot parameter values
Documentation/admin-guide/kernel-parameters.txt | 14 +++----
kernel/audit.c | 49 ++++++++++++++++---------
2 files changed, 39 insertions(+), 24 deletions(-)

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Greg Edwards
One of our CI tests was booting upstream kernels with the "audit=off" kernel
parameter. This was our error; it should have been "audit=0". However,
in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
("audit: don't use simple_strtol() anymore"), and our errant boot parameter
value starting panic'ing the system.
The problem is this happens so early in boot, the console isn't initialized yet
and you don't see the panic message. You have no idea what the problem is
unless you add an "earlyprintk" boot option, e.g.
earlyprintk=serial,ttyS0,115200n8.

Ah ha, that helps explain things - thank you!

Post by Richard Guy Briggs

Post by Greg Edwards
Fix this by having the boot parameter setup function just save the boot
parameter value, and process it later from a call in audit_init(). The console
is initialized by this point, and you can see any panic messages without having
to use an earlyprintk option.

This part all looks good.

I had forgotten how tricky this code can be ... I believe there are a
few issues with patch 1/2 and how it initializes audit (it breaks
auditing for PID 1), but I need to double check a few things first.

I checked (though didn't test) that and I had believed it was fine since
postcore_initcall still determines when audit_init(). However, this
does move audit_enable initialization back to audit_init() in the
initcalls list from its place in __setup() which effectively reverts the
change made in 173743dd99a49c956b124a74c8aacb0384739a4c ("audit: ensure
that 'audit=1' actually enables audit for PID 1") that corrected the
early init messages lost issue.
(See: https://github.com/linux-audit/audit-kernel/issues/66)

Moving from __initcall() to postcore_initcall() still happens after PID
1 init, so this is irrelevant as I should have remembered from last
Hallowe'en's discussion.

So, I agree with Paul that the initialization of audit_enable must be
kept in the call from __setup() so that by the time PID 1 is launched
before any of the initcalls, its value can allow TIF_SYSCALL_AUDIT to be
set in task initialization and permit PID 1 to be audited. Greg, I
originally pictured you leaving audit_enable set in that __setup() call
and store the kernel init audit= command line parameter and a flag (I
suppose the non-NULL pointer to the audit= parameter would do that) and
a specific message to report later if there was an error.

So, with a nod to Paul, I'll retract my "looks good".

Post by Paul Moore

Post by Richard Guy Briggs

Post by Greg Edwards
Additionally, add "on" and "off" as valid audit boot parameter values.

This part is a step in the right direction, but I've got minor concerns
about variations on "0" and "1" that will no longer work, since any
non-zero integer worked previously and will no longer do so.
I would have still used the integer conversion but checked explicitly
for "on" and "off" prior to testing for an integer.

I agree with Richard that testing for "on"/"off" independently, and
first, would be a good idea.
I also just realized that we probably can't default to enabling audit,
at least not how I was thinking anyway.
Anyway, a bit of an apology, I had hoped to review this before I ended
my day today, but I didn't leave myself enough time ... I'll try to
provide proper feedback this weekend, if that doesn't happen you
should see something early next week.
Thanks again for your help thus far, additional hands are always welcome!

Post by Richard Guy Briggs

Post by Greg Edwards
audit: move processing of "audit" boot param to audit_init()
audit: add "on"/"off" as valid boot parameter values
Documentation/admin-guide/kernel-parameters.txt | 14 +++----
kernel/audit.c | 49 ++++++++++++++++---------
2 files changed, 39 insertions(+), 24 deletions(-)

- RGB

paul moore

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635