Message-ID: <012701d1eeee$4ec38a80$ec4a9f80$@cn.fujitsu.com>
Date:	Fri, 5 Aug 2016 15:52:25 +0800
From:	Zhao Lei <zhaolei@...fujitsu.com>
To:	'Andrei Vagin' <avagin@...il.com>
CC:	'LKML' <linux-kernel@...r.kernel.org>,
	'Linux Containers' <containers@...ts.linux-foundation.org>,
	"'Eric W. Biederman'" <ebiederm@...ssion.com>
Subject: RE: [PATCH v2 1/2] Limit dump_pipe program's permission to init for container

Hi, Andrei Vagin

Thanks for your detailed review and suggestion.

> -----Original Message-----
> From: Andrei Vagin [mailto:avagin@...il.com]
> Sent: Friday, August 05, 2016 2:32 PM
> To: Zhao Lei <zhaolei@...fujitsu.com>
> Cc: LKML <linux-kernel@...r.kernel.org>; Linux Containers
> <containers@...ts.linux-foundation.org>; Eric W. Biederman
> <ebiederm@...ssion.com>
> Subject: Re: [PATCH v2 1/2] Limit dump_pipe program's permission to init for
> container
> 
> On Tue, Aug 2, 2016 at 2:08 AM, Zhao Lei <zhaolei@...fujitsu.com> wrote:
> > Currently when we set core_pattern to a pipe, the pipe program is
> > forked by kthread running with root's permission, and write dumpfile
> > into host's filesystem.
> > Same thing happened for container, the dumper and dumpfile are also
> > in host(not in container).
> >
> > It have following program:
> > 1: Not consistent with file_type core_pattern
> >    When we set core_pattern to a file, the container will write dump
> >    into container's filesystem instead of host.
> > 2: Not safe for privileged container
> >    In a privileged container, user can destroy host system by following
> >    command:
> >    # # In a container
> >    # echo "|/bin/dd of=/boot/vmlinuz" >/proc/sys/kernel/core_pattern
> >    # make_dump
> >
> > This patch switch dumper program's environment to init task, so, for
> > container, dumper program have same environment with init task in
> > container, which make dumper program put in container's filesystem, and
> > write coredump into container's filesystem.
> > The dumper's permission is also limited into subset of container's init
> > process.
> 
> Does it change the current behavior? A pid namespace may be used for
> sandboxes. For example, chrome uses it. In this case, we probably want
> to collect core dumps from a root pid namespace.
> 
> If we are going to virtualize core_pattern relative to the pid
> namespace, maybe it's better to make it optional for a pid namespace.
> I mean it core_pattern is not set for the current pid namespace, we
> can check a parent pid namespace and so on. A helper will be executed
> in a pid namespace, where core_pattern is set.
> 
Is it means, we don't want to always gather dump from the container ifself,
wnd we want to custom which level of the container can gather dump
information?

If it is exact, current patch have similar function as above.

If user had not set core_pattern in a namespace, the dump setting in
the parent namespace will be used.
For example,
HOST        change core_pattern
Container1  no change
Container2  change core_pattern
Container3  no change
Container4  no change

If code dump happened in container 2, 3 and 4, the dumper's environment
is same as container2.
And if dump happened in container 1 and HOST, the dumper's environment
will be same as HOST.

So,
1: if a container want to make the custom core dump,
  it can set core_pattern in container, and the gathered information
  will be limited in this container.
2: If the host(or some top level container) want to gather dump information
  more than a container internal, the host(or some top level container)
  can set core_pattern, and not change core_pattern in the container,
  then the pipe program will be run in same environment as the host,
  and gather information from host's env.

> After reading these patches, I think it may be a good idea to add one
> more mode to handle core files, when a core file is sent to an
> existing process instead of executing a usermode helpers. This mode
> will cover of use-cases where the pipe mode is used now. And it looks
> more secure, because in this case we control namespaces and credential
> for the abort daemon (a process which handles core files).u
> 
> How it can be done. An abort daemon creates an abstract listening unix
> socket and writes its name into /proc/sys/kernel/core_pattern. The
> kernel saves this name and the network namespace. Then when any
> process is crashed, the kernel creates a new unix socket and connect
> it to the socket of the abort daemon and streams a core file into this
> socket.
> 
Good idea.

And do it means if we want to custom core_pattern for each container,
we can create more than one abort daemon for it?
If the abort daemon is running in the host, the dump file will be write
into host's env, and if abort daemon is running in container, the dump
file will be write into container's fs.

> >
> > Suggested-by: Eric W. Biederman <ebieyderm@...ssion.com>
> > Suggested-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> >
> > Signed-off-by: Zhao Lei <zhaolei@...fujitsu.com>
> > ---
> >  fs/coredump.c           | 87
> ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/binfmts.h |  1 +
> >  2 files changed, 87 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 281b768..8511267 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -516,6 +516,8 @@ static int umh_pipe_setup(struct subprocess_info
> *info, struct cred *new)
> >  {
> >         struct file *files[2];
> >         struct coredump_params *cp = (struct coredump_params
> *)info->data;
> > +       struct task_struct *base_task;
> > +
> >         int err = create_pipe_files(files, 0);
> >         if (err)
> >                 return err;
> > @@ -524,10 +526,79 @@ static int umh_pipe_setup(struct subprocess_info
> *info, struct cred *new)
> >
> >         err = replace_fd(0, files[0], 0);
> >         fput(files[0]);
> > +       if (err)
> > +               return err;
> > +
> >         /* and disallow core files too */
> >         current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
> >
> > -       return err;
> > +       base_task = cp->base_task;
> > +       if (base_task) {
> > +               const struct cred *base_cred;
> > +
> > +               /* Set fs_root to base_task */
> > +               spin_lock(&base_task->fs->lock);
> > +               set_fs_root(current->fs, &base_task->fs->root);
> > +               spin_unlock(&base_task->fs->lock);
> > +
> > +               /* Set namespaces to base_task */
> > +               get_nsproxy(base_task->nsproxy);
> > +               switch_task_namespaces(current, base_task->nsproxy);
> 
> This task will continue running in the current pid namespace, because
> switch_task_namespaces doesn't change a pid for the task. Ussualy, we
> need to make setns+fork to switch a pid namespace.
> 
Yes.
Now I known why I had not find this problem in my test,
I output pids by a shell command, and the shell command is already got forked.

Seems need double fork(make code complex...) to do it.

> > +
> > +               /* Set cgroup to base_task */
> > +               current->flags &= ~PF_NO_SETAFFINITY;
> > +               err = cgroup_attach_task_all(base_task, current);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               /* Set cred to base_task */
> > +               base_cred = get_task_cred(base_task);
> > +
> 
> I think you can use prepare_kernel_cred here
> 
prepare_kernel_cred() is called by caller of this function(call_usermodehelper_exec_async()),
and this function is a hook, which can set additional cred property like:
call_usermodehelper_exec_async()
{
    prepare_kernel_cred()
    this_function()
    {
        custom_cred struct
    } 
    commit_creds(new);
}

So we can only change cred's container here.

But seems we need rewrite this code when we move to above "double fork".

Before rewrite, could you give me some suggestion, if current code
already can custom to dump information of "container internal" and "parent level container",
is this meet your request?

Thanks
Zhaolei

> > +               new->uid   = base_cred->uid;
> > +               new->gid   = base_cred->gid;
> > +               new->suid  = base_cred->suid;
> > +               new->sgid  = base_cred->sgid;
> > +               new->euid  = base_cred->euid;
> > +               new->egid  = base_cred->egid;
> > +               new->fsuid = base_cred->fsuid;
> > +               new->fsgid = base_cred->fsgid;
> > +
> > +               new->securebits = base_cred->securebits;
> > +
> > +               new->cap_inheritable = base_cred->cap_inheritable;
> > +               new->cap_permitted   = base_cred->cap_permitted;
> > +               new->cap_effective   = base_cred->cap_effective;
> > +               new->cap_bset        = base_cred->cap_bset;
> > +               new->cap_ambient     = base_cred->cap_ambient;
> > +
> > +               security_cred_free(new);
> > +#ifdef CONFIG_SECURITY
> > +               new->security = NULL;
> > +#endif
> > +               err = security_prepare_creds(new, base_cred,
> GFP_KERNEL);
> > +               if (err < 0) {
> > +                       put_cred(base_cred);
> > +                       return err;
> > +               }
> > +
> > +               free_uid(new->user);
> > +               new->user = base_cred->user;
> > +               get_uid(new->user);
> > +
> > +               put_user_ns(new->user_ns);
> > +               new->user_ns = base_cred->user_ns;
> > +               get_user_ns(new->user_ns);
> > +
> > +               put_group_info(new->group_info);
> > +               new->group_info = base_cred->group_info;
> > +               get_group_info(new->group_info);
> > +
> > +               put_cred(base_cred);
> > +
> > +               validate_creds(new);
> > +       }
> > +
> > +       return 0;
> >  }
> >
> >  void do_coredump(const siginfo_t *siginfo)
> > @@ -590,6 +661,7 @@ void do_coredump(const siginfo_t *siginfo)
> >
> >         if (ispipe) {
> >                 int dump_count;
> > +               struct task_struct *vinit_task;
> >                 char **helper_argv;
> >                 struct subprocess_info *sub_info;
> >
> > @@ -631,6 +703,14 @@ void do_coredump(const siginfo_t *siginfo)
> >                         goto fail_dropcount;
> >                 }
> >
> > +               rcu_read_lock();
> > +               vinit_task = find_task_by_vpid(1);
> > +               rcu_read_unlock();
> > +               if (!vinit_task) {
> > +                       printk(KERN_WARNING "failed getting init task
> info, skipping core dump\n");
> > +                       goto fail_dropcount;
> > +               }
> > +
> >                 helper_argv = argv_split(GFP_KERNEL, cn.corename,
> NULL);
> >                 if (!helper_argv) {
> >                         printk(KERN_WARNING "%s failed to allocate
> memory\n",
> > @@ -638,6 +718,10 @@ void do_coredump(const siginfo_t *siginfo)
> >                         goto fail_dropcount;
> >                 }
> >
> > +               get_task_struct(vinit_task);
> > +
> > +               cprm.base_task = vinit_task;
> > +
> >                 retval = -ENOMEM;
> >                 sub_info = call_usermodehelper_setup(helper_argv[0],
> >                                                 helper_argv,
> NULL, GFP_KERNEL,
> > @@ -646,6 +730,7 @@ void do_coredump(const siginfo_t *siginfo)
> >                         retval = call_usermodehelper_exec(sub_info,
> >
> UMH_WAIT_EXEC);
> >
> > +               put_task_struct(vinit_task);
> >                 argv_free(helper_argv);
> >                 if (retval) {
> >                         printk(KERN_INFO "Core dump to |%s pipe
> failed\n",
> > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > index 314b3ca..0c9a72c 100644
> > --- a/include/linux/binfmts.h
> > +++ b/include/linux/binfmts.h
> > @@ -59,6 +59,7 @@ struct linux_binprm {
> >
> >  /* Function parameter for binfmt->coredump */
> >  struct coredump_params {
> > +       struct task_struct *base_task;
> >         const siginfo_t *siginfo;
> >         struct pt_regs *regs;
> >         struct file *file;
> > --
> > 1.8.5.1
> >
> >
> >
> > _______________________________________________
> > Containers mailing list
> > Containers@...ts.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/containers
> 

Powered by blists - more mailing lists