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

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

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

	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