[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191030025621.GA27626@mail.hallyn.com>
Date: Tue, 29 Oct 2019 21:56:21 -0500
From: "Serge E. Hallyn" <serge@...lyn.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
Andy Lutomirski <luto@...capital.net>,
Casey Schaufler <casey@...aufler-ca.com>,
Daniel Borkmann <daniel@...earbox.net>,
David Drysdale <drysdale@...gle.com>,
Florent Revest <revest@...omium.org>,
James Morris <jmorris@...ei.org>, Jann Horn <jann@...jh.net>,
John Johansen <john.johansen@...onical.com>,
Jonathan Corbet <corbet@....net>, Kees Cook <keescook@...omium.org>,
KP Singh <kpsingh@...omium.org>,
Michael Kerrisk <mtk.manpages@...il.com>,
Mickaël Salaün <mickael.salaun@....gouv.fr>,
Paul Moore <paul@...l-moore.com>, Sargun Dhillon <sargun@...gun.me>,
"Serge E . Hallyn" <serge@...lyn.com>,
Shuah Khan <shuah@...nel.org>, Stephen Smalley <sds@...ho.nsa.gov>,
Tejun Heo <tj@...nel.org>,
Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
Tycho Andersen <tycho@...ho.ws>, Will Drewry <wad@...omium.org>,
bpf@...r.kernel.org, kernel-hardening@...ts.openwall.com,
linux-api@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH bpf-next v11 2/7] landlock: Add the management of domains
On Tue, Oct 29, 2019 at 06:15:00PM +0100, Mickaël Salaün wrote:
> A Landlock domain is a set of eBPF programs. There is a list for each
> different program types that can be run on a specific Landlock hook
> (e.g. ptrace). A domain is tied to a set of subjects (i.e. tasks). A
> Landlock program should not try (nor be able) to infer which subject is
> currently enforced, but to have a unique security policy for all
> subjects tied to the same domain. This make the reasoning much easier
> and help avoid pitfalls.
>
> The next commits tie a domain to a task's credentials thanks to
> seccomp(2), but we could use cgroups or a security file-system to
> enforce a sysadmin-defined policy .
>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Andy Lutomirski <luto@...capital.net>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: James Morris <jmorris@...ei.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Serge E. Hallyn <serge@...lyn.com>
> Cc: Will Drewry <wad@...omium.org>
> ---
>
> Changes since v10:
> * rename files and names to clearly define a domain
> * create a standalone patch to ease review
> ---
[...]
> +/**
> + * store_landlock_prog - prepend and deduplicate a Landlock prog_list
> + *
> + * Prepend @prog to @init_domain while ignoring @prog if they are already in
> + * @ref_domain. Whatever is the result of this function call, you can call
> + * bpf_prog_put(@prog) after.
> + *
> + * @init_domain: empty domain to prepend to
> + * @ref_domain: domain to check for duplicate programs
> + * @prog: program to prepend
> + *
> + * Return -errno on error or 0 if @prog was successfully stored.
> + */
> +static int store_landlock_prog(struct landlock_domain *init_domain,
> + const struct landlock_domain *ref_domain,
> + struct bpf_prog *prog)
> +{
> + struct landlock_prog_list *tmp_list = NULL;
> + int err;
> + size_t hook;
> + enum landlock_hook_type last_type;
> + struct bpf_prog *new = prog;
> +
> + /* allocate all the memory we need */
> + struct landlock_prog_list *new_list;
> +
> + last_type = get_hook_type(new);
> +
> + /* ignore duplicate programs */
This comment should be "don't allow" rather than "ignore", right?
> + if (ref_domain) {
> + struct landlock_prog_list *ref;
> +
> + hook = get_hook_index(get_hook_type(new));
> + for (ref = ref_domain->programs[hook]; ref;
> + ref = ref->prev) {
> + if (ref->prog == new)
> + return -EINVAL;
> + }
> + }
> +
> + new = bpf_prog_inc(new);
> + if (IS_ERR(new)) {
> + err = PTR_ERR(new);
> + goto put_tmp_list;
> + }
> + new_list = kzalloc(sizeof(*new_list), GFP_KERNEL);
> + if (!new_list) {
> + bpf_prog_put(new);
> + err = -ENOMEM;
> + goto put_tmp_list;
> + }
> + /* ignore Landlock types in this tmp_list */
> + new_list->prog = new;
> + new_list->prev = tmp_list;
> + refcount_set(&new_list->usage, 1);
> + tmp_list = new_list;
> +
> + if (!tmp_list)
> + /* inform user space that this program was already added */
I'm not following this. You just kzalloc'd new_list, pointed
tmp_list to new_list, so how could tmp_list be NULL? Was there
a bad code reorg here, or am i being dense?
> + return -EEXIST;
> +
> + /* properly store the list (without error cases) */
> + while (tmp_list) {
> + struct landlock_prog_list *new_list;
> +
> + new_list = tmp_list;
> + tmp_list = tmp_list->prev;
> + /* do not increment the previous prog list usage */
> + hook = get_hook_index(get_hook_type(new_list->prog));
> + new_list->prev = init_domain->programs[hook];
> + /* no need to add from the last program to the first because
> + * each of them are a different Landlock type */
> + smp_store_release(&init_domain->programs[hook], new_list);
> + }
> + return 0;
> +
> +put_tmp_list:
> + put_landlock_prog_list(tmp_list);
> + return err;
> +}
> +
> +/* limit Landlock programs set to 256KB */
> +#define LANDLOCK_PROGRAMS_MAX_PAGES (1 << 6)
> +
> +/**
> + * landlock_prepend_prog - attach a Landlock prog_list to @current_domain
> + *
> + * Whatever is the result of this function call, you can call
> + * bpf_prog_put(@prog) after.
> + *
> + * @current_domain: landlock_domain pointer, must be (RCU-)locked (if needed)
> + * to prevent a concurrent put/free. This pointer must not be
> + * freed after the call.
> + * @prog: non-NULL Landlock prog_list to prepend to @current_domain. @prog will
> + * be owned by landlock_prepend_prog() and freed if an error happened.
> + *
> + * Return @current_domain or a new pointer when OK. Return a pointer error
> + * otherwise.
> + */
> +struct landlock_domain *landlock_prepend_prog(
> + struct landlock_domain *current_domain,
> + struct bpf_prog *prog)
> +{
> + struct landlock_domain *new_domain = current_domain;
> + unsigned long pages;
> + int err;
> + size_t i;
> + struct landlock_domain tmp_domain = {};
> +
> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
> + return ERR_PTR(-EINVAL);
> +
> + /* validate memory size allocation */
> + pages = prog->pages;
> + if (current_domain) {
> + size_t i;
> +
> + for (i = 0; i < ARRAY_SIZE(current_domain->programs); i++) {
> + struct landlock_prog_list *walker_p;
> +
> + for (walker_p = current_domain->programs[i];
> + walker_p; walker_p = walker_p->prev)
> + pages += walker_p->prog->pages;
> + }
> + /* count a struct landlock_domain if we need to allocate one */
> + if (refcount_read(¤t_domain->usage) != 1)
> + pages += round_up(sizeof(*current_domain), 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_domain, current_domain, 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_domain and avoid a rollback.
> + */
> + if (!new_domain) {
> + /*
> + * If there is no Landlock domain used by the current task,
> + * then create a new one.
> + */
> + new_domain = new_landlock_domain();
> + if (IS_ERR(new_domain))
> + goto put_tmp_lists;
> + } else if (refcount_read(¤t_domain->usage) > 1) {
> + /*
> + * If the current task is not the sole user of its Landlock
> + * domain, then duplicate it.
> + */
> + new_domain = new_landlock_domain();
> + if (IS_ERR(new_domain))
> + goto put_tmp_lists;
> + for (i = 0; i < ARRAY_SIZE(new_domain->programs); i++) {
> + new_domain->programs[i] =
> + READ_ONCE(current_domain->programs[i]);
> + if (new_domain->programs[i])
> + refcount_inc(&new_domain->programs[i]->usage);
> + }
> +
> + /*
> + * Landlock domain 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_domain(current_domain);
> + }
> +
> + /* prepend tmp_domain to new_domain */
> + for (i = 0; i < ARRAY_SIZE(tmp_domain.programs); i++) {
> + /* get the last new list */
> + struct landlock_prog_list *last_list =
> + tmp_domain.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_domain->programs[i];
> + new_domain->programs[i] = tmp_domain.programs[i];
> + }
> + }
> + return new_domain;
> +
> +put_tmp_lists:
> + for (i = 0; i < ARRAY_SIZE(tmp_domain.programs); i++)
> + put_landlock_prog_list(tmp_domain.programs[i]);
> + return new_domain;
> +}
> diff --git a/security/landlock/domain_manage.h b/security/landlock/domain_manage.h
> new file mode 100644
> index 000000000000..5b5b49f6e3e8
> --- /dev/null
> +++ b/security/landlock/domain_manage.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Landlock LSM - domain management headers
> + *
> + * Copyright © 2016-2019 Mickaël Salaün <mic@...ikod.net>
> + * Copyright © 2018-2019 ANSSI
> + */
> +
> +#ifndef _SECURITY_LANDLOCK_DOMAIN_MANAGE_H
> +#define _SECURITY_LANDLOCK_DOMAIN_MANAGE_H
> +
> +#include <linux/filter.h>
> +
> +#include "common.h"
> +
> +void landlock_get_domain(struct landlock_domain *dom);
> +void landlock_put_domain(struct landlock_domain *dom);
> +
> +struct landlock_domain *landlock_prepend_prog(
> + struct landlock_domain *current_domain,
> + struct bpf_prog *prog);
> +
> +#endif /* _SECURITY_LANDLOCK_DOMAIN_MANAGE_H */
> --
> 2.23.0
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
