linux-kernel - Re: [RFC PATCH ghak86 V1] audit: use audit

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]

Date:   Fri, 1 Jun 2018 18:15:49 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     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>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient

On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2018-05-31 11:48, Paul Moore wrote:
>> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@...hat.com> wrote:
>> > Most uses of audit_enabled don't care about the distinction between
>> > AUDIT_ON and AUDIT_LOCKED, so using audit_enabled as a boolean makes
>> > more sense and is easier to read. Most uses of audit_enabled treat it as
>> > a boolean, so switch the remaining AUDIT_OFF usage to simply use
>> > audit_enabled as a boolean where applicable.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/86
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
>> > ---
>> >  drivers/tty/tty_audit.c      | 2 +-
>> >  include/net/xfrm.h           | 2 +-
>> >  kernel/audit.c               | 8 ++++----
>> >  net/netfilter/xt_AUDIT.c     | 2 +-
>> >  net/netlabel/netlabel_user.c | 2 +-
>> >  5 files changed, 8 insertions(+), 8 deletions(-)
>>
>> I'm not sure I like this idea.  Yes, technically this change is
>> functionally equivalent but I worry that this will increase the chance
>> that non-audit folks will mistake audit_enabled as a true/false value
>> when it is not.  It might work now, but I worry about some subtle
>> problem in the future.
>
> Would you prefer a patch to change the majority (18) of uses of
> audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or
> "audit_enabled != AUDIT_OFF")?
>
> I prefer the approach in this patch because it makes the code smaller
> and significantly easier to read, but either way, I'd like all uses to
> be consistent so that it is easier to read all the code similarly.
>
>> If you are bothered by the comparison to 0 (magic numbers), you could
>> move the AUDIT_OFF/AUDIT_ON/AUDIT_LOCKED definitions into
>> include/linux/audit.h and convert the "audit_enabled == 0" to
>> "audit_enabled == AUDIT_OFF".
>
> I'd be fine doing that if you really dislike this patch's approach.

Like I said, I'm don't really care for the boolean-like approach of
this first patch.

-- 
paul moore
www.paul-moore.com