[PATCH 0/6] coredump: format_corename() fixes/cleanups

[PATCH 0/6] coredump: format_corename() fixes/cleanups

[Oleg Nesterov]

Hello.

On 05/13, Oleg Nesterov wrote:
>
> With the patch below we can trivially fix the problem,
>
>       +       char *fmt = ispipe ? "\e%s\e" : "%s";
>       ...
>       -       err = cn_printf(cn, "%s", current->comm);
>       +       err = cn_printf(cn, fmt, current->comm);
>
> Or this ESC hack is too ugly or can break something?

OK, nobody really nacked "[PATCH] teach argv_split() to ignore the spaces
surrounded by \e", see http://marc.info/?l=linux-kernel&m=136845597401674

I am going to send this patch "officially" and fix format_corename/argv_split
interaction.

But lets fix other format_corename() bugs first: leak and use-after-free.
Plus some cleanups.

Oleg.

 fs/coredump.c |  120 +++++++++++++++++++++++++++-----------------------------
 1 files changed, 58 insertions(+), 62 deletions(-)

[PATCH 1/6] coredump: format_corename() can leak cn->corename

[Oleg Nesterov]

do_coredump() assumes that format_corename() can only fail if
expand_corename() fails and frees cn->corename. This is not true,
for example cn_print_exe_file() can fail and in this case nobody
frees cn->corename.

Change do_coredump() to always do kfree(cn->corename) after it
calls format_corename() (NULL is fine), change expand_corename()
to do nothing if kmalloc() fails.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index dafafba..11bc368 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -58,16 +58,14 @@ static atomic_t call_count = ATOMIC_INIT(1);
 
 static int expand_corename(struct core_name *cn)
 {
-	char *old_corename = cn->corename;
+	int size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count);
+	char *corename = krealloc(cn->corename, size, GFP_KERNEL);
 
-	cn->size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count);
-	cn->corename = krealloc(old_corename, cn->size, GFP_KERNEL);
-
-	if (!cn->corename) {
-		kfree(old_corename);
+	if (!corename)
 		return -ENOMEM;
-	}
 
+	cn->size = size;
+	cn->corename = corename;
 	return 0;
 }
 
@@ -157,10 +155,9 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 	int pid_in_pattern = 0;
 	int err = 0;
 
+	cn->used = 0;
 	cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count);
 	cn->corename = kmalloc(cn->size, GFP_KERNEL);
-	cn->used = 0;
-
 	if (!cn->corename)
 		return -ENOMEM;
 
@@ -549,7 +546,7 @@ void do_coredump(siginfo_t *siginfo)
 		if (ispipe < 0) {
 			printk(KERN_WARNING "format_corename failed\n");
 			printk(KERN_WARNING "Aborting core\n");
-			goto fail_corename;
+			goto fail_unlock;
 		}
 
 		if (cprm.limit == 1) {
@@ -669,7 +666,6 @@ fail_dropcount:
 		atomic_dec(&core_dump_count);
 fail_unlock:
 	kfree(cn.corename);
-fail_corename:
 	coredump_finish(mm, core_dumped);
 	revert_creds(old_cred);
 fail_creds:
-- 
1.5.5.1

[PATCH 2/6] coredump: introduce cn_vprintf()

[Oleg Nesterov]

Turn cn_printf(...) into cn_vprintf(va_list args), reintroduce
cn_printf() as a trivial wrapper.

This simplifies the next change and cn_vprintf() will have more
callers.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 11bc368..c10a43a 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -69,17 +69,13 @@ static int expand_corename(struct core_name *cn)
 	return 0;
 }
 
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
 {
 	char *cur;
 	int need;
 	int ret;
-	va_list arg;
 
-	va_start(arg, fmt);
 	need = vsnprintf(NULL, 0, fmt, arg);
-	va_end(arg);
-
 	if (likely(need < cn->size - cn->used - 1))
 		goto out_printf;
 
@@ -89,9 +85,7 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 
 out_printf:
 	cur = cn->corename + cn->used;
-	va_start(arg, fmt);
 	vsnprintf(cur, need + 1, fmt, arg);
-	va_end(arg);
 	cn->used += need;
 	return 0;
 
@@ -99,6 +93,18 @@ expand_fail:
 	return ret;
 }
 
+static int cn_printf(struct core_name *cn, const char *fmt, ...)
+{
+	va_list arg;
+	int ret;
+
+	va_start(arg, fmt);
+	ret = cn_vprintf(cn, fmt, arg);
+	va_end(arg);
+
+	return ret;
+}
+
 static void cn_escape(char *str)
 {
 	for (; *str; str++)
-- 
1.5.5.1

[PATCH 3/6] coredump: cn_vprintf() has no reason to call vsnprintf() twice

[Oleg Nesterov]

cn_vprintf() looks really overcomplicated and sub-optimal.
We do not need vsnprintf(NULL) to calculate the size we
need, we can simply try to print into the current buffer
and expand/retry only if necessary.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   28 +++++++++++-----------------
 1 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index c10a43a..2b1d1f5 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -71,26 +71,20 @@ static int expand_corename(struct core_name *cn)
 
 static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
 {
-	char *cur;
-	int need;
-	int ret;
-
-	need = vsnprintf(NULL, 0, fmt, arg);
-	if (likely(need < cn->size - cn->used - 1))
-		goto out_printf;
-
-	ret = expand_corename(cn);
-	if (ret)
-		goto expand_fail;
+	int free, need;
+
+again:
+	free = cn->size - cn->used;
+	need = vsnprintf(cn->corename + cn->used, free, fmt, arg);
+	if (need < free) {
+		cn->used += need;
+		return 0;
+	}
 
-out_printf:
-	cur = cn->corename + cn->used;
-	vsnprintf(cur, need + 1, fmt, arg);
-	cn->used += need;
-	return 0;
+	if (!expand_corename(cn))
+		goto again;
 
-expand_fail:
-	return ret;
+	return -ENOMEM;
 }
 
 static int cn_printf(struct core_name *cn, const char *fmt, ...)
-- 
1.5.5.1

[PATCH 4/6] coredump: kill cn_escape(), introduce cn_esc_printf()

[Oleg Nesterov]

The usage of cn_escape() looks really annoying, imho this
sequence needs a wrapper. And it is buggy. If cn_printf()
does expand_corename() cn_escape() writes to the freed
memory.

Introduce cn_esc_printf() which hopefully does this all right.
It records the index before cn_vprintf(), not "char *" which
is no longer valid (in general) after krealloc().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   44 +++++++++++++++++++++-----------------------
 1 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 2b1d1f5..8b42688 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -99,11 +99,21 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static void cn_escape(char *str)
+static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
-	for (; *str; str++)
-		if (*str == '/')
-			*str = '!';
+	int cur = cn->used;
+	va_list arg;
+	int ret;
+
+	va_start(arg, fmt);
+	ret = cn_vprintf(cn, fmt, arg);
+	va_end(arg);
+
+	for (; cur < cn->used; ++cur) {
+		if (cn->corename[cur] == '/')
+			cn->corename[cur] = '!';
+	}
+	return ret;
 }
 
 static int cn_print_exe_file(struct core_name *cn)
@@ -113,12 +123,8 @@ static int cn_print_exe_file(struct core_name *cn)
 	int ret;
 
 	exe_file = get_mm_exe_file(current->mm);
-	if (!exe_file) {
-		char *commstart = cn->corename + cn->used;
-		ret = cn_printf(cn, "%s (path unknown)", current->comm);
-		cn_escape(commstart);
-		return ret;
-	}
+	if (!exe_file)
+		ret = cn_esc_printf(cn, "%s (path unknown)", current->comm);
 
 	pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
 	if (!pathbuf) {
@@ -132,9 +138,7 @@ static int cn_print_exe_file(struct core_name *cn)
 		goto free_buf;
 	}
 
-	cn_escape(path);
-
-	ret = cn_printf(cn, "%s", path);
+	ret = cn_esc_printf(cn, "%s", path);
 
 free_buf:
 	kfree(pathbuf);
@@ -207,22 +211,16 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			}
 			/* hostname */
-			case 'h': {
-				char *namestart = cn->corename + cn->used;
+			case 'h':
 				down_read(&uts_sem);
-				err = cn_printf(cn, "%s",
+				err = cn_esc_printf(cn, "%s",
 					      utsname()->nodename);
 				up_read(&uts_sem);
-				cn_escape(namestart);
 				break;
-			}
 			/* executable */
-			case 'e': {
-				char *commstart = cn->corename + cn->used;
-				err = cn_printf(cn, "%s", current->comm);
-				cn_escape(commstart);
+			case 'e':
+				err = cn_esc_printf(cn, "%s", current->comm);
 				break;
-			}
 			case 'E':
 				err = cn_print_exe_file(cn);
 				break;
-- 
1.5.5.1

[PATCH v2 4/6] coredump: kill cn_escape(), introduce cn_esc_printf()

[Oleg Nesterov]

On 05/15, Oleg Nesterov wrote:
>
> @@ -113,12 +123,8 @@ static int cn_print_exe_file(struct core_name *cn)
>  	int ret;
>  
>  	exe_file = get_mm_exe_file(current->mm);
> -	if (!exe_file) {
> -		char *commstart = cn->corename + cn->used;
> -		ret = cn_printf(cn, "%s (path unknown)", current->comm);
> -		cn_escape(commstart);
> -		return ret;
> -	}
> +	if (!exe_file)
> +		ret = cn_esc_printf(cn, "%s (path unknown)", current->comm);

Argh, sorry, typo... should be "return cn_esc_printf(...);

-------------------------------------------------------------------------------
[PATCH v2 4/6] coredump: kill cn_escape(), introduce cn_esc_printf()

The usage of cn_escape() looks really annoying, imho this
sequence needs a wrapper. And it is buggy. If cn_printf()
does expand_corename() cn_escape() writes to the freed
memory.

Introduce cn_esc_printf() which hopefully does this all right.
It records the index before cn_vprintf(), not "char *" which
is no longer valid (in general) after krealloc().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   44 +++++++++++++++++++++-----------------------
 1 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 2b1d1f5..90d7cee 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -99,11 +99,21 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static void cn_escape(char *str)
+static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
-	for (; *str; str++)
-		if (*str == '/')
-			*str = '!';
+	int cur = cn->used;
+	va_list arg;
+	int ret;
+
+	va_start(arg, fmt);
+	ret = cn_vprintf(cn, fmt, arg);
+	va_end(arg);
+
+	for (; cur < cn->used; ++cur) {
+		if (cn->corename[cur] == '/')
+			cn->corename[cur] = '!';
+	}
+	return ret;
 }
 
 static int cn_print_exe_file(struct core_name *cn)
@@ -113,12 +123,8 @@ static int cn_print_exe_file(struct core_name *cn)
 	int ret;
 
 	exe_file = get_mm_exe_file(current->mm);
-	if (!exe_file) {
-		char *commstart = cn->corename + cn->used;
-		ret = cn_printf(cn, "%s (path unknown)", current->comm);
-		cn_escape(commstart);
-		return ret;
-	}
+	if (!exe_file)
+		return cn_esc_printf(cn, "%s (path unknown)", current->comm);
 
 	pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
 	if (!pathbuf) {
@@ -132,9 +138,7 @@ static int cn_print_exe_file(struct core_name *cn)
 		goto free_buf;
 	}
 
-	cn_escape(path);
-
-	ret = cn_printf(cn, "%s", path);
+	ret = cn_esc_printf(cn, "%s", path);
 
 free_buf:
 	kfree(pathbuf);
@@ -207,22 +211,16 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			}
 			/* hostname */
-			case 'h': {
-				char *namestart = cn->corename + cn->used;
+			case 'h':
 				down_read(&uts_sem);
-				err = cn_printf(cn, "%s",
+				err = cn_esc_printf(cn, "%s",
 					      utsname()->nodename);
 				up_read(&uts_sem);
-				cn_escape(namestart);
 				break;
-			}
 			/* executable */
-			case 'e': {
-				char *commstart = cn->corename + cn->used;
-				err = cn_printf(cn, "%s", current->comm);
-				cn_escape(commstart);
+			case 'e':
+				err = cn_esc_printf(cn, "%s", current->comm);
 				break;
-			}
 			case 'E':
 				err = cn_print_exe_file(cn);
 				break;
-- 
1.5.5.1

[PATCH 5/6] coredump: kill call_count, add core_name_size

[Oleg Nesterov]

Imho, "atomic_t call_count" is ugly and should die. It buys
nothing and in fact it can grow more than necessary, expand
doesn't check if it was already incremented by another task.

Kill it, and introduce "static int core_name_size" updated by
expand_corename(). This is obviously racy too but harmless,
and core_name_size never grows for no reason.

We do not bother to to calculate the "right" new size, we
simply do kmalloc(size_we_need) and use ksize() to rely on
kmalloc_index's decision.

Finally change format_corename() to use expand_corename(),
krealloc(NULL) is fine.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 8b42688..10ba96a 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -45,26 +45,28 @@
 #include <trace/events/sched.h>
 
 int core_uses_pid;
-char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
+char core_pattern[CORENAME_MAX_SIZE] = "core";
+static int core_name_size = CORENAME_MAX_SIZE;
 
 struct core_name {
 	char *corename;
 	int used, size;
 };
-static atomic_t call_count = ATOMIC_INIT(1);
 
 /* The maximal length of core_pattern is also specified in sysctl.c */
 
-static int expand_corename(struct core_name *cn)
+static int expand_corename(struct core_name *cn, int size)
 {
-	int size = CORENAME_MAX_SIZE * atomic_inc_return(&call_count);
 	char *corename = krealloc(cn->corename, size, GFP_KERNEL);
 
 	if (!corename)
 		return -ENOMEM;
 
-	cn->size = size;
+	if (size > core_name_size) /* racy but harmless */
+		core_name_size = size;
+
+	cn->size = ksize(corename);
 	cn->corename = corename;
 	return 0;
 }
@@ -81,7 +83,7 @@ again:
 		return 0;
 	}
 
-	if (!expand_corename(cn))
+	if (!expand_corename(cn, cn->size + need - free + 1))
 		goto again;
 
 	return -ENOMEM;
@@ -160,9 +162,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 	int err = 0;
 
 	cn->used = 0;
-	cn->size = CORENAME_MAX_SIZE * atomic_read(&call_count);
-	cn->corename = kmalloc(cn->size, GFP_KERNEL);
-	if (!cn->corename)
+	cn->corename = NULL;
+	if (expand_corename(cn, core_name_size))
 		return -ENOMEM;
 
 	/* Repeat as long as we have more pattern to process and more output
-- 
1.5.5.1

Re: [PATCH 5/6] coredump: kill call_count, add core_name_size

[Andrew Morton]

On Wed, 15 May 2013 22:12:32 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> Imho, "atomic_t call_count" is ugly and should die. It buys
> nothing and in fact it can grow more than necessary, expand
> doesn't check if it was already incremented by another task.
> 
> Kill it, and introduce "static int core_name_size" updated by
> expand_corename(). This is obviously racy too but harmless,
> and core_name_size never grows for no reason.
> 
> We do not bother to to calculate the "right" new size, we
> simply do kmalloc(size_we_need) and use ksize() to rely on
> kmalloc_index's decision.
> 
> Finally change format_corename() to use expand_corename(),
> krealloc(NULL) is fine.

The code still looks like a bunch of fluff.  I look at it and think
"wtf, why doesn't it just use kasprintf()". 

If there were any comments in there at all which explained the reason
for the code's existence, perhaps I wouldn't think that.  But there
aren't, so I do.

Re: [PATCH 5/6] coredump: kill call_count, add core_name_size

[Oleg Nesterov]

On 05/24, Andrew Morton wrote:
>
> On Wed, 15 May 2013 22:12:32 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Imho, "atomic_t call_count" is ugly and should die. It buys
> > nothing and in fact it can grow more than necessary, expand
> > doesn't check if it was already incremented by another task.
> >
> > Kill it, and introduce "static int core_name_size" updated by
> > expand_corename(). This is obviously racy too but harmless,
> > and core_name_size never grows for no reason.
> >
> > We do not bother to to calculate the "right" new size, we
> > simply do kmalloc(size_we_need) and use ksize() to rely on
> > kmalloc_index's decision.
> >
> > Finally change format_corename() to use expand_corename(),
> > krealloc(NULL) is fine.
>
> The code still looks like a bunch of fluff.  I look at it and think
> "wtf, why doesn't it just use kasprintf()".

But how?

kasprintf() can't replace cn_printf(), and it can't make it simpler.

If it was possible to create va_list dinamically then format_corename()
could construct "char *fmt" and call kvasprintf() once.

Or we can change this code to avoid *printk* altogether, we only need
a very limited subset of "enum format_type". Not sure this makes sense.

> If there were any comments in there at all which explained the reason
> for the code's existence, perhaps I wouldn't think that.  But there
> aren't, so I do.

If you meant "why do we need expand_corename" I can't answer because
I do not know ;) I mean, if CORENAME_MAX_SIZE == 128 is not enough we
can probably just increase it and simplify the code.

Please see 1b0d300b "core_pattern: fix truncation by core_pattern handler
with long parameters" which introduced this.

And yes, we can keep "expand" but simply kill "atomic_t call_count"
(replaced by core_name_size in this patch), I do not think it buys
too much. But at least with this patch this logic becomes really
trivial.


Andrew, it seems that you missed the last patch in this series,
attached below. I sent it a bit later as 7/6 because I didn't notice
this problem when I started these changes.

Oleg.

-------------------------------------------------------------------------------
[PATCH 7/6] coredump: avoid the uninitialized cn->corename if core_pattern is empty

If core_pattern is "" or "|", cn->corename is used uninitialized
by filp_open() or call_usermodehelper_exec().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 5968064..72f816d 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -165,6 +165,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 	cn->corename = NULL;
 	if (expand_corename(cn, core_name_size))
 		return -ENOMEM;
+	cn->corename[0] = '\0';
 
 	if (ispipe)
 		++pat_ptr;
-- 
1.5.5.1

[PATCH 6/6] coredump: '% at the end' shouldn't bypass core_uses_pid logic

[Oleg Nesterov]

"goto end" should not bypass the "Backward compatibility with
core_uses_pid" code, move this label up.

While at it,

	- It is ugly to copy '|' into cn->corename and then inc
	  the pointer for argv_split().

	  Change format_corename() to increment pat_ptr instead.

	- Remove the dead "if (*pat_ptr == 0)" in format_corename(),
	  we already checked it is not zero.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 10ba96a..a2ace9f 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -166,12 +166,13 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 	if (expand_corename(cn, core_name_size))
 		return -ENOMEM;
 
+	if (ispipe)
+		++pat_ptr;
+
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
 		if (*pat_ptr != '%') {
-			if (*pat_ptr == 0)
-				goto out;
 			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
 			switch (*++pat_ptr) {
@@ -240,6 +241,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 			return err;
 	}
 
+out:
 	/* Backward compatibility with core_uses_pid:
 	 *
 	 * If core_pattern does not include a %p (as is the default)
@@ -250,7 +252,6 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 		if (err)
 			return err;
 	}
-out:
 	return ispipe;
 }
 
@@ -580,7 +581,7 @@ void do_coredump(siginfo_t *siginfo)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, cn.corename+1, NULL);
+		helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
@@ -597,7 +598,7 @@ void do_coredump(siginfo_t *siginfo)
 
 		argv_free(helper_argv);
 		if (retval) {
-			printk(KERN_INFO "Core dump to %s pipe failed\n",
+			printk(KERN_INFO "Core dump to |%s pipe failed\n",
 			       cn.corename);
 			goto close_fail;
 		}
-- 
1.5.5.1

Re: [PATCH 0/6] coredump: format_corename() fixes/cleanups

[Neil Horman]

On Wed, May 15, 2013 at 10:11:58PM +0200, Oleg Nesterov wrote:
> Hello.
> 
> On 05/13, Oleg Nesterov wrote:
> >
> > With the patch below we can trivially fix the problem,
> >
> >       +       char *fmt = ispipe ? "\e%s\e" : "%s";
> >       ...
> >       -       err = cn_printf(cn, "%s", current->comm);
> >       +       err = cn_printf(cn, fmt, current->comm);
> >
> > Or this ESC hack is too ugly or can break something?
> 
> OK, nobody really nacked "[PATCH] teach argv_split() to ignore the spaces
> surrounded by \e", see http://marc.info/?l=linux-kernel&m=136845597401674
> 
> I am going to send this patch "officially" and fix format_corename/argv_split
> interaction.
> 
> But lets fix other format_corename() bugs first: leak and use-after-free.
> Plus some cleanups.
> 
> Oleg.
> 
>  fs/coredump.c |  120 +++++++++++++++++++++++++++-----------------------------
>  1 files changed, 58 insertions(+), 62 deletions(-)
> 
> 
For the series

Acked-by: Neil Horman <nhorman@tuxdriver.com>

[PATCH 1/1] usermodehelper: check subprocess_info->path != NULL

[Oleg Nesterov]

argv_split(empty_or_all_spaces) happily succeeds, it simply returns
argc == 0 and argv[0] == NULL. Change call_usermodehelper_exec() to
check sub_info->path != NULL to avoid the crash.

This is the minimal fix, todo:

	- perhaps we should change argv_split() to return NULL or
	  change the callers.

	- kill or justify ->path[0] check

	- narrow the scope of helper_lock()

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
---
 kernel/kmod.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 1296e72..8241906 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -569,6 +569,11 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	int retval = 0;
 
 	helper_lock();
+	if (!sub_info->path) {
+		retval = -EINVAL;
+		goto out;
+	}
+
 	if (sub_info->path[0] == '\0')
 		goto out;
 
-- 
1.5.5.1

Re: [PATCH 1/1] usermodehelper: check subprocess_info->path != NULL

[Lucas De Marchi]

Hi Oleg,

On Thu, May 16, 2013 at 12:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> argv_split(empty_or_all_spaces) happily succeeds, it simply returns
> argc == 0 and argv[0] == NULL. Change call_usermodehelper_exec() to
> check sub_info->path != NULL to avoid the crash.
>
> This is the minimal fix, todo:
>
>         - perhaps we should change argv_split() to return NULL or
>           change the callers.

Changing argv_split() would be the easiest way, but then we can't
differentiate the errors. Right now it returns NULL only on ENOMEM.


>
>         - kill or justify ->path[0] check

I'm not sure about this, it's already there before my refactor and I
don't think it makes any good. From modprobe pespective, I'd say it
would be better to give an error than say everything went ok.


>
>         - narrow the scope of helper_lock()
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  kernel/kmod.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 1296e72..8241906 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -569,6 +569,11 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>         int retval = 0;
>
>         helper_lock();
> +       if (!sub_info->path) {
> +               retval = -EINVAL;
> +               goto out;
> +       }
> +
>         if (sub_info->path[0] == '\0')
>                 goto out;
>
> --

Acked-By: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

Re: [PATCH 1/1] usermodehelper: check subprocess_info->path != NULL

[Oleg Nesterov]

On 05/16, Lucas De Marchi wrote:
>
> >
> >         - kill or justify ->path[0] check
>
> I'm not sure about this, it's already there before my refactor and I
> don't think it makes any good. From modprobe pespective, I'd say it
> would be better to give an error than say everything went ok.

Agreed. And, I forgot to mention, if we kill this check then we do
not need this patch (although I need to recheck), execve will fail
and nothing bad should happen.

Just I think it would be better to start with the trivial fix, then
decide what should we actually do.

> Acked-By: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks,

Oleg.

Re: [PATCH 7/6] coredump: avoid the uninitialized cn->corename if core_pattern is empty

[Oleg Nesterov]

off-topic...

Perhaps do_coredump() should warn if filp_open() fails?

And I'd wish I could understand d_unhashed() check...

And why filp_open() uses O_RDWR passed as hardcoded 2.

On 05/16, Oleg Nesterov wrote:
>
> If core_pattern is "" or "|", cn->corename is used uninitialized
> by filp_open() or call_usermodehelper_exec().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  fs/coredump.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 5968064..72f816d 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -165,6 +165,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  	cn->corename = NULL;
>  	if (expand_corename(cn, core_name_size))
>  		return -ENOMEM;
> +	cn->corename[0] = '\0';
>  
>  	if (ispipe)
>  		++pat_ptr;
> -- 
> 1.5.5.1
>