[PATCH] argv_split: Return NULL if argument contains no non-whitespace.

[PATCH] argv_split: Return NULL if argument contains no non-whitespace.

[Tetsuo Handa]

>From 210f917f3b535bc0d4dcbb20ca4395709e913104 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 14 Sep 2013 16:24:07 +0900
Subject: [PATCH] argv_split: Return NULL if argument contains no non-whitespace.

I tried

  # echo '|' > /proc/sys/kernel/core_pattern

and got

  BUG: unable to handle kernel NULL pointer dereference at   (null)

upon core dump because helper_argv[0] == NULL at

  helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
  call_usermodehelper_setup(helper_argv[0], ...);

if cn.corename == "".

How to check this bug:

  # echo '|' > /proc/sys/kernel/core_pattern
  $ echo 'int main(int argc, char *argv[]) { return *(char *) 0; }' | gcc -x c - -o die
  $ ulimit -c unlimited
  $ ./die

This bug seems to exist since 2.6.19 (the version which core dump to pipe was
added). Depending on kernel version and config, some side effect might follow
immediately after this oops (e.g. kernel panic with 2.6.32-358.18.1.el6).

Assuming that nobody is expecting that argv_split() returns an array with
argv[0] == NULL, this patch fixes this bug by changing argv_split() to return
NULL if argument contains no non-whitespace.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 lib/argv_split.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/argv_split.c b/lib/argv_split.c
index e927ed0..5b828d9 100644
--- a/lib/argv_split.c
+++ b/lib/argv_split.c
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(argv_free);
  * quote processing is performed.  Multiple whitespace characters are
  * considered to be a single argument separator.  The returned array
  * is always NULL-terminated.  Returns NULL on memory allocation
- * failure.
+ * failure or @str being empty or @str containing only white-space.
  *
  * The source string at `str' may be undergoing concurrent alteration via
  * userspace sysctl activity (at least).  The argv_split() implementation
@@ -68,6 +68,10 @@ char **argv_split(gfp_t gfp, const char *str, int *argcp)
 		return NULL;
 
 	argc = count_argc(argv_str);
+	if (!argc) {
+		kfree(argv_str);
+		return NULL;
+	}
 	argv = kmalloc(sizeof(*argv) * (argc + 2), gfp);
 	if (!argv) {
 		kfree(argv_str);
-- 
1.7.1

Re: [PATCH] argv_split: Return NULL if argument contains no non-whitespace.

[Oleg Nesterov]

On 09/14, Tetsuo Handa wrote:
>
>   # echo '|' > /proc/sys/kernel/core_pattern
>
> and got
>
>   BUG: unable to handle kernel NULL pointer dereference at   (null)

Hmm. This was fixed by 264b83c07a8 "usermodehelper: check
subprocess_info->path != NULL".

But then this check was removed by 7f57cfa4e2a "usermodehelper:
kill the sub_info->path[0] check".

Note that the changelog says "do_execve(NULL) is safe" and I certainly
tested this case when I sent the patch...

Now it is crashes in path_openat() because pathname->name is NULL.
Something was changed, perhaps  or (I'm afraid) I misread that code and
my testing was wrong. do_filp_open/etc were changed to accept
"struct filename *" a long ago.

> upon core dump because helper_argv[0] == NULL at
>
>   helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
>   call_usermodehelper_setup(helper_argv[0], ...);

Are you sure? See above.

> --- a/lib/argv_split.c
> +++ b/lib/argv_split.c
> @@ -50,7 +50,7 @@ EXPORT_SYMBOL(argv_free);
>   * quote processing is performed.  Multiple whitespace characters are
>   * considered to be a single argument separator.  The returned array
>   * is always NULL-terminated.  Returns NULL on memory allocation
> - * failure.
> + * failure or @str being empty or @str containing only white-space.
>   *
>   * The source string at `str' may be undergoing concurrent alteration via
>   * userspace sysctl activity (at least).  The argv_split() implementation
> @@ -68,6 +68,10 @@ char **argv_split(gfp_t gfp, const char *str, int *argcp)
>  		return NULL;
>
>  	argc = count_argc(argv_str);
> +	if (!argc) {
> +		kfree(argv_str);
> +		return NULL;
> +	}

Yes, this is what 264b83c07a8 suggested... But I am not sure, if nothing
else pr_warn("failed to allocate memory") from do_coredump() doesn't look
nice in this case.

Perhaps

	--- x/kernel/kmod.c
	+++ x/kernel/kmod.c
	@@ -571,6 +571,9 @@ int call_usermodehelper_exec(struct subp
		DECLARE_COMPLETION_ONSTACK(done);
		int retval = 0;
	 
	+	if (!sub_info->path)
	+		return -EXXX;
	+
		helper_lock();
		if (!khelper_wq || usermodehelper_disabled) {
			retval = -EBUSY;

?

Oleg.

Re: [PATCH] argv_split: Return NULL if argument contains nonon-whitespace.

[Tetsuo Handa]

Oleg Nesterov wrote:
> > upon core dump because helper_argv[0] == NULL at
> >
> >   helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
> >   call_usermodehelper_setup(helper_argv[0], ...);
> 
> Are you sure? See above.
> 

Yes, I'm sure. execve(NULL) from user space is safe, but
do_execve(NULL) from kernel space is not safe.

---------- patch start ----------
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -763,6 +763,9 @@ struct file *open_exec(const char *name)
 		.lookup_flags = LOOKUP_FOLLOW,
 	};
 
+	if (WARN_ON(!name))
+		return ERR_PTR(-EINVAL);
+
 	file = do_filp_open(AT_FDCWD, &tmp, &open_exec_flags);
 	if (IS_ERR(file))
 		goto out;
---------- patch end ----------

---------- dmesg start ----------
die[3924]: segfault at 0 ip 0804839c sp bf9d3c78 error 4 in die[8048000+1000]
------------[ cut here ]------------
WARNING: CPU: 1 PID: 3925 at fs/exec.c:766 open_exec+0xfd/0x110()
Modules linked in: ipv6 binfmt_misc
CPU: 1 PID: 3925 Comm: kworker/u4:0 Not tainted 3.11.0-10050-g3711d86-dirty #111
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 08/15/2008
 00000000 c0557e9e c068909b c0139756 c067af64 00000001 00000f55 c068909b
 000002fe c01da24d c01da24d 00000000 de754a80 df120b50 00000000 c013979b
 00000009 00000000 c01da24d de6626c0 c0158c68 df120b50 00000000 00000000
Call Trace:
 [<c0557e9e>] ? dump_stack+0x3e/0x50
 [<c0139756>] ? warn_slowpath_common+0x86/0xb0
 [<c01da24d>] ? open_exec+0xfd/0x110
 [<c01da24d>] ? open_exec+0xfd/0x110
 [<c013979b>] ? warn_slowpath_null+0x1b/0x20
 [<c01da24d>] ? open_exec+0xfd/0x110
 [<c0158c68>] ? prepare_creds+0x88/0xb0
 [<c01da98c>] ? do_execve+0x18c/0x560
 [<c014aa2c>] ? ____call_usermodehelper+0xbc/0xe0
 [<c05605f7>] ? ret_from_kernel_thread+0x1b/0x28
 [<c014aa50>] ? ____call_usermodehelper+0xe0/0xe0
---[ end trace 63bb92bc8d58b0c2 ]---
Core dump to | pipe failed
---------- dmesg end ----------

> Perhaps
> 
> 	--- x/kernel/kmod.c
> 	+++ x/kernel/kmod.c
> 	@@ -571,6 +571,9 @@ int call_usermodehelper_exec(struct subp
> 		DECLARE_COMPLETION_ONSTACK(done);
> 		int retval = 0;
> 	 
> 	+	if (!sub_info->path)
> 	+		return -EXXX;
> 	+
> 		helper_lock();
> 		if (!khelper_wq || usermodehelper_disabled) {
> 			retval = -EBUSY;
> 
> ?
> 

I'm OK with that.

[PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Tetsuo Handa]

>From fe6723ba2816b42e26697472a3f2a3045614032b Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sun, 15 Sep 2013 23:17:15 +0900
Subject: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

If /proc/sys/kernel/core_pattern contains only "|", NULL pointer dereference
happens upon core dump because argv_split("") returns argv[0] == NULL.

This bug seems to exist since 2.6.19 (the version which core dump to pipe was
added). Depending on kernel version and config, some side effect might happen
immediately after this oops (e.g. kernel panic with 2.6.32-358.18.1.el6).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/kmod.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index fb32636..3b59f6e 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -572,6 +572,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	int retval = 0;
 
 	helper_lock();
+	if (!sub_info->path) {
+		retval = -ENOENT;
+		goto out;
+	}
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
 		goto out;
-- 
1.7.1

Re: [PATCH] argv_split: Return NULL if argument contains nonon-whitespace.

[Oleg Nesterov]

On 09/15, Tetsuo Handa wrote:
>
> Oleg Nesterov wrote:
> > > upon core dump because helper_argv[0] == NULL at
> > >
> > >   helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
> > >   call_usermodehelper_setup(helper_argv[0], ...);
> >
> > Are you sure? See above.
> >
>
> Yes, I'm sure.

I thougt you meant that call_usermodehelper_setup() crashes. "See above"
means that afaics it should crash in do_filp_open().

> execve(NULL) from user space is safe,

because it never does do_execve(NULL),

> but
> do_execve(NULL) from kernel space is not safe.

Yes, this is clear.

> > Perhaps
> >
> > 	--- x/kernel/kmod.c
> > 	+++ x/kernel/kmod.c
> > 	@@ -571,6 +571,9 @@ int call_usermodehelper_exec(struct subp
> > 		DECLARE_COMPLETION_ONSTACK(done);
> > 		int retval = 0;
> >
> > 	+	if (!sub_info->path)
> > 	+		return -EXXX;
> > 	+
> > 		helper_lock();
> > 		if (!khelper_wq || usermodehelper_disabled) {
> > 			retval = -EBUSY;
> >
> > ?
> >
>
> I'm OK with that.

OK,

Oleg.

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Oleg Nesterov]

ACK, but...

On 09/15, Tetsuo Handa wrote:
>
> @@ -572,6 +572,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	int retval = 0;
>  
>  	helper_lock();
> +	if (!sub_info->path) {
> +		retval = -ENOENT;
> +		goto out;
> +	}

May I suggest you to send v2?

It looks a bit ugly to check ->path under helper_lock(), just add

	if (!sub_info->path)
		retval = -ENOENT;

at the start. Otherwise the code looks as if there is a subtle
reason to take the lock before this check.


Perhaps you can also mention that this problems was fixed by
264b83c07a and right after that 7f57cfa4e2 reintroduced it,
because I am stupid.

Oleg.

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Tetsuo Handa]

Oleg Nesterov wrote:
> It looks a bit ugly to check ->path under helper_lock(), just add
> 
> 	if (!sub_info->path)
> 		retval = -ENOENT;
> 
> at the start. Otherwise the code looks as if there is a subtle
> reason to take the lock before this check.

Did you mean this?

 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval = 0;
 
+	if (!sub_info->path) {
+		call_usermodehelper_freeinfo(sub_info);
+		return -ENOENT;
+	}
 	helper_lock();
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
 		goto out;
 	}

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Oleg Nesterov]

On 09/16, Tetsuo Handa wrote:
>
> Oleg Nesterov wrote:
> > It looks a bit ugly to check ->path under helper_lock(), just add
> >
> > 	if (!sub_info->path)
> > 		retval = -ENOENT;
> >
> > at the start. Otherwise the code looks as if there is a subtle
> > reason to take the lock before this check.
>
> Did you mean this?
>
>  	DECLARE_COMPLETION_ONSTACK(done);
>  	int retval = 0;
>
> +	if (!sub_info->path) {
> +		call_usermodehelper_freeinfo(sub_info);

Aah, sorry! I forgot about _freeinfo().

I still think that perhaps it makes sense to update the changelog,
but this is minor.

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Tetsuo Handa]

>From d6ff218545060c5f8b75b15d5b34bffcf625508f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 16 Sep 2013 02:19:10 +0900
Subject: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

If /proc/sys/kernel/core_pattern contains only "|", NULL pointer dereference
happens upon core dump because argv_split("") returns argv[0] == NULL.

This bug was once fixed by commit 264b83c0 "usermodehelper: check
subprocess_info->path != NULL" but was by error reintroduced by commit
7f57cfa4 "usermodehelper: kill the sub_info->path[0] check".

This bug seems to exist since 2.6.19 (the version which core dump to pipe was
added). Depending on kernel version and config, some side effect might happen
immediately after this oops (e.g. kernel panic with 2.6.32-358.18.1.el6).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index fb32636..a962470 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval = 0;
 
+	if (!sub_info->path) {
+		call_usermodehelper_freeinfo(sub_info);
+		return -ENOENT;
+	}
 	helper_lock();
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
-- 
1.7.1

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Oleg Nesterov]

Argh...

Tetsuo, I am sorry. Not only I am stupid, I managed to confuse you.

On 09/16, Tetsuo Handa wrote:
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>

I tried to ack you previous version which I wrongly blamed ;)

However, I agree with this version as well. Feel free to use
either one, you have my ack in any case.

Sorry to all for noise and confusion.

> ---
>  kernel/kmod.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index fb32636..a962470 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	DECLARE_COMPLETION_ONSTACK(done);
>  	int retval = 0;
>  
> +	if (!sub_info->path) {
> +		call_usermodehelper_freeinfo(sub_info);
> +		return -ENOENT;
> +	}
>  	helper_lock();
>  	if (!khelper_wq || usermodehelper_disabled) {
>  		retval = -EBUSY;
> -- 
> 1.7.1

[PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Tetsuo Handa]

Andrew, would you pick up this patch?

Regards.
----------
>From d6ff218545060c5f8b75b15d5b34bffcf625508f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 16 Sep 2013 02:19:10 +0900
Subject: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

If /proc/sys/kernel/core_pattern contains only "|", NULL pointer dereference
happens upon core dump because argv_split("") returns argv[0] == NULL.

This bug was once fixed by commit 264b83c0 "usermodehelper: check
subprocess_info->path != NULL" but was by error reintroduced by commit
7f57cfa4 "usermodehelper: kill the sub_info->path[0] check".

This bug seems to exist since 2.6.19 (the version which core dump to pipe was
added). Depending on kernel version and config, some side effect might happen
immediately after this oops (e.g. kernel panic with 2.6.32-358.18.1.el6).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index fb32636..a962470 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval = 0;
 
+	if (!sub_info->path) {
+		call_usermodehelper_freeinfo(sub_info);
+		return -ENOENT;
+	}
 	helper_lock();
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
-- 
1.7.1

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Andrew Morton]

On Tue, 24 Sep 2013 06:12:34 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Andrew, would you pick up this patch?
> 
> Regards.
> ----------
> >From d6ff218545060c5f8b75b15d5b34bffcf625508f Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 16 Sep 2013 02:19:10 +0900
> Subject: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().
> 
> If /proc/sys/kernel/core_pattern contains only "|", NULL pointer dereference
> happens upon core dump because argv_split("") returns argv[0] == NULL.
> 
> This bug was once fixed by commit 264b83c0 "usermodehelper: check
> subprocess_info->path != NULL" but was by error reintroduced by commit
> 7f57cfa4 "usermodehelper: kill the sub_info->path[0] check".
> 
> This bug seems to exist since 2.6.19 (the version which core dump to pipe was
> added). Depending on kernel version and config, some side effect might happen
> immediately after this oops (e.g. kernel panic with 2.6.32-358.18.1.el6).
> 
> ...
>
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	DECLARE_COMPLETION_ONSTACK(done);
>  	int retval = 0;
>  
> +	if (!sub_info->path) {
> +		call_usermodehelper_freeinfo(sub_info);
> +		return -ENOENT;
> +	}
>  	helper_lock();
>  	if (!khelper_wq || usermodehelper_disabled) {
>  		retval = -EBUSY;

The error is that the user put a bare "|" into
/proc/sys/kernel/core_pattern.  Is ENOENT ("No such file or directory")
the most appropriate error code here?  I think EINVAL ("Invalid
argument")?

Re: [PATCH] kmod: Check for NULL at call_usermodehelper_exec().

[Tetsuo Handa]

Andrew Morton wrote:
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
> >  	DECLARE_COMPLETION_ONSTACK(done);
> >  	int retval = 0;
> >  
> > +	if (!sub_info->path) {
> > +		call_usermodehelper_freeinfo(sub_info);
> > +		return -ENOENT;
> > +	}
> >  	helper_lock();
> >  	if (!khelper_wq || usermodehelper_disabled) {
> >  		retval = -EBUSY;
> 
> The error is that the user put a bare "|" into
> /proc/sys/kernel/core_pattern.  Is ENOENT ("No such file or directory")
> the most appropriate error code here?  I think EINVAL ("Invalid
> argument")?
> 
I'm fine with EINVAL. Updated patch follows.
----------
>From a1068778ed0dd3156dff0ac6245314b8627b8830 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 24 Sep 2013 23:54:05 +0900
Subject: [PATCH] kernel/kmod.c: check for NULL in call_usermodehelper_exec()

If /proc/sys/kernel/core_pattern contains only "|", NULL pointer
dereference happens upon core dump because argv_split("") returns argv[0]
== NULL.

This bug was once fixed by commit 264b83c0 ("usermodehelper: check
subprocess_info->path != NULL") but was by error reintroduced by commit
7f57cfa4 ("usermodehelper: kill the sub_info->path[0] check").

This bug seems to exist since 2.6.19 (the version which core dump to pipe
was added).  Depending on kernel version and config, some side effect
might happen immediately after this oops (e.g.  kernel panic with
2.6.32-358.18.1.el6).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/kmod.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index fb32636..b086006 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -571,6 +571,10 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval = 0;
 
+	if (!sub_info->path) {
+		call_usermodehelper_freeinfo(sub_info);
+		return -EINVAL;
+	}
 	helper_lock();
 	if (!khelper_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
-- 
1.7.1