[NAK] Re: [PATCHv3 1/4] fs: Move core dump functionality into itsown file

[Kees Cook]

Hi Alex,

Overall, this seems like a fine idea. However, I think the code move
has gone very badly. Comments are mismatched, there are typos added,
formatting changed, case changed, trailing white space added, and
punctuation dropped.

NAKed-by: Kees Cook <keescook@xxxxxxxxxxxx>

On Sun, Aug 05, 2012 at 04:18:38AM -0700, Alex Kelly wrote:
> +/* The maximal length of core_pattern is also specified in sysctl.c */
> +static int zap_process(struct task_struct *start, int exit_code)

These two lines have nothing to do with each other. This comment belongs
with "char core_pattern[CORENAME_MAX_SIZE] = "core";", IIUC.

Why is zap_process not with the zap_threads helpers?

> +void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> +[...]
> +	 * so we dump it as root in mode 2, and onlt into a controlled

"onlt"? The removed code didn't have this typo. Was the moved code
not cut/pasted? That got my attention. In fact, I went and compared
all the code that was moved, and this was not the only change -- what
happened here?

--- /tmp/del.code	2012-08-05 11:32:36.602881193 -0700
+++ /tmp/add.code	2012-08-05 11:33:06.615270285 -0700
@@ -327,7 +328,7 @@
 		core_waiters = zap_threads(tsk, mm, core_state, exit_code);
 	up_write(&mm->mmap_sem);
 
-	if (core_waiters > 0) {
+	if (core_waiters > 0){
 		struct core_thread *ptr;
 
 		wait_for_completion(&core_state->startup);
@@ -466,8 +467,8 @@
 	/*
 	 * We cannot trust fsuid as being the "true" uid of the process
 	 * nor do we know its entire history. We only know it was tainted
-	 * so we dump it as root in mode 2, and only into a controlled
-	 * environment (pipe handler or fully qualified path).
+	 * so we dump it as root in mode 2, and onlt into a controlled
+	 * environment (pipe handler or fully qualified path)
 	 */
 	if (__get_dumpable(cprm.mm_flags) == SUID_DUMPABLE_SAFE) {
 		/* Setuid core dump mode */
@@ -505,11 +506,11 @@
 			 *
 			 * Normally core limits are irrelevant to pipes, since
 			 * we're not writing to the file system, but we use
-			 * cprm.limit of 1 here as a speacial value, this is a
+			 * cprm.limit of 1 here as a speacial value, this is a 
 			 * consistent way to catch recursive crashes.
 			 * We can still crash if the core_pattern binary sets
 			 * RLIM_CORE = !1, but it runs as root, and can do
-			 * lots of stupid things.
+			 * lots of stupid things
 			 *
 			 * Note that we use task_tgid_vnr here to grab the pid
 			 * of the process group leader.  That way we get the
@@ -556,7 +557,7 @@
 
 		if (need_nonrelative && cn.corename[0] != '/') {
 			printk(KERN_WARNING "Pid %d(%s) can only dump core "\
-				"to fully qualified path!\n",
+				"To fully qualified path!\n",
 				task_tgid_vnr(current), current->comm);
 			printk(KERN_WARNING "Skipping core dump\n");
 			goto fail_unlock;


-Kees

-- 
Kees Cook                                            @outflux.net
--
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/