Re: audit regressions in 4.11

[Paul Moore <paul@paul-moore.com>]

On Sun, Apr 9, 2017 at 10:40 AM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> On Sun, Apr 09, 2017 at 09:14:03AM -0400, Paul Moore wrote:
>> On Sat, Apr 8, 2017 at 11:02 PM, Seth Forshee
>> <seth.forshee@canonical.com> wrote:
>> > I've observed audit regressions in 4.11-rc when not using a userspace
>> > audit daemon. The most obvious issue is that audit messages are not
>> > appearing in dmesg anymore. If a sufficient number of audit messages are
>> > generated the kernel will also start invoking the OOM killer.
>> >
>> > It looks like previously, when there's no auditd in userspace kauditd
>> > would call kauditd_hold_skb(), which prints the message using printk and
>> > either frees the skb or queues it (with a limit on the number of queued
>> > skb's by default).
>> >
>> > Since 5b52330bbfe6 "audit: fix auditd/kernel connection state tracking"
>> > when there's no auditd kauditd will instead use the retry queue, which
>> > has no limit. But it will not process the retry queue when there's no
>> > auditd, so messages pile up there indefinitely.
>>
>> Hi Seth,
>>
>> Thanks for the report.  Let me play with this and think on it for a
>> bit, but looking at the code again I think the issue is that we check
>> to see if auditd is connected at the top of the kauditd_thread() loop
>> and if it isn't we skip right to the main_queue label and bypass the
>> hold/retry queue processing which has the logic to ensure the retry
>> queue is managed correctly.  My initial thinking is that the fix is to
>> check and see if auditd is connected in kauditd_retry_skb(), if it
>> isn't we skip the retry queue and call kauditd_hold_skb(), if auditd
>> is connected we add the record to the retry queue (what we currently
>> do).
>
> Yeah, my first thought was to make this change:
>
>                 kauditd_send_queue(sk, portid, &audit_queue, 1,
>                                    kauditd_send_multicast_skb,
> -                                  kauditd_retry_skb);
> +                                  sk ? kauditd_retry_skb : kauditd_hold_skb);
>
> However some scenarios could result in unbounded queueing on the hold
> queue as well, so I'm not sure if that's quite enough.

At the moment I think I'd prefer to put the auditd check inside
kauditd_retry_skb() itself, but you've got the basic idea.

Keep in mind that kauditd_hold_skb() already has the logic inside
itself to prevent the hold queue from growing out of control.

-- 
paul moore
www.paul-moore.com