Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Paul Moore <pmoore@redhat.com>]

On Monday, December 21, 2015 05:18:15 PM Steve Grubb wrote:
> On Monday, December 21, 2015 04:48:00 PM Paul Moore wrote:
> > On Wednesday, December 16, 2015 11:23:19 AM Steve Grubb wrote:
> > > On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> > > > Nothing prevents a new auditd starting up and replacing a valid
> > > > audit_pid when an old auditd is still running, effectively starving
> > > > out the old auditd since audit_pid no longer points to the old valid
> > > > auditd.
> > > 
> > > I guess the first question is why do we allow something to start up a
> > > new auditd without killing off the old one?  Would that be a simpler
> > > fix?
> > 
> > I imagine there might be scenarios where you need to forcibly kill an
> > instance of auditd such that things might not get fully cleaned up in the
> > kernel, audit_{pid,sock,etc.}.
> 
> But the first time an event is sent and auditd doesn't exist, it resets the
> audit_pid to 0.
> 
> static void kauditd_send_skb(struct sk_buff *skb)
> {
>         int err;
>         /* take a reference in case we can't send it and we want...
>         skb_get(skb);
>         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
>         if (err < 0) {
>                 BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
>                 if (audit_pid) {
>                         pr_err("*NO* daemon at audit_pid=%d\n", audit_pid);
>                         audit_log_lost("auditd disappeared");
>                         audit_pid = 0;
>                         audit_sock = NULL;
>                 }

As an aside, it doesn't matter in this particular case, but the above code is 
not current.  Please try to use either what is in Linus' tree or audit#next 
when pasting code snippets; it's less confusing.

I still think there is some value in having the ability for an admin to reset 
the kernel's auditd tracking manually as relying on an event to be emitted 
does not seem like a solution I would want to have to justify.  Although I do 
admit that for most systems this shouldn't be a problem as events should 
likely occur often enough.

There really is no harm in merging these patches, and they do provide some, 
admittedly small, value.

> > Keeping the ability to reset the kernel's auditd state, even when the
> > kernel *thinks* auditd is still alive might be a nice thing to keep
> > around for a while longer.
> 
> I'm just thinking its rare that anyone would try to steal away the audit
> socket. Its more work for everyone to create a new event and send it than to
> just not allow it. you can even force an event with "auditctl -m test"
> which should reset the pid if the kernel was out of sync.

I do not want to disallow starting an new instance of auditd, so this patchset 
looks reasonable to me.

-- 
paul moore
security @ redhat