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

[Greg Edwards]

On Tue, Feb 20, 2018 at 04:45:26PM -0500, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
>> 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'
boot parameter (my mistake), the only output you get is:

.

Not terribly enlightening.

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

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

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

[Richard Guy Briggs]

On 2018-02-20 16:45, Paul Moore wrote:
> On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
> > 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.

> > Fixes: 80ab4df62706 ("audit: don't use simple_strtol() anymore")
> > Signed-off-by: Greg Edwards 
> > ---
> >  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, ))
> > -   panic("audit: invalid 'audit' parameter value (%s)\n", str);
> > +   if (kstrtol(str, 0, )) {
> > +   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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

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

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

[Paul Moore]

On Tue, Feb 20, 2018 at 5:00 PM, Greg Edwards  wrote:
> On Tue, Feb 20, 2018 at 04:45:26PM -0500, Paul Moore wrote:
>> On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
>>> 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'
> boot parameter (my mistake), the only output you get is:
>
> .
>
> 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".

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

-- 
paul moore
www.paul-moore.com

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

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

[Paul Moore]

On Tue, Feb 20, 2018 at 4:33 PM, Greg Edwards  wrote:
> 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")
> Signed-off-by: Greg Edwards 
> ---
>  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, ))
> -   panic("audit: invalid 'audit' parameter value (%s)\n", str);
> +   if (kstrtol(str, 0, )) {
> +   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
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit