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

[Steven Rostedt]

On Tue, 4 Feb 2014 12:18:53 -0800
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> That's too ugly to live.

New patch. Not as ugly. Well, I think this one lacks ugly enough to be
worth living for.

It's dependent on another patch that adds a helper function for
tracepoints, that allows users to implicitly use static_key of a
tracepoint to see if it is enabled or not. Basically, it's:

#define tracepoint_enabled(name) \
	static_key_false(&__tracepoint_##name.key)

This uses the same key that the tracepoint has when it is enabled. It
may be enabled before or after the tracepoint is, but in cases like
these, it doesn't really matter.

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().

Note, this still requires some comments added to the code.

Butt-ugly-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

diff --git a/fs/exec.c b/fs/exec.c
index e1529b4..f7902aef 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1421,7 +1421,22 @@ 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);
+		if (tracepoint_enabled(sched_process_exec)) {
+			char *tmp = (char*)__get_free_page(GFP_TEMPORARY);
+			char *pathname;
+
+			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);
+		}
 		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/