From f4d3ef2dd0e3900693805cec3e4cbf8da4928b3d Mon Sep 17 00:00:00 2001 From: Tingmao Wang Date: Tue, 27 May 2025 21:54:48 +0100 Subject: landlock: Minor comments improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch contains some small comment changes. The first three comments for ruleset.c, I sort of made along the way while working on / trying to understand Landlock, and the one from ruleset.h was from the hashtable patch but extracted here. In fs.c, one comment which I found would have been helpful to me when reading this. Signed-off-by: Tingmao Wang Link: https://lore.kernel.org/r/20250602134150.67189-1-m@maowtm.org Link: https://lore.kernel.org/r/20297185fd71ffbb5ce4fec14b38e5444c719c96.1748379182.git.m@maowtm.org [mic: Squash patches with updated description, cosmetic fixes] Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 3 ++- security/landlock/ruleset.c | 12 ++++++++++-- security/landlock/ruleset.h | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) (limited to 'security') diff --git a/security/landlock/fs.c b/security/landlock/fs.c index d9c12b993fa7..97cb7ba4eea4 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -714,7 +714,8 @@ static void test_is_eacces_with_write(struct kunit *const test) * is_access_to_paths_allowed - Check accesses for requests with a common path * * @domain: Domain to check against. - * @path: File hierarchy to walk through. + * @path: File hierarchy to walk through. For refer checks, this would be + * the common mountpoint. * @access_request_parent1: Accesses to check, once @layer_masks_parent1 is * equal to @layer_masks_parent2 (if any). This is tied to the unique * requested path for most actions, or the source in case of a refer action diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index ce7940efea51..dfcdc19ea268 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -83,6 +83,10 @@ static void build_check_rule(void) .num_layers = ~0, }; + /* + * Checks that .num_layers is large enough for at least + * LANDLOCK_MAX_NUM_LAYERS layers. + */ BUILD_BUG_ON(rule.num_layers < LANDLOCK_MAX_NUM_LAYERS); } @@ -290,6 +294,10 @@ static void build_check_layer(void) .access = ~0, }; + /* + * Checks that .level and .access are large enough to contain their expected + * maximum values. + */ BUILD_BUG_ON(layer.level < LANDLOCK_MAX_NUM_LAYERS); BUILD_BUG_ON(layer.access < LANDLOCK_MASK_ACCESS_FS); } @@ -644,8 +652,8 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule, bool is_empty; /* - * Records in @layer_masks which layer grants access to each - * requested access. + * Records in @layer_masks which layer grants access to each requested + * access: bit cleared if the related layer grants access. */ is_empty = true; for_each_set_bit(access_bit, &access_req, masks_array_size) { diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 5da9a64f5af7..1a78cba662b2 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -27,7 +27,7 @@ struct landlock_hierarchy; */ struct landlock_layer { /** - * @level: Position of this layer in the layer stack. + * @level: Position of this layer in the layer stack. Starts from 1. */ u16 level; /** -- cgit v1.2.3 From 49c9e09d961025b22e61ef9ad56aa1c21b6ce2f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Fri, 28 Nov 2025 18:21:56 +0100 Subject: landlock: Fix handling of disconnected directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disconnected files or directories can appear when they are visible and opened from a bind mount, but have been renamed or moved from the source of the bind mount in a way that makes them inaccessible from the mount point (i.e. out of scope). Previously, access rights tied to files or directories opened through a disconnected directory were collected by walking the related hierarchy down to the root of the filesystem, without taking into account the mount point because it couldn't be found. This could lead to inconsistent access results, potential access right widening, and hard-to-debug renames, especially since such paths cannot be printed. For a sandboxed task to create a disconnected directory, it needs to have write access (i.e. FS_MAKE_REG, FS_REMOVE_FILE, and FS_REFER) to the underlying source of the bind mount, and read access to the related mount point. Because a sandboxed task cannot acquire more access rights than those defined by its Landlock domain, this could lead to inconsistent access rights due to missing permissions that should be inherited from the mount point hierarchy, while inheriting permissions from the filesystem hierarchy hidden by this mount point instead. Landlock now handles files and directories opened from disconnected directories by taking into account the filesystem hierarchy when the mount point is not found in the hierarchy walk, and also always taking into account the mount point from which these disconnected directories were opened. This ensures that a rename is not allowed if it would widen access rights [1]. The rationale is that, even if disconnected hierarchies might not be visible or accessible to a sandboxed task, relying on the collected access rights from them improves the guarantee that access rights will not be widened during a rename because of the access right comparison between the source and the destination (see LANDLOCK_ACCESS_FS_REFER). It may look like this would grant more access on disconnected files and directories, but the security policies are always enforced for all the evaluated hierarchies. This new behavior should be less surprising to users and safer from an access control perspective. Remove a wrong WARN_ON_ONCE() canary in collect_domain_accesses() and fix the related comment. Because opened files have their access rights stored in the related file security properties, there is no impact for disconnected or unlinked files. Cc: Christian Brauner Cc: Günther Noack Cc: Song Liu Reported-by: Tingmao Wang Closes: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org Closes: https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER") Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control") Link: https://lore.kernel.org/r/b0f46246-f2c5-42ca-93ce-0d629702a987@maowtm.org [1] Reviewed-by: Tingmao Wang Link: https://lore.kernel.org/r/20251128172200.760753-2-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/errata/abi-1.h | 16 ++++++++++++++++ security/landlock/fs.c | 40 ++++++++++++++++++++++++++++------------ 2 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 security/landlock/errata/abi-1.h (limited to 'security') diff --git a/security/landlock/errata/abi-1.h b/security/landlock/errata/abi-1.h new file mode 100644 index 000000000000..e8a2bff2e5b6 --- /dev/null +++ b/security/landlock/errata/abi-1.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/** + * DOC: erratum_3 + * + * Erratum 3: Disconnected directory handling + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * This fix addresses an issue with disconnected directories that occur when a + * directory is moved outside the scope of a bind mount. The change ensures + * that evaluated access rights include both those from the disconnected file + * hierarchy down to its filesystem root and those from the related mount point + * hierarchy. This prevents access right widening through rename or link + * actions. + */ +LANDLOCK_ERRATUM(3) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 97cb7ba4eea4..2521acde6039 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -910,21 +910,31 @@ jump_up: break; } } + if (unlikely(IS_ROOT(walker_path.dentry))) { - /* - * Stops at disconnected root directories. Only allows - * access to internal filesystems (e.g. nsfs, which is - * reachable through /proc//ns/). - */ - if (walker_path.mnt->mnt_flags & MNT_INTERNAL) { + if (likely(walker_path.mnt->mnt_flags & MNT_INTERNAL)) { + /* + * Stops and allows access when reaching disconnected root + * directories that are part of internal filesystems (e.g. nsfs, + * which is reachable through /proc//ns/). + */ allowed_parent1 = true; allowed_parent2 = true; + break; } - break; + + /* + * We reached a disconnected root directory from a bind mount. + * Let's continue the walk with the mount point we missed. + */ + dput(walker_path.dentry); + walker_path.dentry = walker_path.mnt->mnt_root; + dget(walker_path.dentry); + } else { + parent_dentry = dget_parent(walker_path.dentry); + dput(walker_path.dentry); + walker_path.dentry = parent_dentry; } - parent_dentry = dget_parent(walker_path.dentry); - dput(walker_path.dentry); - walker_path.dentry = parent_dentry; } path_put(&walker_path); @@ -1022,6 +1032,9 @@ static access_mask_t maybe_remove(const struct dentry *const dentry) * file. While walking from @dir to @mnt_root, we record all the domain's * allowed accesses in @layer_masks_dom. * + * Because of disconnected directories, this walk may not reach @mnt_dir. In + * this case, the walk will continue to @mnt_dir after this call. + * * This is similar to is_access_to_paths_allowed() but much simpler because it * only handles walking on the same mount point and only checks one set of * accesses. @@ -1063,8 +1076,11 @@ static bool collect_domain_accesses( break; } - /* We should not reach a root other than @mnt_root. */ - if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir))) + /* + * Stops at the mount point or the filesystem root for a disconnected + * directory. + */ + if (dir == mnt_root || unlikely(IS_ROOT(dir))) break; parent_dentry = dget_parent(dir); -- cgit v1.2.3 From f7ef7de6b9bcec1314af2cdcfd0c952eadd6a779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Fri, 28 Nov 2025 18:21:57 +0100 Subject: landlock: Improve variable scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is now possible thanks to the disconnected directory fix. Cc: Günther Noack Cc: Song Liu Cc: Tingmao Wang Link: https://lore.kernel.org/r/20251128172200.760753-3-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 2521acde6039..6fadb54496a0 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -838,7 +838,6 @@ static bool is_access_to_paths_allowed( * restriction. */ while (true) { - struct dentry *parent_dentry; const struct landlock_rule *rule; /* @@ -931,7 +930,9 @@ jump_up: walker_path.dentry = walker_path.mnt->mnt_root; dget(walker_path.dentry); } else { - parent_dentry = dget_parent(walker_path.dentry); + struct dentry *const parent_dentry = + dget_parent(walker_path.dentry); + dput(walker_path.dentry); walker_path.dentry = parent_dentry; } -- cgit v1.2.3