audit: Tidy up audit_context and stop bprm recursion

PATCH In review Richard Guy Briggs 2 participants 6 messages Oct 31, 2013

Subsystems (3)

VFS exec and binfmt Audit

lore

Share & tools

lore

Richard Guy Briggs

author

2013-10-31 15:37 UTC #0 lore raw

v2 cover audit: Tidy up audit_context and stop bprm recursion

This patchset is a clean up of the audit_aux_data and audit_context structures and the audit_bprm() call that was needlessly recursing, allocating more resources than necessary.

Eric W. Biederman (1):
audit: Kill the unused struct audit_aux_data_capset

Richard Guy Briggs (3):
audit: remove unused envc member of audit_aux_data_execve
audit: move audit_aux_data_execve contents into audit_context union
audit: call audit_bprm() only once to add AUDIT_EXECVE information

 fs/exec.c             |    5 +----
 include/linux/audit.h |   13 +++++--------
 kernel/audit.h        |    3 +++
 kernel/auditsc.c      |   49 ++++++++++---------------------------------------
 4 files changed, 19 insertions(+), 51 deletions(-)

Richard Guy Briggs

author

2013-10-31 15:37 UTC #1 lore raw

PATCH v2 1/4 audit: Kill the unused struct audit_aux_data_capset

From: Eric W. Biederman <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

(cherry picked from commit 6904431d6b41190e42d6b94430b67cb4e7e6a4b7)

(cherry picked from commit 2b3a6c617396a9e6eedae9a56b2d9642da0216b6)

---
 kernel/auditsc.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

kernel/auditsc.c +0 -6

index 95293ab..24047f4 100644
@@ -121,12 +121,6 @@ struct audit_aux_data_bprm_fcaps {
 	struct audit_cap_data	new_pcap;
 };
 
-struct audit_aux_data_capset {
-	struct audit_aux_data	d;
-	pid_t			pid;
-	struct audit_cap_data	cap;
-};
-
 struct audit_tree_refs {
 	struct audit_tree_refs *next;
 	struct audit_chunk *c[31];

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2013-10-31 15:37 UTC #2 lore raw

PATCH v2 2/4 audit: remove unused envc member of audit_aux_data_execve

In reply to Richard Guy Briggs

Get rid of write-only audit_aux_data_exeve structure member envc.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditsc.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

kernel/auditsc.c +0 -2

index 24047f4..c9abaa0 100644
@@ -98,7 +98,6 @@ struct audit_aux_data {
 struct audit_aux_data_execve {
 	struct audit_aux_data	d;
 	int argc;
-	int envc;
 	struct mm_struct *mm;
 };

@@ -2132,7 +2131,6 @@ int __audit_bprm(struct linux_binprm *bprm)
 		return -ENOMEM;
 
 	ax->argc = bprm->argc;
-	ax->envc = bprm->envc;
 	ax->mm = bprm->mm;
 	ax->d.type = AUDIT_EXECVE;
 	ax->d.next = context->aux;

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2013-10-31 15:37 UTC #3 lore raw

PATCH v2 3/4 audit: move audit_aux_data_execve contents into audit_context union

In reply to Richard Guy Briggs

audit_bprm() was being called to add an AUDIT_EXECVE record to the audit context every time search_binary_handler() was recursively called. Only one reference is necessary, so just update it. Move the the contents of audit_aux_data_execve into the union in audit_context, removing dependence on a kmalloc along the way.

Reported-by Oleg Nesterov <[email protected]>

Cc Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h |    4 ++--
 kernel/audit.h        |    4 ++++
 kernel/auditsc.c      |   41 ++++++++++++-----------------------------
 3 files changed, 18 insertions(+), 31 deletions(-)

include/linux/audit.h +2 -2

index 729a4d1..fffefbd 100644
@@ -207,7 +207,7 @@ static inline int audit_get_sessionid(struct task_struct *tsk)
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
-extern int __audit_bprm(struct linux_binprm *bprm);
+extern void __audit_bprm(struct linux_binprm *bprm);
 extern int __audit_socketcall(int nargs, unsigned long *args);
 extern int __audit_sockaddr(int len, void *addr);
 extern void __audit_fd_pair(int fd1, int fd2);

@@ -239,7 +239,7 @@ static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid
 static inline int audit_bprm(struct linux_binprm *bprm)
 {
 	if (unlikely(!audit_dummy_context()))
-		return __audit_bprm(bprm);
+		__audit_bprm(bprm);
 	return 0;
 }
 static inline int audit_socketcall(int nargs, unsigned long *args)

kernel/audit.h +4 -0

index 123c9b7..e7b94ab 100644
@@ -197,6 +197,10 @@ struct audit_context {
 			int			fd;
 			int			flags;
 		} mmap;
+		struct {
+			int			argc;
+			struct mm_struct	*mm;
+		} execve;
 	};
 	int fds[2];

kernel/auditsc.c +12 ~~-29~~

89 lines not on this page · raw message

Signature · 1 line

1.7.1

Oleg Nesterov

2013-10-31 17:16 UTC #4 lore raw

Re: [PATCH 3/4][v2] audit: move audit_aux_data_execve contents into audit_context union

kernel/audit.h struct audit_context { · line 203

Quoted 11 lines · Richard Guy Briggs

kernel/audit.h +4 -0

@@ -197,6 +197,10 @@ struct audit_context {
 			int			fd;
 			int			flags;
 		} mmap;
+		struct {
+			int			argc;
+			struct mm_struct	*mm;
+		} execve;

Ah, nice, so we should not worry about kmalloc(GFP_KERNEL) failure.

I am in no position to ack the changes in this area, but the whole series looks fine to me.

Oleg.

Richard Guy Briggs

author

2013-10-31 15:37 UTC #5 lore raw

PATCH v2 4/4 audit: call audit_bprm() only once to add AUDIT_EXECVE information

In reply to Richard Guy Briggs

Move the audit_bprm() call from search_binary_handler() to exec_binprm(). This allows us to get rid of the mm member of struct audit_aux_data_execve since bprm->mm will equal current->mm.

This also mitigates the issue that ->argc could be modified by the load_binary() call in search_binary_handler().

audit_bprm() was being called to add an AUDIT_EXECVE record to the audit context every time search_binary_handler() was recursively called. Only one reference is necessary.

Reported-by Oleg Nesterov <[email protected]>

Cc Eric Paris <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/exec.c             |    5 +----
 include/linux/audit.h |    9 +++------
 kernel/audit.h        |    1 -
 kernel/auditsc.c      |    4 ----
 4 files changed, 4 insertions(+), 15 deletions(-)

fs/exec.c +1 -4

index 8875dd1..47d7edb 100644
@@ -1385,10 +1385,6 @@ int search_binary_handler(struct linux_binprm *bprm)
 	if (retval)
 		return retval;
 
-	retval = audit_bprm(bprm);
-	if (retval)
-		return retval;
-
 	retval = -ENOENT;
  retry:
 	read_lock(&binfmt_lock);

@@ -1436,6 +1432,7 @@ static int exec_binprm(struct linux_binprm *bprm)
 
 	ret = search_binary_handler(bprm);
 	if (ret >= 0) {
+		audit_bprm(bprm);
 		trace_sched_process_exec(current, old_pid, bprm);
 		ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
 		current->did_exec = 1;

include/linux/audit.h +3 -6

index fffefbd..a757e6c 100644
@@ -236,11 +236,10 @@ static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid
 	if (unlikely(!audit_dummy_context()))
 		__audit_ipc_set_perm(qbytes, uid, gid, mode);
 }
-static inline int audit_bprm(struct linux_binprm *bprm)
+static inline void audit_bprm(struct linux_binprm *bprm)
 {
 	if (unlikely(!audit_dummy_context()))
 		__audit_bprm(bprm);
-	return 0;
 }
 static inline int audit_socketcall(int nargs, unsigned long *args)
 {

@@ -367,10 +366,8 @@ static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
 					gid_t gid, umode_t mode)
 { }
-static inline int audit_bprm(struct linux_binprm *bprm)
-{
-	return 0;
-}
+static inline void audit_bprm(struct linux_binprm *bprm)
+{ }
 static inline int audit_socketcall(int nargs, unsigned long *args)
 {
 	return 0;

kernel/audit.h +0 -1

index e7b94ab..b779642 100644
@@ -199,7 +199,6 @@ struct audit_context {
 		} mmap;
 		struct {
 			int			argc;
-			struct mm_struct	*mm;
 		} execve;
 	};
 	int fds[2];

kernel/auditsc.c +0 -4

index eabe76a..dc1adee 100644
@@ -1145,9 +1145,6 @@ static void audit_log_execve_info(struct audit_context *context,
 	const char __user *p;
 	char *buf;
 
-	if (context->execve.mm != current->mm)
-		return; /* execve failed, no additional info */
-
 	p = (const char __user *)current->mm->arg_start;
 
 	audit_log_format(*ab, "argc=%d", context->execve.argc);

@@ -2118,7 +2115,6 @@ void __audit_bprm(struct linux_binprm *bprm)
 
 	context->type = AUDIT_EXECVE;
 	context->execve.argc = bprm->argc;
-	context->execve.mm = bprm->mm;
 }

Signature · 1 line

1.7.1