linux-kernel - Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhT82w2RtdD9EKRgTSUz+tp4Ru=HmmQ8BQwrisoMRMHP1A@mail.gmail.com>
Date:   Tue, 17 Apr 2018 22:01:46 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Steve Grubb <sgrubb@...hat.com>,
        Richard Guy Briggs <rgb@...hat.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Paris <eparis@...hat.com>
Subject: Re: [PATCH ghak80 V1] audit: add syscall information to
 FEATURE_CHANGE records

On Tue, Apr 17, 2018 at 6:10 PM, Steve Grubb <sgrubb@...hat.com> wrote:
> On Tuesday, April 17, 2018 6:06:24 PM EDT Paul Moore wrote:
>> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs <rgb@...hat.com> wrote:
>> > Tie syscall information to FEATURE_CHANGE calls since it is a result of
>> > user action.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/80
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
>> > ---
>> >
>> >  kernel/audit.c | 5 ++---
>> >  1 file changed, 2 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 8da24ef..23f125b 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which,
>> > u32 old_feature, u32 new_feature>
>> >  {
>> >
>> >         struct audit_buffer *ab;
>> >
>> > -       if (audit_enabled == AUDIT_OFF)
>> > +       if (!audit_enabled)
>>
>> Sooo, this is an unrelated style change, why?  Looking at the rest of
>> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so
>> why are you adding noise to this patch?
>>
>> >                 return;
>> >
>> > -
>> > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
>> > +       ab = audit_log_start(current->audit_context, GFP_KERNEL,
>> > AUDIT_FEATURE_CHANGE);
>> This is the important part, and the Right Thing To Do.
>
> This is an unexpected change. I have asked questions on the github issue
> tracker but have not gotten a satisfactory answer. Please do not merge this
> until there's agreement on this.

It shouldn't be surprising, we've been talking about connecting
records for some time now, in different contexts and both on and off
list.  Not only does it helps pave the way for the audit container ID
work, it just makes sense.  I've seen your questions in this
particular GitHub issue and I think Richard has answered them
satisfactorily.

Once Richard removes the style change, or provides a good enough
reason for why it should stay in this patch, I plan on merging this
into audit/next.

-- 
paul moore
www.paul-moore.com