[PATCH 2/2] audit: Enable cacheing of cmdline in audit_context
Richard Guy Briggs
rgb at redhat.com
Tue Nov 19 16:00:27 UTC 2013
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 at 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 at 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 at 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 at 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
More information about the Linux-audit
mailing list