From 6471c9c4c4d26f85d89e1493c0d51fb6f2b6f273 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Thu, 30 Nov 2023 10:36:15 +0100 Subject: [PATCH 1/7] selftests/landlock: Add tests to check unknown rule's access rights MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two tests to make sure that we cannot add a rule with access rights that are unknown: * fs: layout0.rule_with_unknown_access * net: mini.rule_with_unknown_access Rename unknown_access_rights tests to ruleset_with_unknown_access . Cc: Konstantin Meskhidze Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20231130093616.67340-2-mic@digikod.net Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/fs_test.c | 29 ++++++++++++++++++++- tools/testing/selftests/landlock/net_test.c | 27 ++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 18e1f86a6234..1e6c474e3d08 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -589,7 +589,7 @@ TEST_F_FORK(layout1, file_and_dir_access_rights) ASSERT_EQ(0, close(ruleset_fd)); } -TEST_F_FORK(layout0, unknown_access_rights) +TEST_F_FORK(layout0, ruleset_with_unknown_access) { __u64 access_mask; @@ -605,6 +605,33 @@ TEST_F_FORK(layout0, unknown_access_rights) } } +TEST_F_FORK(layout0, rule_with_unknown_access) +{ + __u64 access; + struct landlock_path_beneath_attr path_beneath = {}; + const struct landlock_ruleset_attr ruleset_attr = { + .handled_access_fs = ACCESS_ALL, + }; + const int ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + + ASSERT_LE(0, ruleset_fd); + + path_beneath.parent_fd = + open(TMP_DIR, O_PATH | O_DIRECTORY | O_CLOEXEC); + ASSERT_LE(0, path_beneath.parent_fd); + + for (access = 1ULL << 63; access != ACCESS_LAST; access >>= 1) { + path_beneath.allowed_access = access; + EXPECT_EQ(-1, landlock_add_rule(ruleset_fd, + LANDLOCK_RULE_PATH_BENEATH, + &path_beneath, 0)); + EXPECT_EQ(EINVAL, errno); + } + ASSERT_EQ(0, close(path_beneath.parent_fd)); + ASSERT_EQ(0, close(ruleset_fd)); +} + static void add_path_beneath(struct __test_metadata *const _metadata, const int ruleset_fd, const __u64 allowed_access, const char *const path) diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index 929e21c4db05..83d9abc3ee55 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -1260,7 +1260,7 @@ TEST_F(mini, network_access_rights) } /* Checks invalid attribute, out of landlock network access range. */ -TEST_F(mini, unknown_access_rights) +TEST_F(mini, ruleset_with_unknown_access) { __u64 access_mask; @@ -1276,6 +1276,31 @@ TEST_F(mini, unknown_access_rights) } } +TEST_F(mini, rule_with_unknown_access) +{ + const struct landlock_ruleset_attr ruleset_attr = { + .handled_access_net = ACCESS_ALL, + }; + struct landlock_net_port_attr net_port = { + .port = sock_port_start, + }; + int ruleset_fd; + __u64 access; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + for (access = 1ULL << 63; access != ACCESS_LAST; access >>= 1) { + net_port.allowed_access = access; + EXPECT_EQ(-1, + landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, + &net_port, 0)); + EXPECT_EQ(EINVAL, errno); + } + EXPECT_EQ(0, close(ruleset_fd)); +} + TEST_F(mini, inval) { const struct landlock_ruleset_attr ruleset_attr = { From e2780a0b95a1b5d137ccf0e0747b77f174f55511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Thu, 30 Nov 2023 10:36:16 +0100 Subject: [PATCH 2/7] selftests/landlock: Add tests to check unhandled rule's access rights MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two tests to make sure that we cannot add a rule to a ruleset if the rule's access rights that are not handled by the ruleset: * fs: layout1.rule_with_unhandled_access * net: mini.rule_with_unhandled_access Cc: Konstantin Meskhidze Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20231130093616.67340-3-mic@digikod.net Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/fs_test.c | 34 +++++++++++++++++++++ tools/testing/selftests/landlock/net_test.c | 32 +++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 1e6c474e3d08..a1d17ab527ae 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -632,6 +632,40 @@ TEST_F_FORK(layout0, rule_with_unknown_access) ASSERT_EQ(0, close(ruleset_fd)); } +TEST_F_FORK(layout1, rule_with_unhandled_access) +{ + struct landlock_ruleset_attr ruleset_attr = { + .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE, + }; + struct landlock_path_beneath_attr path_beneath = {}; + int ruleset_fd; + __u64 access; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC); + ASSERT_LE(0, path_beneath.parent_fd); + + for (access = 1; access > 0; access <<= 1) { + int err; + + path_beneath.allowed_access = access; + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, + &path_beneath, 0); + if (access == ruleset_attr.handled_access_fs) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + } + } + + EXPECT_EQ(0, close(path_beneath.parent_fd)); + EXPECT_EQ(0, close(ruleset_fd)); +} + static void add_path_beneath(struct __test_metadata *const _metadata, const int ruleset_fd, const __u64 allowed_access, const char *const path) diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index 83d9abc3ee55..ea5f727dd257 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -1301,6 +1301,38 @@ TEST_F(mini, rule_with_unknown_access) EXPECT_EQ(0, close(ruleset_fd)); } +TEST_F(mini, rule_with_unhandled_access) +{ + struct landlock_ruleset_attr ruleset_attr = { + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP, + }; + struct landlock_net_port_attr net_port = { + .port = sock_port_start, + }; + int ruleset_fd; + __u64 access; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + for (access = 1; access > 0; access <<= 1) { + int err; + + net_port.allowed_access = access; + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, + &net_port, 0); + if (access == ruleset_attr.handled_access_net) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + } + } + + EXPECT_EQ(0, close(ruleset_fd)); +} + TEST_F(mini, inval) { const struct landlock_ruleset_attr ruleset_attr = { From 8fd80721ec0791826a9ab56656d26931811702f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= Date: Fri, 8 Dec 2023 16:51:13 +0100 Subject: [PATCH 3/7] landlock: Remove remaining "inline" modifiers in .c files [v5.15] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For module-internal static functions, compilers are already in a good position to decide whether to inline them or not. Suggested-by: Mickaël Salaün Signed-off-by: Günther Noack Link: https://lore.kernel.org/r/20231208155121.1943775-2-gnoack@google.com [mic: Split patch for Linux 5.15] Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 6 +++--- security/landlock/ruleset.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index bc7c126deea2..8eccf170532e 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -193,7 +193,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, * * Returns NULL if no rule is found or if @dentry is negative. */ -static inline const struct landlock_rule * +static const struct landlock_rule * find_rule(const struct landlock_ruleset *const domain, const struct dentry *const dentry) { @@ -565,8 +565,8 @@ static inline int check_access_path(const struct landlock_ruleset *const domain, return -EACCES; } -static inline int current_check_access_path(const struct path *const path, - const access_mask_t access_request) +static int current_check_access_path(const struct path *const path, + const access_mask_t access_request) { const struct landlock_ruleset *const dom = get_current_fs_domain(); diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index ffedc99f2b68..789c81b26a50 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -305,7 +305,7 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset, return insert_rule(ruleset, id, &layers, ARRAY_SIZE(layers)); } -static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy) +static void get_hierarchy(struct landlock_hierarchy *const hierarchy) { if (hierarchy) refcount_inc(&hierarchy->usage); From da279087b9d9f288609380baf2b6bb89874769d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= Date: Fri, 8 Dec 2023 16:51:13 +0100 Subject: [PATCH 4/7] landlock: Remove remaining "inline" modifiers in .c files [v6.1] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For module-internal static functions, compilers are already in a good position to decide whether to inline them or not. Suggested-by: Mickaël Salaün Signed-off-by: Günther Noack Link: https://lore.kernel.org/r/20231208155121.1943775-2-gnoack@google.com [mic: Split patch for Linux 6.1] Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 8eccf170532e..b67990e8f32f 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -220,7 +220,7 @@ find_rule(const struct landlock_ruleset *const domain, * sockfs, pipefs), but can still be reachable through * /proc//fd/ */ -static inline bool is_nouser_or_private(const struct dentry *dentry) +static bool is_nouser_or_private(const struct dentry *dentry) { return (dentry->d_sb->s_flags & SB_NOUSER) || (d_is_positive(dentry) && @@ -264,7 +264,7 @@ static const struct landlock_ruleset *get_current_fs_domain(void) * * @layer_masks_child2: Optional child masks. */ -static inline bool no_more_access( +static bool no_more_access( const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS], const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS], const bool child1_is_directory, @@ -316,7 +316,7 @@ static inline bool no_more_access( * * Returns true if the request is allowed, false otherwise. */ -static inline bool +static bool scope_to_request(const access_mask_t access_request, layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]) { @@ -335,7 +335,7 @@ scope_to_request(const access_mask_t access_request, * Returns true if there is at least one access right different than * LANDLOCK_ACCESS_FS_REFER. */ -static inline bool +static bool is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS], const access_mask_t access_request) { @@ -551,9 +551,9 @@ jump_up: return allowed_parent1 && allowed_parent2; } -static inline int check_access_path(const struct landlock_ruleset *const domain, - const struct path *const path, - access_mask_t access_request) +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] = {}; @@ -575,7 +575,7 @@ static int current_check_access_path(const struct path *const path, return check_access_path(dom, path, access_request); } -static inline access_mask_t get_mode_access(const umode_t mode) +static access_mask_t get_mode_access(const umode_t mode) { switch (mode & S_IFMT) { case S_IFLNK: @@ -600,7 +600,7 @@ static inline access_mask_t get_mode_access(const umode_t mode) } } -static inline access_mask_t maybe_remove(const struct dentry *const dentry) +static access_mask_t maybe_remove(const struct dentry *const dentry) { if (d_is_negative(dentry)) return 0; From 3406ebade1a84d1cdb0c342e1506b97a579d3834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= Date: Fri, 8 Dec 2023 16:51:13 +0100 Subject: [PATCH 5/7] landlock: Remove remaining "inline" modifiers in .c files [v6.6] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For module-internal static functions, compilers are already in a good position to decide whether to inline them or not. Suggested-by: Mickaël Salaün Signed-off-by: Günther Noack Link: https://lore.kernel.org/r/20231208155121.1943775-2-gnoack@google.com [mic: Split patch for Linux 6.6] 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 b67990e8f32f..9ba989ef46a5 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -1086,7 +1086,7 @@ static int hook_path_truncate(const struct path *const path) * Returns the access rights that are required for opening the given file, * depending on the file type and open mode. */ -static inline access_mask_t +static access_mask_t get_required_file_open_access(const struct file *const file) { access_mask_t access = 0; From b838dd7612f80b75e4363599f7a0d743011dd0d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= Date: Fri, 8 Dec 2023 16:51:14 +0100 Subject: [PATCH 6/7] selftests/landlock: Rename "permitted" to "allowed" in ftruncate tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Suggested-by: Mickaël Salaün Signed-off-by: Günther Noack Link: https://lore.kernel.org/r/20231208155121.1943775-3-gnoack@google.com Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/fs_test.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index a1d17ab527ae..50818904397c 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -3688,7 +3688,7 @@ FIXTURE_TEARDOWN(ftruncate) FIXTURE_VARIANT(ftruncate) { const __u64 handled; - const __u64 permitted; + const __u64 allowed; const int expected_open_result; const int expected_ftruncate_result; }; @@ -3697,7 +3697,7 @@ FIXTURE_VARIANT(ftruncate) FIXTURE_VARIANT_ADD(ftruncate, w_w) { /* clang-format on */ .handled = LANDLOCK_ACCESS_FS_WRITE_FILE, - .permitted = LANDLOCK_ACCESS_FS_WRITE_FILE, + .allowed = LANDLOCK_ACCESS_FS_WRITE_FILE, .expected_open_result = 0, .expected_ftruncate_result = 0, }; @@ -3706,7 +3706,7 @@ FIXTURE_VARIANT_ADD(ftruncate, w_w) { FIXTURE_VARIANT_ADD(ftruncate, t_t) { /* clang-format on */ .handled = LANDLOCK_ACCESS_FS_TRUNCATE, - .permitted = LANDLOCK_ACCESS_FS_TRUNCATE, + .allowed = LANDLOCK_ACCESS_FS_TRUNCATE, .expected_open_result = 0, .expected_ftruncate_result = 0, }; @@ -3715,7 +3715,7 @@ FIXTURE_VARIANT_ADD(ftruncate, t_t) { FIXTURE_VARIANT_ADD(ftruncate, wt_w) { /* clang-format on */ .handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE, - .permitted = LANDLOCK_ACCESS_FS_WRITE_FILE, + .allowed = LANDLOCK_ACCESS_FS_WRITE_FILE, .expected_open_result = 0, .expected_ftruncate_result = EACCES, }; @@ -3724,8 +3724,7 @@ FIXTURE_VARIANT_ADD(ftruncate, wt_w) { FIXTURE_VARIANT_ADD(ftruncate, wt_wt) { /* clang-format on */ .handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE, - .permitted = LANDLOCK_ACCESS_FS_WRITE_FILE | - LANDLOCK_ACCESS_FS_TRUNCATE, + .allowed = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE, .expected_open_result = 0, .expected_ftruncate_result = 0, }; @@ -3734,7 +3733,7 @@ FIXTURE_VARIANT_ADD(ftruncate, wt_wt) { FIXTURE_VARIANT_ADD(ftruncate, wt_t) { /* clang-format on */ .handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE, - .permitted = LANDLOCK_ACCESS_FS_TRUNCATE, + .allowed = LANDLOCK_ACCESS_FS_TRUNCATE, .expected_open_result = EACCES, }; @@ -3744,7 +3743,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate) const struct rule rules[] = { { .path = path, - .access = variant->permitted, + .access = variant->allowed, }, {}, }; @@ -3785,7 +3784,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes) const struct rule rules[] = { { .path = path, - .access = variant->permitted, + .access = variant->allowed, }, {}, }; From 0daaa610c8e033cdfb420db728c2b40eb3a75134 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Noack?= Date: Fri, 8 Dec 2023 16:51:15 +0100 Subject: [PATCH 7/7] landlock: Optimize the number of calls to get_access_mask slightly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This call is now going through a function pointer, and it is not as obvious any more that it will be inlined. Signed-off-by: Günther Noack Link: https://lore.kernel.org/r/20231208155121.1943775-4-gnoack@google.com Fixes: 7a11275c3787 ("landlock: Refactor layer helpers") Signed-off-by: Mickaël Salaün --- security/landlock/ruleset.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index 789c81b26a50..e0a5fbf9201a 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -723,11 +723,12 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain, /* Saves all handled accesses per layer. */ for (layer_level = 0; layer_level < domain->num_layers; layer_level++) { const unsigned long access_req = access_request; + const access_mask_t access_mask = + get_access_mask(domain, layer_level); unsigned long access_bit; for_each_set_bit(access_bit, &access_req, num_access) { - if (BIT_ULL(access_bit) & - get_access_mask(domain, layer_level)) { + if (BIT_ULL(access_bit) & access_mask) { (*layer_masks)[access_bit] |= BIT_ULL(layer_level); handled_accesses |= BIT_ULL(access_bit);