[PATCH 0/2] audit boot parameter cleanups

[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(-)

-- 
2.14.3

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

[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 <gedwards@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> */
-- 
2.14.3

[PATCH 2/2] audit: add "on"/"off" as valid boot parameter values

[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 <gedwards@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;
-- 
2.14.3

Re: [PATCH 0/2] audit boot parameter cleanups

[Richard Guy Briggs]

On 2018-02-22 17:22, Greg Edwards wrote:
> 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.

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

> 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(-)

- RGB

--
Richard Guy Briggs <rgb@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

Re: [PATCH 0/2] audit boot parameter cleanups

[Paul Moore]

On Fri, Feb 23, 2018 at 11:07 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-02-22 17:22, Greg Edwards wrote:
>> 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!

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

>> 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!

>> 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(-)
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] audit boot parameter cleanups

[Richard Guy Briggs]

On 2018-02-23 18:58, Paul Moore wrote:
> On Fri, Feb 23, 2018 at 11:07 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-02-22 17:22, Greg Edwards wrote:
> >> 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!
> 
> >> 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".

> >> 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!
> 
> >> 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(-)
> >
> > - RGB
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@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

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

[Paul Moore]

On Thu, Feb 22, 2018 at 7:22 PM, Greg Edwards <gedwards@ddn.com> wrote:
> 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 <gedwards@ddn.com>
> ---
>  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;
}

-- 
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

On 2018-02-26 19:00, Paul Moore wrote:
> On Thu, Feb 22, 2018 at 7:22 PM, Greg Edwards <gedwards@ddn.com> wrote:
> > 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 <gedwards@ddn.com>
> > ---
> >  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.

> 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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@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

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

[Paul Moore]

On Tue, Feb 27, 2018 at 12:53 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-02-26 19:00, Paul Moore wrote:
>> On Thu, Feb 22, 2018 at 7:22 PM, Greg Edwards <gedwards@ddn.com> wrote:
>> > 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 <gedwards@ddn.com>
>> > ---
>> >  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.

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

>> 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
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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
www.paul-moore.com

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

[Greg Edwards]

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

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

[Richard Guy Briggs]

On 2018-02-27 07:43, Paul Moore wrote:
> On Tue, Feb 27, 2018 at 12:53 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-02-26 19:00, Paul Moore wrote:
> >> On Thu, Feb 22, 2018 at 7:22 PM, Greg Edwards <gedwards@ddn.com> wrote:
> >> > 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 <gedwards@ddn.com>
> >> > ---
> >> >  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.

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

> >> 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
> >> Linux-audit@redhat.com
> >> https://www.redhat.com/mailman/listinfo/linux-audit
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@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
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@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

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

[Paul Moore]

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?

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

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

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

-- 
paul moore
www.paul-moore.com

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

[Greg Edwards]

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

Re: [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@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@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

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

[Greg Edwards]

On Fri, Mar 02, 2018 at 03:33:54PM -0500, Paul Moore wrote:
> On Tue, Feb 27, 2018 at 5:52 PM, Greg Edwards <gedwards@ddn.com> wrote:
>> 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
>
> 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.

Greg

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

[Paul Moore]

On Fri, Mar 2, 2018 at 4:46 PM, Greg Edwards <gedwards@ddn.com> wrote:
> On Fri, Mar 02, 2018 at 03:33:54PM -0500, Paul Moore wrote:
>> On Tue, Feb 27, 2018 at 5:52 PM, Greg Edwards <gedwards@ddn.com> wrote:
>>> 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
>>
>> 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? ;)

-- 
paul moore
www.paul-moore.com