Re: [RFC PATCH v14 01/10] landlock: Add object and rule management

["Mickaël Salaün" <mic@digikod.net>]

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);
>> +}
>> +
>