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

[Richard Guy Briggs]

On 15/09/25, Paul Moore wrote:
> On Friday, September 25, 2015 07:10:19 AM Richard Guy Briggs wrote:
> > On 15/09/24, Paul Moore wrote:
> > > On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 18cdfe2..3399ab2 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > > > 
> > > >  	return 0;
> > > >  
> > > >  }
> > > > 
> > > > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > > > +{
> > > > +	struct sk_buff *skb = audit_make_reply(portid,seq,AUDIT_PING,0,0,
> > > > +                                       &pid, sizeof(pid));
> > > 
> > > This is almost surely going to end up using the wrong netlink sequence
> > > number and portid since you are passing the new requestor's information
> > > below.  I didn't chase down the netlink_unicast() guts to see if it
> > > replaces the portid, it might (it probably does), but that still leaves
> > > the sequence number.
> >
> > It is intended to use the new pid and new netlink sequence number to the
> > old audit_sock and old portid.
> 
> You don't want to put the new portid/seqno in a netlink header that is being 
> sent to the old auditd.

Ok, you are right, both should be set to zero, since nothing else makes sense.

> > There is no other sequence number available and it is this new sequence
> > number and pid that needs reporting to the old auditd.
> 
> The audit_make_reply() function is the wrong thing to be using here, we should 
> create our own buffer from scratch like most other records.  Also, yes, we 
> want to include the new pid, but I really don't think there is any value in 
> including the seqno of the AUDIT_SET/AUDIT_STATUS_PID message.

Most other records use audit_log_start(), which isn't what we want here,
since we want to bypass the queue to test if it is still alive.  We
don't care if it is delivered.  We just care if the socket is still
alive.  We don't want a context either.

So, I believ audit_make_reply() can be used just fine, setting portid,
seq, done and multi to zero.

> > > Also, this is more of a attempted hijack message and not a simple ping,
> > > right?
> >
> > Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
> > have a problem changing it, but I think the pid of the hijacker would be
> > useful information to the ping-ee unless the ping message was only ever
> > issues in a contextless kernel-initiated message.
> 
> Let's change the message name, this isn't a ping message and we may want to 
> have a ping message at some point in the future.

Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
representation of the PID of the task attempting to replace it.

> > > If we want to create a simple ping message, leave the pid out of it; if we
> > > want to indicate to an existing auditd that another process is attempting
> > > to hijack the audit connection then we should probably create a proper
> > > audit record with a type other than AUDIT_PING.  I tend to think there is
> > > more value in the hijack message than the ping message, but I can be
> > > convinced either way.
> > 
> > Is there any compelling reason to create a pure ping message that gets
> > sent out periodically to test if auditd is still alive (audit_pid,
> > audit_sock and audit_nlk_portid are valid)?
> 
> Possibly, but let's leave that as a future experiment.

Ok.

> > Is there any reason to reserve that AUDIT_PING macro at this time should it
> > be determined that it is necessary in the future?
> 
> I don't think we need to reserve it now, we can allocate the ping message type 
> if/when we decide to implement it.

Ok.

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs at redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545