From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DE395CA9EAE for ; Wed, 30 Oct 2019 02:56:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A85A620856 for ; Wed, 30 Oct 2019 02:56:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727208AbfJ3C40 (ORCPT ); Tue, 29 Oct 2019 22:56:26 -0400 Received: from mail.hallyn.com ([178.63.66.53]:45816 "EHLO mail.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726990AbfJ3C40 (ORCPT ); Tue, 29 Oct 2019 22:56:26 -0400 Received: by mail.hallyn.com (Postfix, from userid 1001) id A8A80AC2; Tue, 29 Oct 2019 21:56:21 -0500 (CDT) Date: Tue, 29 Oct 2019 21:56:21 -0500 From: "Serge E. Hallyn" To: =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= Cc: linux-kernel@vger.kernel.org, Alexei Starovoitov , Andy Lutomirski , Casey Schaufler , Daniel Borkmann , David Drysdale , Florent Revest , James Morris , Jann Horn , John Johansen , Jonathan Corbet , Kees Cook , KP Singh , Michael Kerrisk , =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Stephen Smalley , Tejun Heo , Tetsuo Handa , Tycho Andersen , Will Drewry , bpf@vger.kernel.org, kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org, linux-security-module@vger.kernel.org Subject: Re: [PATCH bpf-next v11 2/7] landlock: Add the management of domains Message-ID: <20191030025621.GA27626@mail.hallyn.com> References: <20191029171505.6650-1-mic@digikod.net> <20191029171505.6650-3-mic@digikod.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20191029171505.6650-3-mic@digikod.net> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Alexei Starovoitov > Cc: Andy Lutomirski > Cc: Daniel Borkmann > Cc: James Morris > Cc: Kees Cook > Cc: Serge E. Hallyn > Cc: Will Drewry > --- > > 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 > + * Copyright © 2018-2019 ANSSI > + */ > + > +#ifndef _SECURITY_LANDLOCK_DOMAIN_MANAGE_H > +#define _SECURITY_LANDLOCK_DOMAIN_MANAGE_H > + > +#include > + > +#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