* [PATCH 0/2] audit boot parameter cleanups @ 2018-02-23 0:22 Greg Edwards 2018-02-23 0:22 ` [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() Greg Edwards ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Greg Edwards @ 2018-02-23 0:22 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-23 0:22 [PATCH 0/2] audit boot parameter cleanups Greg Edwards @ 2018-02-23 0:22 ` Greg Edwards 2018-02-27 0:00 ` Paul Moore 2018-02-23 0:22 ` [PATCH 2/2] audit: add "on"/"off" as valid boot parameter values Greg Edwards 2018-02-23 16:07 ` [PATCH 0/2] audit boot parameter cleanups Richard Guy Briggs 2 siblings, 1 reply; 16+ messages in thread From: Greg Edwards @ 2018-02-23 0:22 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-23 0:22 ` [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() Greg Edwards @ 2018-02-27 0:00 ` Paul Moore 2018-02-27 5:53 ` Richard Guy Briggs 2018-02-27 15:59 ` Greg Edwards 0 siblings, 2 replies; 16+ messages in thread From: Paul Moore @ 2018-02-27 0:00 UTC (permalink / raw) To: Greg Edwards, Richard Guy Briggs; +Cc: linux-audit 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 0:00 ` Paul Moore @ 2018-02-27 5:53 ` Richard Guy Briggs 2018-02-27 12:43 ` Paul Moore 2018-02-27 15:59 ` Greg Edwards 1 sibling, 1 reply; 16+ messages in thread From: Richard Guy Briggs @ 2018-02-27 5:53 UTC (permalink / raw) To: Paul Moore; +Cc: Greg Edwards, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 5:53 ` Richard Guy Briggs @ 2018-02-27 12:43 ` Paul Moore 2018-02-27 19:01 ` Richard Guy Briggs 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2018-02-27 12:43 UTC (permalink / raw) To: Richard Guy Briggs; +Cc: Greg Edwards, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 12:43 ` Paul Moore @ 2018-02-27 19:01 ` Richard Guy Briggs 0 siblings, 0 replies; 16+ messages in thread From: Richard Guy Briggs @ 2018-02-27 19:01 UTC (permalink / raw) To: Paul Moore; +Cc: Greg Edwards, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 0:00 ` Paul Moore 2018-02-27 5:53 ` Richard Guy Briggs @ 2018-02-27 15:59 ` Greg Edwards 2018-02-27 22:28 ` Paul Moore 1 sibling, 1 reply; 16+ messages in thread From: Greg Edwards @ 2018-02-27 15:59 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 15:59 ` Greg Edwards @ 2018-02-27 22:28 ` Paul Moore 2018-02-27 22:52 ` Greg Edwards 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2018-02-27 22:28 UTC (permalink / raw) To: Greg Edwards; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 22:28 ` Paul Moore @ 2018-02-27 22:52 ` Greg Edwards 2018-03-02 20:33 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Greg Edwards @ 2018-02-27 22:52 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-02-27 22:52 ` Greg Edwards @ 2018-03-02 20:33 ` Paul Moore 2018-03-02 21:46 ` Greg Edwards 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2018-03-02 20:33 UTC (permalink / raw) To: Greg Edwards; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-03-02 20:33 ` Paul Moore @ 2018-03-02 21:46 ` Greg Edwards 2018-03-02 22:30 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Greg Edwards @ 2018-03-02 21:46 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() 2018-03-02 21:46 ` Greg Edwards @ 2018-03-02 22:30 ` Paul Moore 0 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2018-03-02 22:30 UTC (permalink / raw) To: Greg Edwards; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] audit: add "on"/"off" as valid boot parameter values 2018-02-23 0:22 [PATCH 0/2] audit boot parameter cleanups Greg Edwards 2018-02-23 0:22 ` [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() Greg Edwards @ 2018-02-23 0:22 ` Greg Edwards 2018-02-23 16:07 ` [PATCH 0/2] audit boot parameter cleanups Richard Guy Briggs 2 siblings, 0 replies; 16+ messages in thread From: Greg Edwards @ 2018-02-23 0:22 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] audit boot parameter cleanups 2018-02-23 0:22 [PATCH 0/2] audit boot parameter cleanups Greg Edwards 2018-02-23 0:22 ` [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() Greg Edwards 2018-02-23 0:22 ` [PATCH 2/2] audit: add "on"/"off" as valid boot parameter values Greg Edwards @ 2018-02-23 16:07 ` Richard Guy Briggs 2018-02-23 23:58 ` Paul Moore 2 siblings, 1 reply; 16+ messages in thread From: Richard Guy Briggs @ 2018-02-23 16:07 UTC (permalink / raw) To: Greg Edwards; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] audit boot parameter cleanups 2018-02-23 16:07 ` [PATCH 0/2] audit boot parameter cleanups Richard Guy Briggs @ 2018-02-23 23:58 ` Paul Moore 2018-02-26 4:58 ` Richard Guy Briggs 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2018-02-23 23:58 UTC (permalink / raw) To: Greg Edwards; +Cc: Richard Guy Briggs, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] audit boot parameter cleanups 2018-02-23 23:58 ` Paul Moore @ 2018-02-26 4:58 ` Richard Guy Briggs 0 siblings, 0 replies; 16+ messages in thread From: Richard Guy Briggs @ 2018-02-26 4:58 UTC (permalink / raw) To: Paul Moore; +Cc: Greg Edwards, linux-audit 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-02 22:30 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-23 0:22 [PATCH 0/2] audit boot parameter cleanups Greg Edwards 2018-02-23 0:22 ` [PATCH 1/2] audit: move processing of "audit" boot param to audit_init() Greg Edwards 2018-02-27 0:00 ` Paul Moore 2018-02-27 5:53 ` Richard Guy Briggs 2018-02-27 12:43 ` Paul Moore 2018-02-27 19:01 ` Richard Guy Briggs 2018-02-27 15:59 ` Greg Edwards 2018-02-27 22:28 ` Paul Moore 2018-02-27 22:52 ` Greg Edwards 2018-03-02 20:33 ` Paul Moore 2018-03-02 21:46 ` Greg Edwards 2018-03-02 22:30 ` Paul Moore 2018-02-23 0:22 ` [PATCH 2/2] audit: add "on"/"off" as valid boot parameter values Greg Edwards 2018-02-23 16:07 ` [PATCH 0/2] audit boot parameter cleanups Richard Guy Briggs 2018-02-23 23:58 ` Paul Moore 2018-02-26 4:58 ` Richard Guy Briggs
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.