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

[William Roberts]

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?

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

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