[PATCH] audit: do not panic kernel on invalid audit parameter

[Paul Moore]

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.

I'm guessing the problem is that there was too much info dumped to the
console and the error message was lost (there is one, to say there is
"no output" isn't completely correct), is that what happened? Or was
there honestly *no* output on the console?

This seems overly harsh. Instead, print the error indicating an invalid
audit parameter value and leave auditing disabled.

There are some audit requirements which appear rather bizarre at
times, e.g. the need to panic the kernel instead of losing an audit
event. Steve is the one who follows most of these audit requirements
so I'm going to wait until he has a chance to look at this.

There is also another issue in this patch, on error you have the audit
subsystem default to off, we may want to change this to default to on
in case of error (fail safely).

Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..d8af7682d6a3 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
{
long val;
- if (kstrtol(str, 0, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_OFF;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);
if (audit_default == AUDIT_OFF)
--
2.14.3

[Paul Moore]

Post by Paul Moore

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.

I'm guessing the problem is that there was too much info dumped to the
console and the error message was lost (there is one, to say there is
"no output" isn't completely correct), is that what happened? Or was
there honestly *no* output on the console?

Booting a 4.16-rc2 VM with defconfig + kvmconfig with the 'audit=off'
.
Not terribly enlightening.

Oooo, fun.

I wonder if the call to panic() is happening before the kernel
initializes the console. Ungh.

I'll have to play with this some, but if that is the case we may not
be able to display anything meaningful, and we may just have to
default to "on".

Post by Paul Moore

This seems overly harsh. Instead, print the error indicating an invalid
audit parameter value and leave auditing disabled.

There are some audit requirements which appear rather bizarre at
times, e.g. the need to panic the kernel instead of losing an audit
event. Steve is the one who follows most of these audit requirements
so I'm going to wait until he has a chance to look at this.
There is also another issue in this patch, on error you have the audit
subsystem default to off, we may want to change this to default to on
in case of error (fail safely).

Sure, that is fine. I just took a stab at what to do for the error
case. I'm happy to default it to enabled, if that would be more
appropriate.
Greg

[Richard Guy Briggs]

Post by Paul Moore

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.

I'm guessing the problem is that there was too much info dumped to the
console and the error message was lost (there is one, to say there is
"no output" isn't completely correct), is that what happened? Or was
there honestly *no* output on the console?

This seems overly harsh. Instead, print the error indicating an invalid
audit parameter value and leave auditing disabled.

There are some audit requirements which appear rather bizarre at
times, e.g. the need to panic the kernel instead of losing an audit
event. Steve is the one who follows most of these audit requirements
so I'm going to wait until he has a chance to look at this.
There is also another issue in this patch, on error you have the audit
subsystem default to off, we may want to change this to default to on
in case of error (fail safely).

Like Paul, I would have to support the default to on in case of error.

Post by Paul Moore

Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..d8af7682d6a3 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
{
long val;
- if (kstrtol(str, 0, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_OFF;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);
if (audit_default == AUDIT_OFF)
--
2.14.3

--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

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

[Greg Edwards]

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.

Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.

Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
Signed-off-by: Greg Edwards <***@ddn.com>
---
Changes v1 -> v2:
- default to auditing enabled for the error case

kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..9b80e9895107 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
{
long val;

- if (kstrtol(str, 0, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_ON;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);

if (audit_default == AUDIT_OFF)

[Paul Moore]

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.
Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.
Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
---
- default to auditing enabled for the error case
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Thanks for the quick follow-up, it's actually a little *too* quick if I'm honest, I still haven't fully thought through all the different options here :)

However, in the interest in capitalizing on your enthusiasm and willingness to help, here are some of the things I was thinking about, in no particular order:

#1 - We might want to consider accepting both "0" and "off" as acceptable inputs. It should be a trivial change, and if we are going to default to on/enabled it seems like we should make a reasonable effort to do the right thing when people attempt to disable audit (unfortunately the kernel command line parameters seem to use both "0" and "off" so we can't blame people too much when they use "off").

#2 - If panic("<msg>") doesn't work, does pr_err("<msg>")? If it does, I would be curious to understand why.

#3 - Related to #2 above, but there are other calls to panic() and pr_*() in audit_enable() that should probably be re-evaluated and changed. If we can't notify users/admins here, why are we trying?

#4 - Related to #2 and #3, if we can't emit messages in audit_enable() we need to find a way to "remember" that the user specified a bogus audit setting and let them know as soon as we can. One possibility might be to overload the audit_default variable (most places seem to treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value (make it non-zero, say "3"?) and we could display a message in audit_init() or similar. Full disclosure, this *should* work ... I think ... but I might be missing some crucial detail.

I realize this is probably much more than you bargained for when you first submitted your patch, and if you're not interested in taking this any further I understand .... however, if you are willing to play a bit more I would be very grateful :)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..9b80e9895107 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
{
long val;
- if (kstrtol(str, 0, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_ON;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);
if (audit_default == AUDIT_OFF)
--
2.14.3

--
paul moore
www.paul-moore.com

[Steve Grubb]

Post by Paul Moore

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.
Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.
Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
---
- default to auditing enabled for the error case
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Thanks for the quick follow-up, it's actually a little *too* quick if I'm
honest, I still haven't fully thought through all the different options
here :)
However, in the interest in capitalizing on your enthusiasm and willingness
to help, here are some of the things I was thinking about, in no
#1 - We might want to consider accepting both "0" and "off" as acceptable
inputs. It should be a trivial change, and if we are going to default to
on/enabled it seems like we should make a reasonable effort to do the
right thing when people attempt to disable audit (unfortunately the kernel
command line parameters seem to use both "0" and "off" so we can't blame
people too much when they use "off").
#2 - If panic("<msg>") doesn't work, does pr_err("<msg>")? If it does, I
would be curious to understand why.
#3 - Related to #2 above, but there are other calls to panic() and pr_*()
in audit_enable() that should probably be re-evaluated and changed. If we
can't notify users/admins here, why are we trying?
#4 - Related to #2 and #3, if we can't emit messages in audit_enable() we
need to find a way to "remember" that the user specified a bogus audit
setting and let them know as soon as we can. One possibility might be to
overload the audit_default variable (most places seem to treat it as a
true/false value) with a "AUDIT_DEFAULT_INVALID" value (make it non-zero,
say "3"?) and we could display a message in audit_init() or similar. Full
disclosure, this *should* work ... I think ... but I might be missing some
crucial detail.

Well, auditd will probably have a big problem starting up and that should be
a big clue. Also, this could be remembered in a way that a fault indication
is returned by auditctl -s? Loading audit rules leads to checking audit
status which journald keeps around.

-Steve

Post by Paul Moore
I realize this is probably much more than you bargained for when you first
submitted your patch, and if you're not interested in taking this any
further I understand .... however, if you are willing to play a bit more I
would be very grateful :)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..9b80e9895107 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1572,8 +1572,10 @@ static int __init audit_enable(char *str)
{
long val;
- if (kstrtol(str, 0, &val))
- panic("audit: invalid 'audit' parameter value (%s)\n", str);
+ if (kstrtol(str, 0, &val)) {
+ pr_err("invalid 'audit' parameter value (%s)\n", str);
+ val = AUDIT_ON;
+ }
audit_default = (val ? AUDIT_ON : AUDIT_OFF);
if (audit_default == AUDIT_OFF)

--
paul moore
www.paul-moore.com

[Greg Edwards]

Post by Paul Moore

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.
Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.

Thanks for the quick follow-up, it's actually a little *too* quick if
I'm honest, I still haven't fully thought through all the different
options here :)
However, in the interest in capitalizing on your enthusiasm and
willingness to help, here are some of the things I was thinking about,
#1 - We might want to consider accepting both "0" and "off" as
acceptable inputs. It should be a trivial change, and if we are going
to default to on/enabled it seems like we should make a reasonable
effort to do the right thing when people attempt to disable audit
(unfortunately the kernel command line parameters seem to use both "0"
and "off" so we can't blame people too much when they use "off").

Yes, I think this would be a good idea, and for what it's worth,
'audit=off' worked until 4.15. One of our CI tests that verifies
upstream kernels picked this up starting with 4.15.

For example, booting a 4.14.20 kernel (defconfig + kvmconfig):

[ 0.000000] Linux version 4.14.20 (***@psuche) (gcc version 7.3.1 20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:14:25 M
ST 2018
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: disabled (until reboot)
^^^^^^^^

Post by Paul Moore
#2 - If panic("<msg>") doesn't work, does pr_err("<msg>")? If it
does, I would be curious to understand why.

Yes, pr_err() does work. Booting 4.16-rc2 (defconfig + kvmconfig) with
this patch:

[ 0.000000] Linux version 4.16.0-rc2+ (***@psuche) (gcc version 7.3.1 20180130 (Red Hat 7.3.1-2) (GCC)) #1 SMP Wed Feb 21 15:23:10 MST 2018
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: invalid 'audit' parameter value (off)
[ 0.000000] audit: enabled (after initialization)


I suspect what is happening with the current audit_enable() code is the
serial console has not been fully initialized yet by the time we call
panic(), so we never see the early printk messages queued up. I will
try and confirm.

Post by Paul Moore
#3 - Related to #2 above, but there are other calls to panic() and
pr_*() in audit_enable() that should probably be re-evaluated and
changed. If we can't notify users/admins here, why are we trying?

I haven't looked at those other calls to panic(), but I would bet they
display the same behavior.

Post by Paul Moore
#4 - Related to #2 and #3, if we can't emit messages in audit_enable()
we need to find a way to "remember" that the user specified a bogus
audit setting and let them know as soon as we can. One possibility
might be to overload the audit_default variable (most places seem to
treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value
(make it non-zero, say "3"?) and we could display a message in
audit_init() or similar. Full disclosure, this *should* work ... I
think ... but I might be missing some crucial detail.

I'm unclear why we would need this, given that #2 above does work. This
is the first time I've ever looked at the audit code, though. I was
just doing a drive-by. ;)

Post by Paul Moore
I realize this is probably much more than you bargained for when you
first submitted your patch, and if you're not interested in taking
this any further I understand .... however, if you are willing to play
a bit more I would be very grateful :)

Sure, I'm happy to look at the above.

Greg

[Richard Guy Briggs]

Post by Greg Edwards

Post by Paul Moore

If you pass in an invalid audit kernel boot parameter, e.g. 'audit=off',
the kernel panics very early in boot with no output on the console
indicating the problem.
Instead, print the error indicating an invalid audit parameter value,
but leave auditing enabled.

Thanks for the quick follow-up, it's actually a little *too* quick if
I'm honest, I still haven't fully thought through all the different
options here :)
However, in the interest in capitalizing on your enthusiasm and
willingness to help, here are some of the things I was thinking about,
#1 - We might want to consider accepting both "0" and "off" as
acceptable inputs. It should be a trivial change, and if we are going
to default to on/enabled it seems like we should make a reasonable
effort to do the right thing when people attempt to disable audit
(unfortunately the kernel command line parameters seem to use both "0"
and "off" so we can't blame people too much when they use "off").

Yes, I think this would be a good idea, and for what it's worth,
'audit=off' worked until 4.15. One of our CI tests that verifies
upstream kernels picked this up starting with 4.15.

Huh, at first I wondered if the earlier audit init was at play here, but
now I'm suspecting
80ab4df62706b882922c3bb0b05ce2c8ab10828a
("audit: don't use simple_strtol() anymore")
is the primary culprit, exacerbated by earlier init from the same
patchset.

Post by Greg Edwards
ST 2018
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: disabled (until reboot)
^^^^^^^^

Post by Paul Moore
#2 - If panic("<msg>") doesn't work, does pr_err("<msg>")? If it
does, I would be curious to understand why.

Yes, pr_err() does work. Booting 4.16-rc2 (defconfig + kvmconfig) with
[ 0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
...
[ 0.000000] Kernel command line: root=/dev/vda1 console=ttyS0,115200n8 audit=off
[ 0.000000] audit: invalid 'audit' parameter value (off)
[ 0.000000] audit: enabled (after initialization)
I suspect what is happening with the current audit_enable() code is the
serial console has not been fully initialized yet by the time we call
panic(), so we never see the early printk messages queued up. I will
try and confirm.

Post by Paul Moore
#3 - Related to #2 above, but there are other calls to panic() and
pr_*() in audit_enable() that should probably be re-evaluated and
changed. If we can't notify users/admins here, why are we trying?

I haven't looked at those other calls to panic(), but I would bet they
display the same behavior.

Post by Paul Moore
#4 - Related to #2 and #3, if we can't emit messages in audit_enable()
we need to find a way to "remember" that the user specified a bogus
audit setting and let them know as soon as we can. One possibility
might be to overload the audit_default variable (most places seem to
treat it as a true/false value) with a "AUDIT_DEFAULT_INVALID" value
(make it non-zero, say "3"?) and we could display a message in
audit_init() or similar. Full disclosure, this *should* work ... I
think ... but I might be missing some crucial detail.

I'm unclear why we would need this, given that #2 above does work. This
is the first time I've ever looked at the audit code, though. I was
just doing a drive-by. ;)

Post by Paul Moore
I realize this is probably much more than you bargained for when you
first submitted your patch, and if you're not interested in taking
this any further I understand .... however, if you are willing to play
a bit more I would be very grateful :)

Sure, I'm happy to look at the above.
Greg

- RGB

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

[Greg Edwards]

If you pass in an invalid audit boot parameter value, e.g. "audit=off",
the kernel panics very early in boot before the regular console is
initialized. Unless you have earlyprintk enabled, there is no
indication of what the problem is on the console.

Convert the panic() calls to pr_err(), and leave auditing enabled if an
invalid parameter value was passed in.

Modify the parameter to also accept "on" or "off" as valid values, and
update the documentation accordingly.

Signed-off-by: Greg Edwards <***@ddn.com>
---
Changes v2 -> v3:
- convert panic() calls to pr_err()
- add handling of "on"/"off" as valid values
- update documentation

Changes v1 -> v2:
- default to auditing enabled for the error case

Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 21 ++++++++++++++-------
2 files changed, 21 insertions(+), 14 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 227db99b0f19..8fccea5ded71 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1567,19 +1567,26 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);

-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+/*
+ * Process kernel command-line parameter at boot time.
+ * audit={0|off} or audit={1|on}.
+ */
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)");

[Richard Guy Briggs]

Post by Greg Edwards
If you pass in an invalid audit boot parameter value, e.g. "audit=off",
the kernel panics very early in boot before the regular console is
initialized. Unless you have earlyprintk enabled, there is no
indication of what the problem is on the console.
Convert the panic() calls to pr_err(), and leave auditing enabled if an
invalid parameter value was passed in.
Modify the parameter to also accept "on" or "off" as valid values, and
update the documentation accordingly.
---
- convert panic() calls to pr_err()
- add handling of "on"/"off" as valid values
- update documentation
- default to auditing enabled for the error case
Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 21 ++++++++++++++-------
2 files changed, 21 insertions(+), 14 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 227db99b0f19..8fccea5ded71 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1567,19 +1567,26 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);
-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+/*
+ * Process kernel command-line parameter at boot time.
+ * audit={0|off} or audit={1|on}.
+ */
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);

This patch looks good.
However, I wonder if this second panic should be left alone, since it
isn't related to the two audit_default options above.
audit_set_enabled() can't be sent AUDIT_LOCKED from here, there must be
an error returned from looking up the security context when trying to
log the config change. There is already an audit_panic when that is
detected, but this is so early that audit_panic won't be configured to
panic yet and defaults to printk. If it is also so early that no LSMs
have been loaded yet then this concern is moot. There is still the
question of just how useful it is to panic this early.

Post by Greg Edwards
pr_info("%s\n", audit_default ?
"enabled (after initialization)" : "disabled (until reboot)");
--
2.14.3

- RGB

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

[Paul Moore]

Post by Richard Guy Briggs

Post by Greg Edwards
If you pass in an invalid audit boot parameter value, e.g. "audit=off",
the kernel panics very early in boot before the regular console is
initialized. Unless you have earlyprintk enabled, there is no
indication of what the problem is on the console.
Convert the panic() calls to pr_err(), and leave auditing enabled if an
invalid parameter value was passed in.
Modify the parameter to also accept "on" or "off" as valid values, and
update the documentation accordingly.
---
- convert panic() calls to pr_err()
- add handling of "on"/"off" as valid values
- update documentation
- default to auditing enabled for the error case
Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 21 ++++++++++++++-------
2 files changed, 21 insertions(+), 14 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 227db99b0f19..8fccea5ded71 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1567,19 +1567,26 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);
-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+/*
+ * Process kernel command-line parameter at boot time.
+ * audit={0|off} or audit={1|on}.
+ */
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);

This patch looks good.

On quick glance, I agree. I'll look at it a bit closer later today
and likely merge it.

Thanks Greg.

Post by Richard Guy Briggs
However, I wonder if this second panic should be left alone, since it
isn't related to the two audit_default options above.

There is still the silent-panic problem that started this entire
discussion/patch. If we really need to panic() here (and I currently
don't think we need to panic), then we need to devise another solution
(see the previous patches that we discussed).

Post by Richard Guy Briggs
audit_set_enabled() can't be sent AUDIT_LOCKED from here, there must be
an error returned from looking up the security context when trying to
log the config change.

Keep in mind that we aren't actually logging anything here as
audit_initialized isn't set to AUDIT_INITIALIZED yet. We're using
audit_set_enabled() simply so we consolidate all of the enable/disable
code in one place (see the original commit where I switched it over to
use audit_set_enabled()).

Post by Richard Guy Briggs
There is already an audit_panic when that is
detected, but this is so early that audit_panic won't be configured to
panic yet and defaults to printk. If it is also so early that no LSMs
have been loaded yet then this concern is moot. There is still the
question of just how useful it is to panic this early.

Not an issue, we are never going to get past audit_log_start() as we
haven't initialized the audit subsystem yet.

Post by Richard Guy Briggs

Post by Greg Edwards
pr_info("%s\n", audit_default ?
"enabled (after initialization)" : "disabled (until reboot)");
--
2.14.3

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

[Paul Moore]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Greg Edwards
If you pass in an invalid audit boot parameter value, e.g. "audit=off",
the kernel panics very early in boot before the regular console is
initialized. Unless you have earlyprintk enabled, there is no
indication of what the problem is on the console.
Convert the panic() calls to pr_err(), and leave auditing enabled if an
invalid parameter value was passed in.
Modify the parameter to also accept "on" or "off" as valid values, and
update the documentation accordingly.
---
- convert panic() calls to pr_err()
- add handling of "on"/"off" as valid values
- update documentation
- default to auditing enabled for the error case
Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 21 ++++++++++++++-------
2 files changed, 21 insertions(+), 14 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 227db99b0f19..8fccea5ded71 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1567,19 +1567,26 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);
-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+/*
+ * Process kernel command-line parameter at boot time.
+ * audit={0|off} or audit={1|on}.
+ */
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);

This patch looks good.

On quick glance, I agree. I'll look at it a bit closer later today
and likely merge it.
Thanks Greg.

It's merge now. Thanks again everyone!

[Richard Guy Briggs]

Post by Paul Moore

Post by Paul Moore

Post by Richard Guy Briggs

Post by Greg Edwards
If you pass in an invalid audit boot parameter value, e.g. "audit=off",
the kernel panics very early in boot before the regular console is
initialized. Unless you have earlyprintk enabled, there is no
indication of what the problem is on the console.
Convert the panic() calls to pr_err(), and leave auditing enabled if an
invalid parameter value was passed in.
Modify the parameter to also accept "on" or "off" as valid values, and
update the documentation accordingly.
---
- convert panic() calls to pr_err()
- add handling of "on"/"off" as valid values
- update documentation
- default to auditing enabled for the error case
Documentation/admin-guide/kernel-parameters.txt | 14 +++++++-------
kernel/audit.c | 21 ++++++++++++++-------
2 files changed, 21 insertions(+), 14 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 227db99b0f19..8fccea5ded71 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1567,19 +1567,26 @@ static int __init audit_init(void)
}
postcore_initcall(audit_init);
-/* Process kernel command-line parameter at boot time. audit=0 or audit=1. */
+/*
+ * Process kernel command-line parameter at boot time.
+ * audit={0|off} or audit={1|on}.
+ */
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);

This patch looks good.

On quick glance, I agree. I'll look at it a bit closer later today
and likely merge it.
Thanks Greg.

It's merge now. Thanks again everyone!
paul moore

- RGB

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