From b665ee5f08df401a9e4a68e7d5f662d3f72ba1e1 Mon Sep 17 00:00:00 2001 From: Ba Jing Date: Mon, 18 Nov 2024 12:24:07 +0800 Subject: [PATCH 01/16] selftests/landlock: Remove unused macros in ptrace_test.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After reviewing the code, it was found that these macros are never referenced in the code. Just remove them. Signed-off-by: Ba Jing Link: https://lore.kernel.org/r/20241118042407.12900-1-bajing@cmss.chinamobile.com [mic: Reword subject] Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/ptrace_test.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c index a19db4d0b3bdc..8f31b673ff2dc 100644 --- a/tools/testing/selftests/landlock/ptrace_test.c +++ b/tools/testing/selftests/landlock/ptrace_test.c @@ -22,8 +22,6 @@ /* Copied from security/yama/yama_lsm.c */ #define YAMA_SCOPE_DISABLED 0 #define YAMA_SCOPE_RELATIONAL 1 -#define YAMA_SCOPE_CAPABILITY 2 -#define YAMA_SCOPE_NO_ATTACH 3 static void create_domain(struct __test_metadata *const _metadata) { From 078bf9438a31567e2c0587159ccefde835fb1ced Mon Sep 17 00:00:00 2001 From: Zichen Xie Date: Wed, 27 Nov 2024 21:29:56 -0600 Subject: [PATCH 02/16] samples/landlock: Fix possible NULL dereference in parse_path() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit malloc() may return NULL, leading to NULL dereference. Add a NULL check. Fixes: ba84b0bf5a16 ("samples/landlock: Add a sandbox manager example") Signed-off-by: Zichen Xie Link: https://lore.kernel.org/r/20241128032955.11711-1-zichenxie0106@gmail.com [mic: Simplify fix] Signed-off-by: Mickaël Salaün --- samples/landlock/sandboxer.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c index 57565dfd74a26..07fab2ef534e8 100644 --- a/samples/landlock/sandboxer.c +++ b/samples/landlock/sandboxer.c @@ -91,6 +91,9 @@ static int parse_path(char *env_path, const char ***const path_list) } } *path_list = malloc(num_paths * sizeof(**path_list)); + if (!*path_list) + return -1; + for (i = 0; i < num_paths; i++) (*path_list)[i] = strsep(&env_path, ENV_DELIMITER); @@ -127,6 +130,10 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd, env_path_name = strdup(env_path_name); unsetenv(env_var); num_paths = parse_path(env_path_name, &path_list); + if (num_paths < 0) { + fprintf(stderr, "Failed to allocate memory\n"); + goto out_free_name; + } if (num_paths == 1 && path_list[0][0] == '\0') { /* * Allows to not use all possible restrictions (e.g. use From 49440290a0935f428a1e43a5ac8dc275a647ff80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Fri, 10 Jan 2025 16:39:13 +0100 Subject: [PATCH 03/16] landlock: Handle weird files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A corrupted filesystem (e.g. bcachefs) might return weird files. Instead of throwing a warning and allowing access to such file, treat them as regular files. Cc: Dave Chinner Cc: Kent Overstreet Cc: Paul Moore Reported-by: syzbot+34b68f850391452207df@syzkaller.appspotmail.com Closes: https://lore.kernel.org/r/000000000000a65b35061cffca61@google.com Reported-by: syzbot+360866a59e3c80510a62@syzkaller.appspotmail.com Closes: https://lore.kernel.org/r/67379b3f.050a0220.85a0.0001.GAE@google.com Reported-by: Ubisectech Sirius Closes: https://lore.kernel.org/r/c426821d-8380-46c4-a494-7008bbd7dd13.bugreport@ubisectech.com Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control") Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20250110153918.241810-1-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index e31b97a9f175a..7adb25150488f 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -937,10 +937,6 @@ static access_mask_t get_mode_access(const umode_t mode) switch (mode & S_IFMT) { case S_IFLNK: return LANDLOCK_ACCESS_FS_MAKE_SYM; - case 0: - /* A zero mode translates to S_IFREG. */ - case S_IFREG: - return LANDLOCK_ACCESS_FS_MAKE_REG; case S_IFDIR: return LANDLOCK_ACCESS_FS_MAKE_DIR; case S_IFCHR: @@ -951,9 +947,12 @@ static access_mask_t get_mode_access(const umode_t mode) return LANDLOCK_ACCESS_FS_MAKE_FIFO; case S_IFSOCK: return LANDLOCK_ACCESS_FS_MAKE_SOCK; + case S_IFREG: + case 0: + /* A zero mode translates to S_IFREG. */ default: - WARN_ON_ONCE(1); - return 0; + /* Treats weird files as regular files. */ + return LANDLOCK_ACCESS_FS_MAKE_REG; } } From 25ccc75f5de6684fd6a497e44297497ccc7e0603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Fri, 10 Jan 2025 16:39:14 +0100 Subject: [PATCH 04/16] landlock: Constify get_mode_access() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use __attribute_const__ for get_mode_access(). Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20250110153918.241810-2-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 7adb25150488f..f81d0335b8257 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -932,7 +932,7 @@ static int current_check_access_path(const struct path *const path, return check_access_path(dom, path, access_request); } -static access_mask_t get_mode_access(const umode_t mode) +static __attribute_const__ access_mask_t get_mode_access(const umode_t mode) { switch (mode & S_IFMT) { case S_IFLNK: From d32f79a59ae1a90f27735c75f9920c585e6ceb8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Mon, 13 Jan 2025 17:11:09 +0100 Subject: [PATCH 05/16] landlock: Use scoped guards for ruleset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify error handling by replacing goto statements with automatic calls to landlock_put_ruleset() when going out of scope. This change will be easy to backport to v6.6 if needed, only the kernel.h include line conflicts. As for any other similar changes, we should be careful when backporting without goto statements. Add missing include file. Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20250113161112.452505-2-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/ruleset.c | 22 ++++++++++------------ security/landlock/ruleset.h | 5 +++++ security/landlock/syscalls.c | 25 ++++++++----------------- 3 files changed, 23 insertions(+), 29 deletions(-) diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index a93bdbf52fff8..f27b7bdb19b92 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -8,11 +8,13 @@ #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -537,7 +539,7 @@ struct landlock_ruleset * landlock_merge_ruleset(struct landlock_ruleset *const parent, struct landlock_ruleset *const ruleset) { - struct landlock_ruleset *new_dom; + struct landlock_ruleset *new_dom __free(landlock_put_ruleset) = NULL; u32 num_layers; int err; @@ -557,29 +559,25 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent, new_dom = create_ruleset(num_layers); if (IS_ERR(new_dom)) return new_dom; + new_dom->hierarchy = kzalloc(sizeof(*new_dom->hierarchy), GFP_KERNEL_ACCOUNT); - if (!new_dom->hierarchy) { - err = -ENOMEM; - goto out_put_dom; - } + if (!new_dom->hierarchy) + return ERR_PTR(-ENOMEM); + refcount_set(&new_dom->hierarchy->usage, 1); /* ...as a child of @parent... */ err = inherit_ruleset(parent, new_dom); if (err) - goto out_put_dom; + return ERR_PTR(err); /* ...and including @ruleset. */ err = merge_ruleset(new_dom, ruleset); if (err) - goto out_put_dom; - - return new_dom; + return ERR_PTR(err); -out_put_dom: - landlock_put_ruleset(new_dom); - return ERR_PTR(err); + return no_free_ptr(new_dom); } /* diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 631e24d4ffe96..70e5b53d1c71b 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -11,6 +11,8 @@ #include #include +#include +#include #include #include #include @@ -252,6 +254,9 @@ landlock_create_ruleset(const access_mask_t access_mask_fs, void landlock_put_ruleset(struct landlock_ruleset *const ruleset); void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset); +DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *, + if (!IS_ERR_OR_NULL(_T)) landlock_put_ruleset(_T)) + int landlock_insert_rule(struct landlock_ruleset *const ruleset, const struct landlock_id id, const access_mask_t access); diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 4ed8e70c25ede..5a7f1f77292ed 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -456,10 +457,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd, SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32, flags) { - struct landlock_ruleset *new_dom, *ruleset; + struct landlock_ruleset *new_dom, + *ruleset __free(landlock_put_ruleset) = NULL; struct cred *new_cred; struct landlock_cred_security *new_llcred; - int err; if (!is_initialized()) return -EOPNOTSUPP; @@ -483,10 +484,9 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32, /* Prepares new credentials. */ new_cred = prepare_creds(); - if (!new_cred) { - err = -ENOMEM; - goto out_put_ruleset; - } + if (!new_cred) + return -ENOMEM; + new_llcred = landlock_cred(new_cred); /* @@ -495,21 +495,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32, */ new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset); if (IS_ERR(new_dom)) { - err = PTR_ERR(new_dom); - goto out_put_creds; + abort_creds(new_cred); + return PTR_ERR(new_dom); } /* Replaces the old (prepared) domain. */ landlock_put_ruleset(new_llcred->domain); new_llcred->domain = new_dom; - - landlock_put_ruleset(ruleset); return commit_creds(new_cred); - -out_put_creds: - abort_creds(new_cred); - -out_put_ruleset: - landlock_put_ruleset(ruleset); - return err; } From 16a6f4d3b558bd55b52892f2becad8f33cb62ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Mon, 13 Jan 2025 17:11:10 +0100 Subject: [PATCH 06/16] landlock: Use scoped guards for ruleset in landlock_add_rule() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify error handling by replacing goto statements with automatic calls to landlock_put_ruleset() when going out of scope. This change depends on the TCP support. Cc: Konstantin Meskhidze Cc: Mikhail Ivanov Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20250113161112.452505-3-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/syscalls.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 5a7f1f77292ed..a9760d252fc2d 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -399,8 +399,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd, const enum landlock_rule_type, rule_type, const void __user *const, rule_attr, const __u32, flags) { - struct landlock_ruleset *ruleset; - int err; + struct landlock_ruleset *ruleset __free(landlock_put_ruleset) = NULL; if (!is_initialized()) return -EOPNOTSUPP; @@ -416,17 +415,12 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd, switch (rule_type) { case LANDLOCK_RULE_PATH_BENEATH: - err = add_rule_path_beneath(ruleset, rule_attr); - break; + return add_rule_path_beneath(ruleset, rule_attr); case LANDLOCK_RULE_NET_PORT: - err = add_rule_net_port(ruleset, rule_attr); - break; + return add_rule_net_port(ruleset, rule_attr); default: - err = -EINVAL; - break; + return -EINVAL; } - landlock_put_ruleset(ruleset); - return err; } /* Enforcement */ From 0e4db4f843c2c0115b5981bd6f6b75dea62e7d60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 15 Jan 2025 15:54:07 +0100 Subject: [PATCH 07/16] selftests/landlock: Fix build with non-default pthread linking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old toolchains require explicit -lpthread (e.g. on Debian 11). Cc: Nathan Chancellor Cc: Tahera Fahimi Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads") Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20250115145409.312226-1-mic@digikod.net Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile index 348e2dbdb4e0b..480f13e77fcc4 100644 --- a/tools/testing/selftests/landlock/Makefile +++ b/tools/testing/selftests/landlock/Makefile @@ -13,11 +13,11 @@ TEST_GEN_PROGS := $(src_test:.c=) TEST_GEN_PROGS_EXTENDED := true # Short targets: -$(TEST_GEN_PROGS): LDLIBS += -lcap +$(TEST_GEN_PROGS): LDLIBS += -lcap -lpthread $(TEST_GEN_PROGS_EXTENDED): LDFLAGS += -static include ../lib.mk # Targets with $(OUTPUT)/ prefix: -$(TEST_GEN_PROGS): LDLIBS += -lcap +$(TEST_GEN_PROGS): LDLIBS += -lcap -lpthread $(TEST_GEN_PROGS_EXTENDED): LDFLAGS += -static From 924f4403d869ad24bd2c54ad97ad87d4b838d09c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:11 +0100 Subject: [PATCH 08/16] landlock: Factor out check_access_path() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge check_access_path() into current_check_access_path() and make hook_path_mknod() use it. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-4-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index f81d0335b8257..4023354dd8e36 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -908,28 +908,22 @@ static bool is_access_to_paths_allowed( return allowed_parent1 && allowed_parent2; } -static int check_access_path(const struct landlock_ruleset *const domain, - const struct path *const path, - access_mask_t access_request) -{ - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; - - access_request = landlock_init_layer_masks( - domain, access_request, &layer_masks, LANDLOCK_KEY_INODE); - if (is_access_to_paths_allowed(domain, path, access_request, - &layer_masks, NULL, 0, NULL, NULL)) - return 0; - return -EACCES; -} - static int current_check_access_path(const struct path *const path, - const access_mask_t access_request) + access_mask_t access_request) { const struct landlock_ruleset *const dom = get_current_fs_domain(); + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; if (!dom) return 0; - return check_access_path(dom, path, access_request); + + access_request = landlock_init_layer_masks( + dom, access_request, &layer_masks, LANDLOCK_KEY_INODE); + if (is_access_to_paths_allowed(dom, path, access_request, &layer_masks, + NULL, 0, NULL, NULL)) + return 0; + + return -EACCES; } static __attribute_const__ access_mask_t get_mode_access(const umode_t mode) @@ -1413,11 +1407,7 @@ static int hook_path_mknod(const struct path *const dir, struct dentry *const dentry, const umode_t mode, const unsigned int dev) { - const struct landlock_ruleset *const dom = get_current_fs_domain(); - - if (!dom) - return 0; - return check_access_path(dom, dir, get_mode_access(mode)); + return current_check_access_path(dir, get_mode_access(mode)); } static int hook_path_symlink(const struct path *const dir, From 622e2f5954763385c4fa1f9a11a11366952a9b60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:13 +0100 Subject: [PATCH 09/16] landlock: Move access types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move LANDLOCK_ACCESS_FS_INITIALLY_DENIED, access_mask_t, struct access_mask, and struct access_masks_all to a dedicated access.h file. Rename LANDLOCK_ACCESS_FS_INITIALLY_DENIED to _LANDLOCK_ACCESS_FS_INITIALLY_DENIED to make it clear that it's not part of UAPI. Add some newlines when appropriate. This file will be extended with following commits, and it will help to avoid dependency loops. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-6-mic@digikod.net [mic: Fix rebase conflict because of the new cleanup headers] Signed-off-by: Mickaël Salaün --- security/landlock/access.h | 62 +++++++++++++++++++++++++++++++++++++ security/landlock/fs.c | 3 +- security/landlock/fs.h | 1 + security/landlock/ruleset.c | 1 + security/landlock/ruleset.h | 47 ++-------------------------- 5 files changed, 68 insertions(+), 46 deletions(-) create mode 100644 security/landlock/access.h diff --git a/security/landlock/access.h b/security/landlock/access.h new file mode 100644 index 0000000000000..9ee4b30a87e67 --- /dev/null +++ b/security/landlock/access.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Landlock LSM - Access types and helpers + * + * Copyright © 2016-2020 Mickaël Salaün + * Copyright © 2018-2020 ANSSI + * Copyright © 2024-2025 Microsoft Corporation + */ + +#ifndef _SECURITY_LANDLOCK_ACCESS_H +#define _SECURITY_LANDLOCK_ACCESS_H + +#include +#include +#include +#include + +#include "limits.h" + +/* + * All access rights that are denied by default whether they are handled or not + * by a ruleset/layer. This must be ORed with all ruleset->access_masks[] + * entries when we need to get the absolute handled access masks. + */ +/* clang-format off */ +#define _LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \ + LANDLOCK_ACCESS_FS_REFER) +/* clang-format on */ + +typedef u16 access_mask_t; + +/* Makes sure all filesystem access rights can be stored. */ +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); +/* Makes sure all network access rights can be stored. */ +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET); +/* Makes sure all scoped rights can be stored. */ +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE); +/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */ +static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); + +/* Ruleset access masks. */ +struct access_masks { + access_mask_t fs : LANDLOCK_NUM_ACCESS_FS; + access_mask_t net : LANDLOCK_NUM_ACCESS_NET; + access_mask_t scope : LANDLOCK_NUM_SCOPE; +}; + +union access_masks_all { + struct access_masks masks; + u32 all; +}; + +/* Makes sure all fields are covered. */ +static_assert(sizeof(typeof_member(union access_masks_all, masks)) == + sizeof(typeof_member(union access_masks_all, all))); + +typedef u16 layer_mask_t; + +/* Makes sure all layers can be checked. */ +static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS); + +#endif /* _SECURITY_LANDLOCK_ACCESS_H */ diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 4023354dd8e36..e323f7fb5a98a 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -36,6 +36,7 @@ #include #include +#include "access.h" #include "common.h" #include "cred.h" #include "fs.h" @@ -393,7 +394,7 @@ get_handled_fs_accesses(const struct landlock_ruleset *const domain) { /* Handles all initially denied by default access rights. */ return landlock_union_access_masks(domain).fs | - LANDLOCK_ACCESS_FS_INITIALLY_DENIED; + _LANDLOCK_ACCESS_FS_INITIALLY_DENIED; } static const struct access_masks any_fs = { diff --git a/security/landlock/fs.h b/security/landlock/fs.h index 1487e1f023a19..d445f411c26a1 100644 --- a/security/landlock/fs.h +++ b/security/landlock/fs.h @@ -13,6 +13,7 @@ #include #include +#include "access.h" #include "ruleset.h" #include "setup.h" diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index f27b7bdb19b92..4aeab215d7c53 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -22,6 +22,7 @@ #include #include +#include "access.h" #include "limits.h" #include "object.h" #include "ruleset.h" diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 70e5b53d1c71b..52f4f0af6ab07 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -9,60 +9,17 @@ #ifndef _SECURITY_LANDLOCK_RULESET_H #define _SECURITY_LANDLOCK_RULESET_H -#include -#include #include #include -#include #include #include #include #include -#include +#include "access.h" #include "limits.h" #include "object.h" -/* - * All access rights that are denied by default whether they are handled or not - * by a ruleset/layer. This must be ORed with all ruleset->access_masks[] - * entries when we need to get the absolute handled access masks. - */ -/* clang-format off */ -#define LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \ - LANDLOCK_ACCESS_FS_REFER) -/* clang-format on */ - -typedef u16 access_mask_t; -/* Makes sure all filesystem access rights can be stored. */ -static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); -/* Makes sure all network access rights can be stored. */ -static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET); -/* Makes sure all scoped rights can be stored. */ -static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE); -/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */ -static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); - -/* Ruleset access masks. */ -struct access_masks { - access_mask_t fs : LANDLOCK_NUM_ACCESS_FS; - access_mask_t net : LANDLOCK_NUM_ACCESS_NET; - access_mask_t scope : LANDLOCK_NUM_SCOPE; -}; - -union access_masks_all { - struct access_masks masks; - u32 all; -}; - -/* Makes sure all fields are covered. */ -static_assert(sizeof(typeof_member(union access_masks_all, masks)) == - sizeof(typeof_member(union access_masks_all, all))); - -typedef u16 layer_mask_t; -/* Makes sure all layers can be checked. */ -static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS); - /** * struct landlock_layer - Access rights for a given layer */ @@ -371,7 +328,7 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset, { /* Handles all initially denied by default access rights. */ return ruleset->access_masks[layer_level].fs | - LANDLOCK_ACCESS_FS_INITIALLY_DENIED; + _LANDLOCK_ACCESS_FS_INITIALLY_DENIED; } static inline access_mask_t From d6c7cf84a24fff332ff65ffe236302216474b834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:14 +0100 Subject: [PATCH 10/16] landlock: Simplify initially denied access rights MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upgrade domain's handled access masks when creating a domain from a ruleset, instead of converting them at runtime. This is more consistent and helps with audit support. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-7-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/access.h | 17 ++++++++++++++++- security/landlock/fs.c | 10 +--------- security/landlock/ruleset.c | 3 ++- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/security/landlock/access.h b/security/landlock/access.h index 9ee4b30a87e67..74fd8f399fbd6 100644 --- a/security/landlock/access.h +++ b/security/landlock/access.h @@ -20,7 +20,8 @@ /* * All access rights that are denied by default whether they are handled or not * by a ruleset/layer. This must be ORed with all ruleset->access_masks[] - * entries when we need to get the absolute handled access masks. + * entries when we need to get the absolute handled access masks, see + * landlock_upgrade_handled_access_masks(). */ /* clang-format off */ #define _LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \ @@ -59,4 +60,18 @@ typedef u16 layer_mask_t; /* Makes sure all layers can be checked. */ static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS); +/* Upgrades with all initially denied by default access rights. */ +static inline struct access_masks +landlock_upgrade_handled_access_masks(struct access_masks access_masks) +{ + /* + * All access rights that are denied by default whether they are + * explicitly handled or not. + */ + if (access_masks.fs) + access_masks.fs |= _LANDLOCK_ACCESS_FS_INITIALLY_DENIED; + + return access_masks; +} + #endif /* _SECURITY_LANDLOCK_ACCESS_H */ diff --git a/security/landlock/fs.c b/security/landlock/fs.c index e323f7fb5a98a..4eb972f2292ff 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -389,14 +389,6 @@ static bool is_nouser_or_private(const struct dentry *dentry) unlikely(IS_PRIVATE(d_backing_inode(dentry)))); } -static access_mask_t -get_handled_fs_accesses(const struct landlock_ruleset *const domain) -{ - /* Handles all initially denied by default access rights. */ - return landlock_union_access_masks(domain).fs | - _LANDLOCK_ACCESS_FS_INITIALLY_DENIED; -} - static const struct access_masks any_fs = { .fs = ~0, }; @@ -788,7 +780,7 @@ static bool is_access_to_paths_allowed( * a superset of the meaningful requested accesses). */ access_masked_parent1 = access_masked_parent2 = - get_handled_fs_accesses(domain); + landlock_union_access_masks(domain).fs; is_dom_check = true; } else { if (WARN_ON_ONCE(dentry_child1 || dentry_child2)) diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index 4aeab215d7c53..241ce44375b6a 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -387,7 +387,8 @@ static int merge_ruleset(struct landlock_ruleset *const dst, err = -EINVAL; goto out_unlock; } - dst->access_masks[dst->num_layers - 1] = src->access_masks[0]; + dst->access_masks[dst->num_layers - 1] = + landlock_upgrade_handled_access_masks(src->access_masks[0]); /* Merges the @src inode tree. */ err = merge_tree(dst, src, LANDLOCK_KEY_INODE); From 058518c2092081f224edb37cbc236bed5c28852d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:19 +0100 Subject: [PATCH 11/16] landlock: Align partial refer access checks with final ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a logical issue that could have been visible if the source or the destination of a rename/link action was allowed for either the source or the destination but not both. However, this logical bug is unreachable because either: - the rename/link action is allowed by the access rights tied to the same mount point (without relying on access rights in a parent mount point) and the access request is allowed (i.e. allow_parent1 and allow_parent2 are true in current_check_refer_path), - or a common rule in a parent mount point updates the access check for the source and the destination (cf. is_access_to_paths_allowed). See the following layout1.refer_part_mount_tree_is_allowed test that work with and without this fix. This fix does not impact current code but it is required for the audit support. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-12-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 4eb972f2292ff..110b8cfaab9c1 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -565,6 +565,12 @@ static void test_no_more_access(struct kunit *const test) #undef NMA_TRUE #undef NMA_FALSE +static bool is_layer_masks_allowed( + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]) +{ + return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); +} + /* * Removes @layer_masks accesses that are not requested. * @@ -582,7 +588,8 @@ scope_to_request(const access_mask_t access_request, for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) (*layer_masks)[access_bit] = 0; - return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); + + return is_layer_masks_allowed(layer_masks); } #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -771,9 +778,14 @@ static bool is_access_to_paths_allowed( if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1)) return false; + allowed_parent1 = is_layer_masks_allowed(layer_masks_parent1); + if (unlikely(layer_masks_parent2)) { if (WARN_ON_ONCE(!dentry_child1)) return false; + + allowed_parent2 = is_layer_masks_allowed(layer_masks_parent2); + /* * For a double request, first check for potential privilege * escalation by looking at domain handled accesses (which are From 12264f721f64a235f81e845e2cf95ad4a267613a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:20 +0100 Subject: [PATCH 12/16] selftests/landlock: Add test to check partial access in a mount tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add layout1.refer_part_mount_tree_is_allowed to test the masked logical issue regarding collect_domain_accesses() calls followed by the is_access_to_paths_allowed() check in current_check_refer_path(). See previous commit. This test should work without the previous fix as well, but it enables us to make sure future changes will not have impact regarding this behavior. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-13-mic@digikod.net Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/fs_test.c | 54 ++++++++++++++++++++-- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 6788762188fea..42ce1e79ba828 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -85,6 +85,9 @@ static const char file1_s3d1[] = TMP_DIR "/s3d1/f1"; /* dir_s3d2 is a mount point. */ static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2"; static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; +static const char file1_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3/f1"; +static const char dir_s3d4[] = TMP_DIR "/s3d1/s3d2/s3d4"; +static const char file1_s3d4[] = TMP_DIR "/s3d1/s3d2/s3d4/f1"; /* * layout1 hierarchy: @@ -108,8 +111,11 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; * │   └── f2 * └── s3d1 *    ├── f1 - * └── s3d2 - * └── s3d3 + * └── s3d2 [mount point] + *    ├── s3d3 + *    │ └── f1 + *    └── s3d4 + *    └── f1 */ static bool fgrep(FILE *const inf, const char *const str) @@ -358,7 +364,8 @@ static void create_layout1(struct __test_metadata *const _metadata) ASSERT_EQ(0, mount_opt(&mnt_tmp, dir_s3d2)); clear_cap(_metadata, CAP_SYS_ADMIN); - ASSERT_EQ(0, mkdir(dir_s3d3, 0700)); + create_file(_metadata, file1_s3d3); + create_file(_metadata, file1_s3d4); } static void remove_layout1(struct __test_metadata *const _metadata) @@ -378,7 +385,8 @@ static void remove_layout1(struct __test_metadata *const _metadata) EXPECT_EQ(0, remove_path(dir_s2d2)); EXPECT_EQ(0, remove_path(file1_s3d1)); - EXPECT_EQ(0, remove_path(dir_s3d3)); + EXPECT_EQ(0, remove_path(file1_s3d3)); + EXPECT_EQ(0, remove_path(file1_s3d4)); set_cap(_metadata, CAP_SYS_ADMIN); umount(dir_s3d2); clear_cap(_metadata, CAP_SYS_ADMIN); @@ -2444,6 +2452,44 @@ TEST_F_FORK(layout1, refer_mount_root_deny) EXPECT_EQ(0, close(root_fd)); } +TEST_F_FORK(layout1, refer_part_mount_tree_is_allowed) +{ + const struct rule layer1[] = { + { + /* Parent mount point. */ + .path = dir_s3d1, + .access = LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_MAKE_REG, + }, + { + /* + * Removing the source file is allowed because its + * access rights are already a superset of the + * destination. + */ + .path = dir_s3d4, + .access = LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_MAKE_REG | + LANDLOCK_ACCESS_FS_REMOVE_FILE, + }, + {}, + }; + int ruleset_fd; + + ASSERT_EQ(0, unlink(file1_s3d3)); + ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_MAKE_REG | + LANDLOCK_ACCESS_FS_REMOVE_FILE, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + ASSERT_EQ(0, rename(file1_s3d4, file1_s3d3)); +} + TEST_F_FORK(layout1, reparent_link) { const struct rule layer1[] = { From d617f0d72d8041c7099fd04a62db0f0fa5331c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:21 +0100 Subject: [PATCH 13/16] landlock: Optimize file path walks and prepare for audit support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Always synchronize access_masked_parent* with access_request_parent* according to allowed_parent*. This is required for audit support to be able to get back to the reason of denial. In a rename/link action, instead of always checking a rule two times for the same parent directory of the source and the destination files, only check it when an action on a child was not already allowed. This also enables us to keep consistent allowed_parent* status, which is required to get back to the reason of denial. For internal mount points, only upgrade allowed_parent* to true but do not wrongfully set both of them to false otherwise. This is also required to get back to the reason of denial. This does not impact the current behavior but slightly optimize code and prepare for audit support that needs to know the exact reason why an access was denied. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-14-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 44 ++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 110b8cfaab9c1..71b9dc331aae8 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -852,15 +852,6 @@ static bool is_access_to_paths_allowed( child1_is_directory, layer_masks_parent2, layer_masks_child2, child2_is_directory))) { - allowed_parent1 = scope_to_request( - access_request_parent1, layer_masks_parent1); - allowed_parent2 = scope_to_request( - access_request_parent2, layer_masks_parent2); - - /* Stops when all accesses are granted. */ - if (allowed_parent1 && allowed_parent2) - break; - /* * Now, downgrades the remaining checks from domain * handled accesses to requested accesses. @@ -868,15 +859,32 @@ static bool is_access_to_paths_allowed( is_dom_check = false; access_masked_parent1 = access_request_parent1; access_masked_parent2 = access_request_parent2; + + allowed_parent1 = + allowed_parent1 || + scope_to_request(access_masked_parent1, + layer_masks_parent1); + allowed_parent2 = + allowed_parent2 || + scope_to_request(access_masked_parent2, + layer_masks_parent2); + + /* Stops when all accesses are granted. */ + if (allowed_parent1 && allowed_parent2) + break; } rule = find_rule(domain, walker_path.dentry); - allowed_parent1 = landlock_unmask_layers( - rule, access_masked_parent1, layer_masks_parent1, - ARRAY_SIZE(*layer_masks_parent1)); - allowed_parent2 = landlock_unmask_layers( - rule, access_masked_parent2, layer_masks_parent2, - ARRAY_SIZE(*layer_masks_parent2)); + allowed_parent1 = allowed_parent1 || + landlock_unmask_layers( + rule, access_masked_parent1, + layer_masks_parent1, + ARRAY_SIZE(*layer_masks_parent1)); + allowed_parent2 = allowed_parent2 || + landlock_unmask_layers( + rule, access_masked_parent2, + layer_masks_parent2, + ARRAY_SIZE(*layer_masks_parent2)); /* Stops when a rule from each layer grants access. */ if (allowed_parent1 && allowed_parent2) @@ -900,8 +908,10 @@ static bool is_access_to_paths_allowed( * access to internal filesystems (e.g. nsfs, which is * reachable through /proc//ns/). */ - allowed_parent1 = allowed_parent2 = - !!(walker_path.mnt->mnt_flags & MNT_INTERNAL); + if (walker_path.mnt->mnt_flags & MNT_INTERNAL) { + allowed_parent1 = true; + allowed_parent2 = true; + } break; } parent_dentry = dget_parent(walker_path.dentry); From 2107c35128ad751b201eb92fe91443450d9e5c37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:28 +0100 Subject: [PATCH 14/16] selftests/landlock: Fix error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The global variable errno may not be set in test_execute(). Do not use it in related error message. Cc: Günther Noack Fixes: e1199815b47b ("selftests/landlock: Add user space tests") Link: https://lore.kernel.org/r/20250108154338.1129069-21-mic@digikod.net Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/fs_test.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 42ce1e79ba828..a359c0d3107f2 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -2011,8 +2011,7 @@ static void test_execute(struct __test_metadata *const _metadata, const int err, ASSERT_EQ(1, WIFEXITED(status)); ASSERT_EQ(err ? 2 : 0, WEXITSTATUS(status)) { - TH_LOG("Unexpected return code for \"%s\": %s", path, - strerror(errno)); + TH_LOG("Unexpected return code for \"%s\"", path); }; } From 5147779d5e1b6425f30eb57071717be2bb65fa3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:29 +0100 Subject: [PATCH 15/16] selftests/landlock: Add wrappers.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract syscall wrappers to make them usable by standalone binaries (see next commit). Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-22-mic@digikod.net [mic: Fix comments] Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/common.h | 37 +--------------- tools/testing/selftests/landlock/wrappers.h | 47 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 36 deletions(-) create mode 100644 tools/testing/selftests/landlock/wrappers.h diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h index 61056fa074bb2..8391ab574f641 100644 --- a/tools/testing/selftests/landlock/common.h +++ b/tools/testing/selftests/landlock/common.h @@ -9,17 +9,15 @@ #include #include -#include #include #include #include -#include -#include #include #include #include #include "../kselftest_harness.h" +#include "wrappers.h" #define TMP_DIR "tmp" @@ -30,34 +28,6 @@ /* TEST_F_FORK() should not be used for new tests. */ #define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name) -#ifndef landlock_create_ruleset -static inline int -landlock_create_ruleset(const struct landlock_ruleset_attr *const attr, - const size_t size, const __u32 flags) -{ - return syscall(__NR_landlock_create_ruleset, attr, size, flags); -} -#endif - -#ifndef landlock_add_rule -static inline int landlock_add_rule(const int ruleset_fd, - const enum landlock_rule_type rule_type, - const void *const rule_attr, - const __u32 flags) -{ - return syscall(__NR_landlock_add_rule, ruleset_fd, rule_type, rule_attr, - flags); -} -#endif - -#ifndef landlock_restrict_self -static inline int landlock_restrict_self(const int ruleset_fd, - const __u32 flags) -{ - return syscall(__NR_landlock_restrict_self, ruleset_fd, flags); -} -#endif - static void _init_caps(struct __test_metadata *const _metadata, bool drop_all) { cap_t cap_p; @@ -250,11 +220,6 @@ struct service_fixture { }; }; -static pid_t __maybe_unused sys_gettid(void) -{ - return syscall(__NR_gettid); -} - static void __maybe_unused set_unix_address(struct service_fixture *const srv, const unsigned short index) { diff --git a/tools/testing/selftests/landlock/wrappers.h b/tools/testing/selftests/landlock/wrappers.h new file mode 100644 index 0000000000000..65548323e45d9 --- /dev/null +++ b/tools/testing/selftests/landlock/wrappers.h @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Syscall wrappers + * + * Copyright © 2017-2020 Mickaël Salaün + * Copyright © 2019-2020 ANSSI + * Copyright © 2021-2025 Microsoft Corporation + */ + +#define _GNU_SOURCE +#include +#include +#include +#include + +#ifndef landlock_create_ruleset +static inline int +landlock_create_ruleset(const struct landlock_ruleset_attr *const attr, + const size_t size, const __u32 flags) +{ + return syscall(__NR_landlock_create_ruleset, attr, size, flags); +} +#endif + +#ifndef landlock_add_rule +static inline int landlock_add_rule(const int ruleset_fd, + const enum landlock_rule_type rule_type, + const void *const rule_attr, + const __u32 flags) +{ + return syscall(__NR_landlock_add_rule, ruleset_fd, rule_type, rule_attr, + flags); +} +#endif + +#ifndef landlock_restrict_self +static inline int landlock_restrict_self(const int ruleset_fd, + const __u32 flags) +{ + return syscall(__NR_landlock_restrict_self, ruleset_fd, flags); +} +#endif + +static inline pid_t sys_gettid(void) +{ + return syscall(__NR_gettid); +} From 2a794ee613617b5d8fd978b7ef08d64aa07ff2e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 8 Jan 2025 16:43:30 +0100 Subject: [PATCH 16/16] selftests/landlock: Add layout1.umount_sandboxer tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check that a domain is not tied to the executable file that created it. For instance, that could happen if a Landlock domain took a reference to a struct path. Move global path names to common.h and replace copy_binary() with a more generic copy_file() helper. Test coverage for security/landlock is 92.7% of 1133 lines according to gcc/gcov-14. Cc: Günther Noack Link: https://lore.kernel.org/r/20250108154338.1129069-23-mic@digikod.net [mic: Update date and add test coverage] Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/Makefile | 2 +- tools/testing/selftests/landlock/common.h | 3 + tools/testing/selftests/landlock/fs_test.c | 94 +++++++++++++++++-- .../selftests/landlock/sandbox-and-launch.c | 82 ++++++++++++++++ tools/testing/selftests/landlock/wait-pipe.c | 42 +++++++++ 5 files changed, 213 insertions(+), 10 deletions(-) create mode 100644 tools/testing/selftests/landlock/sandbox-and-launch.c create mode 100644 tools/testing/selftests/landlock/wait-pipe.c diff --git a/tools/testing/selftests/landlock/Makefile b/tools/testing/selftests/landlock/Makefile index 480f13e77fcc4..5cb0828f05141 100644 --- a/tools/testing/selftests/landlock/Makefile +++ b/tools/testing/selftests/landlock/Makefile @@ -10,7 +10,7 @@ src_test := $(wildcard *_test.c) TEST_GEN_PROGS := $(src_test:.c=) -TEST_GEN_PROGS_EXTENDED := true +TEST_GEN_PROGS_EXTENDED := true sandbox-and-launch wait-pipe # Short targets: $(TEST_GEN_PROGS): LDLIBS += -lcap -lpthread diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h index 8391ab574f641..a604ea5d8297c 100644 --- a/tools/testing/selftests/landlock/common.h +++ b/tools/testing/selftests/landlock/common.h @@ -28,6 +28,9 @@ /* TEST_F_FORK() should not be used for new tests. */ #define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name) +static const char bin_sandbox_and_launch[] = "./sandbox-and-launch"; +static const char bin_wait_pipe[] = "./wait-pipe"; + static void _init_caps(struct __test_metadata *const _metadata, bool drop_all) { cap_t cap_p; diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index a359c0d3107f2..8ac9aaf38eaa8 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -59,7 +59,7 @@ int open_tree(int dfd, const char *filename, unsigned int flags) #define RENAME_EXCHANGE (1 << 1) #endif -#define BINARY_PATH "./true" +static const char bin_true[] = "./true"; /* Paths (sibling number and depth) */ static const char dir_s1d1[] = TMP_DIR "/s1d1"; @@ -1965,8 +1965,8 @@ TEST_F_FORK(layout1, relative_chroot_chdir) test_relative_path(_metadata, REL_CHROOT_CHDIR); } -static void copy_binary(struct __test_metadata *const _metadata, - const char *const dst_path) +static void copy_file(struct __test_metadata *const _metadata, + const char *const src_path, const char *const dst_path) { int dst_fd, src_fd; struct stat statbuf; @@ -1976,11 +1976,10 @@ static void copy_binary(struct __test_metadata *const _metadata, { TH_LOG("Failed to open \"%s\": %s", dst_path, strerror(errno)); } - src_fd = open(BINARY_PATH, O_RDONLY | O_CLOEXEC); + src_fd = open(src_path, O_RDONLY | O_CLOEXEC); ASSERT_LE(0, src_fd) { - TH_LOG("Failed to open \"" BINARY_PATH "\": %s", - strerror(errno)); + TH_LOG("Failed to open \"%s\": %s", src_path, strerror(errno)); } ASSERT_EQ(0, fstat(src_fd, &statbuf)); ASSERT_EQ(statbuf.st_size, @@ -2028,9 +2027,9 @@ TEST_F_FORK(layout1, execute) create_ruleset(_metadata, rules[0].access, rules); ASSERT_LE(0, ruleset_fd); - copy_binary(_metadata, file1_s1d1); - copy_binary(_metadata, file1_s1d2); - copy_binary(_metadata, file1_s1d3); + copy_file(_metadata, bin_true, file1_s1d1); + copy_file(_metadata, bin_true, file1_s1d2); + copy_file(_metadata, bin_true, file1_s1d3); enforce_ruleset(_metadata, ruleset_fd); ASSERT_EQ(0, close(ruleset_fd)); @@ -2048,6 +2047,83 @@ TEST_F_FORK(layout1, execute) test_execute(_metadata, 0, file1_s1d3); } +TEST_F_FORK(layout1, umount_sandboxer) +{ + int pipe_child[2], pipe_parent[2]; + char buf_parent; + pid_t child; + int status; + + copy_file(_metadata, bin_sandbox_and_launch, file1_s3d3); + ASSERT_EQ(0, pipe2(pipe_child, 0)); + ASSERT_EQ(0, pipe2(pipe_parent, 0)); + + child = fork(); + ASSERT_LE(0, child); + if (child == 0) { + char pipe_child_str[12], pipe_parent_str[12]; + char *const argv[] = { (char *)file1_s3d3, + (char *)bin_wait_pipe, pipe_child_str, + pipe_parent_str, NULL }; + + /* Passes the pipe FDs to the executed binary and its child. */ + EXPECT_EQ(0, close(pipe_child[0])); + EXPECT_EQ(0, close(pipe_parent[1])); + snprintf(pipe_child_str, sizeof(pipe_child_str), "%d", + pipe_child[1]); + snprintf(pipe_parent_str, sizeof(pipe_parent_str), "%d", + pipe_parent[0]); + + /* + * We need bin_sandbox_and_launch (copied inside the mount as + * file1_s3d3) to execute bin_wait_pipe (outside the mount) to + * make sure the mount point will not be EBUSY because of + * file1_s3d3 being in use. This avoids a potential race + * condition between the following read() and umount() calls. + */ + ASSERT_EQ(0, execve(argv[0], argv, NULL)) + { + TH_LOG("Failed to execute \"%s\": %s", argv[0], + strerror(errno)); + }; + _exit(1); + return; + } + + EXPECT_EQ(0, close(pipe_child[1])); + EXPECT_EQ(0, close(pipe_parent[0])); + + /* Waits for the child to sandbox itself. */ + EXPECT_EQ(1, read(pipe_child[0], &buf_parent, 1)); + + /* Tests that the sandboxer is tied to its mount point. */ + set_cap(_metadata, CAP_SYS_ADMIN); + EXPECT_EQ(-1, umount(dir_s3d2)); + EXPECT_EQ(EBUSY, errno); + clear_cap(_metadata, CAP_SYS_ADMIN); + + /* Signals the child to launch a grandchild. */ + EXPECT_EQ(1, write(pipe_parent[1], ".", 1)); + + /* Waits for the grandchild. */ + EXPECT_EQ(1, read(pipe_child[0], &buf_parent, 1)); + + /* Tests that the domain's sandboxer is not tied to its mount point. */ + set_cap(_metadata, CAP_SYS_ADMIN); + EXPECT_EQ(0, umount(dir_s3d2)) + { + TH_LOG("Failed to umount \"%s\": %s", dir_s3d2, + strerror(errno)); + }; + clear_cap(_metadata, CAP_SYS_ADMIN); + + /* Signals the grandchild to terminate. */ + EXPECT_EQ(1, write(pipe_parent[1], ".", 1)); + ASSERT_EQ(child, waitpid(child, &status, 0)); + ASSERT_EQ(1, WIFEXITED(status)); + ASSERT_EQ(0, WEXITSTATUS(status)); +} + TEST_F_FORK(layout1, link) { const struct rule layer1[] = { diff --git a/tools/testing/selftests/landlock/sandbox-and-launch.c b/tools/testing/selftests/landlock/sandbox-and-launch.c new file mode 100644 index 0000000000000..3e32e1a51ac53 --- /dev/null +++ b/tools/testing/selftests/landlock/sandbox-and-launch.c @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Sandbox itself and execute another program (in a different mount point). + * + * Used by layout1.umount_sandboxer from fs_test.c + * + * Copyright © 2024-2025 Microsoft Corporation + */ + +#define _GNU_SOURCE +#include +#include +#include +#include +#include + +#include "wrappers.h" + +int main(int argc, char *argv[]) +{ + struct landlock_ruleset_attr ruleset_attr = { + .scoped = LANDLOCK_SCOPE_SIGNAL, + }; + int pipe_child, pipe_parent, ruleset_fd; + char buf; + + /* + * The first argument must be the file descriptor number of a pipe. + * The second argument must be the program to execute. + */ + if (argc != 4) { + fprintf(stderr, "Wrong number of arguments (not three)\n"); + return 1; + } + + pipe_child = atoi(argv[2]); + pipe_parent = atoi(argv[3]); + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + if (ruleset_fd < 0) { + perror("Failed to create ruleset"); + return 1; + } + + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { + perror("Failed to call prctl()"); + return 1; + } + + if (landlock_restrict_self(ruleset_fd, 0)) { + perror("Failed to restrict self"); + return 1; + } + + if (close(ruleset_fd)) { + perror("Failed to close ruleset"); + return 1; + } + + /* Signals that we are sandboxed. */ + errno = 0; + if (write(pipe_child, ".", 1) != 1) { + perror("Failed to write to the second argument"); + return 1; + } + + /* Waits for the parent to try to umount. */ + if (read(pipe_parent, &buf, 1) != 1) { + perror("Failed to write to the third argument"); + return 1; + } + + /* Shifts arguments. */ + argv[0] = argv[1]; + argv[1] = argv[2]; + argv[2] = argv[3]; + argv[3] = NULL; + execve(argv[0], argv, NULL); + perror("Failed to execute the provided binary"); + return 1; +} diff --git a/tools/testing/selftests/landlock/wait-pipe.c b/tools/testing/selftests/landlock/wait-pipe.c new file mode 100644 index 0000000000000..0dbcd260a0faf --- /dev/null +++ b/tools/testing/selftests/landlock/wait-pipe.c @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Write in a pipe and wait. + * + * Used by layout1.umount_sandboxer from fs_test.c + * + * Copyright © 2024-2025 Microsoft Corporation + */ + +#define _GNU_SOURCE +#include +#include +#include + +int main(int argc, char *argv[]) +{ + int pipe_child, pipe_parent; + char buf; + + /* The first argument must be the file descriptor number of a pipe. */ + if (argc != 3) { + fprintf(stderr, "Wrong number of arguments (not two)\n"); + return 1; + } + + pipe_child = atoi(argv[1]); + pipe_parent = atoi(argv[2]); + + /* Signals that we are waiting. */ + if (write(pipe_child, ".", 1) != 1) { + perror("Failed to write to first argument"); + return 1; + } + + /* Waits for the parent do its test. */ + if (read(pipe_parent, &buf, 1) != 1) { + perror("Failed to write to the second argument"); + return 1; + } + + return 0; +}