[DRAFT v3.4] - audit cmdline updates

[DRAFT v3.4] - audit cmdline updates

[William Roberts]

Changes since last publish:
* Ran all patches through checkpatch, some elluded me.
* Changed cmdline copy/length API to reduce task_mm_get() mmput() calls

Still need to know:
* Any major objecttions to this still?
* My public API changes are in proc, is this the best spot for those?

As always, thanks.

[PATCH 1/4] audit: Allow auditing of proc/self/cmdline value
[PATCH 2/4] audit: Enable cacheing of cmdline in audit_context
[PATCH 3/4] audit: dont allocate whole pages
[PATCH 4/4] SQUASH audit: Change cmdline get API to reduce locking

[PATCH 1/4] 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/4] audit: Enable cacheing of cmdline in audit_context

[William Roberts]

Rather then reading from userspace on every call,
cache the page in the audit_context and couple
to that objects life-cycle.

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..27f8224 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -270,6 +270,7 @@ struct audit_context {
 		} mmap;
 	};
 	int fds[2];
+	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((unsigned long)ctx->cmdline);
+	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 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 NULL;
 	}
 	len = proc_pid_cmdline(tsk, (char *)page);
 	if (len <= 0) {
 		free_page(page);
-		audit_log_untrustedstring(ab, msg);
-		return;
+		return NULL;
 	}
 	/*
 	 * Ensure NULL terminated! Application could
 	 * could be using setproctitle(3).
 	 */
 	((char *)page)[len-1] = '\0';
-	msg = (char *)page;
+
+	/* XXX: Re-alloc to something smaller then a page here? */
+	return (char *)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 = context->cmdline;
+		goto out;
+	}
+	/* Not cached yet */
+	context->cmdline = audit_cmdline_get_page(ab, tsk);
+	if (!context->cmdline)
+		goto out;
+	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

[PATCH 3/4] audit: dont allocate whole pages

[William Roberts]

Rather then cacheing whole pages, use kmalloc to potentially
cache a smaller size.

Change-Id: I9fb749dc2bdac506d1bc6f2259fbbdeeec87b298
Signed-off-by: William Roberts <wroberts@tresys.com>
---
 fs/proc/base.c          |   93 +++++++++++++++++++++++++++++++++++++++--------
 include/linux/proc_fs.h |    5 ++-
 kernel/auditsc.c        |   43 ++++++++++++++--------
 3 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 25b73d3..a0751ed 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -209,35 +209,76 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
 	return mm_access(task, PTRACE_MODE_READ);
 }
 
-int proc_pid_cmdline(struct task_struct *task, char *buffer)
+/*
+ * Returns the length of the VM area containing the tasks cmdline info.
+ * 0 indicates success
+ */
+int proc_pid_cmdline_length(struct task_struct *task, unsigned int *len)
+{
+	int res = -1;
+	struct mm_struct *mm;
+
+	if (!task || !len)
+		return 0;
+
+	mm = get_task_mm(task);
+	if (!mm)
+		goto out;
+	if (!mm->arg_end)
+		goto out_mm;	/* Shh! No looking before we're done */
+
+	*len = mm->arg_end - mm->arg_start;
+	res = 0;
+out_mm:
+	mmput(mm);
+out:
+	return res;
+}
+
+/* Copy's the tasks cmdline data into buf, truncating at buflen */
+int proc_pid_copy_cmdline_to_buf(struct task_struct *task, char *buf,
+				 unsigned int buflen)
 {
 	int res = 0;
 	unsigned int len;
-	struct mm_struct *mm = get_task_mm(task);
+	struct mm_struct *mm;
+
+	if (!buflen || !buf)
+		return 0;
+
+	mm = get_task_mm(task);
 	if (!mm)
 		goto out;
 	if (!mm->arg_end)
 		goto out_mm;	/* Shh! No looking before we're done */
 
- 	len = mm->arg_end - mm->arg_start;
- 
-	if (len > PAGE_SIZE)
-		len = PAGE_SIZE;
- 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+	res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
+	if (res <= 0)
+		goto out_mm;
+
+	/* Truncate to res if buflen is longer */
+	if (res > buflen)
+		res = buflen;
 
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
-		len = strnlen(buffer, res);
+	/*
+	 * If the nul at the end of args has been overwritten, then
+	 * assume application is using setproctitle(3).
+	 */
+	if (buf[res-1] != '\0') {
+		/* Nul between start and end of vm space?
+		   If so, then truncate size down */
+		len = strnlen(buf, res);
 		if (len < res) {
 		    res = len;
 		} else {
+			/* No nul, truncate to buflen if too big to fit */
 			len = mm->env_end - mm->env_start;
-			if (len > PAGE_SIZE - res)
-				len = PAGE_SIZE - res;
-			res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
-			res = strnlen(buffer, res);
+			if (len > buflen - res)
+				len = buflen - res;
+			/* Copy in any remaining data */
+			res += access_process_vm(task, mm->env_start, buf+res,
+						 len, 0);
+			res = strnlen(buf, res);
 		}
 	}
 out_mm:
@@ -246,6 +287,26 @@ out:
 	return res;
 }
 
+static int proc_pid_cmdline(struct task_struct *task, char *buffer)
+{
+	unsigned int len;
+	int res = proc_pid_cmdline_length(task, &len);
+	if (res)
+		return 0;
+
+	/* The caller of this allocates a page */
+	if (len > PAGE_SIZE)
+		len = PAGE_SIZE;
+
+	res = proc_pid_copy_cmdline_to_buf(task, buffer, len);
+	/*
+	 * Ensure NULL terminated! Application could
+	 * could be using setproctitle(3).
+	 */
+	buffer[res-1] = '\0';
+	return res;
+}
+
 static int proc_pid_auxv(struct task_struct *task, char *buffer)
 {
 	struct mm_struct *mm = mm_for_maps(task);
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d85ac14..f76deb3 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -118,7 +118,10 @@ 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);
+
+extern int proc_pid_cmdline_length(struct task_struct *task, unsigned int *len);
+extern int proc_pid_copy_cmdline_to_buf(struct task_struct *task, char *buf,
+					unsigned int buflen);
 
 /*
  * proc_tty.c
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 27f8224..34a6c1d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1049,7 +1049,7 @@ static inline void audit_cmdline_free(struct audit_context *ctx)
 {
 	if (!ctx->cmdline)
 		return;
-	free_page((unsigned long)ctx->cmdline);
+	kfree(ctx->cmdline);
 	ctx->cmdline = NULL;
 }
 
@@ -1164,30 +1164,43 @@ error_path:
 
 EXPORT_SYMBOL(audit_log_task_context);
 
-static char *audit_cmdline_get_page(struct audit_buffer *ab,
+static char *audit_cmdline_get(struct audit_buffer *ab,
 				  struct task_struct *tsk)
 {
 	int len;
-	unsigned long page;
+	int res;
+	char *buf;
 
-	/* Get the process cmdline */
-	page = __get_free_page(GFP_TEMPORARY);
-	if (!page) {
+	res = proc_pid_cmdline_length(tsk, &len);
+	if (res != 0 || len == 0)
 		return NULL;
-	}
-	len = proc_pid_cmdline(tsk, (char *)page);
-	if (len <= 0) {
-		free_page(page);
+
+	if (len > PATH_MAX)
+		len = PATH_MAX;
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return NULL;
+
+	res = proc_pid_copy_cmdline_to_buf(tsk, buf, len);
+	if (res <= 0) {
+		kfree(buf);
 		return NULL;
 	}
+
+	/*
+	 * res is guarenteed not to be longer than
+	 * the buf as it was truncated to len
+	 * in proc_pid_copy_cmdline_to_buf()
+	 */
+	len = res;
+
 	/*
 	 * Ensure NULL terminated! Application could
 	 * could be using setproctitle(3).
 	 */
-	((char *)page)[len-1] = '\0';
-
-	/* XXX: Re-alloc to something smaller then a page here? */
-	return (char *)page;
+	buf[len-1] = '\0';
+	return buf;
 }
 
 static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
@@ -1203,7 +1216,7 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
 		goto out;
 	}
 	/* Not cached yet */
-	context->cmdline = audit_cmdline_get_page(ab, tsk);
+	context->cmdline = audit_cmdline_get(ab, tsk);
 	if (!context->cmdline)
 		goto out;
 	msg = context->cmdline;
-- 
1.7.9.5

[PATCH 4/4] SQUASH audit: Change cmdline get API to reduce locking

[William Roberts]

Each call to length copy required a call to get_task_mm() and mmput.
Just require the caller to aquire and pass a valid mm.

Change-Id: Id7069b80f1cbea5b30032a0a459dd54b7446f665
Signed-off-by: William Roberts <wroberts@tresys.com>
---
 fs/proc/base.c          |   63 +++++++++++++++--------------------------------
 include/linux/proc_fs.h |   13 +++++++---
 kernel/auditsc.c        |   28 +++++++++++++++------
 3 files changed, 51 insertions(+), 53 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a0751ed..4d74830 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -210,51 +210,21 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
 }
 
 /*
- * Returns the length of the VM area containing the tasks cmdline info.
- * 0 indicates success
+ * Copy's the tasks cmdline data into buf, truncating at buflen
+ * must call holding semaphore on mm
  */
-int proc_pid_cmdline_length(struct task_struct *task, unsigned int *len)
-{
-	int res = -1;
-	struct mm_struct *mm;
-
-	if (!task || !len)
-		return 0;
-
-	mm = get_task_mm(task);
-	if (!mm)
-		goto out;
-	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
-
-	*len = mm->arg_end - mm->arg_start;
-	res = 0;
-out_mm:
-	mmput(mm);
-out:
-	return res;
-}
-
-/* Copy's the tasks cmdline data into buf, truncating at buflen */
-int proc_pid_copy_cmdline_to_buf(struct task_struct *task, char *buf,
-				 unsigned int buflen)
+int proc_pid_copy_cmdline_to_buf(struct task_struct *task, struct mm_struct *mm,
+				 char *buf, unsigned int buflen)
 {
 	int res = 0;
 	unsigned int len;
-	struct mm_struct *mm;
 
-	if (!buflen || !buf)
+	if (!task || !mm || !buf)
 		return 0;
 
-	mm = get_task_mm(task);
-	if (!mm)
-		goto out;
-	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
-
 	res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
 	if (res <= 0)
-		goto out_mm;
+		return 0;
 
 	/* Truncate to res if buflen is longer */
 	if (res > buflen)
@@ -281,29 +251,36 @@ int proc_pid_copy_cmdline_to_buf(struct task_struct *task, char *buf,
 			res = strnlen(buf, res);
 		}
 	}
-out_mm:
-	mmput(mm);
-out:
 	return res;
 }
 
 static int proc_pid_cmdline(struct task_struct *task, char *buffer)
 {
-	unsigned int len;
-	int res = proc_pid_cmdline_length(task, &len);
-	if (res)
+	int res = 0;
+	unsigned int len = 0;
+	struct mm_struct *mm = get_task_mm(task);
+	if (!mm)
 		return 0;
 
+	len = proc_pid_cmdline_length(mm);
+	if (!len)
+		goto mm_out;
+
 	/* The caller of this allocates a page */
 	if (len > PAGE_SIZE)
 		len = PAGE_SIZE;
 
-	res = proc_pid_copy_cmdline_to_buf(task, buffer, len);
+	res = proc_pid_copy_cmdline_to_buf(task, mm, buffer, len);
+	if (!res)
+		goto mm_out;
+
 	/*
 	 * Ensure NULL terminated! Application could
 	 * could be using setproctitle(3).
 	 */
 	buffer[res-1] = '\0';
+mm_out:
+	mmput(mm);
 	return res;
 }
 
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index f76deb3..8bc2718 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -6,6 +6,7 @@
 #include <linux/spinlock.h>
 #include <linux/magic.h>
 #include <linux/atomic.h>
+#include <linux/mm.h>
 
 struct net;
 struct completion;
@@ -119,10 +120,16 @@ 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_length(struct task_struct *task, unsigned int *len);
-extern int proc_pid_copy_cmdline_to_buf(struct task_struct *task, char *buf,
-					unsigned int buflen);
+/* must call holding semaphore on mm */
+static inline unsigned int proc_pid_cmdline_length(struct mm_struct *mm)
+{
+	 return mm->arg_end ? mm->arg_end - mm->arg_start : 0;
+}
 
+extern int proc_pid_copy_cmdline_to_buf(struct task_struct *task,
+					struct mm_struct *mm,
+					char *buf,
+					unsigned int buflen);
 /*
  * proc_tty.c
  */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 34a6c1d..8bd0549 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1165,29 +1165,37 @@ error_path:
 EXPORT_SYMBOL(audit_log_task_context);
 
 static char *audit_cmdline_get(struct audit_buffer *ab,
-				  struct task_struct *tsk)
+			       struct task_struct *tsk)
 {
 	int len;
 	int res;
 	char *buf;
+	struct mm_struct *mm;
+
+	if (!ab || !tsk)
+		return NULL;
 
-	res = proc_pid_cmdline_length(tsk, &len);
-	if (res != 0 || len == 0)
+	mm = get_task_mm(tsk);
+	if (!mm)
 		return NULL;
 
+	len = proc_pid_cmdline_length(mm);
+	if (!len)
+		goto mm_err;
+
 	if (len > PATH_MAX)
 		len = PATH_MAX;
 
 	buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
-		return NULL;
+		goto mm_err;
 
-	res = proc_pid_copy_cmdline_to_buf(tsk, buf, len);
+	res = proc_pid_copy_cmdline_to_buf(tsk, mm, buf, len);
 	if (res <= 0) {
-		kfree(buf);
-		return NULL;
+		goto alloc_err;
 	}
 
+	mmput(mm);
 	/*
 	 * res is guarenteed not to be longer than
 	 * the buf as it was truncated to len
@@ -1201,6 +1209,12 @@ static char *audit_cmdline_get(struct audit_buffer *ab,
 	 */
 	buf[len-1] = '\0';
 	return buf;
+
+alloc_err:
+	kfree(buf);
+mm_err:
+	mmput(mm);
+	return NULL;
 }
 
 static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
-- 
1.7.9.5

Re: [DRAFT v3.4] - audit cmdline updates

[William Roberts]

Also, updating to a master kernel now, to try and publish relative to that.

On Wed, Nov 20, 2013 at 5:29 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> Changes since last publish:
> * Ran all patches through checkpatch, some elluded me.
> * Changed cmdline copy/length API to reduce task_mm_get() mmput() calls
>
> Still need to know:
> * Any major objecttions to this still?
> * My public API changes are in proc, is this the best spot for those?
>
> As always, thanks.
>
> [PATCH 1/4] audit: Allow auditing of proc/self/cmdline value
> [PATCH 2/4] audit: Enable cacheing of cmdline in audit_context
> [PATCH 3/4] audit: dont allocate whole pages
> [PATCH 4/4] SQUASH audit: Change cmdline get API to reduce locking
>



-- 
Respectfully,

William C Roberts