Re: [PATCH 1/2] audit: log binding and unbinding to netlink multicast socket

[Paul Moore <pmoore@redhat.com>]

On Tuesday, July 28, 2015 10:31:54 AM Steve Grubb wrote:
> On Friday, July 24, 2015 06:54:27 PM Paul Moore wrote:
> > On Thursday, July 23, 2015 04:45:10 PM Steve Grubb wrote:
> > > The audit subsystem could use a function that logs the commonly needed
> > > fields for a typical audit event. This logs less that
> > > audit_log_task_info
> > > and reduces the need to hand code individual fields.
> > > 
> > > Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> > > ---
> > > 
> > >  include/linux/audit.h |  5 +++++
> > >  kernel/audit.c        | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> > 
> > Additional comments below, but I'd like to see this patch change
> > audit_log_task_info() to call audit_log_task_simple()
> 
> They really can't without messing up parsers. The order is different for a
> reason. The audit_log_task_info records all kinds of stuff that is really
> not needed. It does pids, current credentials, extended uid, extended gid,
> and then tty and session, comm, exe, and then context. This wastes disk
> space.

If we can't use _task_simple() inside of _task_info() then just use 
audit_log_task_info().  Yes, it probably wastes a few extra bytes each time 
these records are generated, but these records aren't likely to be frequent.

> The new function is what should be used for most cases because it sticks to
> what is necessary for "hardwired" events - those that are not dictated by
> syscall or file watches. It provides pid, uid, auid, tty, session, context,
> comm, exe. Because it jettisons all the stuff that doesn't matter, one
> cannot call the other.

Where can we use _task_simple() beyond these new records?  Show me this has 
some reuse in the existing code base and I'll reconsider keeping 
_task_simple(), but right now it just looks like code duplication to me.
 
> > ... or, why not just call audit_log_task_info() if the audit bind/unbind
> > is going to be the only one to benefit from audit_log_task_simple()?  Yes,
> > I know that audit_log_task_info() records more than you need, but this
> > duplication of code because of the record format mess makes me very
> > grumpy.
> 
> I'd rather see us move some other things to audit_log_task_simple over the
> long term than hand code things.

See above; we're not going to hand code things, just use _task_info().

Long term we are going to be ditching this awful fixed string format.

> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 1c13e42..29fb38b 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1100,6 +1100,41 @@ static void audit_receive(struct sk_buff  *skb)
> > > 
> > >  	mutex_unlock(&audit_cmd_mutex);
> > >  
> > >  }
> > > 
> > > +/* This function logs the essential information needed to understand
> > > + * what or who is causing the event */
> > > +void audit_log_task_simple(struct audit_buffer *ab, struct task_struct
> > > *tsk)
> > 
> > ...
> > 
> > > +	audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > > +			 task_pid_nr(tsk),
> > > +			 from_kuid(&init_user_ns, cred->uid),
> > > +			 from_kuid(&init_user_ns, audit_get_loginuid(tsk)),
> > > +			 tty, audit_get_sessionid(tsk));
> > 
> > You should check the format string against audit_log_task_info(); they
> > don't match.
> 
> That is correct. It mostly matches the order of just about everything else.
> For example, user space originating events get this:

I was talking about some of the scalar format specifiers, e.g. "%u" vs "%d", 
but it doesn't matter so much anymore as it looks like we'll need to use 
_task_info().

-- 
paul moore
security @ redhat