Inter-revision diff: patch 2

Comparing v12 (message) to v11 (message)

--- v12
+++ v11
@@ -20,26 +20,15 @@
 Cc: Will Drewry <wad@chromium.org>
 ---
 
-Changes since v11:
-* remove old code from previous refactoring (removing the program
-  chaining concept) and simplify program prepending (reported by Serge
-  E. Hallyn):
-  * simplify landlock_prepend_prog() and merge it with
-    store_landlock_prog()
-  * add new_prog_list() and rework new_landlock_domain()
-  * remove the extra page allocation checks, only rely on the eBPF
-    program checks
-* replace the -EINVAL for the duplicate program check with the -EEXIST
-
 Changes since v10:
 * rename files and names to clearly define a domain
 * create a standalone patch to ease review
 ---
  security/landlock/Makefile        |   3 +-
- security/landlock/common.h        |  38 +++++++
- security/landlock/domain_manage.c | 173 ++++++++++++++++++++++++++++++
- security/landlock/domain_manage.h |  23 ++++
- 4 files changed, 236 insertions(+), 1 deletion(-)
+ security/landlock/common.h        |  38 +++++
+ security/landlock/domain_manage.c | 265 ++++++++++++++++++++++++++++++
+ security/landlock/domain_manage.h |  23 +++
+ 4 files changed, 328 insertions(+), 1 deletion(-)
  create mode 100644 security/landlock/domain_manage.c
  create mode 100644 security/landlock/domain_manage.h
 
@@ -110,10 +99,10 @@
  	switch (prog->expected_attach_type) {
 diff --git a/security/landlock/domain_manage.c b/security/landlock/domain_manage.c
 new file mode 100644
-index 000000000000..8b7ce988a221
+index 000000000000..c955b9c95c84
 --- /dev/null
 +++ b/security/landlock/domain_manage.c
-@@ -0,0 +1,173 @@
+@@ -0,0 +1,265 @@
 +// SPDX-License-Identifier: GPL-2.0-only
 +/*
 + * Landlock LSM - domain management
@@ -138,7 +127,7 @@
 +	refcount_inc(&dom->usage);
 +}
 +
-+static void put_prog_list(struct landlock_prog_list *prog_list)
++static void put_landlock_prog_list(struct landlock_prog_list *prog_list)
 +{
 +	struct landlock_prog_list *orig = prog_list;
 +
@@ -159,67 +148,118 @@
 +		size_t i;
 +
 +		for (i = 0; i < ARRAY_SIZE(domain->programs); i++)
-+			put_prog_list(domain->programs[i]);
++			put_landlock_prog_list(domain->programs[i]);
 +		kfree(domain);
 +	}
 +}
 +
-+static struct landlock_prog_list *new_prog_list(struct bpf_prog *prog)
-+{
++static struct landlock_domain *new_landlock_domain(void)
++{
++	struct landlock_domain *domain;
++
++	/* array filled with NULL values */
++	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
++	if (!domain)
++		return ERR_PTR(-ENOMEM);
++	refcount_set(&domain->usage, 1);
++	return domain;
++}
++
++/**
++ * 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;
 +
-+	if (WARN_ON(IS_ERR_OR_NULL(prog)))
-+		return ERR_PTR(-EFAULT);
-+	if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
-+		return ERR_PTR(-EINVAL);
-+	prog = bpf_prog_inc(prog);
-+	if (IS_ERR(prog))
-+		return ERR_CAST(prog);
++	last_type = get_hook_type(new);
++
++	/* ignore duplicate programs */
++	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(prog);
-+		return ERR_PTR(-ENOMEM);
-+	}
-+	new_list->prog = prog;
++		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);
-+	return new_list;
-+}
-+
-+/* @prog can legitimately be NULL */
-+static struct landlock_domain *new_landlock_domain(struct bpf_prog *prog)
-+{
-+	struct landlock_domain *new_domain;
-+	struct landlock_prog_list *new_list;
-+	size_t hook;
-+
-+	/* programs[] filled with NULL values */
-+	new_domain = kzalloc(sizeof(*new_domain), GFP_KERNEL);
-+	if (!new_domain)
-+		return ERR_PTR(-ENOMEM);
-+	refcount_set(&new_domain->usage, 1);
-+	if (!prog)
-+		return new_domain;
-+	new_list = new_prog_list(prog);
-+	if (IS_ERR(new_list)) {
-+		kfree(new_domain);
-+		return ERR_CAST(new_list);
-+	}
-+	hook = get_hook_index(get_hook_type(prog));
-+	new_domain->programs[hook] = new_list;
-+	return new_domain;
-+}
++	tmp_list = new_list;
++
++	if (!tmp_list)
++		/* inform user space that this program was already added */
++		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 program to @current_domain
-+ *
-+ * Prepend @prog to @current_domain if @prog is not already in @current_domain.
-+ *
-+ * @current_domain: landlock_domain pointer which is garantee to not be
-+ *                  modified elsewhere. This pointer should not be used nor
-+ *                  put/freed after the call.
-+ * @prog: non-NULL Landlock program to prepend to @current_domain. @prog will
-+ *        be owned by landlock_prepend_prog(). You can then call
-+ *        bpf_prog_put(@prog) after.
++ * 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.
@@ -228,64 +268,105 @@
 +		struct landlock_domain *current_domain,
 +		struct bpf_prog *prog)
 +{
-+	struct landlock_domain *oneref_domain;
-+	struct landlock_prog_list *new_list, *walker;
-+	size_t hook;
-+
-+	if (WARN_ON(!prog))
-+		return ERR_PTR(-EFAULT);
++	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(&current_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 domain contains an array of prog_list pointers.  If a domain is
-+	 * used by more than one credential, then this domain is first
-+	 * duplicated and then @prog is prepended to this new domain.  We then
-+	 * have the garantee that a domain is immutable when shared, and it can
-+	 * only be modified if it is referenced only once (by the modifier).
++	 * 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 (!current_domain)
-+		return new_landlock_domain(prog);
-+
-+	hook = get_hook_index(get_hook_type(prog));
-+	/* check for similar program */
-+	for (walker = current_domain->programs[hook]; walker;
-+			walker = walker->prev) {
-+		/* don't allow duplicate programs */
-+		if (prog == walker->prog)
-+			return ERR_PTR(-EEXIST);
-+	}
-+
-+	new_list = new_prog_list(prog);
-+	if (IS_ERR(new_list))
-+		return ERR_CAST(new_list);
-+
-+	/* duplicate the domain if not referenced only once */
-+	if (refcount_read(&current_domain->usage) == 1) {
-+		oneref_domain = current_domain;
-+	} else {
-+		size_t i;
-+
-+		oneref_domain = new_landlock_domain(NULL);
-+		if (IS_ERR(oneref_domain)) {
-+			put_prog_list(new_list);
-+			return oneref_domain;
++	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(&current_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);
 +		}
-+		for (i = 0; i < ARRAY_SIZE(oneref_domain->programs); i++) {
-+			oneref_domain->programs[i] =
-+				current_domain->programs[i];
-+			if (oneref_domain->programs[i])
-+				refcount_inc(&oneref_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];
 +		}
-+		landlock_put_domain(current_domain);
-+		/* @current_domain may be a dangling pointer now */
-+		current_domain = NULL;
-+	}
-+
-+	/* no need to increment usage (pointer replacement) */
-+	new_list->prev = oneref_domain->programs[hook];
-+	oneref_domain->programs[hook] = new_list;
-+	return oneref_domain;
++	}
++	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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help