[PATCH v3.4] - audit cmdline on events

[PATCH v3.4] - audit cmdline on events

[William Roberts]

Draft versions of some work I have been doing auditing the cmdline
value on events. The reason for this, is that I need to get the
package name in Android in the audit records. Often times, the app dies
before userspace would be able to get it from procfs.


I'll (attempt) to summarize the feedback so far.

* RGB - Can we make this dynamic?
** This was nak'd by Steve Grubb and subsequently dropped from these patches.

* Stephen Smalley - Can we cache this in audit struct for performance concerns?
** I think I address this in patch 2

* Steve Grubb - Is cmdline generic enough? Should we extend
                prctl for an extended comm field?
** The heart of the matter is some spot the process can stick
   more than 16 chars of data. I think this meets that, without
   having to modify prctl.

* Steve Grubb - Can you use a user audit record?
** I can, but the downside is that it doesnt
   keep the same id with the related issues, you
   have to combine them by hand, by pid. Doesn't
   seem like a generic solution.


Right now, the cache never gets invalidated, as their is no kernel
interface on which to invalidate the cache on. This would be one
win for adding to prctl.

Once we have a clear way forward on this, I can make the effort
to port to master.

[PATCH 1/2] audit: Allow auditing of proc/self/cmdline value
[PATCH 2/2] audit: Enable cacheing of cmdline in audit_context

[PATCH 1/2] audit: Allow auditing of proc/self/cmdline value

[William Roberts]

Audit records will now contain a new field, cmdline.
This is the value that is stored in proc/self/cmdline,
and is useful for debugging when processes are being run
via VM's. A primary example of this is Android, in which
package names are set in this location, and thread names
are set via PR_SET_NAME. The other benefit is this
is not limited to 16 bytes as COMM historically has.

Change-Id: I9bf0928a8aa249d22ecd55fa9cd27325dd394eb1
Signed-off-by: William Roberts <wroberts@tresys.com>
---
 fs/proc/base.c          |    2 +-
 include/linux/proc_fs.h |    1 +
 kernel/auditsc.c        |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2f198da..25b73d3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -209,7 +209,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
 	return mm_access(task, PTRACE_MODE_READ);
 }
 
-static int proc_pid_cmdline(struct task_struct *task, char * buffer)
+int proc_pid_cmdline(struct task_struct *task, char *buffer)
 {
 	int res = 0;
 	unsigned int len;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 85c5073..d85ac14 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -118,6 +118,7 @@ struct pid_namespace;
 
 extern int pid_ns_prepare_proc(struct pid_namespace *ns);
 extern void pid_ns_release_proc(struct pid_namespace *ns);
+extern int proc_pid_cmdline(struct task_struct *task, char *buffer);
 
 /*
  * proc_tty.c
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 27ad9dd..45fd3d0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -67,6 +67,7 @@
 #include <linux/syscalls.h>
 #include <linux/capability.h>
 #include <linux/fs_struct.h>
+#include <linux/proc_fs.h>
 
 #include "audit.h"
 
@@ -1153,6 +1154,37 @@ error_path:
 
 EXPORT_SYMBOL(audit_log_task_context);
 
+static void audit_log_add_cmdline(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;
+	}
+	len = proc_pid_cmdline(tsk, (char *)page);
+	if (len <= 0) {
+		free_page(page);
+		audit_log_untrustedstring(ab, msg);
+		return;
+	}
+	/*
+	 * Ensure NULL terminated! Application could
+	 * could be using setproctitle(3).
+	 */
+	((char *)page)[len-1] = '\0';
+	msg = (char *)page;
+	audit_log_untrustedstring(ab, msg);
+	free_page(page);
+}
+
 static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 {
 	char name[sizeof(tsk->comm)];
@@ -1179,6 +1211,7 @@ 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);
 }
 
-- 
1.7.9.5

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

[William Roberts]

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;
 
 #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;
+}
+
 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,
 				  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;
 	}
 	len = proc_pid_cmdline(tsk, (char *)page);
 	if (len <= 0) {
 		free_page(page);
-		audit_log_untrustedstring(ab, msg);
-		return;
+		return 0;
 	}
 	/*
 	 * 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;
+		goto out;
+	}
+	/* Not cached yet */
+	context->cmdline = audit_cmdline_get_page(ab, tsk);
+	if (!context->cmdline)
+		goto out;
+	msg = (char *)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

Re: [PATCH 1/2] audit: Allow auditing of proc/self/cmdline value

[Richard Guy Briggs]

On Mon, Nov 18, 2013 at 04:41:19PM -0800, William Roberts wrote:
> Audit records will now contain a new field, cmdline.
> This is the value that is stored in proc/self/cmdline,
> and is useful for debugging when processes are being run
> via VM's. A primary example of this is Android, in which
> package names are set in this location, and thread names
> are set via PR_SET_NAME. The other benefit is this
> is not limited to 16 bytes as COMM historically has.

This patch looks good to me.

> Change-Id: I9bf0928a8aa249d22ecd55fa9cd27325dd394eb1
> Signed-off-by: William Roberts <wroberts@tresys.com>
> ---
>  fs/proc/base.c          |    2 +-
>  include/linux/proc_fs.h |    1 +
>  kernel/auditsc.c        |   33 +++++++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 2f198da..25b73d3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -209,7 +209,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
>  	return mm_access(task, PTRACE_MODE_READ);
>  }
>  
> -static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> +int proc_pid_cmdline(struct task_struct *task, char *buffer)
>  {
>  	int res = 0;
>  	unsigned int len;
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 85c5073..d85ac14 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -118,6 +118,7 @@ struct pid_namespace;
>  
>  extern int pid_ns_prepare_proc(struct pid_namespace *ns);
>  extern void pid_ns_release_proc(struct pid_namespace *ns);
> +extern int proc_pid_cmdline(struct task_struct *task, char *buffer);
>  
>  /*
>   * proc_tty.c
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 27ad9dd..45fd3d0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -67,6 +67,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/capability.h>
>  #include <linux/fs_struct.h>
> +#include <linux/proc_fs.h>
>  
>  #include "audit.h"
>  
> @@ -1153,6 +1154,37 @@ error_path:
>  
>  EXPORT_SYMBOL(audit_log_task_context);
>  
> +static void audit_log_add_cmdline(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;
> +	}
> +	len = proc_pid_cmdline(tsk, (char *)page);
> +	if (len <= 0) {
> +		free_page(page);
> +		audit_log_untrustedstring(ab, msg);
> +		return;
> +	}
> +	/*
> +	 * Ensure NULL terminated! Application could
> +	 * could be using setproctitle(3).
> +	 */
> +	((char *)page)[len-1] = '\0';
> +	msg = (char *)page;
> +	audit_log_untrustedstring(ab, msg);
> +	free_page(page);
> +}
> +
>  static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  {
>  	char name[sizeof(tsk->comm)];
> @@ -1179,6 +1211,7 @@ 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);
>  }
>  
> -- 
> 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

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

[Richard Guy Briggs]

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

Re: [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@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@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@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

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

[Richard Guy Briggs]

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

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

[William Roberts]

On Tue, Nov 19, 2013 at 8:00 AM, Richard Guy Briggs <rgb@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@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@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@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@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