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

Steve Grubb
2015-12-21T22:18:19Z

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 to hold it */
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;
}

> 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.

The

> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 843540c..cf84991 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -70,6 +70,7 @@
> > >
> > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */
> > > #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or off
*/
> > > #define AUDIT_GET_FEATURE 1019 /* Get which features are enabled
*/
> > >
> > > +#define AUDIT_REPLACE 1020 /* Replace auditd if this pack...
*/
> >
> > In every case, events in the 1000 block are to configure the kernel or to
> > ask the kernel a question. This is user space initiating. Kernel
> > initiating
> > events are in the 1300 block of numbers.
>
> Change the audit event number as Steve suggests and I'll toss the patches
> into my audit next queue, although considering we are at 4.4-rc6 at
> present, I'll probably hold this until after the merge window closes,
> meaning it is 4.6 material.