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;

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[] = {

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;

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@inte... 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 */

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)

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. */