[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e6c59db1-0fb5-704d-6693-335fa199101c@digikod.net>
Date: Wed, 30 Oct 2019 15:03:10 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: "Serge E. Hallyn" <serge@...lyn.com>
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>,
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 30/10/2019 03:56, Serge E. Hallyn wrote:
> 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?
Exactly, fixed.
>
>> + 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?
Indeed, this was introduce with a code refactoring (while removing a
program "chaining" concept) where this code snippet was in a loop, hence
the weird use of tmp_list. I'm cleaning this up (and simplifying this
whole code), and replacing the -EINVAL for the duplicate program check
with the -EEXIST.
Thanks!
>
>> + 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.
