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

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(a)redhat.com&gt; 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(a)tresys.com&gt; > >> --- > >> 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(a)redhat.com&gt; > > 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(a)redhat.com&gt; 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