linux-kernel - Re: [PATCH v2 5/5] audit: do not use exclusive wait in 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]

Message-ID: <F900B719-7760-4E22-82A2-933ED775AA19@nutanix.com>
Date:   Tue, 23 May 2023 03:58:30 +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 v2 5/5] audit: do not use exclusive wait in
 audit_receive()



> On May 22, 2023, at 13:44, Eiichi Tsukata <eiichi.tsukata@...anix.com> wrote:
> 
> 
> 
>> On May 20, 2023, at 5:54, Paul Moore <paul@...l-moore.com> wrote:
>> 
>> On May 11, 2023 Eiichi Tsukata <eiichi.tsukata@...anix.com> wrote:
>>> 
>>> kauditd thread issues wake_up() before it goes to sleep. The wake_up()
>>> call wakes up only one process as waiter side uses exclusive wait.
>>> This can be problematic when there are multiple processes (one is in
>>> audit_receive() and others are in audit_log_start()) waiting on
>>> audit_backlog_wait queue.
>>> 
>>> For example, if there are two processes waiting:
>>> 
>>> Process (A): in audit_receive()
>>> Process (B): in audit_log_start()
>>> 
>>> And (A) is at the head of the wait queue. Then kauditd's wake_up() only
>>> wakes up (A) leaving (B) as it is even if @audit_queue is drained. As a
>>> result, (B) can be blocked for up to backlog_wait_time.
>>> 
>>> To prevent the issue, use non-exclusive wait in audit_receive() so that
>>> kauditd can wake up all waiters in audit_receive().
>>> 
>>> Fixes: 8f110f530635 ("audit: ensure userspace is penalized the same as the kernel when under pressure")
>>> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@...anix.com>
>>> ---
>>> kernel/audit.c | 17 +++++++++++------
>>> 1 file changed, 11 insertions(+), 6 deletions(-)
>> 
>> This was also discussed in the last patchset.
>> 
>> 
> 
> This bug is much easily reproducible on real environments and can cause problematic
> user space failure like SSH connection timeout.
> Let’s not keep the bug unfixed.
> (Of course we’ve already carefully tuned audit related params and user space auditd config so that our product won’t hit backlog full.)
> 
> Other ideas in my minds are:
> 
> (1) Use different wait queues in audit_receive() and audit_log_start() to guarantee kautid 
>  wake_up() tries to wake up a waiter in audit_log_start().
> 
> (2) Periodically (say in every 1 sec) check if @audit_queue is full in audit_receive() to prevent 
>  audit_receive() from unnecessarily waiting for so long time. 
> 
> BTW, the default backlog_wait_time is 60 * HZ which seems pretty large.
> I’d appreciate if you could tell me the reason behind that value.
> 
> Eiichi

I came up with a better idea:

(3) Move wait_for_kauditd() in audit_receive() *before* audit_ctl_lock() 
 and restrict penalty only for msg_type which can queue a new audit record. (AUDIT_USER, AUDIT_TRIM, AUDIT_MAKE_EQUIV, ..)

Originally, it’s not reasonable to give penalty for innocent operation
like AUDIT_GET.
This approach makes successive audit_log_end() wake up kauditd.
Also it prevents audit_log_end() from queueing skb ignoring backlog_limit.

Eiichi