linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: "Mickaël Salaün" <mic@digikod.net>,
	LKML <linux-kernel@vger.kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"David Drysdale" <drysdale@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	"James Morris" <james.l.morris@oracle.com>,
	"Jann Horn" <jann@thejh.net>, "Jonathan Corbet" <corbet@lwn.net>,
	"Michael Kerrisk" <mtk.manpages@gmail.com>,
	"Kees Cook" <keescook@chromium.org>,
	"Paul Moore" <paul@paul-moore.com>,
	"Sargun Dhillon" <sargun@sargun.me>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	"Shuah Khan" <shuah@kernel.org>, "Tejun Heo" <tj@kernel.org>,
	"Thomas Graf" <tgraf@suug.ch>, "Tycho Andersen" <tycho@tycho.ws>,
	"Will Drewry" <wad@chromium.org>,
	"Kernel Hardening" <kernel-hardening@lists.openwall.com>,
	"Linux API" <linux-api@vger.kernel.org>,
	"LSM List" <linux-security-module@vger.kernel.org>,
	"Network Development" <netdev@vger.kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>
Subject: Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
Date: Mon, 26 Feb 2018 20:54:59 -0800	[thread overview]
Message-ID: <20180227045458.wjrbbsxf3po656du@ast-mbp> (raw)
In-Reply-To: <CALCETrVKRYnGJ9XNW-x7eHdMt+eGP90j7cAed-KTzp1KT_kMeQ@mail.gmail.com>

On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
> >> The seccomp(2) syscall can be used by a task to apply a Landlock program
> >> to itself. As a seccomp filter, a Landlock program is enforced for the
> >> current task and all its future children. A program is immutable and a
> >> task can only add new restricting programs to itself, forming a list of
> >> programss.
> >>
> >> A Landlock program is tied to a Landlock hook. If the action on a kernel
> >> object is allowed by the other Linux security mechanisms (e.g. DAC,
> >> capabilities, other LSM), then a Landlock hook related to this kind of
> >> object is triggered. The list of programs for this hook is then
> >> evaluated. Each program return a 32-bit value which can deny the action
> >> on a kernel object with a non-zero value. If every programs of the list
> >> return zero, then the action on the object is allowed.
> >>
> >> Multiple Landlock programs can be chained to share a 64-bits value for a
> >> call chain (e.g. evaluating multiple elements of a file path).  This
> >> chaining is restricted when a process construct this chain by loading a
> >> program, but additional checks are performed when it requests to apply
> >> this chain of programs to itself.  The restrictions ensure that it is
> >> not possible to call multiple programs in a way that would imply to
> >> handle multiple shared values (i.e. cookies) for one chain.  For now,
> >> only a fs_pick program can be chained to the same type of program,
> >> because it may make sense if they have different triggers (cf. next
> >> commits).  This restrictions still allows to reuse Landlock programs in
> >> a safe way (e.g. use the same loaded fs_walk program with multiple
> >> chains of fs_pick programs).
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> >
> > ...
> >
> >> +struct landlock_prog_set *landlock_prepend_prog(
> >> +             struct landlock_prog_set *current_prog_set,
> >> +             struct bpf_prog *prog)
> >> +{
> >> +     struct landlock_prog_set *new_prog_set = current_prog_set;
> >> +     unsigned long pages;
> >> +     int err;
> >> +     size_t i;
> >> +     struct landlock_prog_set tmp_prog_set = {};
> >> +
> >> +     if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
> >> +             return ERR_PTR(-EINVAL);
> >> +
> >> +     /* validate memory size allocation */
> >> +     pages = prog->pages;
> >> +     if (current_prog_set) {
> >> +             size_t i;
> >> +
> >> +             for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
> >> +                     struct landlock_prog_list *walker_p;
> >> +
> >> +                     for (walker_p = current_prog_set->programs[i];
> >> +                                     walker_p; walker_p = walker_p->prev)
> >> +                             pages += walker_p->prog->pages;
> >> +             }
> >> +             /* count a struct landlock_prog_set if we need to allocate one */
> >> +             if (refcount_read(&current_prog_set->usage) != 1)
> >> +                     pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
> >> +                             / PAGE_SIZE;
> >> +     }
> >> +     if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
> >> +             return ERR_PTR(-E2BIG);
> >> +
> >> +     /* ensure early that we can allocate enough memory for the new
> >> +      * prog_lists */
> >> +     err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
> >> +     if (err)
> >> +             return ERR_PTR(err);
> >> +
> >> +     /*
> >> +      * Each task_struct points to an array of prog list pointers.  These
> >> +      * tables are duplicated when additions are made (which means each
> >> +      * table needs to be refcounted for the processes using it). When a new
> >> +      * table is created, all the refcounters on the prog_list are bumped (to
> >> +      * track each table that references the prog). When a new prog is
> >> +      * added, it's just prepended to the list for the new table to point
> >> +      * at.
> >> +      *
> >> +      * Manage all the possible errors before this step to not uselessly
> >> +      * duplicate current_prog_set and avoid a rollback.
> >> +      */
> >> +     if (!new_prog_set) {
> >> +             /*
> >> +              * If there is no Landlock program set used by the current task,
> >> +              * then create a new one.
> >> +              */
> >> +             new_prog_set = new_landlock_prog_set();
> >> +             if (IS_ERR(new_prog_set))
> >> +                     goto put_tmp_lists;
> >> +     } else if (refcount_read(&current_prog_set->usage) > 1) {
> >> +             /*
> >> +              * If the current task is not the sole user of its Landlock
> >> +              * program set, then duplicate them.
> >> +              */
> >> +             new_prog_set = new_landlock_prog_set();
> >> +             if (IS_ERR(new_prog_set))
> >> +                     goto put_tmp_lists;
> >> +             for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
> >> +                     new_prog_set->programs[i] =
> >> +                             READ_ONCE(current_prog_set->programs[i]);
> >> +                     if (new_prog_set->programs[i])
> >> +                             refcount_inc(&new_prog_set->programs[i]->usage);
> >> +             }
> >> +
> >> +             /*
> >> +              * Landlock program set from the current task will not be freed
> >> +              * here because the usage is strictly greater than 1. It is
> >> +              * only prevented to be freed by another task thanks to the
> >> +              * caller of landlock_prepend_prog() which should be locked if
> >> +              * needed.
> >> +              */
> >> +             landlock_put_prog_set(current_prog_set);
> >> +     }
> >> +
> >> +     /* prepend tmp_prog_set to new_prog_set */
> >> +     for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
> >> +             /* get the last new list */
> >> +             struct landlock_prog_list *last_list =
> >> +                     tmp_prog_set.programs[i];
> >> +
> >> +             if (last_list) {
> >> +                     while (last_list->prev)
> >> +                             last_list = last_list->prev;
> >> +                     /* no need to increment usage (pointer replacement) */
> >> +                     last_list->prev = new_prog_set->programs[i];
> >> +                     new_prog_set->programs[i] = tmp_prog_set.programs[i];
> >> +             }
> >> +     }
> >> +     new_prog_set->chain_last = tmp_prog_set.chain_last;
> >> +     return new_prog_set;
> >> +
> >> +put_tmp_lists:
> >> +     for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
> >> +             put_landlock_prog_list(tmp_prog_set.programs[i]);
> >> +     return new_prog_set;
> >> +}
> >
> > Nack on the chaining concept.
> > Please do not reinvent the wheel.
> > There is an existing mechanism for attaching/detaching/quering multiple
> > programs attached to cgroup and tracing hooks that are also
> > efficiently executed via BPF_PROG_RUN_ARRAY.
> > Please use that instead.
> >
> 
> I don't see how that would help.  Suppose you add a filter, then
> fork(), and then the child adds another filter.  Do you want to
> duplicate the entire array?  You certainly can't *modify* the array
> because you'll affect processes that shouldn't be affected.
> 
> In contrast, doing this through seccomp like the earlier patches
> seemed just fine to me, and seccomp already had the right logic.

it doesn't look to me that existing seccomp side of managing fork
situation can be reused. Here there is an attempt to add 'chaining'
concept which sort of an extension of existing seccomp style,
but somehow heavily done on bpf side and contradicts cgroup/tracing.


  reply	other threads:[~2018-02-27  4:55 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27  0:41 [PATCH bpf-next v8 00/11] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 01/11] fs,security: Add a security blob to nameidata Mickaël Salaün
2018-02-27  0:57   ` Al Viro
2018-02-27  1:23     ` Al Viro
2018-03-11 20:14       ` Mickaël Salaün
2018-02-28 16:27   ` kbuild test robot
2018-02-28 16:58   ` kbuild test robot
2018-02-27  0:41 ` [PATCH bpf-next v8 02/11] fs,security: Add a new file access type: MAY_CHROOT Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 03/11] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 04/11] bpf,landlock: Define an eBPF program type for Landlock hooks Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy Mickaël Salaün
2018-02-27  2:08   ` Alexei Starovoitov
2018-02-27  4:40     ` Andy Lutomirski
2018-02-27  4:54       ` Alexei Starovoitov [this message]
2018-02-27  5:20         ` Andy Lutomirski
2018-02-27  5:32           ` Alexei Starovoitov
2018-02-27 16:39             ` Andy Lutomirski
2018-02-27 17:30               ` Casey Schaufler
2018-02-27 17:36                 ` Andy Lutomirski
2018-02-27 18:03                   ` Casey Schaufler
2018-02-27 21:48               ` Mickaël Salaün
2018-04-08 13:13                 ` Mickaël Salaün
2018-04-08 21:06                   ` Andy Lutomirski
2018-04-08 22:01                     ` Mickaël Salaün
2018-04-10  4:48                       ` Alexei Starovoitov
2018-04-11 22:18                         ` Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 06/11] bpf,landlock: Add a new map type: inode Mickaël Salaün
2018-02-28 17:35   ` kbuild test robot
2018-02-27  0:41 ` [PATCH bpf-next v8 07/11] landlock: Handle filesystem access control Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 08/11] landlock: Add ptrace restrictions Mickaël Salaün
2018-02-27  4:17   ` Andy Lutomirski
2018-02-27  5:01     ` Andy Lutomirski
2018-02-27 22:14       ` Mickaël Salaün
2018-02-27 23:02         ` Andy Lutomirski
2018-02-27 23:23           ` Andy Lutomirski
2018-02-28  0:00             ` Mickaël Salaün
2018-02-28  0:09               ` Andy Lutomirski
2018-03-06 22:28                 ` Mickaël Salaün
2018-04-01 22:48                   ` Mickaël Salaün
2018-02-27 22:18     ` Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 09/11] bpf: Add a Landlock sandbox example Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 10/11] bpf,landlock: Add tests for Landlock Mickaël Salaün
2018-02-27  0:41 ` [PATCH bpf-next v8 11/11] landlock: Add user and kernel documentation " Mickaël Salaün
2018-02-27  4:36 ` [PATCH bpf-next v8 00/11] Landlock LSM: Toward unprivileged sandboxing Andy Lutomirski
2018-02-27 22:03   ` Mickaël Salaün
2018-02-27 23:09     ` Andy Lutomirski
2018-03-06 22:25       ` Mickaël Salaün
2018-03-06 22:33         ` Andy Lutomirski
2018-03-06 22:46           ` Tycho Andersen
2018-03-06 23:06             ` Mickaël Salaün
2018-03-07  1:21               ` Andy Lutomirski
2018-03-08 23:51                 ` Mickaël Salaün
2018-03-08 23:53                   ` Andy Lutomirski
2018-04-01 22:04                     ` Mickaël Salaün
2018-04-02  0:39                       ` Tycho Andersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180227045458.wjrbbsxf3po656du@ast-mbp \
    --to=alexei.starovoitov@gmail.com \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ast@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=drysdale@google.com \
    --cc=ebiederm@xmission.com \
    --cc=james.l.morris@oracle.com \
    --cc=jann@thejh.net \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mic@digikod.net \
    --cc=mtk.manpages@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=sargun@sargun.me \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=tgraf@suug.ch \
    --cc=tj@kernel.org \
    --cc=tycho@tycho.ws \
    --cc=wad@chromium.org \
    --subject='Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).