linux-kernel - Re: [PATCH] audit: use pid.is_auditd to make auditd_test

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7FCAD214-5E79-4D07-A111-3657E91758B1@nutanix.com>
Date:   Mon, 17 Apr 2023 11:42:15 +0000
From:   Eiichi Tsukata <eiichi.tsukata@...anix.com>
To:     Paul Moore <paul@...l-moore.com>
CC:     "eparis@...hat.com" <eparis@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "audit@...r.kernel.org" <audit@...r.kernel.org>
Subject: Re: [PATCH] audit: use pid.is_auditd to make auditd_test_task()
 faster



> On Apr 14, 2023, at 23:44, Paul Moore <paul@...l-moore.com> wrote:
> 
> On Thu, Apr 13, 2023 at 11:14 PM Eiichi Tsukata
> <eiichi.tsukata@...anix.com> wrote:
>> 
>> auditd_test_task() is a hot path of system call auditing. This patch
>> introduces a new bit field "is_auditd" in pid struct which can be used
>> for faster check of registered audit daemon.
>> 
>> Benchmark
>> =========
>> 
>> Run the following command:
>> 
>>  dd if=/dev/zero of=/dev/null bs=1 count=5M
>> 
>> With rule:
>> 
>>  -a never,exit -F arch=b64 -S uname
>> 
>> Result:
>> 
>>  Base line    : 2.572 sec
>>  /w this patch: 2.412 sec (6.6% faster)
>> 
>> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@...anix.com>
>> ---
>> include/linux/pid.h |  4 ++++
>> kernel/audit.c      | 22 ++--------------------
>> kernel/audit.h      |  3 ++-
>> kernel/pid.c        |  3 +++
>> 4 files changed, 11 insertions(+), 21 deletions(-)
>> 
>> diff --git a/include/linux/pid.h b/include/linux/pid.h
>> index 343abf22092e..5fe38e254c9a 100644
>> --- a/include/linux/pid.h
>> +++ b/include/linux/pid.h
>> @@ -68,6 +68,10 @@ struct pid
>>        wait_queue_head_t wait_pidfd;
>>        struct rcu_head rcu;
>>        struct upid numbers[1];
>> +#ifdef CONFIG_AUDIT
>> +       /* registered audit daemon tgid */
>> +       unsigned is_auditd:1;
>> +#endif
>> };
> 
> Thank you for the patch, but I don't think we want to add an audit
> specific field to the pid struct at this time.
> 

Hi Paul

I agree “is_auditd” is too specific.

How about having global “auditd_pid” struct pid pointer and let auditd_test_task() use it?
I mean:
#define auditd_test_task(tsk) (READ_ONCE(auditd_pid) == task_tgid(tsk))

By the way, it’s a bit different topic,  I may have found a race in usage of auditd_pid_vnr().
 In AUDIT_SET handling, the variable auditd_pid is referenced outside of the spinlock so it can be changed while it’s referenced.
So there is a TOCTOU race like this:

CPU0                                        CPU1
=====                                       =====
auditd = auditd_pid_vnr() 
                                                  auditd = auditd_pid_vnr() 
if (auditd_pid) {…}
                                                  if (auditd_pid) {…}
auditd_set()
                                                  auditd_set()



If auditd_pid_vnr() returns 0, this case can lead to replacement of a healthy auditd, which seems to be prohibited judging from the code comment "/* replacing a healthy auditd is not allowed */“.

Please correct me if I’m wrong.

Thanks
Eiichi