[PATCH v1 0/9] Landlock fixes

[PATCH v1 0/9] Landlock fixes

[Mickaël Salaün]

Hi,

This patch series fixes some issues and makes the Landlock filesystem
access-control more consistent and deterministic when stacking multiple
rulesets.  This is checked by current and new tests.  I also extended
documentation and example to help users.

This series can be applied on top of
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/log/?h=landlock_lsm

Regards,

Mickaël Salaün (9):
  landlock: Fix memory allocation error handling
  landlock: Cosmetic fixes for filesystem management
  landlock: Enforce deterministic interleaved path rules
  landlock: Always intersect access rights
  landlock: Add extra checks when inserting a rule
  selftests/landlock: Extend layout1.inherit_superset
  landlock: Clean up get_ruleset_from_fd()
  landlock: Add help to enable Landlock as a stacked LSM
  landlock: Extend documentation about limitations

 Documentation/userspace-api/landlock.rst   |  17 +++
 samples/landlock/sandboxer.c               |  21 +++-
 security/landlock/Kconfig                  |   4 +-
 security/landlock/fs.c                     |  67 +++++-----
 security/landlock/object.c                 |   5 +-
 security/landlock/ruleset.c                |  34 ++---
 security/landlock/syscall.c                |  24 ++--
 tools/testing/selftests/landlock/fs_test.c | 140 +++++++++++++++++++--
 8 files changed, 239 insertions(+), 73 deletions(-)


base-commit: 96b3198c4025c11347651700b77e45a686d78553
-- 
2.29.2

[PATCH v1 1/9] landlock: Fix memory allocation error handling

[Mickaël Salaün]

Handle memory allocation errors in landlock_create_object() call.  This
prevent to inadvertently hold an inode.  Also, make get_inode_object()
more readable.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/fs.c     | 5 +++++
 security/landlock/object.c | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index d8c5d19ac2af..b67c821bb40b 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -9,6 +9,7 @@
 #include <linux/atomic.h>
 #include <linux/compiler_types.h>
 #include <linux/dcache.h>
+#include <linux/err.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -98,6 +99,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	 * holding any locks).
 	 */
 	new_object = landlock_create_object(&landlock_fs_underops, inode);
+	if (IS_ERR(new_object))
+		return new_object;
 
 	spin_lock(&inode->i_lock);
 	object = rcu_dereference_protected(inode_sec->object,
@@ -145,6 +148,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
 	access_rights |= _LANDLOCK_ACCESS_FS_MASK & ~ruleset->fs_access_mask;
 	rule.access = access_rights;
 	rule.object = get_inode_object(d_backing_inode(path->dentry));
+	if (IS_ERR(rule.object))
+		return PTR_ERR(rule.object);
 	mutex_lock(&ruleset->lock);
 	err = landlock_insert_rule(ruleset, &rule, false);
 	mutex_unlock(&ruleset->lock);
diff --git a/security/landlock/object.c b/security/landlock/object.c
index a71644ee72a7..54ba0327002a 100644
--- a/security/landlock/object.c
+++ b/security/landlock/object.c
@@ -8,6 +8,7 @@
 
 #include <linux/bug.h>
 #include <linux/compiler_types.h>
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/rcupdate.h>
 #include <linux/refcount.h>
@@ -23,10 +24,10 @@ struct landlock_object *landlock_create_object(
 	struct landlock_object *new_object;
 
 	if (WARN_ON_ONCE(!underops || !underobj))
-		return NULL;
+		return ERR_PTR(-ENOENT);
 	new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT);
 	if (!new_object)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	refcount_set(&new_object->usage, 1);
 	spin_lock_init(&new_object->lock);
 	new_object->underops = underops;
-- 
2.29.2

[PATCH v1 2/9] landlock: Cosmetic fixes for filesystem management

[Mickaël Salaün]

Improve comments and make get_inode_object() more readable.  The kfree()
call is correct but we should mimimize as much as possible lock windows.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/fs.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index b67c821bb40b..33fc7ae17c7f 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -85,8 +85,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 			return object;
 		}
 		/*
-		 * We're racing with release_inode(), the object is going away.
-		 * Wait for release_inode(), then retry.
+		 * We are racing with release_inode(), the object is going
+		 * away.  Wait for release_inode(), then retry.
 		 */
 		spin_lock(&object->lock);
 		spin_unlock(&object->lock);
@@ -107,21 +107,21 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 			lockdep_is_held(&inode->i_lock));
 	if (unlikely(object)) {
 		/* Someone else just created the object, bail out and retry. */
-		kfree(new_object);
 		spin_unlock(&inode->i_lock);
+		kfree(new_object);
 
 		rcu_read_lock();
 		goto retry;
-	} else {
-		rcu_assign_pointer(inode_sec->object, new_object);
-		/*
-		 * @inode will be released by hook_sb_delete() on its
-		 * superblock shutdown.
-		 */
-		ihold(inode);
-		spin_unlock(&inode->i_lock);
-		return new_object;
 	}
+
+	rcu_assign_pointer(inode_sec->object, new_object);
+	/*
+	 * @inode will be released by hook_sb_delete() on its superblock
+	 * shutdown.
+	 */
+	ihold(inode);
+	spin_unlock(&inode->i_lock);
+	return new_object;
 }
 
 /* All access rights which can be tied to files. */
-- 
2.29.2

Re: [PATCH v1 2/9] landlock: Cosmetic fixes for filesystem management

[James Morris]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

On Wed, 11 Nov 2020, Mickaël Salaün wrote:

> Improve comments and make get_inode_object() more readable.  The kfree()
> call is correct but we should mimimize as much as possible lock windows.
> 
> Cc: James Morris <jmorris@namei.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>

> ---
>  security/landlock/fs.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index b67c821bb40b..33fc7ae17c7f 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -85,8 +85,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  			return object;
>  		}
>  		/*
> -		 * We're racing with release_inode(), the object is going away.
> -		 * Wait for release_inode(), then retry.
> +		 * We are racing with release_inode(), the object is going
> +		 * away.  Wait for release_inode(), then retry.
>  		 */
>  		spin_lock(&object->lock);
>  		spin_unlock(&object->lock);
> @@ -107,21 +107,21 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  			lockdep_is_held(&inode->i_lock));
>  	if (unlikely(object)) {
>  		/* Someone else just created the object, bail out and retry. */
> -		kfree(new_object);
>  		spin_unlock(&inode->i_lock);
> +		kfree(new_object);
>  
>  		rcu_read_lock();
>  		goto retry;
> -	} else {
> -		rcu_assign_pointer(inode_sec->object, new_object);
> -		/*
> -		 * @inode will be released by hook_sb_delete() on its
> -		 * superblock shutdown.
> -		 */
> -		ihold(inode);
> -		spin_unlock(&inode->i_lock);
> -		return new_object;
>  	}
> +
> +	rcu_assign_pointer(inode_sec->object, new_object);
> +	/*
> +	 * @inode will be released by hook_sb_delete() on its superblock
> +	 * shutdown.
> +	 */
> +	ihold(inode);
> +	spin_unlock(&inode->i_lock);
> +	return new_object;
>  }
>  
>  /* All access rights which can be tied to files. */
> 

-- 
James Morris
<jmorris@namei.org>

[PATCH v1 3/9] landlock: Enforce deterministic interleaved path rules

[Mickaël Salaün]

To have consistent layered rules, granting access to a path implies that
all accesses tied to inodes, from the requested file to the real root,
must be checked.  Otherwise, stacked rules may result to overzealous
restrictions.  By excluding the ability to add exceptions in the same
layer (e.g. /a allowed, /a/b denied, and /a/b/c allowed), we get
deterministic interleaved path rules.  This removes an optimization
which could be replaced by a proper cache mechanism.  This also further
simplifies and explain check_access_path_continue().

Add a layout1.interleaved_masked_accesses test to check corner-case
layered rule combinations.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/fs.c                     | 38 +++++----
 tools/testing/selftests/landlock/fs_test.c | 95 ++++++++++++++++++++++
 2 files changed, 115 insertions(+), 18 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 33fc7ae17c7f..2ca4dce1e9ed 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -180,16 +180,24 @@ static bool check_access_path_continue(
 			rcu_dereference(landlock_inode(inode)->object));
 	rcu_read_unlock();
 
-	/* Checks for matching layers. */
-	if (rule && (rule->layers | *layer_mask)) {
-		if ((rule->access & access_request) == access_request) {
-			*layer_mask &= ~rule->layers;
-			return true;
-		} else {
-			return false;
-		}
+	if (!rule)
+		/* Continues to walk if there is no rule for this inode. */
+		return true;
+	/*
+	 * We must check all layers for each inode because we may encounter
+	 * multiple different accesses from the same layer in a walk.  Each
+	 * layer must at least allow the access request one time (i.e. with one
+	 * inode).  This enables to have a deterministic behavior whatever
+	 * inode is tagged within interleaved layers.
+	 */
+	if ((rule->access & access_request) == access_request) {
+		/* Validates layers for which all accesses are allowed. */
+		*layer_mask &= ~rule->layers;
+		/* Continues to walk until all layers are validated. */
+		return true;
 	}
-	return true;
+	/* Stops if a rule in the path don't allow all requested access. */
+	return false;
 }
 
 static int check_access_path(const struct landlock_ruleset *const domain,
@@ -231,12 +239,6 @@ static int check_access_path(const struct landlock_ruleset *const domain,
 				&layer_mask)) {
 		struct dentry *parent_dentry;
 
-		/* Stops when a rule from each layer granted access. */
-		if (layer_mask == 0) {
-			allowed = true;
-			break;
-		}
-
 jump_up:
 		/*
 		 * Does not work with orphaned/private mounts like overlayfs
@@ -248,10 +250,10 @@ static int check_access_path(const struct landlock_ruleset *const domain,
 				goto jump_up;
 			} else {
 				/*
-				 * Stops at the real root.  Denies access
-				 * because not all layers have granted access.
+				 * Stops at the real root.  Denies access if
+				 * not all layers granted access.
 				 */
-				allowed = false;
+				allowed = (layer_mask == 0);
 				break;
 			}
 		}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 8aed28081ec8..ade0ad8728d8 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -629,6 +629,101 @@ TEST_F(layout1, ruleset_overlap)
 	EXPECT_EQ(0, close(open_fd));
 }
 
+TEST_F(layout1, interleaved_masked_accesses)
+{
+	/*
+	 * Checks overly restrictive rules:
+	 * layer 1: allows s1d1/s1d2/s1d3/file1
+	 * layer 2: allows s1d1/s1d2/s1d3
+	 *          denies s1d1/s1d2
+	 * layer 3: allows s1d1
+	 * layer 4: allows s1d1/s1d2
+	 */
+	const struct rule layer1[] = {
+		/* Allows access to file1_s1d3 with the first layer. */
+		{
+			.path = file1_s1d3,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{}
+	};
+	const struct rule layer2[] = {
+		/* Start by granting access to file1_s1d3 with this rule... */
+		{
+			.path = dir_s1d3,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		/* ...but finally denies access to file1_s1d3. */
+		{
+			.path = dir_s1d2,
+			.access = 0,
+		},
+		{}
+	};
+	const struct rule layer3[] = {
+		/* Try to allows access to file1_s1d3. */
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{}
+	};
+	const struct rule layer4[] = {
+		/* Try to bypass layer2. */
+		{
+			.path = dir_s1d2,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{}
+	};
+	int open_fd, ruleset_fd;
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer1);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Checks that access is granted for file1_s1d3. */
+	open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
+	ASSERT_LE(0, open_fd);
+	EXPECT_EQ(0, close(open_fd));
+	ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer2);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Now, checks that access is denied for file1_s1d3. */
+	ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer3);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Checks that access rights are unchanged with layer 3. */
+	ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer4);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Checks that access rights are unchanged with layer 4. */
+	ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+}
+
 TEST_F(layout1, inherit_subset)
 {
 	const struct rule rules[] = {
-- 
2.29.2

[PATCH v1 4/9] landlock: Always intersect access rights

[Mickaël Salaün]

Following the previous commit logic, make ruleset updates more
consistent by always intersecting access rights (boolean AND) instead of
combining them (boolean OR) for the same layer.

This defensive approach could also help avoid user space to
inadvertently allow multiple access rights for the same object (e.g.
write and execute access on a path hierarchy) instead of dealing with
such inconsistency.  This can happen when there is no deduplication of
objects (e.g. paths and underlying inodes) whereas they get different
access rights with landlock_add_rule(2).

Update layout1.ruleset_overlap and layout1.inherit_subset tests
accordingly.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/ruleset.c                | 17 ++++-----
 tools/testing/selftests/landlock/fs_test.c | 41 +++++++++++++++-------
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 9fe92b2f5fbd..7654a66cea43 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -82,11 +82,11 @@ static void put_rule(struct landlock_rule *const rule)
 /**
  * landlock_insert_rule - Insert a rule in a ruleset
  *
+ * Intersects access rights of the rule with those of the ruleset.
+ *
  * @ruleset: The ruleset to be updated.
  * @rule: Read-only payload to be inserted (not owned by this function).
- * @is_merge: If true, intersects access rights and updates the rule's layers
- *            (e.g. merge two rulesets), else do a union of access rights and
- *            keep the rule's layers (e.g. extend a ruleset)
+ * @is_merge: If true, handle the rule layers.
  *
  * Assumptions:
  *
@@ -117,16 +117,11 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
 		}
 
 		/* If there is a matching rule, updates it. */
-		if (is_merge) {
-			/* Intersects access rights. */
-			this->access &= rule->access;
-
+		if (is_merge)
 			/* Updates the rule layers with the next one. */
 			this->layers |= BIT_ULL(ruleset->nb_layers);
-		} else {
-			/* Extends access rights. */
-			this->access |= rule->access;
-		}
+		/* Intersects access rights. */
+		this->access &= rule->access;
 		return 0;
 	}
 
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index ade0ad8728d8..1885174b2770 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -591,14 +591,16 @@ TEST_F(layout1, unhandled_access)
 TEST_F(layout1, ruleset_overlap)
 {
 	const struct rule rules[] = {
-		/* These rules should be ORed among them. */
+		/* These rules should be ANDed among them. */
 		{
 			.path = dir_s1d2,
-			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				LANDLOCK_ACCESS_FS_WRITE_FILE,
 		},
 		{
 			.path = dir_s1d2,
-			.access = LANDLOCK_ACCESS_FS_READ_DIR,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				LANDLOCK_ACCESS_FS_READ_DIR,
 		},
 		{}
 	};
@@ -609,24 +611,37 @@ TEST_F(layout1, ruleset_overlap)
 	enforce_ruleset(_metadata, ruleset_fd);
 	EXPECT_EQ(0, close(ruleset_fd));
 
+	/* Checks s1d1 hierarchy. */
+	ASSERT_EQ(-1, open(file1_s1d1, O_RDONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
 	ASSERT_EQ(-1, open(file1_s1d1, O_WRONLY | O_CLOEXEC));
 	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file1_s1d1, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
 	ASSERT_EQ(-1, open(dir_s1d1, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
 	ASSERT_EQ(EACCES, errno);
 
-	open_fd = open(file1_s1d2, O_WRONLY | O_CLOEXEC);
-	ASSERT_LE(0, open_fd);
-	EXPECT_EQ(0, close(open_fd));
-	open_fd = open(dir_s1d2, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	/* Checks s1d2 hierarchy. */
+	open_fd = open(file1_s1d2, O_RDONLY | O_CLOEXEC);
 	ASSERT_LE(0, open_fd);
 	EXPECT_EQ(0, close(open_fd));
+	ASSERT_EQ(-1, open(file1_s1d2, O_WRONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(dir_s1d2, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
 
-	open_fd = open(file1_s1d3, O_WRONLY | O_CLOEXEC);
-	ASSERT_LE(0, open_fd);
-	EXPECT_EQ(0, close(open_fd));
-	open_fd = open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	/* Checks s1d3 hierarchy. */
+	open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
 	ASSERT_LE(0, open_fd);
 	EXPECT_EQ(0, close(open_fd));
+	ASSERT_EQ(-1, open(file1_s1d3, O_WRONLY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(file1_s1d3, O_RDWR | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
+	ASSERT_EQ(-1, open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
+	ASSERT_EQ(EACCES, errno);
 }
 
 TEST_F(layout1, interleaved_masked_accesses)
@@ -766,8 +781,8 @@ TEST_F(layout1, inherit_subset)
 	 * any new access, only remove some.  Once enforced, these rules are
 	 * ANDed with the previous ones.
 	 */
-	add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
-			dir_s1d2);
+	add_path_beneath(_metadata, ruleset_fd, rules[0].access |
+			LANDLOCK_ACCESS_FS_WRITE_FILE, dir_s1d2);
 	/*
 	 * According to ruleset_fd, dir_s1d2 should now have the
 	 * LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
-- 
2.29.2

[PATCH v1 5/9] landlock: Add extra checks when inserting a rule

[Mickaël Salaün]

Make sure that there is always an (allocated) object in each used rules.
This could have prevented the bug fixed with a previous commit.

When the rules from a ruleset are merged in a domain with
landlock_enforce_ruleset_current(2), these new rules should be assigned
to the last layer.  However, when a rule is just extending a ruleset
with landlock_add_rule(2), the number of layers of this updated ruleset
should always be 0.  Checking such use of landlock_insert_rule() could
enable to detect bugs in future developments.

Replace the hardcoded 1 with SINGLE_DEPTH_NESTING.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/ruleset.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 7654a66cea43..1fb85daeb750 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -102,6 +102,10 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
 
 	might_sleep();
 	lockdep_assert_held(&ruleset->lock);
+	if (WARN_ON_ONCE(!rule->object))
+		return -ENOENT;
+	if (!is_merge && WARN_ON_ONCE(ruleset->nb_layers != 0))
+		return -EINVAL;
 	walker_node = &(ruleset->root.rb_node);
 	while (*walker_node) {
 		struct landlock_rule *const this = rb_entry(*walker_node,
@@ -223,12 +227,7 @@ static struct landlock_ruleset *inherit_ruleset(
 		return new_ruleset;
 
 	mutex_lock(&new_ruleset->lock);
-	mutex_lock_nested(&parent->lock, 1);
-	new_ruleset->nb_layers = parent->nb_layers;
-	new_ruleset->fs_access_mask = parent->fs_access_mask;
-	WARN_ON_ONCE(!parent->hierarchy);
-	get_hierarchy(parent->hierarchy);
-	new_ruleset->hierarchy->parent = parent->hierarchy;
+	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
 
 	/* Copies the @parent tree. */
 	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
@@ -237,6 +236,12 @@ static struct landlock_ruleset *inherit_ruleset(
 		if (err)
 			goto out_unlock;
 	}
+	new_ruleset->nb_layers = parent->nb_layers;
+	new_ruleset->fs_access_mask = parent->fs_access_mask;
+	WARN_ON_ONCE(!parent->hierarchy);
+	get_hierarchy(parent->hierarchy);
+	new_ruleset->hierarchy->parent = parent->hierarchy;
+
 	mutex_unlock(&parent->lock);
 	mutex_unlock(&new_ruleset->lock);
 	return new_ruleset;
-- 
2.29.2

[PATCH v1 6/9] selftests/landlock: Extend layout1.inherit_superset

[Mickaël Salaün]

These additional checks test that layers are handled as expected in the
superset use case, which complete the inherit_subset checks.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 tools/testing/selftests/landlock/fs_test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 1885174b2770..c2f65034e4ee 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -907,6 +907,10 @@ TEST_F(layout1, inherit_superset)
 	open_fd = open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
 	ASSERT_LE(0, open_fd);
 	ASSERT_EQ(0, close(open_fd));
+	/* File access is allowed for file1_s1d3. */
+	open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
+	ASSERT_LE(0, open_fd);
+	ASSERT_EQ(0, close(open_fd));
 
 	/* Now dir_s1d2, parent of dir_s1d3, gets a new rule tied to it. */
 	add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_READ_FILE |
@@ -922,6 +926,10 @@ TEST_F(layout1, inherit_superset)
 	open_fd = open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
 	ASSERT_LE(0, open_fd);
 	ASSERT_EQ(0, close(open_fd));
+	/* File access is now denied for file1_s1d3. */
+	open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
+	ASSERT_LE(-1, open_fd);
+	ASSERT_EQ(EACCES, errno);
 }
 
 TEST_F(layout1, max_layers)
-- 
2.29.2

[PATCH v1 7/9] landlock: Clean up get_ruleset_from_fd()

[Mickaël Salaün]

Rewrite get_ruleset_from_fd() to please the 0-DAY CI Kernel Test Service
that reported an uninitialized variable (false positive).  Anyway, it is
cleaner like this.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Link: https://lore.kernel.org/linux-security-module/202011101854.zGbWwusK-lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/syscall.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/security/landlock/syscall.c b/security/landlock/syscall.c
index 486136d4f46e..543ae36cd339 100644
--- a/security/landlock/syscall.c
+++ b/security/landlock/syscall.c
@@ -196,24 +196,26 @@ static struct landlock_ruleset *get_ruleset_from_fd(const int fd,
 {
 	struct fd ruleset_f;
 	struct landlock_ruleset *ruleset;
-	int err;
 
 	ruleset_f = fdget(fd);
 	if (!ruleset_f.file)
 		return ERR_PTR(-EBADF);
 
 	/* Checks FD type and access right. */
-	err = 0;
-	if (ruleset_f.file->f_op != &ruleset_fops)
-		err = -EBADFD;
-	else if (!(ruleset_f.file->f_mode & mode))
-		err = -EPERM;
-	if (!err) {
-		ruleset = ruleset_f.file->private_data;
-		landlock_get_ruleset(ruleset);
+	if (ruleset_f.file->f_op != &ruleset_fops) {
+		ruleset = ERR_PTR(-EBADFD);
+		goto out_fdput;
 	}
+	if (!(ruleset_f.file->f_mode & mode)) {
+		ruleset = ERR_PTR(-EPERM);
+		goto out_fdput;
+	}
+	ruleset = ruleset_f.file->private_data;
+	landlock_get_ruleset(ruleset);
+
+out_fdput:
 	fdput(ruleset_f);
-	return err ? ERR_PTR(err) : ruleset;
+	return ruleset;
 }
 
 /* Path handling */
-- 
2.29.2

[PATCH v1 8/9] landlock: Add help to enable Landlock as a stacked LSM

[Mickaël Salaün]

When using make oldconfig with a previous configuration already
including the CONFIG_LSM variable, no question is asked to update its
content.

Update the Kconfig help and add hints to the sample to help user
understand the required configuration.

This also cut long strings to fit in 100 columns.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 samples/landlock/sandboxer.c | 21 +++++++++++++++++++--
 security/landlock/Kconfig    |  4 +++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index ee5ec1203cb7..127fb063c23a 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -169,7 +169,8 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		fprintf(stderr, "usage: %s=\"...\" %s=\"...\" %s <cmd> [args]...\n\n",
 				ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv[0]);
 		fprintf(stderr, "Launch a command in a restricted environment.\n\n");
-		fprintf(stderr, "Environment variables containing paths, each separated by a colon:\n");
+		fprintf(stderr, "Environment variables containing paths, "
+				"each separated by a colon:\n");
 		fprintf(stderr, "* %s: list of paths allowed to be used in a read-only way.\n",
 				ENV_FS_RO_NAME);
 		fprintf(stderr, "* %s: list of paths allowed to be used in a read-write way.\n",
@@ -185,6 +186,21 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
 	if (ruleset_fd < 0) {
 		perror("Failed to create a ruleset");
+		switch (errno) {
+		case ENOSYS:
+			fprintf(stderr, "Hint: Landlock is not supported by the current kernel. "
+					"To support it, build the kernel with "
+					"CONFIG_SECURITY_LANDLOCK=y and prepend "
+					"\"landlock,\" to the content of CONFIG_LSM.\n");
+			break;
+		case EOPNOTSUPP:
+			fprintf(stderr, "Hint: Landlock is currently disabled. "
+					"It can be enabled in the kernel configuration by "
+					"prepending \"landlock,\" to the content of CONFIG_LSM, "
+					"or at boot time by setting the same content to the "
+					"\"lsm\" kernel parameter.\n");
+			break;
+		}
 		return 1;
 	}
 	if (populate_ruleset(ENV_FS_RO_NAME, ruleset_fd,
@@ -210,7 +226,8 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	execvpe(cmd_path, cmd_argv, envp);
 	fprintf(stderr, "Failed to execute \"%s\": %s\n", cmd_path,
 			strerror(errno));
-	fprintf(stderr, "Hint: access to the binary, the interpreter or shared libraries may be denied.\n");
+	fprintf(stderr, "Hint: access to the binary, the interpreter or "
+			"shared libraries may be denied.\n");
 	return 1;
 
 err_close_ruleset:
diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
index cbf88bb7fd97..43e5b0bb0706 100644
--- a/security/landlock/Kconfig
+++ b/security/landlock/Kconfig
@@ -16,4 +16,6 @@ config SECURITY_LANDLOCK
 
 	  See Documentation/userspace-api/landlock.rst for further information.
 
-	  If you are unsure how to answer this question, answer N.
+	  If you are unsure how to answer this question, answer N.  Otherwise, you
+	  should also prepend "landlock," to the content of CONFIG_LSM to enable
+	  Landlock at boot time.
-- 
2.29.2

[PATCH v1 9/9] landlock: Extend documentation about limitations

[Mickaël Salaün]

Explain limitations for the maximum number of stacked ruleset, and the
memory usage restrictions.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 Documentation/userspace-api/landlock.rst | 17 +++++++++++++++++
 security/landlock/syscall.c              |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 8f727de20479..7e83e5def1bc 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -186,6 +186,23 @@ Enforcing a ruleset
 Current limitations
 ===================
 
+Ruleset layers
+--------------
+
+There is a limit of 64 layers of stacked rulesets.  This can be an issue for a
+task willing to enforce a new ruleset in complement to its 64 inherited
+rulesets.  Once this limit is reached, sys_landlock_enforce_ruleset_current()
+returns E2BIG.  It is then strongly suggested to carefully build rulesets once
+in the life of a thread, especially for applications able to launch other
+applications which may also want to sandbox themselves (e.g. shells, container
+managers, etc.).
+
+Memory usage
+------------
+
+Kernel memory allocated to create rulesets is accounted and can be restricted
+by the :doc:`/admin-guide/cgroup-v1/memory`.
+
 File renaming and linking
 -------------------------
 
diff --git a/security/landlock/syscall.c b/security/landlock/syscall.c
index 543ae36cd339..045bcac79e17 100644
--- a/security/landlock/syscall.c
+++ b/security/landlock/syscall.c
@@ -361,6 +361,8 @@ SYSCALL_DEFINE4(landlock_add_rule,
  * - EPERM: @ruleset_fd has no read access to the underlying ruleset, or the
  *   current thread is not running with no_new_privs, or it doesn't have
  *   CAP_SYS_ADMIN in its namespace.
+ * - E2BIG: The maximum number of stacked rulesets is reached for the current
+ *   task.
  */
 SYSCALL_DEFINE2(landlock_enforce_ruleset_current,
 		const int, ruleset_fd, const __u32, flags)
-- 
2.29.2

Re: [PATCH v1 0/9] Landlock fixes

[James Morris]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

On Wed, 11 Nov 2020, Mickaël Salaün wrote:

> Hi,
> 
> This patch series fixes some issues and makes the Landlock filesystem
> access-control more consistent and deterministic when stacking multiple
> rulesets.  This is checked by current and new tests.  I also extended
> documentation and example to help users.
> 
> This series can be applied on top of
> https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/log/?h=landlock_lsm

Actually, given the number of fixes here, please respin so we get a 
cleaner initial PR for Linus.

> 
> Regards,
> 
> Mickaël Salaün (9):
>   landlock: Fix memory allocation error handling
>   landlock: Cosmetic fixes for filesystem management
>   landlock: Enforce deterministic interleaved path rules
>   landlock: Always intersect access rights
>   landlock: Add extra checks when inserting a rule
>   selftests/landlock: Extend layout1.inherit_superset
>   landlock: Clean up get_ruleset_from_fd()
>   landlock: Add help to enable Landlock as a stacked LSM
>   landlock: Extend documentation about limitations
> 
>  Documentation/userspace-api/landlock.rst   |  17 +++
>  samples/landlock/sandboxer.c               |  21 +++-
>  security/landlock/Kconfig                  |   4 +-
>  security/landlock/fs.c                     |  67 +++++-----
>  security/landlock/object.c                 |   5 +-
>  security/landlock/ruleset.c                |  34 ++---
>  security/landlock/syscall.c                |  24 ++--
>  tools/testing/selftests/landlock/fs_test.c | 140 +++++++++++++++++++--
>  8 files changed, 239 insertions(+), 73 deletions(-)
> 
> 
> base-commit: 96b3198c4025c11347651700b77e45a686d78553
> 

-- 
James Morris
<jmorris@namei.org>