Re: [PATCH 2/2] audit: Enable cacheing of cmdline in audit_context

[Richard Guy Briggs <rgb@redhat.com>]

On Tue, Nov 19, 2013 at 07:44:33AM -0800, William Roberts wrote:
> On Tue, Nov 19, 2013 at 7:40 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On Mon, Nov 18, 2013 at 04:41:20PM -0800, William Roberts wrote:
> >> Not enitrely sure if this is getting us any benefit, as in my
> >> environment, these contexts are getting free'd immediately.
> >>
> >> Change-Id: Ia0d432bc4aba8588840f0dc0026a1e9483e5b485
> >> Signed-off-by: William Roberts <wroberts@tresys.com>
> >> ---
> >>  kernel/auditsc.c |   48 +++++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 37 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 45fd3d0..4b30c5d 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -270,6 +270,7 @@ struct audit_context {
> >>               } mmap;
> >>       };
> >>       int fds[2];
> >> +     unsigned long cmdline;
> >
> > Would this be better declared to avoid all the casts?:
> 
> This is indirectly coupled with my todo as well. So the value could be
> up to a page big, and I allocate and cache a page (which is returned
> as an usnigned long).
> But, often times, the value is much smaller. Should I just alloc a
> seperate and smaller buffer with kmalloc, and
> cache that as a char * to avoid the casts? Then free the page?

Do you know the size ahead of time?  If so, just allocate that.

> Either way, I have no objection to taking the interface to a char *, I
> battled that decision in my head while I was coding.

Perhaps I am being naive, but is there any benefit to referring to a
page as an unsigned long rather than as a char*?  If it is only ever
used as a char* (except by free_page()) can it hurt?

For that matter, is there a benefit to doing a __get_free_page() rather
than kmalloc()?

> >         char *cmdline;
> >
> >>  #if AUDIT_DEBUG
> >>       int                 put_count;
> >> @@ -1044,6 +1045,14 @@ static inline void audit_free_aux(struct audit_context *context)
> >>       }
> >>  }
> >>
> >> +static inline void audit_cmdline_free(struct audit_context *ctx)
> >> +{
> >> +     if (!ctx->cmdline)
> >> +             return;
> >> +     free_page(ctx->cmdline);
> >> +     ctx->cmdline = 0;
> >
> >         ctx->cmdline = NULL;
> >
> >> +}
> >> +
> >>  static inline void audit_zero_context(struct audit_context *context,
> >>                                     enum audit_state state)
> >>  {
> >> @@ -1118,6 +1127,7 @@ static inline void audit_free_context(struct audit_context *context)
> >>               audit_free_aux(context);
> >>               kfree(context->filterkey);
> >>               kfree(context->sockaddr);
> >> +             audit_cmdline_free(context);
> >>               kfree(context);
> >>               context  = previous;
> >>       } while (context);
> >> @@ -1154,35 +1164,51 @@ error_path:
> >>
> >>  EXPORT_SYMBOL(audit_log_task_context);
> >>
> >> -static void audit_log_add_cmdline(struct audit_buffer *ab,
> >> +static unsigned long audit_cmdline_get_page(struct audit_buffer *ab,
> >
> >    static char* audit_cmdline_get_page(struct audit_buffer *ab,
> >
> >>                                 struct task_struct *tsk)
> >>  {
> >>       int len;
> >>       unsigned long page;
> >> -     char *msg = "(null)";
> >> -
> >> -     audit_log_format(ab, " cmdline=");
> >>
> >>       /* Get the process cmdline */
> >>       page = __get_free_page(GFP_TEMPORARY);
> >>       if (!page) {
> >> -             audit_log_untrustedstring(ab, msg);
> >> -             return;
> >> +             return 0;
> >
> >                 return NULL;
> >
> >>       }
> >>       len = proc_pid_cmdline(tsk, (char *)page);
> >>       if (len <= 0) {
> >>               free_page(page);
> >> -             audit_log_untrustedstring(ab, msg);
> >> -             return;
> >> +             return 0;
> >
> >                 return NULL;
> >
> >>       }
> >>       /*
> >>        * Ensure NULL terminated! Application could
> >>        * could be using setproctitle(3).
> >>        */
> >>       ((char *)page)[len-1] = '\0';
> >> -     msg = (char *)page;
> >> +
> >> +     /* TODO: Re-alloc to something smaller then a page here? */
> >> +     return page;
> >> +}
> >> +
> >> +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
> >> +                           struct audit_context *context)
> >> +{
> >> +     char *msg = "(null)";
> >> +
> >> +     audit_log_format(ab, " cmdline=");
> >> +
> >> +     /* Already cached */
> >> +     if (context->cmdline) {
> >> +             msg = (char *)context->cmdline;
> >
> >                 msg = context->cmdline;
> >
> >> +             goto out;
> >> +     }
> >> +     /* Not cached yet */
> >> +     context->cmdline = audit_cmdline_get_page(ab, tsk);
> >> +     if (!context->cmdline)
> >> +             goto out;
> >> +     msg = (char *)context->cmdline;
> >
> >         msg = context->cmdline;
> >
> >> +out:
> >>       audit_log_untrustedstring(ab, msg);
> >> -     free_page(page);
> >>  }
> >>
> >>  static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> >> @@ -1211,7 +1237,6 @@ static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk
> >>               }
> >>               up_read(&mm->mmap_sem);
> >>       }
> >> -     audit_log_add_cmdline(ab, tsk);
> >>       audit_log_task_context(ab);
> >>  }
> >>
> >> @@ -1679,6 +1704,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
> >>
> >>
> >>       audit_log_task_info(ab, tsk);
> >> +     audit_log_cmdline(ab, tsk, context);
> >>       audit_log_key(ab, context->filterkey);
> >>       audit_log_end(ab);
> >>
> >> --
> >> 1.7.9.5
> >>
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rbriggs@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
> 
> 
> 
> -- 
> Respectfully,
> 
> William C Roberts

- RGB

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