[PATCH] core_pattern: set core helpers root and namespace to crashing process

[PATCH] core_pattern: set core helpers root and namespace to crashing process

[Neil Horman]

As its currently implemented, redirection of core dumps to a pipe reader should
be executed such that the reader runs in the namespace of the crashing process,
and it currently does not. This is the only sane way to deal with namespaces
properly it seems to me, and this patch implements that functionality.

Theres one problem I currently see with it, and that is that I'm not sure we can
change the current behavior of how the root fs is set for the pipe reader, lest
we break some user space expectations. As such, I've added a sysctl in this
patch to allow administrators to globally select if a core reader specified via
/proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
chrooted fs of the crashing process.

I've tested this using both settings for the new sysctl, and it works well.

For the sake of history, this was discussed before:
http://www.spinics.net/lists/linux-containers/msg21531.html

It seems there was some modicum of consensus as to how this should work, but
there was no action taken on it.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Daniel Berrange <berrange@redhat.com>
CC: Daniel Berrange <berrange@redhat.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Serge Hallyn <serge.hallyn@canonical.com>
CC: containers@lists.linux-foundation.org
---
 fs/coredump.c           | 11 +++++++++++
 include/linux/binfmts.h |  1 +
 kernel/sysctl.c         |  8 ++++++++
 3 files changed, 20 insertions(+)

diff --git a/fs/coredump.c b/fs/coredump.c
index ce47379..e7d8ca2 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -47,6 +47,7 @@
 int core_uses_pid;
 char core_pattern[CORENAME_MAX_SIZE] = "core";
 unsigned int core_pipe_limit;
+unsigned int core_pipe_global_root;
 
 struct core_name {
 	char *corename;
@@ -443,6 +444,7 @@ static void wait_for_dump_helpers(struct file *file)
 static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 {
 	struct file *files[2];
+	struct path root;
 	struct coredump_params *cp = (struct coredump_params *)info->data;
 	int err = create_pipe_files(files, 0);
 	if (err)
@@ -455,6 +457,14 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	/* and disallow core files too */
 	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
 
+
+	if (!core_pipe_global_root) {
+		get_fs_root(cp->cprocess->fs, &root);
+		set_fs_root(current->fs, &root);
+	}
+
+	switch_task_namespaces(current, cp->cprocess->nsproxy);
+
 	return err;
 }
 
@@ -476,6 +486,7 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
 		.siginfo = siginfo,
 		.regs = regs,
 		.limit = rlimit(RLIMIT_CORE),
+		.cprocess = current,
 		/*
 		 * We must use the same mm->flags while dumping core to avoid
 		 * inconsistency of bit flags, since this flag is not protected
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index cfcc6bf..08e7bcb 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -61,6 +61,7 @@ struct coredump_params {
 	siginfo_t *siginfo;
 	struct pt_regs *regs;
 	struct file *file;
+	struct task_struct *cprocess;
 	unsigned long limit;
 	unsigned long mm_flags;
 };
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 26f65ea..be7b69d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -102,6 +102,7 @@ extern int suid_dumpable;
 extern int core_uses_pid;
 extern char core_pattern[];
 extern unsigned int core_pipe_limit;
+extern unsigned int core_pipe_global_root;
 #endif
 extern int pid_max;
 extern int min_free_kbytes;
@@ -430,6 +431,13 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "core_pipe_global_root",
+		.data		= &core_pipe_global_root,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
 #endif
 #ifdef CONFIG_PROC_SYSCTL
 	{
-- 
1.7.11.7

Re: [PATCH] core_pattern: set core helpers root and namespace to crashing process

[Serge Hallyn]

Quoting Neil Horman (nhorman@tuxdriver.com):
> Theres one problem I currently see with it, and that is that I'm not sure we can
> change the current behavior of how the root fs is set for the pipe reader, lest
> we break some user space expectations. As such, I've added a sysctl in this
> patch to allow administrators to globally select if a core reader specified via
> /proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
> chrooted fs of the crashing process.

Practical question:  How is the admin to make an educated decision on
how to set the sysctl?

-serge

Re: [PATCH] core_pattern: set core helpers root and namespace to crashing process

[Neil Horman]

On Thu, Dec 13, 2012 at 06:20:48AM -0600, Serge Hallyn wrote:
> Quoting Neil Horman (nhorman@tuxdriver.com):
> > Theres one problem I currently see with it, and that is that I'm not sure we can
> > change the current behavior of how the root fs is set for the pipe reader, lest
> > we break some user space expectations. As such, I've added a sysctl in this
> > patch to allow administrators to globally select if a core reader specified via
> > /proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
> > chrooted fs of the crashing process.
> 
> Practical question:  How is the admin to make an educated decision on
> how to set the sysctl?
> 
My thought was that the admin typically wouldn't touch this at all.  I really
added it as a backwards compatibility option only.  Setting the user space
helper task to the root of the crashing parent has the possibility of breaking
existing installs because the core_pattern helper might be expecting global file
system access.  Moving forward, my expectation would be that core_pattern
helpers would be written with the default setting in mind, and we could
eventually deprecate the control entirely.

If you have a better mechanism in mind however (or if you think that removing
the control is a resaonable approach), I'm certainly open to that.

Neil

> -serge
> 

Re: [PATCH] core_pattern: set core helpers root and namespace to crashing process

[Andrew Morton]

On Thu, 13 Dec 2012 13:12:20 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

> On Thu, Dec 13, 2012 at 06:20:48AM -0600, Serge Hallyn wrote:
> > Quoting Neil Horman (nhorman@tuxdriver.com):
> > > Theres one problem I currently see with it, and that is that I'm not sure we can
> > > change the current behavior of how the root fs is set for the pipe reader, lest
> > > we break some user space expectations. As such, I've added a sysctl in this
> > > patch to allow administrators to globally select if a core reader specified via
> > > /proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
> > > chrooted fs of the crashing process.
> > 
> > Practical question:  How is the admin to make an educated decision on
> > how to set the sysctl?

By reading the documentation which Neil didn't include?

> My thought was that the admin typically wouldn't touch this at all.  I really
> added it as a backwards compatibility option only.  Setting the user space
> helper task to the root of the crashing parent has the possibility of breaking
> existing installs because the core_pattern helper might be expecting global file
> system access.  Moving forward, my expectation would be that core_pattern
> helpers would be written with the default setting in mind, and we could
> eventually deprecate the control entirely.
> 
> If you have a better mechanism in mind however (or if you think that removing
> the control is a resaonable approach), I'm certainly open to that.

Yeah, this is a tiresome patch but I can't think of a better way.

Except, perhaps, adding a new token to the core_pattern which says
"switch namespaces"?

Is there any propect that the core_pattern itself will later become a
per-namespace containerised thing?  I guess that if the per-container
core_pattern has been configured, we can implicitly do the namespace
switch as well.

Re: [PATCH] core_pattern: set core helpers root and namespace to crashing process

[Neil Horman]

On Thu, Dec 13, 2012 at 02:25:34PM -0800, Andrew Morton wrote:
> On Thu, 13 Dec 2012 13:12:20 -0500
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Thu, Dec 13, 2012 at 06:20:48AM -0600, Serge Hallyn wrote:
> > > Quoting Neil Horman (nhorman@tuxdriver.com):
> > > > Theres one problem I currently see with it, and that is that I'm not sure we can
> > > > change the current behavior of how the root fs is set for the pipe reader, lest
> > > > we break some user space expectations. As such, I've added a sysctl in this
> > > > patch to allow administrators to globally select if a core reader specified via
> > > > /proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
> > > > chrooted fs of the crashing process.
> > > 
> > > Practical question:  How is the admin to make an educated decision on
> > > how to set the sysctl?
> 
> By reading the documentation which Neil didn't include?
> 
Yeah, that was stupid of me, I'll respin this with docs.

> > My thought was that the admin typically wouldn't touch this at all.  I really
> > added it as a backwards compatibility option only.  Setting the user space
> > helper task to the root of the crashing parent has the possibility of breaking
> > existing installs because the core_pattern helper might be expecting global file
> > system access.  Moving forward, my expectation would be that core_pattern
> > helpers would be written with the default setting in mind, and we could
> > eventually deprecate the control entirely.
> > 
> > If you have a better mechanism in mind however (or if you think that removing
> > the control is a resaonable approach), I'm certainly open to that.
> 
> Yeah, this is a tiresome patch but I can't think of a better way.
> 
> Except, perhaps, adding a new token to the core_pattern which says
> "switch namespaces"?
>
I like that idea, perhaps '||' instead of '|' as the leading token can indicate
"use the namespace root" vs. "use the global root".  Thoughts?

> Is there any propect that the core_pattern itself will later become a
> per-namespace containerised thing?  I guess that if the per-container
> core_pattern has been configured, we can implicitly do the namespace
> switch as well.
Yes, that makes sense.  Unfortunately, I don't see proc containerization
happening any time soon.  I suppose if we do the above tokenization, that can be
used despite any future containerization that takes place.

I'll respin the patch with documentation, and replace the extra sysctl with the
above tokenization in the AM.

Best
Neil
 
> 
> 

Re: [PATCH] core_pattern: set core helpers root and namespace to crashing process

[Daniel P. Berrange]

On Thu, Dec 13, 2012 at 09:49:39PM -0500, Neil Horman wrote:
> On Thu, Dec 13, 2012 at 02:25:34PM -0800, Andrew Morton wrote:
> > On Thu, 13 Dec 2012 13:12:20 -0500
> > Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > > On Thu, Dec 13, 2012 at 06:20:48AM -0600, Serge Hallyn wrote:
> > > > Quoting Neil Horman (nhorman@tuxdriver.com):
> > > > > Theres one problem I currently see with it, and that is that I'm not sure we can
> > > > > change the current behavior of how the root fs is set for the pipe reader, lest
> > > > > we break some user space expectations. As such, I've added a sysctl in this
> > > > > patch to allow administrators to globally select if a core reader specified via
> > > > > /proc/sys/kernel/core_pattern should use the global rootfs, or the (possibly)
> > > > > chrooted fs of the crashing process.
> > > > 
> > > > Practical question:  How is the admin to make an educated decision on
> > > > how to set the sysctl?
> > 
> > By reading the documentation which Neil didn't include?
> > 
> Yeah, that was stupid of me, I'll respin this with docs.
> 
> > > My thought was that the admin typically wouldn't touch this at all.  I really
> > > added it as a backwards compatibility option only.  Setting the user space
> > > helper task to the root of the crashing parent has the possibility of breaking
> > > existing installs because the core_pattern helper might be expecting global file
> > > system access.  Moving forward, my expectation would be that core_pattern
> > > helpers would be written with the default setting in mind, and we could
> > > eventually deprecate the control entirely.
> > > 
> > > If you have a better mechanism in mind however (or if you think that removing
> > > the control is a resaonable approach), I'm certainly open to that.
> > 
> > Yeah, this is a tiresome patch but I can't think of a better way.
> > 
> > Except, perhaps, adding a new token to the core_pattern which says
> > "switch namespaces"?
> >
> I like that idea, perhaps '||' instead of '|' as the leading token can indicate
> "use the namespace root" vs. "use the global root".  Thoughts?

That seems like a nicer idea to me, than the extra sysctl knob.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

[PATCH v2] core_pattern: set core helpers root and namespace to crashing process

[Neil Horman]

As its currently implemented, redirection of core dumps to a pipe reader should
be executed such that the reader runs in the namespace of the crashing process,
and it currently does not. This is the only sane way to deal with namespaces
properly it seems to me, and this patch implements that functionality.

Theres one problem I currently see with it, and that is that I'm not sure we can
change the current behavior of how the root fs is set for the pipe reader, lest
we break some user space expectations.  As such I've made the switching of
namespaces configurable based on the addition of an extra '|' token in the
core_pattern sysctl.  1 '|' indicates a pipe using the global namespace, while
2 '|' indicates that the namespace and root of the crashing process should be
used.

I've tested this using both settings for the new sysctl, and it works well.

For the sake of history, this was discussed before:
http://www.spinics.net/lists/linux-containers/msg21531.html

It seems there was some modicum of consensus as to how this should work, but
there was no action taken on it.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Daniel Berrange <berrange@redhat.com>
CC: Daniel Berrange <berrange@redhat.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Serge Hallyn <serge.hallyn@canonical.com>
CC: containers@lists.linux-foundation.org
---
 Documentation/sysctl/kernel.txt |  7 ++++++-
 fs/coredump.c                   | 23 +++++++++++++++++++++++
 include/linux/binfmts.h         |  2 ++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 2907ba6..f853fca 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -191,7 +191,12 @@ core_pattern is used to specify a core dumpfile pattern name.
 	%<OTHER> both are dropped
 . If the first character of the pattern is a '|', the kernel will treat
   the rest of the pattern as a command to run.  The core dump will be
-  written to the standard input of that program instead of to a file.
+  written to the standard input of that program instead of to a file.  Note that
+  when using |, the core pipe reader that is executed will be run in the global
+  namespace and root filesystem.  If two | tokens (i.e. ||) are supplied as the
+  first two characters of the core_pattern sysctl, the kernel will preform the
+  same pipe operation, but the core pipe reader will be executed using the
+  namespace and root fs of the crashing process.
 
 ==============================================================
 
diff --git a/fs/coredump.c b/fs/coredump.c
index ce47379..d4b71c0 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -164,6 +164,18 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 	if (!cn->corename)
 		return -ENOMEM;
 
+	if (ispipe) {
+		/*
+ 		 * If we have 2 | tokens at the head of core_pattern, it
+ 		 * indicates we are a pipe and the reader should inherit the
+ 		 * namespaces of the crashing process
+ 		 */
+		cprm->switch_ns = (*(pat_ptr+1) == '|') ? true : false;
+		if (cprm->switch_ns)
+			/* Advance pat_ptr so as not to mess up corename */
+			pat_ptr++;
+	}
+
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
@@ -443,6 +455,7 @@ static void wait_for_dump_helpers(struct file *file)
 static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 {
 	struct file *files[2];
+	struct path root;
 	struct coredump_params *cp = (struct coredump_params *)info->data;
 	int err = create_pipe_files(files, 0);
 	if (err)
@@ -455,6 +468,14 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	/* and disallow core files too */
 	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
 
+
+	if (cp->switch_ns) {
+		get_fs_root(cp->cprocess->fs, &root);
+		set_fs_root(current->fs, &root);
+		switch_task_namespaces(current, cp->cprocess->nsproxy);
+	}
+
+
 	return err;
 }
 
@@ -476,6 +497,8 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
 		.siginfo = siginfo,
 		.regs = regs,
 		.limit = rlimit(RLIMIT_CORE),
+		.cprocess = current,
+		.switch_ns = false,
 		/*
 		 * We must use the same mm->flags while dumping core to avoid
 		 * inconsistency of bit flags, since this flag is not protected
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index cfcc6bf..3f06261 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -61,6 +61,8 @@ struct coredump_params {
 	siginfo_t *siginfo;
 	struct pt_regs *regs;
 	struct file *file;
+	struct task_struct *cprocess;
+	bool switch_ns;
 	unsigned long limit;
 	unsigned long mm_flags;
 };
-- 
1.7.11.7

Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process

[Andrew Morton]

On Fri, 14 Dec 2012 16:04:08 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

> As its currently implemented, redirection of core dumps to a pipe reader should
> be executed such that the reader runs in the namespace of the crashing process,
> and it currently does not. This is the only sane way to deal with namespaces
> properly it seems to me, and this patch implements that functionality.
> 
> Theres one problem I currently see with it, and that is that I'm not sure we can
> change the current behavior of how the root fs is set for the pipe reader, lest
> we break some user space expectations.  As such I've made the switching of
> namespaces configurable based on the addition of an extra '|' token in the
> core_pattern sysctl.  1 '|' indicates a pipe using the global namespace, while
> 2 '|' indicates that the namespace and root of the crashing process should be
> used.
> 
> I've tested this using both settings for the new sysctl, and it works well.
> 
> For the sake of history, this was discussed before:
> http://www.spinics.net/lists/linux-containers/msg21531.html
> 
> It seems there was some modicum of consensus as to how this should work, but
> there was no action taken on it.
> 
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -164,6 +164,18 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  	if (!cn->corename)
>  		return -ENOMEM;
>  
> +	if (ispipe) {
> +		/*
> + 		 * If we have 2 | tokens at the head of core_pattern, it
> + 		 * indicates we are a pipe and the reader should inherit the
> + 		 * namespaces of the crashing process
> + 		 */
> +		cprm->switch_ns = (*(pat_ptr+1) == '|') ? true : false;
> +		if (cprm->switch_ns)
> +			/* Advance pat_ptr so as not to mess up corename */
> +			pat_ptr++;
> +	}
> +

That was, umm, intricate.  How's about this?

	if (ispipe && pat_ptr[1] == '|') {
		/*
 		 * We have 2 | tokens at the head of core_pattern which
 		 * indicates we are a pipe and the reader should inherit the
 		 * namespaces of the crashing process
 		 */
		cprm->switch_ns = true;
		pat_ptr++;
	}


--- a/Documentation/sysctl/kernel.txt~core_pattern-set-core-helpers-root-and-namespace-to-crashing-process-fix
+++ a/Documentation/sysctl/kernel.txt
@@ -194,7 +194,7 @@ core_pattern is used to specify a core d
   written to the standard input of that program instead of to a file.  Note that
   when using |, the core pipe reader that is executed will be run in the global
   namespace and root filesystem.  If two | tokens (i.e. ||) are supplied as the
-  first two characters of the core_pattern sysctl, the kernel will preform the
+  first two characters of the core_pattern sysctl, the kernel will perform the
   same pipe operation, but the core pipe reader will be executed using the
   namespace and root fs of the crashing process.
 
--- a/fs/coredump.c~core_pattern-set-core-helpers-root-and-namespace-to-crashing-process-fix
+++ a/fs/coredump.c
@@ -164,16 +164,14 @@ static int format_corename(struct core_n
 	if (!cn->corename)
 		return -ENOMEM;
 
-	if (ispipe) {
+	if (ispipe && pat_ptr[1] == '|') {
 		/*
- 		 * If we have 2 | tokens at the head of core_pattern, it
+ 		 * We have 2 | tokens at the head of core_pattern which
  		 * indicates we are a pipe and the reader should inherit the
  		 * namespaces of the crashing process
  		 */
-		cprm->switch_ns = (*(pat_ptr+1) == '|') ? true : false;
-		if (cprm->switch_ns)
-			/* Advance pat_ptr so as not to mess up corename */
-			pat_ptr++;
+		cprm->switch_ns = true;
+		pat_ptr++;
 	}
 
 	/* Repeat as long as we have more pattern to process and more output
_

Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process

[Eric W. Biederman]

Neil Horman <nhorman@tuxdriver.com> writes:

> As its currently implemented, redirection of core dumps to a pipe reader should
> be executed such that the reader runs in the namespace of the crashing process,
> and it currently does not. This is the only sane way to deal with namespaces
> properly it seems to me, and this patch implements that functionality.

I actually rather strongly disagree.

While we have a global core dump pattern core dumps to a a pipe reader
should be executed such that the reader runs in the namespace of the
process that set the pattern.  We can easily restrict that to the
initial namespaces to make the implementation simpler.

If you want to play namespace games you can implement all of those in
user space once my tree merges for v3.8.

I am really not a fan of the trigger process being able to control the
environment of a privileged process.  It makes writing the privileged
process much trickier.

Eric

Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process

[Neil Horman]

On Fri, Dec 14, 2012 at 03:10:30PM -0800, Eric W. Biederman wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > As its currently implemented, redirection of core dumps to a pipe reader should
> > be executed such that the reader runs in the namespace of the crashing process,
> > and it currently does not. This is the only sane way to deal with namespaces
> > properly it seems to me, and this patch implements that functionality.
> 
> I actually rather strongly disagree.
> 
> While we have a global core dump pattern core dumps to a a pipe reader
> should be executed such that the reader runs in the namespace of the
> process that set the pattern.  We can easily restrict that to the
> initial namespaces to make the implementation simpler.
> 
> If you want to play namespace games you can implement all of those in
> user space once my tree merges for v3.8.
> 
> I am really not a fan of the trigger process being able to control the
> environment of a privileged process.  It makes writing the privileged
> process much trickier.
> 
Why?  What specific problem do you see with allowing a privlidged process to
execute within a specific namespace, that doesn't also exist with having the
pipe reader execute in the init namespace?  Note I'm not saying that a poorly
constructed pipe reader application doesn't have security problems if it doesn't
validate the environment that its running in, but thats something that the pipe
reader needs to be sure about.

Note also, that if the token in core_pattern is set such that the core_pattern
is namespace/root relative, that container needs to install the application
relative its root as well (e.g. positive action still needs to be taken on the
part of the container admin to make this work).  For example, if you set
core_pattern="||/usr/bin/foo"
Then a process running in a chroot based at /sub/root/ still needs to install a
file /sub/root/usr/bin/foo, or the core dump will fail.  So its not like a
container can just have a core reader execute without first making an
administrative decision to do so.

Neil