Message-ID: <20190201224052.h7aj2rcdebexpogf@madcap2.tricolour.ca>
Date:   Fri, 1 Feb 2019 17:40:52 -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-02-01 17:24, Paul Moore wrote:
> On Thu, Jan 31, 2019 at 10:53 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> > > 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.
> >
> > I also expect us the significance to grow over time as we start to
> > deal with the event routing problem; one solution would be to track
> > the audit container ID as a field in the audit_context.

Agreed.

> > Basically I see the audit_context as the audit "event" data structure
> > where the audit_buffer is the audit "record" data structure.  Their
> > use doesn't always line up perfectly with those definitions at
> > present, but I tend to think of the deviations as problems to correct
> > over time.

That was starting to become more clear to me after I first read this
paragraph above, having first started to understand it in my previous
reply.

> > > 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...
> >
> > I would say "no" if for no other reason than what I said about about
> > the audit_context being the "event" data structure.
> 
> Based on your comments in another thread I realize you think I've
> queued this for acceptance; I haven't.

No, I've not assumed that at all.  It is queued for review, but I've
made no such assumptions it is merged.

> I'll be a bit more clear: leave the audit_context pointer in the audit_buffer.

I had gradually come to realize that distinction between the buffer for
the record and the context for the entire event.  I agree the timestamp
and serial number must stay with the event.  We could try and make a
furether separation between the event and the context just to hold the
timestamp and serial number but given the possibility of syscall support
coming to the remainer of arches I don't think it is worthwhile.

> 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