Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL

[Richard Guy Briggs]

On 2019-01-29 18:07, Paul Moore wrote:
> On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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;

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

> >         ab->gfp_mask = gfp_mask;
> >
> >         return ab;
> > @@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> >                 return NULL;
> >         }
> >
> > +#ifdef CONFIG_AUDITSYSCALL
> >         audit_get_stamp(ab->ctx, &t, &serial);
> > +#else
> > +       audit_get_stamp(NULL, &t, &serial);
> > +#endif
> 
> If ab->ctx is NULL we don't really need this, do we?

We do if ctx isn't part of struct audit_buffer when
!CONFIG_AUDITSYSCALL.

> >         audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> >                          (unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
> >
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@xxxxxxxxxx>
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