On Tue, Feb 27, 2018 at 10:59 AM, Greg Edwards <gedwa...@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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
