[PATCH 2/2] audit: Enable cacheing of cmdline in audit_context
William Roberts
bill.c.roberts at gmail.com
Tue Nov 19 16:25:33 UTC 2013
On Tue, Nov 19, 2013 at 8:00 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
> 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.
Yes, I can get it, I would need to rafactor proc/fs/base.c proc_pid_cmdline()
and separate it into a function to get the length, and one to fill in
the buffer.
>
>> 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?
no, i'm pretty sure unsigned long stays consistant with pointer width
looking at at its usage elsewhere in the kernel.
>
> For that matter, is there a benefit to doing a __get_free_page() rather
> than kmalloc()?
This was suggested by Stephen Smalley that I just do the same
allocation scheme as what procfs was doing. But kmalloc
can boil down to a get_free_page() depending on what parts of pages
the kernel has free (i think). I'm not 100% sure of the
allocation details I might be missing.
>
>> > 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
--
Respectfully,
William C Roberts
More information about the Linux-audit
mailing list