lkml.org 
[lkml]   [2020]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v14 01/10] landlock: Add object and rule management
    From
    Date


    On 27/02/2020 05:20, Hillf Danton wrote:
    >
    > On Mon, 24 Feb 2020 17:02:06 +0100 Mickaël Salaün
    >> A Landlock object enables to identify a kernel object (e.g. an inode).
    >> A Landlock rule is a set of access rights allowed on an object. Rules
    >> are grouped in rulesets that may be tied to a set of processes (i.e.
    >> subjects) to enforce a scoped access-control (i.e. a domain).
    >>
    >> Because Landlock's goal is to empower any process (especially
    >> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
    >> object identification such as file extended attributes. Indeed, we need
    >> innocuous, composable and modular access-controls.
    >>
    >> The main challenge with this constraints is to identify kernel objects
    >> while this identification is useful (i.e. when a security policy makes
    >> use of this object). But this identification data should be freed once
    >> no policy is using it. This ephemeral tagging should not and may not be
    >> written in the filesystem. We then need to manage the lifetime of a
    >> rule according to the lifetime of its object. To avoid a global lock,
    >> this implementation make use of RCU and counters to safely reference
    >> objects.
    >>
    >> A following commit uses this generic object management for inodes.
    >>
    >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
    >> Cc: Andy Lutomirski <luto@amacapital.net>
    >> Cc: James Morris <jmorris@namei.org>
    >> Cc: Kees Cook <keescook@chromium.org>
    >> Cc: Serge E. Hallyn <serge@hallyn.com>
    >> ---
    >>
    >> Changes since v13:
    >> * New dedicated implementation, removing the need for eBPF.
    >>
    >> Previous version:
    >> https://lore.kernel.org/lkml/20190721213116.23476-6-mic@digikod.net/
    >> ---
    >> MAINTAINERS | 10 ++
    >> security/Kconfig | 1 +
    >> security/Makefile | 2 +
    >> security/landlock/Kconfig | 15 ++
    >> security/landlock/Makefile | 3 +
    >> security/landlock/object.c | 339 +++++++++++++++++++++++++++++++++++++
    >> security/landlock/object.h | 134 +++++++++++++++
    >> 7 files changed, 504 insertions(+)
    >> create mode 100644 security/landlock/Kconfig
    >> create mode 100644 security/landlock/Makefile
    >> create mode 100644 security/landlock/object.c
    >> create mode 100644 security/landlock/object.h
    >>
    >> diff --git a/MAINTAINERS b/MAINTAINERS
    >> index fcd79fc38928..206f85768cd9 100644
    >> --- a/MAINTAINERS
    >> +++ b/MAINTAINERS
    >> @@ -9360,6 +9360,16 @@ F: net/core/skmsg.c
    >> F: net/core/sock_map.c
    >> F: net/ipv4/tcp_bpf.c
    >>
    >> +LANDLOCK SECURITY MODULE
    >> +M: Mickaël Salaün <mic@digikod.net>
    >> +L: linux-security-module@vger.kernel.org
    >> +W: https://landlock.io
    >> +T: git https://github.com/landlock-lsm/linux.git
    >> +S: Supported
    >> +F: security/landlock/
    >> +K: landlock
    >> +K: LANDLOCK
    >> +
    >> LANTIQ / INTEL Ethernet drivers
    >> M: Hauke Mehrtens <hauke@hauke-m.de>
    >> L: netdev@vger.kernel.org
    >> diff --git a/security/Kconfig b/security/Kconfig
    >> index 2a1a2d396228..9d9981394fb0 100644
    >> --- a/security/Kconfig
    >> +++ b/security/Kconfig
    >> @@ -238,6 +238,7 @@ source "security/loadpin/Kconfig"
    >> source "security/yama/Kconfig"
    >> source "security/safesetid/Kconfig"
    >> source "security/lockdown/Kconfig"
    >> +source "security/landlock/Kconfig"
    >>
    >> source "security/integrity/Kconfig"
    >>
    >> diff --git a/security/Makefile b/security/Makefile
    >> index 746438499029..2472ef96d40a 100644
    >> --- a/security/Makefile
    >> +++ b/security/Makefile
    >> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA) += yama
    >> subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
    >> subdir-$(CONFIG_SECURITY_SAFESETID) += safesetid
    >> subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown
    >> +subdir-$(CONFIG_SECURITY_LANDLOCK) += landlock
    >>
    >> # always enable default capabilities
    >> obj-y += commoncap.o
    >> @@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
    >> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
    >> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
    >> obj-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown/
    >> +obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/
    >> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
    >>
    >> # Object integrity file lists
    >> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
    >> new file mode 100644
    >> index 000000000000..4a321d5b3f67
    >> --- /dev/null
    >> +++ b/security/landlock/Kconfig
    >> @@ -0,0 +1,15 @@
    >> +# SPDX-License-Identifier: GPL-2.0-only
    >> +
    >> +config SECURITY_LANDLOCK
    >> + bool "Landlock support"
    >> + depends on SECURITY
    >> + default n
    >> + help
    >> + This selects Landlock, a safe sandboxing mechanism. It enables to
    >> + restrict processes on the fly (i.e. enforce an access control policy),
    >> + which can complement seccomp-bpf. The security policy is a set of access
    >> + rights tied to an object, which could be a file, a socket or a process.
    >> +
    >> + See Documentation/security/landlock/ for further information.
    >> +
    >> + If you are unsure how to answer this question, answer N.
    >> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
    >> new file mode 100644
    >> index 000000000000..cb6deefbf4c0
    >> --- /dev/null
    >> +++ b/security/landlock/Makefile
    >> @@ -0,0 +1,3 @@
    >> +obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
    >> +
    >> +landlock-y := object.o
    >> diff --git a/security/landlock/object.c b/security/landlock/object.c
    >> new file mode 100644
    >> index 000000000000..38fbbb108120
    >> --- /dev/null
    >> +++ b/security/landlock/object.c
    >> @@ -0,0 +1,339 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Landlock LSM - Object and rule management
    >> + *
    >> + * Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
    >> + * Copyright © 2018-2020 ANSSI
    >> + *
    >> + * Principles and constraints of the object and rule management:
    >> + * - Do not leak memory.
    >> + * - Try as much as possible to free a memory allocation as soon as it is
    >> + * unused.
    >> + * - Do not use global lock.
    >> + * - Do not charge processes other than the one requesting a Landlock
    >> + * operation.
    >> + */
    >> +
    >> +#include <linux/bug.h>
    >> +#include <linux/compiler.h>
    >> +#include <linux/compiler_types.h>
    >> +#include <linux/err.h>
    >> +#include <linux/errno.h>
    >> +#include <linux/fs.h>
    >> +#include <linux/kernel.h>
    >> +#include <linux/list.h>
    >> +#include <linux/rbtree.h>
    >> +#include <linux/rcupdate.h>
    >> +#include <linux/refcount.h>
    >> +#include <linux/slab.h>
    >> +#include <linux/spinlock.h>
    >> +#include <linux/workqueue.h>
    >> +
    >> +#include "object.h"
    >> +
    >> +struct landlock_object *landlock_create_object(
    >> + const enum landlock_object_type type, void *underlying_object)
    >> +{
    >> + struct landlock_object *object;
    >> +
    >> + if (WARN_ON_ONCE(!underlying_object))
    >> + return NULL;
    >> + object = kzalloc(sizeof(*object), GFP_KERNEL);
    >> + if (!object)
    >> + return NULL;
    >> + refcount_set(&object->usage, 1);
    >> + refcount_set(&object->cleaners, 1);
    >> + spin_lock_init(&object->lock);
    >> + INIT_LIST_HEAD(&object->rules);
    >> + object->type = type;
    >> + WRITE_ONCE(object->underlying_object, underlying_object);
    >> + return object;
    >> +}
    >> +
    >> +struct landlock_object *landlock_get_object(struct landlock_object *object)
    >> + __acquires(object->usage)
    >> +{
    >> + __acquire(object->usage);
    >> + /*
    >> + * If @object->usage equal 0, then it will be ignored by writers, and
    >> + * underlying_object->object may be replaced, but this is not an issue
    >> + * for release_object().
    >> + */
    >> + if (object && refcount_inc_not_zero(&object->usage)) {
    >> + /*
    >> + * It should not be possible to get a reference to an object if
    >> + * its underlying object is being terminated (e.g. with
    >> + * landlock_release_object()), because an object is only
    >> + * modifiable through such underlying object. This is not the
    >> + * case with landlock_get_object_cleaner().
    >> + */
    >> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
    >> + return object;
    >> + }
    >> + return NULL;
    >> +}
    >> +
    >> +static struct landlock_object *get_object_cleaner(
    >> + struct landlock_object *object)
    >> + __acquires(object->cleaners)
    >> +{
    >> + __acquire(object->cleaners);
    >> + if (object && refcount_inc_not_zero(&object->cleaners))
    >> + return object;
    >> + return NULL;
    >> +}
    >> +
    >> +/*
    >> + * There is two cases when an object should be free and the reference to the
    >> + * underlying object should be put:
    >> + * - when the last rule tied to this object is removed, which is handled by
    >> + * landlock_put_rule() and then release_object();
    >> + * - when the object is being terminated (e.g. no more reference to an inode),
    >> + * which is handled by landlock_put_object().
    >> + */
    >> +static void put_object_free(struct landlock_object *object)
    >> + __releases(object->cleaners)
    >> +{
    >> + __release(object->cleaners);
    >> + if (!refcount_dec_and_test(&object->cleaners))
    >> + return;
    >> + WARN_ON_ONCE(refcount_read(&object->usage));
    >> + /*
    >> + * Ensures a safe use of @object in the RCU block from
    >> + * landlock_put_rule().
    >> + */
    >> + kfree_rcu(object, rcu_free);
    >> +}
    >> +
    >> +/*
    >> + * Destroys a newly created and useless object.
    >> + */
    >> +void landlock_drop_object(struct landlock_object *object)
    >> +{
    >> + if (WARN_ON_ONCE(!refcount_dec_and_test(&object->usage)))
    >> + return;
    >> + __acquire(object->cleaners);
    >> + put_object_free(object);
    >> +}
    >> +
    >> +/*
    >> + * Puts the underlying object (e.g. inode) if it is the first request to
    >> + * release @object, without calling landlock_put_object().
    >> + *
    >> + * Return true if this call effectively marks @object as released, false
    >> + * otherwise.
    >> + */
    >> +static bool release_object(struct landlock_object *object)
    >> + __releases(&object->lock)
    >> +{
    >> + void *underlying_object;
    >> +
    >> + lockdep_assert_held(&object->lock);
    >> +
    >> + underlying_object = xchg(&object->underlying_object, NULL);
    >
    > A one-line comment looks needed for xchg.

    Ok. This is to have a guarantee that the underlying_object (e.g. the
    inode pointer) is only used once. I'll add a comment.

    >
    >> + spin_unlock(&object->lock);
    >> + might_sleep();
    >
    > Have trouble working out what might_sleep is put for.

    Patch 5 adds a call to landlock_release_inode(underlying_object, object)
    (LANDLOCK_OBJECT_INODE case), which can sleep e.g., with a call to iput().

    >
    >> + if (!underlying_object)
    >> + return false;
    >> +
    >> + switch (object->type) {
    >> + case LANDLOCK_OBJECT_INODE:
    >> + break;
    >> + default:
    >> + WARN_ON_ONCE(1);
    >> + }
    >> + return true;
    >> +}
    >> +
    >> +static void put_object_cleaner(struct landlock_object *object)
    >> + __releases(object->cleaners)
    >> +{
    >> + /* Let's try an early lockless check. */
    >> + if (list_empty(&object->rules) &&
    >> + READ_ONCE(object->underlying_object)) {
    >> + /*
    >> + * Puts @object if there is no rule tied to it and the
    >> + * remaining user is the underlying object. This check is
    >> + * atomic because @object->rules and @object->underlying_object
    >> + * are protected by @object->lock.
    >> + */
    >> + spin_lock(&object->lock);
    >> + if (list_empty(&object->rules) &&
    >> + READ_ONCE(object->underlying_object) &&
    >> + refcount_dec_if_one(&object->usage)) {
    >> + /*
    >> + * Releases @object, in place of
    >> + * landlock_release_object().
    >> + *
    >> + * @object is already empty, implying that all its
    >> + * previous rules are already disabled.
    >> + *
    >> + * Unbalance the @object->cleaners counter to reflect
    >> + * the underlying object release.
    >> + */
    >> + if (!WARN_ON_ONCE(!release_object(object))) {
    >
    > Two ! hurt more than help.

    Well, it may not look nice but don't you think it is better than a
    WARN_ON_ONCE(1) in the if block?

    >> + __acquire(object->cleaners);
    >> + put_object_free(object);
    >
    > Why put object more than once?

    I just replied to Jann about this subject. This is to "unbalance" the
    counter to potentially free it (if there is no more user). I explain it
    here:
    https://lore.kernel.org/lkml/67465638-e22c-5d1a-df37-862b31d999a1@digikod.net/

    >
    >> + }
    >> + } else {
    >> + spin_unlock(&object->lock);
    >> + }
    >> + }
    >> + put_object_free(object);
    >> +}
    >> +
    >

    \
     
     \ /
      Last update: 2020-02-27 18:01    [W:3.901 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site