Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
On Fri, Dec 14, 2012 at 03:10:30PM -0800, Eric W. Biederman wrote: > Neil Horman 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
Neil Horman 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
On Fri, 14 Dec 2012 16:04:08 -0500
Neil Horman 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
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] core_pattern: set core helpers root and namespace to crashing process
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
Reported-by: Daniel Berrange
CC: Daniel Berrange
CC: Alexander Viro
CC: Andrew Morton
CC: Serge Hallyn
CC: contain...@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.
% 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, );
+ set_fs_root(current->fs, );
+ 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;
[PATCH v2] core_pattern: set core helpers root and namespace to crashing process
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 nhor...@tuxdriver.com
Reported-by: Daniel Berrange berra...@redhat.com
CC: Daniel Berrange berra...@redhat.com
CC: Alexander Viro v...@zeniv.linux.org.uk
CC: Andrew Morton a...@linux-foundation.org
CC: Serge Hallyn serge.hal...@canonical.com
CC: contain...@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
Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
On Fri, 14 Dec 2012 16:04:08 -0500
Neil Horman nhor...@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
_
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
Neil Horman nhor...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] core_pattern: set core helpers root and namespace to crashing process
On Fri, Dec 14, 2012 at 03:10:30PM -0800, Eric W. Biederman wrote: Neil Horman nhor...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
