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

2018-02-25 Thread Richard Guy Briggs 
On 2018-02-23 18:58, Paul Moore wrote:
> On Fri, Feb 23, 2018 at 11:07 AM, Richard Guy Briggs  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 
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 0/2] audit boot parameter cleanups

2018-02-23 Thread Paul Moore 
On Fri, Feb 23, 2018 at 11:07 AM, Richard Guy Briggs  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 
> 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

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

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

2018-02-23 Thread Richard Guy Briggs 
On 2018-02-22 17:22, Greg Edwards wrote:
> One of our CI tests was booting upstream kernels with the "audit=off" kernel
> parameter.  This was our error; it should have been "audit=0".  However,
> in 4.15 the verification of the boot parameter got more strict in 80ab4df62706
> ("audit: don't use simple_strtol() anymore"), and our errant boot parameter
> value starting panic'ing the system.
> 
> The problem is this happens so early in boot, the console isn't initialized 
> yet
> and you don't see the panic message.  You have no idea what the problem is
> unless you add an "earlyprintk" boot option, e.g.
> earlyprintk=serial,ttyS0,115200n8.
> 
> Fix this by having the boot parameter setup function just save the boot
> parameter value, and process it later from a call in audit_init().  The 
> console
> is initialized by this point, and you can see any panic messages without 
> having
> to use an earlyprintk option.

This part all looks good.

> Additionally, add "on" and "off" as valid audit boot parameter values.

This part is a step in the right direction, but I've got minor concerns
about variations on "0" and "1" that will no longer work, since any
non-zero integer worked previously and will no longer do so.

I would have still used the integer conversion but checked explicitly
for "on" and "off" prior to testing for an integer.

> Greg Edwards (2):
>   audit: move processing of "audit" boot param to audit_init()
>   audit: add "on"/"off" as valid boot parameter values
> 
>  Documentation/admin-guide/kernel-parameters.txt | 14 +++
>  kernel/audit.c  | 49 
> -
>  2 files changed, 39 insertions(+), 24 deletions(-)

- RGB

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