Date:   Tue, 29 Jan 2019 21:54:43 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_
 AUDIT and not AUDITSYSCALL

On 2019-01-29 18:26, Paul Moore wrote:
> On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> > On 2019-01-29 18:07, Paul Moore wrote:
> > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> > > > Remove audit_context from struct task_struct and struct audit_buffer
> > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > > >
> > > > Also, audit_log_name() (and supporting inode and fcaps functions) should
> > > > have been put back in auditsc.c when soft and hard link logging was
> > > > normalized since it is only used by syscall auditing.
> > > >
> > > > See github issue https://github.com/linux-audit/audit-kernel/issues/105
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > > > ---
> > > > Changelog:
> > > > v2:
> > > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > >
> > > >  include/linux/sched.h |   4 +-
> > > >  kernel/audit.c        | 157 +++-----------------------------------------------
> > > >  kernel/audit.h        |   9 ---
> > > >  kernel/auditsc.c      | 150 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 161 insertions(+), 159 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > >   * use simultaneously. */
> > > >  struct audit_buffer {
> > > >         struct sk_buff       *skb;      /* formatted skb ready to send */
> > > > +#ifdef CONFIG_AUDITSYSCALL
> > > >         struct audit_context *ctx;      /* NULL or associated context */
> > > > +#endif
> > > >         gfp_t                gfp_mask;
> > > >  };
> > > >
> > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
> > > >         if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > >                 goto err;
> > > >
> > > > +#ifdef CONFIG_AUDITSYSCALL
> > > >         ab->ctx = ctx;
> > > > +#endif
> > >
> > > I vaguely remember reading/hearing something in the past about
> > > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> > > you say for certain that "ab" in this case is always going to be
> > > zero'd out?  This is an honest question.
> >
> > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > kmem_cache_alloc() in audit_buffer_alloc()?  (as I've done in
> > the last patch of ghak81/first patch of ghak90)
> >
> > If this is too much overhead, then we can initialize ctx = NULL;
> 
> We don't need zalloc() since we're setting all the fields, although
> more on this below ...

Ok...

> > > If we can't guarantee that "ab" is zero'd out, we should manually set
> > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> >
> > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL.  It
> > is #ifdef-ed out.  What am I missing?
> 
> You're not, I am.  I saw the obvious bit where you removed it from the
> task_struct, but completely glossed over the bit where you also
> removed it from the audit_buffer struct.  My mistake.
> 
> Once the audit container ID stuff lands we are going to need to have
> the audit_context pointer in the audit_buffer regardless of the
> CONFIG_AUDITSYSCALL setting, right?  Assuming the answer is yes, I
> think I'd just assume leave the pointer in the audit_buffer (setting
> it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
> #ifdef's in the middle of the functions (I generally like to avoid
> those if possible).  I think it's still worth making the changes to
> task_struct, as that is the right thing to do and doesn't have the
> same level of impact.

I like to avoid #ifdef compiler directives out when I can too, creating
stubs in the header file to do the job.

Why do we need an audit_context pointer in struct audit_buffer?  I'll
take a stab at answering this...  I was thinking it wasn't necessary,
but now I think I see what I was missing.  I think the only reason is to
connect records to one event through the timestamp and serial number
when syscall is disabled.  Up until now it wasn't needed unless full
syscall functionality was present, but once we have an audit container
identifier aux record we will need to join them, at minimum with a local
context for user and netfilter_pkt records.

So I have to ask, does it make sense to restructure things so that the
struct audit_buffer has a serial and ctime field so that it isn't needed
in the struct audit_context?  I'm not sure if this is possible.  I'll
have to go back and look at the code to see if this is the case...

> paul moore

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Powered by blists - more mailing lists