Re: [RFC][PATCH] exec: Fix use after free of tracepointtrace_sched_process_exec

[Steven Rostedt]

On Tue, 4 Feb 2014 18:28:52 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:


> At least this patch keeps the ugliness with the code. I could even
> encapsulate that in a static inline function to remove the ugliness out
> of exec_binprm().

New version that moves all the ugliness into a static inline helper
function.

I'll start testing this and the added helper, and if there's no
problems or arguments from you, I'll post them tomorrow.

-- Steve

diff --git a/fs/exec.c b/fs/exec.c
index e1529b4..06101e9 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1407,6 +1407,36 @@ int search_binary_handler(struct linux_binprm *bprm)
 }
 EXPORT_SYMBOL(search_binary_handler);
 
+static __always_inline void
+do_trace_sched_process_exec(pid_t old_pid, struct linux_binprm *bprm)
+{
+	char *pathname;
+	char *tmp;
+
+	/*
+	 * We can't use bprm->filename here because it can be freed
+	 * within the search_binary_handler() call (by mm_release()
+	 * waking up the cgroup call_usermodehelper parent, that
+	 * will free what bprm->filename points to). Instead, if
+	 * the sched_process_exec tracepoint is enabled, we need
+	 * to create the pathname from the file dentry pointer.
+	 */
+	if (tracepoint_enabled(sched_process_exec)) {
+		tmp = (char*)__get_free_page(GFP_TEMPORARY);
+		if (tmp)
+			pathname = dentry_path_raw(bprm->file->f_dentry,
+						   tmp, PAGE_SIZE);
+		else
+			pathname = ERR_PTR(-ENOMEM);
+		if (IS_ERR(pathname))
+			trace_sched_process_exec(current, old_pid,
+						 "//error-no-mem");
+		else
+			trace_sched_process_exec(current, old_pid, pathname);
+		free_page((unsigned long)tmp);
+	}
+}
+
 static int exec_binprm(struct linux_binprm *bprm)
 {
 	pid_t old_pid, old_vpid;
@@ -1421,7 +1451,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);
+		do_trace_sched_process_exec(old_pid, bprm);
 		ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
 		proc_exec_connector(current);
 	}
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 67e1bbf..520ba9a 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -282,18 +282,18 @@ TRACE_EVENT(sched_process_fork,
 TRACE_EVENT(sched_process_exec,
 
 	TP_PROTO(struct task_struct *p, pid_t old_pid,
-		 struct linux_binprm *bprm),
+		 const char *pathname),
 
-	TP_ARGS(p, old_pid, bprm),
+	TP_ARGS(p, old_pid, pathname),
 
 	TP_STRUCT__entry(
-		__string(	filename,	bprm->filename	)
+		__string(	filename,	pathname	)
 		__field(	pid_t,		pid		)
 		__field(	pid_t,		old_pid		)
 	),
 
 	TP_fast_assign(
-		__assign_str(filename, bprm->filename);
+		__assign_str(filename, pathname);
 		__entry->pid		= p->pid;
 		__entry->old_pid	= old_pid;
 	),
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/