[PATCH 1/2] fs: add loopback/bind mount specific security hook

[PATCH 1/2] fs: add loopback/bind mount specific security hook

[Shervin Oloumi]

The main mount security hook (security_sb_mount) is called early in the
process before the mount type is determined and the arguments are
validated and converted to the appropriate format. Specifically, the
source path is surfaced as a string, which is not appropriate for
checking bind mount requests. For bind mounts the source should be
validated and passed as a path struct (same as destination), after the
mount type is determined. This allows the hook users to evaluate the
mount attributes without the need to perform any validations or
conversions out of band, which can introduce a TOCTOU race condition.

The newly introduced hook is invoked only if the security_sb_mount hook
passes, and only if the MS_BIND flag is detected. At this point the
source of the mount has been successfully converted to a path struct
using the kernel's kern_path API. This allows LSMs to target bind mount
requests at the right stage, and evaluate the attributes in the right
format, based on the type of mount.

This does not affect the functionality of the existing mount security
hooks, including security_sb_mount. The new hook, can be utilized as a
supplement to the main hook for further analyzing bind mount requests.
This means that there is still the option of only using the main hook
function, if all one wants to do is indiscriminately reject all bind
mount requests, regardless of the source and destination arguments.
However, if one needs to evaluate the source and destination of a bind
mount request before making a decision, this hook function should be
preferred. Of course, if a bind mount request does not make it past the
security_sb_mount check, the bind mount hook function is never invoked.

Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
---
 fs/namespace.c                |  4 ++++
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  1 +
 security/security.c           | 16 ++++++++++++++++
 4 files changed, 22 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 23e81c2a1e3f..c902608c9759 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
 	if (err)
 		return err;
 
+	err = security_sb_bindmount(&old_path, path);
+	if (err)
+		goto out;
+
 	err = -EINVAL;
 	if (mnt_ns_loop(old_path.dentry))
 		goto out;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eb2937599cb0..3d1940239556 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -71,6 +71,7 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
 LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
 LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
 	 const char *type, unsigned long flags, void *data)
+LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path, const struct path *path)
 LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
 LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
 	 const struct path *new_path)
diff --git a/include/linux/security.h b/include/linux/security.h
index cbdba435b798..512ac656500e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
 int security_sb_statfs(struct dentry *dentry);
 int security_sb_mount(const char *dev_name, const struct path *path,
 		      const char *type, unsigned long flags, void *data);
+int security_sb_bindmount(const struct path *old_path, const struct path *path);
 int security_sb_umount(struct vfsmount *mnt, int flags);
 int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
 int security_sb_set_mnt_opts(struct super_block *sb,
diff --git a/security/security.c b/security/security.c
index 09664e09fec9..bd7cb3df16f4 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path,
 	return call_int_hook(sb_mount, dev_name, path, type, flags, data);
 }
 
+/**
+ * security_sb_bindmount() - Loopback/bind mount specific permission check
+ * @old_path: source of loopback/bind mount
+ * @path: mount point
+ *
+ * This check is performed in addition to security_sb_mount and only if the
+ * mount type is determined to be loopback/bind mount (flags & MS_BIND).  It
+ * surfaces the mount source as a path struct.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_sb_bindmount(const struct path *old_path, const struct path *path)
+{
+	return call_int_hook(sb_bindmount, old_path, path);
+}
+
 /**
  * security_sb_umount() - Check permission for unmounting a filesystem
  * @mnt: mounted filesystem

base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH 2/2] landlock: add support for private bind mount

[Shervin Oloumi]

This patch introduces a new LandLock rule, LANDLOCK_ACCESS_FS_MOUNT for
controlling bind mount operations. While this patch focuses on bind
mounts only, the long-term goal is to utilize this rule for controlling
regular device mounts as well. To perform a successful bind mount, both
the source parent and the destination need to have the REFER rule and
the destination needs to carry the MOUNT rule. Note that this does imply
that the MOUNT rule needs to also exist in the source hierarchy, to
avoid rejection due to privilege escalation. The same path access logic
as REFER (used for rename/move and link) is used to check for any
possible privilege escalations before allowing the bind mount.

Additionally, only private bind mounts are allowed. This is to avoid
filesystem hierarchy changes that happen through mount propagation
between peer mount groups. For instance, bind mounting a directory in a
shared mount namespace, can result in hierarchy changes propagating
across the peer groups in that namespace. Such changes cannot be checked
for privilege escalation as the REFER check only covers the source and
the destination surfaced in the bind mount hook function.

If a bind mount request carries a --make-shared flag, or any flag other
than --make-private, the bind mount succeeds while the subsequent
propagation type change fails. This is because, the kernel calls the
mount hook once with the --bind mount flag, and then once more with the
flag for the propagation type change. If the bind mount request carries
the --make-private flag, both the mount operation and the subsequent
propagation type change succeed. Any mount request with the
--make-private flag only is also allowed. Such requests operate on
existing mount points, changing the propagation type to private. In this
case any previously propagated mounts would continue to exist, but
additional bind mounts under the newly privatized mount point, would not
propagate anymore.

Finally, any existing mounts or bind mounts before the process enters a
LandLock domain remain as they are. Such mounts can be of the shared
propagation type, and they would continue to share updates with the rest
of their peer group. While this is an existing behavior, after this
patch such mounts can also be remounted as private, or be unmounted
after the process enters the sandbox. Existing mounts are outside the
scope of LandLock and should be considered before entering the sandbox.

Tests: Regular mount is rejected. Bind mount is rejected if either the
source parent or the mount point do not have the REFER rule, or if the
destination does not have the MOUNT rule. Bind mount is allowed only if
the REFER check passes, i.e.: no restrictions are dropped. Bind mounting
a directory onto itself is always allowed. Mount calls with the flag
--make-private are always allowed. Unmounting is always allowed. Link
and rename functionality is unaffected. All LandLock self-tests pass.

Link: https://github.com/landlock-lsm/linux/issues/14
Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
---
 include/uapi/linux/landlock.h              |   1 +
 security/landlock/fs.c                     | 105 +++++++++++-------
 security/landlock/limits.h                 |   2 +-
 security/landlock/ruleset.h                |   2 +-
 tools/testing/selftests/landlock/fs_test.c | 119 +++++++++++++++++++--
 5 files changed, 181 insertions(+), 48 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 33745642f787..10541a001f2f 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -259,6 +259,7 @@ struct landlock_net_port_attr {
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 #define LANDLOCK_ACCESS_FS_IOCTL_DEV			(1ULL << 15)
+#define LANDLOCK_ACCESS_FS_MOUNT			(1ULL << 16)
 /* clang-format on */
 
 /**
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index e31b97a9f175..1ae18324bd02 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -35,6 +35,7 @@
 #include <linux/workqueue.h>
 #include <uapi/linux/fiemap.h>
 #include <uapi/linux/landlock.h>
+#include <uapi/linux/mount.h>
 
 #include "common.h"
 #include "cred.h"
@@ -1033,34 +1034,36 @@ static bool collect_domain_accesses(
 }
 
 /**
- * current_check_refer_path - Check if a rename or link action is allowed
+ * current_check_reparent_path - Check if a reparent action is allowed
  *
- * @old_dentry: File or directory requested to be moved or linked.
- * @new_dir: Destination parent directory.
+ * @old_dentry: Source file or directory for move, link or bind mount.
+ * @new_dir: Destination parent directory for move and link, or destination
+ *     directory itself for bind mount (mount point).
  * @new_dentry: Destination file or directory.
  * @removable: Sets to true if it is a rename operation.
  * @exchange: Sets to true if it is a rename operation with RENAME_EXCHANGE.
+ * @bind_mount: Sets to true if it is a bind mount operation.
  *
  * Because of its unprivileged constraints, Landlock relies on file hierarchies
- * (and not only inodes) to tie access rights to files.  Being able to link or
- * rename a file hierarchy brings some challenges.  Indeed, moving or linking a
- * file (i.e. creating a new reference to an inode) can have an impact on the
- * actions allowed for a set of files if it would change its parent directory
- * (i.e. reparenting).
+ * (and not only inodes) to tie access rights to files.  Being able to link,
+ * rename or bind mount a file hierarchy brings some challenges.  Indeed, these
+ * operations create a new reference to an inode, which can have an impact on
+ * the actions allowed for a set of files if it would change its parent
+ * directory (i.e. reparenting).
  *
  * To avoid trivial access right bypasses, Landlock first checks if the file or
  * directory requested to be moved would gain new access rights inherited from
  * its new hierarchy.  Before returning any error, Landlock then checks that
  * the parent source hierarchy and the destination hierarchy would allow the
- * link or rename action.  If it is not the case, an error with EACCES is
- * returned to inform user space that there is no way to remove or create the
- * requested source file type.  If it should be allowed but the new inherited
- * access rights would be greater than the source access rights, then the
- * kernel returns an error with EXDEV.  Prioritizing EACCES over EXDEV enables
+ * link, rename or bind mount action.  If it is not the case, an error with
+ * EACCES is returned to inform user space that there is no way to remove or
+ * create the requested source file type.  If it should be allowed but the new
+ * inherited access rights would be greater than the source access rights, then
+ * the kernel returns an error with EXDEV or EPERM.  Prioritizing EACCES enables
  * user space to abort the whole operation if there is no way to do it, or to
  * manually copy the source to the destination if this remains allowed, e.g.
  * because file creation is allowed on the destination directory but not direct
- * linking.
+ * linking, or bind mounting.
  *
  * To achieve this goal, the kernel needs to compare two file hierarchies: the
  * one identifying the source file or directory (including itself), and the
@@ -1082,13 +1085,18 @@ static bool collect_domain_accesses(
  *
  * Returns:
  * - 0 if access is allowed;
- * - -EXDEV if @old_dentry would inherit new access rights from @new_dir;
+ * - -EXDEV if @old_dentry would inherit new access rights from @new_dir through
+ *       link or rename.
+ * - -EPERM if @old_dentry would inherit new access rights from @new_dir through
+ *       bind mount.
  * - -EACCES if file removal or creation is denied.
  */
-static int current_check_refer_path(struct dentry *const old_dentry,
-				    const struct path *const new_dir,
-				    struct dentry *const new_dentry,
-				    const bool removable, const bool exchange)
+static int current_check_reparent_path(struct dentry *const old_dentry,
+				       const struct path *const new_dir,
+				       struct dentry *const new_dentry,
+				       const bool removable,
+				       const bool exchange,
+				       const bool bind_mount)
 {
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 	bool allow_parent1, allow_parent2;
@@ -1120,7 +1128,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	}
 
 	/* The mount points are the same for old and new paths, cf. EXDEV. */
-	if (old_dentry->d_parent == new_dir->dentry) {
+	if ((bind_mount && old_dentry == new_dentry) ||
+	    (!bind_mount && old_dentry->d_parent == new_dentry->d_parent)) {
 		/*
 		 * The LANDLOCK_ACCESS_FS_REFER access right is not required
 		 * for same-directory referer (i.e. no reparenting).
@@ -1160,6 +1169,14 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	if (allow_parent1 && allow_parent2)
 		return 0;
 
+	if (bind_mount) {
+		if (!is_access_to_paths_allowed(
+			    dom, new_dir, LANDLOCK_ACCESS_FS_MOUNT,
+			    &layer_masks_parent2, NULL, 0, NULL, NULL)) {
+			return -EPERM;
+		}
+	}
+
 	/*
 	 * To be able to compare source and destination domain access rights,
 	 * take into account the @old_dentry access rights aggregated with its
@@ -1173,7 +1190,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		return 0;
 
 	/*
-	 * This prioritizes EACCES over EXDEV for all actions, including
+	 * This prioritizes EACCES over EXDEV/EPERM for all actions, including
 	 * renames with RENAME_EXCHANGE.
 	 */
 	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
@@ -1186,6 +1203,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
 	 * source or the destination.
 	 */
+	if (bind_mount)
+		return -EPERM;
 	return -EXDEV;
 }
 
@@ -1319,9 +1338,11 @@ static void hook_sb_delete(struct super_block *const sb)
  * topology (i.e. the mount namespace), changing it may grant access to files
  * not previously allowed.
  *
- * To make it simple, deny any filesystem topology modification by landlocked
- * processes.  Non-landlocked processes may still change the namespace of a
- * landlocked process, but this kind of threat must be handled by a system-wide
+ * Currently, we can safely handle private bind mounts, as the source and
+ * destination restrictions can be compared, however all other types of mount
+ * system calls are blocked, including shared bind mounts and device mounts.
+ * Non-landlocked processes may still change the namespace of a landlocked
+ * process, but this kind of threat must be handled by a system-wide
  * access-control security policy.
  *
  * This could be lifted in the future if Landlock can safely handle mount
@@ -1338,22 +1359,27 @@ static int hook_sb_mount(const char *const dev_name,
 {
 	if (!get_current_fs_domain())
 		return 0;
+
+	/*
+	 * Only allow mount system calls with the --bind flag or --make-private
+	 * flag. Further security checks are performed in the bind mount hook.
+	 */
+	if (flags && (!(~MS_BIND & flags) || !(~MS_PRIVATE & flags)))
+		return 0;
 	return -EPERM;
 }
 
-static int hook_move_mount(const struct path *const from_path,
-			   const struct path *const to_path)
+static int hook_sb_bindmount(const struct path *const old_path,
+			     const struct path *const path)
 {
-	if (!get_current_fs_domain())
-		return 0;
-	return -EPERM;
+	return current_check_reparent_path(old_path->dentry,
+					   path,
+					   path->dentry,
+					   false, false, true);
 }
 
-/*
- * Removing a mount point may reveal a previously hidden file hierarchy, which
- * may then grant access to files, which may have previously been forbidden.
- */
-static int hook_sb_umount(struct vfsmount *const mnt, const int flags)
+static int hook_move_mount(const struct path *const from_path,
+			   const struct path *const to_path)
 {
 	if (!get_current_fs_domain())
 		return 0;
@@ -1389,8 +1415,8 @@ static int hook_path_link(struct dentry *const old_dentry,
 			  const struct path *const new_dir,
 			  struct dentry *const new_dentry)
 {
-	return current_check_refer_path(old_dentry, new_dir, new_dentry, false,
-					false);
+	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
+					   false, false, false);
 }
 
 static int hook_path_rename(const struct path *const old_dir,
@@ -1400,8 +1426,9 @@ static int hook_path_rename(const struct path *const old_dir,
 			    const unsigned int flags)
 {
 	/* old_dir refers to old_dentry->d_parent and new_dir->mnt */
-	return current_check_refer_path(old_dentry, new_dir, new_dentry, true,
-					!!(flags & RENAME_EXCHANGE));
+	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
+					   true, !!(flags & RENAME_EXCHANGE),
+					   false);
 }
 
 static int hook_path_mkdir(const struct path *const dir,
@@ -1652,8 +1679,8 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 
 	LSM_HOOK_INIT(sb_delete, hook_sb_delete),
 	LSM_HOOK_INIT(sb_mount, hook_sb_mount),
+	LSM_HOOK_INIT(sb_bindmount, hook_sb_bindmount),
 	LSM_HOOK_INIT(move_mount, hook_move_mount),
-	LSM_HOOK_INIT(sb_umount, hook_sb_umount),
 	LSM_HOOK_INIT(sb_remount, hook_sb_remount),
 	LSM_HOOK_INIT(sb_pivotroot, hook_sb_pivotroot),
 
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 15f7606066c8..b495b15df25d 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_MOUNT
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 631e24d4ffe9..eeff66a207de 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -31,7 +31,7 @@
 	LANDLOCK_ACCESS_FS_REFER)
 /* clang-format on */
 
-typedef u16 access_mask_t;
+typedef u32 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. */
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6788762188fe..2553b473a02c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -274,6 +274,11 @@ static int mount_opt(const struct mnt_opt *const mnt, const char *const target)
 		     mnt->data);
 }
 
+static int bindmount(const char *const source, const char *const target)
+{
+	return mount(source, target, NULL, MS_BIND, NULL);
+}
+
 static void prepare_layout_opt(struct __test_metadata *const _metadata,
 			       const struct mnt_opt *const mnt)
 {
@@ -558,7 +563,7 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
 	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_MOUNT
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -572,7 +577,8 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	LANDLOCK_ACCESS_FS_REFER)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_MOUNT)
 
 /* clang-format on */
 
@@ -1735,14 +1741,14 @@ TEST_F_FORK(layout1, topology_changes_with_net_only)
 	enforce_ruleset(_metadata, ruleset_fd);
 	ASSERT_EQ(0, close(ruleset_fd));
 
-	/* Mount, remount, move_mount, umount, and pivot_root checks. */
+	/* Mount, remount, move_mount, pivot_root and umount checks. */
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(0, mount_opt(&mnt_tmp, dir_s1d2));
 	ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC, NULL));
 	ASSERT_EQ(0, syscall(__NR_move_mount, AT_FDCWD, dir_s1d2, AT_FDCWD,
 			     dir_s2d2, 0));
-	ASSERT_EQ(0, umount(dir_s2d2));
 	ASSERT_EQ(0, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
+	ASSERT_EQ(0, umount(dir_s2d2));
 	ASSERT_EQ(0, chdir("/"));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 }
@@ -1763,7 +1769,7 @@ TEST_F_FORK(layout1, topology_changes_with_net_and_fs)
 	enforce_ruleset(_metadata, ruleset_fd);
 	ASSERT_EQ(0, close(ruleset_fd));
 
-	/* Mount, remount, move_mount, umount, and pivot_root checks. */
+	/* Mount, remount, move_mount, pivot_root and umount checks. */
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(-1, mount_opt(&mnt_tmp, dir_s1d2));
 	ASSERT_EQ(EPERM, errno);
@@ -1772,10 +1778,9 @@ TEST_F_FORK(layout1, topology_changes_with_net_and_fs)
 	ASSERT_EQ(-1, syscall(__NR_move_mount, AT_FDCWD, dir_s3d2, AT_FDCWD,
 			      dir_s2d2, 0));
 	ASSERT_EQ(EPERM, errno);
-	ASSERT_EQ(-1, umount(dir_s3d2));
-	ASSERT_EQ(EPERM, errno);
 	ASSERT_EQ(-1, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
 	ASSERT_EQ(EPERM, errno);
+	ASSERT_EQ(0, umount(dir_s3d2));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 }
 
@@ -3029,6 +3034,106 @@ TEST_F_FORK(layout1, reparent_remove)
 	ASSERT_EQ(EACCES, errno);
 }
 
+TEST_F_FORK(layout1, reparent_bindmount_deny)
+{
+	const struct rule layer1[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_READ_DIR,
+		},
+		{
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_READ_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR,
+		},
+		{
+			.path = dir_s3d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_READ_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR,
+		},
+		{},
+	};
+	const int ruleset_fd = create_ruleset(
+		_metadata,
+		LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MOUNT |
+		LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_MAKE_DIR,
+		layer1);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* Privilege escalation (FS_MAKE_DIR). */
+	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s2d2));
+	ASSERT_EQ(EPERM, errno);
+
+	/* Mount point missing FS_MOUNT. */
+	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s3d2));
+	ASSERT_EQ(EPERM, errno);
+
+	/* Mount point missing permissions other than FS_MOUNT. */
+	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s1d2));
+	ASSERT_EQ(EACCES, errno);
+
+	/* Missing permissions other than FS_MOUNT, for a self bind mount. */
+	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s1d2));
+	ASSERT_EQ(EACCES, errno);
+
+
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+TEST_F_FORK(layout1, reparent_bindmount_allow)
+{
+	const struct rule layer1[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT,
+		},
+		{
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{
+			.path = dir_s3d1,
+			.access = LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{},
+	};
+	const int ruleset_fd = create_ruleset(
+		_metadata,
+		LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MOUNT |
+		LANDLOCK_ACCESS_FS_EXECUTE,
+		layer1);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* Mount point has more restrictions than the source. */
+	ASSERT_EQ(0, bindmount(dir_s2d2, dir_s1d2));
+	ASSERT_EQ(0, umount(dir_s1d2));
+
+	/* Bind mounting a directory on itself with no FS_MOUNT. */
+	ASSERT_EQ(0, bindmount(dir_s3d2, dir_s3d2));
+	ASSERT_EQ(0, umount(dir_s3d2));
+
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+
 TEST_F_FORK(layout1, reparent_dom_superset)
 {
 	const struct rule layer1[] = {
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[kernel test robot]

Hi Shervin,

kernel test robot noticed the following build errors:

[auto build test ERROR on fc033cf25e612e840e545f8d5ad2edd6ba613ed5]

url:    https://github.com/intel-lab-lkp/linux/commits/Shervin-Oloumi/landlock-add-support-for-private-bind-mount/20241231-094806
base:   fc033cf25e612e840e545f8d5ad2edd6ba613ed5
patch link:    https://lore.kernel.org/r/20241231014632.589049-1-enlightened%40chromium.org
patch subject: [PATCH 1/2] fs: add loopback/bind mount specific security hook
config: sparc-randconfig-002-20241231 (https://download.01.org/0day-ci/archive/20241231/202412311303.Cb16SNL3-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241231/202412311303.Cb16SNL3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412311303.Cb16SNL3-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/namespace.c: In function 'do_loopback':
>> fs/namespace.c:2768:15: error: implicit declaration of function 'security_sb_bindmount'; did you mean 'security_sb_umount'? [-Wimplicit-function-declaration]
    2768 |         err = security_sb_bindmount(&old_path, path);
         |               ^~~~~~~~~~~~~~~~~~~~~
         |               security_sb_umount


vim +2768 fs/namespace.c

  2751	
  2752	/*
  2753	 * do loopback mount.
  2754	 */
  2755	static int do_loopback(struct path *path, const char *old_name,
  2756					int recurse)
  2757	{
  2758		struct path old_path;
  2759		struct mount *mnt = NULL, *parent;
  2760		struct mountpoint *mp;
  2761		int err;
  2762		if (!old_name || !*old_name)
  2763			return -EINVAL;
  2764		err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
  2765		if (err)
  2766			return err;
  2767	
> 2768		err = security_sb_bindmount(&old_path, path);
  2769		if (err)
  2770			goto out;
  2771	
  2772		err = -EINVAL;
  2773		if (mnt_ns_loop(old_path.dentry))
  2774			goto out;
  2775	
  2776		mp = lock_mount(path);
  2777		if (IS_ERR(mp)) {
  2778			err = PTR_ERR(mp);
  2779			goto out;
  2780		}
  2781	
  2782		parent = real_mount(path->mnt);
  2783		if (!check_mnt(parent))
  2784			goto out2;
  2785	
  2786		mnt = __do_loopback(&old_path, recurse);
  2787		if (IS_ERR(mnt)) {
  2788			err = PTR_ERR(mnt);
  2789			goto out2;
  2790		}
  2791	
  2792		err = graft_tree(mnt, parent, mp);
  2793		if (err) {
  2794			lock_mount_hash();
  2795			umount_tree(mnt, UMOUNT_SYNC);
  2796			unlock_mount_hash();
  2797		}
  2798	out2:
  2799		unlock_mount(mp);
  2800	out:
  2801		path_put(&old_path);
  2802		return err;
  2803	}
  2804	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[kernel test robot]

Hi Shervin,

kernel test robot noticed the following build errors:

[auto build test ERROR on fc033cf25e612e840e545f8d5ad2edd6ba613ed5]

url:    https://github.com/intel-lab-lkp/linux/commits/Shervin-Oloumi/landlock-add-support-for-private-bind-mount/20241231-094806
base:   fc033cf25e612e840e545f8d5ad2edd6ba613ed5
patch link:    https://lore.kernel.org/r/20241231014632.589049-1-enlightened%40chromium.org
patch subject: [PATCH 1/2] fs: add loopback/bind mount specific security hook
config: um-randconfig-001-20241231 (https://download.01.org/0day-ci/archive/20241231/202412311322.DkS3TbED-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 319b89197348b7cad1215e235bdc7b5ec8f9b72c)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241231/202412311322.DkS3TbED-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412311322.DkS3TbED-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/namespace.c:2768:8: error: call to undeclared function 'security_sb_bindmount'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    2768 |         err = security_sb_bindmount(&old_path, path);
         |               ^
   1 error generated.


vim +/security_sb_bindmount +2768 fs/namespace.c

  2751	
  2752	/*
  2753	 * do loopback mount.
  2754	 */
  2755	static int do_loopback(struct path *path, const char *old_name,
  2756					int recurse)
  2757	{
  2758		struct path old_path;
  2759		struct mount *mnt = NULL, *parent;
  2760		struct mountpoint *mp;
  2761		int err;
  2762		if (!old_name || !*old_name)
  2763			return -EINVAL;
  2764		err = kern_path(old_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
  2765		if (err)
  2766			return err;
  2767	
> 2768		err = security_sb_bindmount(&old_path, path);
  2769		if (err)
  2770			goto out;
  2771	
  2772		err = -EINVAL;
  2773		if (mnt_ns_loop(old_path.dentry))
  2774			goto out;
  2775	
  2776		mp = lock_mount(path);
  2777		if (IS_ERR(mp)) {
  2778			err = PTR_ERR(mp);
  2779			goto out;
  2780		}
  2781	
  2782		parent = real_mount(path->mnt);
  2783		if (!check_mnt(parent))
  2784			goto out2;
  2785	
  2786		mnt = __do_loopback(&old_path, recurse);
  2787		if (IS_ERR(mnt)) {
  2788			err = PTR_ERR(mnt);
  2789			goto out2;
  2790		}
  2791	
  2792		err = graft_tree(mnt, parent, mp);
  2793		if (err) {
  2794			lock_mount_hash();
  2795			umount_tree(mnt, UMOUNT_SYNC);
  2796			unlock_mount_hash();
  2797		}
  2798	out2:
  2799		unlock_mount(mp);
  2800	out:
  2801		path_put(&old_path);
  2802		return err;
  2803	}
  2804	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[Casey Schaufler]

On 12/30/2024 5:46 PM, Shervin Oloumi wrote:
> The main mount security hook (security_sb_mount) is called early in the
> process before the mount type is determined and the arguments are
> validated and converted to the appropriate format. Specifically, the
> source path is surfaced as a string, which is not appropriate for
> checking bind mount requests. For bind mounts the source should be
> validated and passed as a path struct (same as destination), after the
> mount type is determined. This allows the hook users to evaluate the
> mount attributes without the need to perform any validations or
> conversions out of band, which can introduce a TOCTOU race condition.
>
> The newly introduced hook is invoked only if the security_sb_mount hook
> passes, and only if the MS_BIND flag is detected. At this point the
> source of the mount has been successfully converted to a path struct
> using the kernel's kern_path API. This allows LSMs to target bind mount
> requests at the right stage, and evaluate the attributes in the right
> format, based on the type of mount.

I am not very happy about an LSM hook that is this specific.
Why restrict it to bind mounts? It seems that there might be other
valuable restrictions on mounts that are based on the path.

>
> This does not affect the functionality of the existing mount security
> hooks, including security_sb_mount. The new hook, can be utilized as a
> supplement to the main hook for further analyzing bind mount requests.
> This means that there is still the option of only using the main hook
> function, if all one wants to do is indiscriminately reject all bind
> mount requests, regardless of the source and destination arguments.
> However, if one needs to evaluate the source and destination of a bind
> mount request before making a decision, this hook function should be
> preferred. Of course, if a bind mount request does not make it past the
> security_sb_mount check, the bind mount hook function is never invoked.
>
> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> ---
>  fs/namespace.c                |  4 ++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  1 +
>  security/security.c           | 16 ++++++++++++++++
>  4 files changed, 22 insertions(+)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 23e81c2a1e3f..c902608c9759 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
>  	if (err)
>  		return err;
>  
> +	err = security_sb_bindmount(&old_path, path);

I can easily envision uses for this other than bind mounts.
Perhaps security_sb_mount_path()?

> +	if (err)
> +		goto out;
> +
>  	err = -EINVAL;
>  	if (mnt_ns_loop(old_path.dentry))
>  		goto out;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index eb2937599cb0..3d1940239556 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -71,6 +71,7 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
>  LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
>  LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
>  	 const char *type, unsigned long flags, void *data)
> +LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path, const struct path *path)
>  LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
>  LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
>  	 const struct path *new_path)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index cbdba435b798..512ac656500e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
>  int security_sb_statfs(struct dentry *dentry);
>  int security_sb_mount(const char *dev_name, const struct path *path,
>  		      const char *type, unsigned long flags, void *data);
> +int security_sb_bindmount(const struct path *old_path, const struct path *path);
>  int security_sb_umount(struct vfsmount *mnt, int flags);
>  int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
>  int security_sb_set_mnt_opts(struct super_block *sb,
> diff --git a/security/security.c b/security/security.c
> index 09664e09fec9..bd7cb3df16f4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path,
>  	return call_int_hook(sb_mount, dev_name, path, type, flags, data);
>  }
>  
> +/**
> + * security_sb_bindmount() - Loopback/bind mount specific permission check
> + * @old_path: source of loopback/bind mount
> + * @path: mount point
> + *
> + * This check is performed in addition to security_sb_mount and only if the
> + * mount type is determined to be loopback/bind mount (flags & MS_BIND).  It
> + * surfaces the mount source as a path struct.
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_sb_bindmount(const struct path *old_path, const struct path *path)
> +{
> +	return call_int_hook(sb_bindmount, old_path, path);
> +}
> +
>  /**
>   * security_sb_umount() - Check permission for unmounting a filesystem
>   * @mnt: mounted filesystem
>
> base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5

Re: [PATCH 2/2] landlock: add support for private bind mount

[kernel test robot]

Hi Shervin,

kernel test robot noticed the following build errors:

[auto build test ERROR on fc033cf25e612e840e545f8d5ad2edd6ba613ed5]

url:    https://github.com/intel-lab-lkp/linux/commits/Shervin-Oloumi/landlock-add-support-for-private-bind-mount/20241231-094806
base:   fc033cf25e612e840e545f8d5ad2edd6ba613ed5
patch link:    https://lore.kernel.org/r/20241231014632.589049-2-enlightened%40chromium.org
patch subject: [PATCH 2/2] landlock: add support for private bind mount
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20250101/202501010450.vIVxrod1-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250101/202501010450.vIVxrod1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501010450.vIVxrod1-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/init.h:5,
                    from security/landlock/setup.c:9:
>> include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(typeof_member(union access_masks_all, masks)) == sizeof(typeof_member(union access_masks_all, all))"
      78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
         |                                         ^~~~~~~~~~~~~~
   include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
      77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
         |                                  ^~~~~~~~~~~~~~~
   security/landlock/ruleset.h:57:1: note: in expansion of macro 'static_assert'
      57 | static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
         | ^~~~~~~~~~~~~


vim +78 include/linux/build_bug.h

bc6245e5efd70c4 Ian Abbott       2017-07-10  60  
6bab69c65013bed Rasmus Villemoes 2019-03-07  61  /**
6bab69c65013bed Rasmus Villemoes 2019-03-07  62   * static_assert - check integer constant expression at build time
6bab69c65013bed Rasmus Villemoes 2019-03-07  63   *
6bab69c65013bed Rasmus Villemoes 2019-03-07  64   * static_assert() is a wrapper for the C11 _Static_assert, with a
6bab69c65013bed Rasmus Villemoes 2019-03-07  65   * little macro magic to make the message optional (defaulting to the
6bab69c65013bed Rasmus Villemoes 2019-03-07  66   * stringification of the tested expression).
6bab69c65013bed Rasmus Villemoes 2019-03-07  67   *
6bab69c65013bed Rasmus Villemoes 2019-03-07  68   * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
6bab69c65013bed Rasmus Villemoes 2019-03-07  69   * scope, but requires the expression to be an integer constant
6bab69c65013bed Rasmus Villemoes 2019-03-07  70   * expression (i.e., it is not enough that __builtin_constant_p() is
6bab69c65013bed Rasmus Villemoes 2019-03-07  71   * true for expr).
6bab69c65013bed Rasmus Villemoes 2019-03-07  72   *
6bab69c65013bed Rasmus Villemoes 2019-03-07  73   * Also note that BUILD_BUG_ON() fails the build if the condition is
6bab69c65013bed Rasmus Villemoes 2019-03-07  74   * true, while static_assert() fails the build if the expression is
6bab69c65013bed Rasmus Villemoes 2019-03-07  75   * false.
6bab69c65013bed Rasmus Villemoes 2019-03-07  76   */
6bab69c65013bed Rasmus Villemoes 2019-03-07  77  #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
6bab69c65013bed Rasmus Villemoes 2019-03-07 @78  #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
6bab69c65013bed Rasmus Villemoes 2019-03-07  79  
07a368b3f55a79d Maxim Levitsky   2022-10-25  80  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[Paul Moore]

On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi <enlightened@chromium.org> wrote:
>
> The main mount security hook (security_sb_mount) is called early in the
> process before the mount type is determined and the arguments are
> validated and converted to the appropriate format. Specifically, the
> source path is surfaced as a string, which is not appropriate for
> checking bind mount requests. For bind mounts the source should be
> validated and passed as a path struct (same as destination), after the
> mount type is determined. This allows the hook users to evaluate the
> mount attributes without the need to perform any validations or
> conversions out of band, which can introduce a TOCTOU race condition.
>
> The newly introduced hook is invoked only if the security_sb_mount hook
> passes, and only if the MS_BIND flag is detected. At this point the
> source of the mount has been successfully converted to a path struct
> using the kernel's kern_path API. This allows LSMs to target bind mount
> requests at the right stage, and evaluate the attributes in the right
> format, based on the type of mount.
>
> This does not affect the functionality of the existing mount security
> hooks, including security_sb_mount. The new hook, can be utilized as a
> supplement to the main hook for further analyzing bind mount requests.
> This means that there is still the option of only using the main hook
> function, if all one wants to do is indiscriminately reject all bind
> mount requests, regardless of the source and destination arguments.
> However, if one needs to evaluate the source and destination of a bind
> mount request before making a decision, this hook function should be
> preferred. Of course, if a bind mount request does not make it past the
> security_sb_mount check, the bind mount hook function is never invoked.
>
> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> ---
>  fs/namespace.c                |  4 ++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  1 +
>  security/security.c           | 16 ++++++++++++++++
>  4 files changed, 22 insertions(+)

Like Casey I'm not really excited about such a specific LSM hook, but
unfortunately we can't simply call kern_path() in the existing
security_sb_mount() callback as that could end up resolving to
something different than the call in do_loopback() which would be bad.
Moving the kern_path() call up into path_mount() just looks like a bad
idea anyway you look at it.  Unfortunately I don't really see an
alternative to what you're proposing, so I guess we're kinda stuck
with this as a solution, unless someone can think of something better.

I'm going to need to see an ACK from the VFS folks on this before I
merge the new hook.

I'd also stick with the security_sb_bindmount() name as opposed to the
XXX_path() suggestion from Casey simply to help distinguish it from
the pathname based LSM hooks.  Yes, this is operating on the pathname,
but bind mounts are a bit of a special case.

Unrelated, but I just noticed that we are calling security_sb_mount()
*before* may_mount(); that's the opposite order for most LSM hook
placements where we do the discretionary/capabilities checks first and
the LSM checks.  That's something we should look at, perhaps there is
a good reason for the ordering being different, perhaps it's a
mistake.

> diff --git a/fs/namespace.c b/fs/namespace.c
> index 23e81c2a1e3f..c902608c9759 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
>         if (err)
>                 return err;
>
> +       err = security_sb_bindmount(&old_path, path);
> +       if (err)
> +               goto out;

I might make a mention in the commit description that the
do_reconfigure_mnt() case (MS_REMOUNT|MS_BIND) should be able to be
handled using the existing security_sb_mount() hook.

>         err = -EINVAL;
>         if (mnt_ns_loop(old_path.dentry))
>                 goto out;

...

> diff --git a/security/security.c b/security/security.c
> index 09664e09fec9..bd7cb3df16f4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path,
>         return call_int_hook(sb_mount, dev_name, path, type, flags, data);
>  }
>
> +/**
> + * security_sb_bindmount() - Loopback/bind mount specific permission check
> + * @old_path: source of loopback/bind mount
> + * @path: mount point
> + *
> + * This check is performed in addition to security_sb_mount and only if the
> + * mount type is determined to be loopback/bind mount (flags & MS_BIND).  It
> + * surfaces the mount source as a path struct.

I wouldn't mention security_sb_mount() above as that makes the comment
somewhat fragile in the face of changing hooks.  I would suggest
something along these lines:

"Beyond any general mounting hooks, this check is performed on an
 initial loopback/bind mount (MS_BIND) with the mount source presented
 as a path struct in @old_path."

> + * Return: Returns 0 if permission is granted.
> + */
> +int security_sb_bindmount(const struct path *old_path, const struct path *path)
> +{
> +       return call_int_hook(sb_bindmount, old_path, path);
> +}
> +
>  /**
>   * security_sb_umount() - Check permission for unmounting a filesystem
>   * @mnt: mounted filesystem
>
> base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
> --
> 2.47.1.613.gc27f4b7a9f-goog

-- 
paul-moore.com

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[Jan Kara]

On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:
> The main mount security hook (security_sb_mount) is called early in the
> process before the mount type is determined and the arguments are
> validated and converted to the appropriate format. Specifically, the
> source path is surfaced as a string, which is not appropriate for
> checking bind mount requests. For bind mounts the source should be
> validated and passed as a path struct (same as destination), after the
> mount type is determined. This allows the hook users to evaluate the
> mount attributes without the need to perform any validations or
> conversions out of band, which can introduce a TOCTOU race condition.
> 
> The newly introduced hook is invoked only if the security_sb_mount hook
> passes, and only if the MS_BIND flag is detected. At this point the
> source of the mount has been successfully converted to a path struct
> using the kernel's kern_path API. This allows LSMs to target bind mount
> requests at the right stage, and evaluate the attributes in the right
> format, based on the type of mount.
> 
> This does not affect the functionality of the existing mount security
> hooks, including security_sb_mount. The new hook, can be utilized as a
> supplement to the main hook for further analyzing bind mount requests.
> This means that there is still the option of only using the main hook
> function, if all one wants to do is indiscriminately reject all bind
> mount requests, regardless of the source and destination arguments.
> However, if one needs to evaluate the source and destination of a bind
> mount request before making a decision, this hook function should be
> preferred. Of course, if a bind mount request does not make it past the
> security_sb_mount check, the bind mount hook function is never invoked.
> 
> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>

Christian is much more experienced in this area than me but let me share my
thoughts before he returns from vacation.

> diff --git a/fs/namespace.c b/fs/namespace.c
> index 23e81c2a1e3f..c902608c9759 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
>  	if (err)
>  		return err;
>  
> +	err = security_sb_bindmount(&old_path, path);
> +	if (err)
> +		goto out;
> +

So this gets triggered for the legacy mount API path (mount(2) syscall).
For the new mount API, I can see open_tree() does not have any security
hook. Is that intented? Are you catching equivalent changes done through
the new mount API inside security_move_mount() hook?

Also what caught my eye is that the LSM doesn't care at all whether this is
a recursive bind mount (copying all mounts beneath the specified one) or
just a normal one (copying only a single mount). Maybe that's fine but it
seems a bit counter-intuitive to me as AFAIU it implicitly places a
requirement on the policy that if doing some bind mount is forbidden, then
doing bind mount of any predecessor must be forbidden as well (otherwise
the policy will be inconsistent).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 1/2] fs: add loopback/bind mount specific security hook

[Shervin Oloumi]

The main mount security hook (security_sb_mount) is called early in the
process before the mount type is determined and the arguments are
validated and converted to the appropriate format. Specifically, the
source path is surfaced as a string, which is not appropriate for
checking bind mount requests. For bind mounts the source should be
validated and passed as a path struct (same as destination), after the
mount type is determined. This allows the hook users to evaluate the
mount attributes without the need to perform any validations or
conversions out of band, which can introduce a TOCTOU race condition.

The newly introduced hook is invoked only if the security_sb_mount hook
passes, and only if the MS_BIND flag is detected. The
do_reconfigure_mnt() case (MS_REMOUNT | MS_BIND) is still handled by the
existing security_sb_mount() hook. When the new bind mount hook is
called, the source of the mount has already been successfully converted
to a path struct using the kernel's kern_path API. This allows LSMs to
target bind mount requests at the right stage, and evaluate the
attributes in the right format, based on the type of mount. This bind
mount hook also signals the existence of MS_REC flag via a boolean.

This does not affect the functionality of the existing mount security
hooks, including security_sb_mount. The new hook, can be utilized as a
supplement to the main hook for further analyzing bind mount requests.
This means that there is still the option of only using the main hook
function, if all one wants to do is indiscriminately reject all bind
mount requests, regardless of the source and destination arguments.
However, if one needs to evaluate the source and destination of a bind
mount request before making a decision, this hook function should be
preferred. Of course, if a bind mount request does not make it past the
security_sb_mount check, the bind mount hook function is never invoked.

Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
---

Changes since v2:
- Reword the commit to include how do_reconfigure_mnt() is handled and
  mention that MS_REC is also surfaced in the hook

Changes since v1:
- Indicate whether the mount is recursive in the hook. This can be a
  factor when deciding if a mount should be allowed
- Add default capabilities function for the new hook in security.h. This
  is required for some tests to pass
- Reword the hook description to be more future proof
---
 fs/namespace.c                |  4 ++++
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/security.h      |  7 +++++++
 security/security.c           | 18 ++++++++++++++++++
 4 files changed, 31 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 23e81c2a1e3f..535a1841c200 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
 	if (err)
 		return err;
 
+	err = security_sb_bindmount(&old_path, path, recurse ? true : false);
+	if (err)
+		goto out;
+
 	err = -EINVAL;
 	if (mnt_ns_loop(old_path.dentry))
 		goto out;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eb2937599cb0..94f5a3530b98 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -71,6 +71,8 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
 LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
 LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
 	 const char *type, unsigned long flags, void *data)
+LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path,
+	 const struct path *path, bool recurse)
 LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
 LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
 	 const struct path *new_path)
diff --git a/include/linux/security.h b/include/linux/security.h
index cbdba435b798..d4a4c71865e3 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
 int security_sb_statfs(struct dentry *dentry);
 int security_sb_mount(const char *dev_name, const struct path *path,
 		      const char *type, unsigned long flags, void *data);
+int security_sb_bindmount(const struct path *old_path, const struct path *path, bool recurse);
 int security_sb_umount(struct vfsmount *mnt, int flags);
 int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
 int security_sb_set_mnt_opts(struct super_block *sb,
@@ -801,6 +802,12 @@ static inline int security_sb_mount(const char *dev_name, const struct path *pat
 	return 0;
 }
 
+static inline int security_sb_bindmount(const struct path *old_path,
+					const struct path *path, bool recurse)
+{
+	return 0;
+}
+
 static inline int security_sb_umount(struct vfsmount *mnt, int flags)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 09664e09fec9..c115d8627e2d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1564,6 +1564,24 @@ int security_sb_mount(const char *dev_name, const struct path *path,
 	return call_int_hook(sb_mount, dev_name, path, type, flags, data);
 }
 
+/**
+ * security_sb_bindmount() - Loopback/bind mount permission check
+ * @old_path: source of loopback/bind mount
+ * @path: mount point
+ * @recurse: true if recursive (MS_REC)
+ *
+ * Beyond any general mounting hooks, this check is performed on an initial
+ * loopback/bind mount (MS_BIND) with the mount source presented as a path
+ * struct in @old_path.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_sb_bindmount(const struct path *old_path, const struct path *path,
+			  bool recurse)
+{
+	return call_int_hook(sb_bindmount, old_path, path, recurse);
+}
+
 /**
  * security_sb_umount() - Check permission for unmounting a filesystem
  * @mnt: mounted filesystem

base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 2/2] landlock: add support for private bind mount

[Shervin Oloumi]

This patch introduces a new LandLock rule, LANDLOCK_ACCESS_FS_MOUNT for
controlling bind mount operations. While this patch mostly focuses on
bind mounts, the long-term goal is to utilize this rule for controlling
regular device mounts as well. To perform a successful bind mount, both
the source parent and the destination need to have the REFER rule and
the destination needs to carry the MOUNT rule. Note that this does imply
that the MOUNT rule needs to also exist in the source hierarchy, to
avoid rejection due to privilege escalation. The same path access logic
as REFER (used for rename/move and link) is used to check for any
possible privilege escalations before allowing the bind mount.

Additionally, only private bind mounts are allowed. This is to avoid
filesystem hierarchy changes that happen through mount propagation
between peer mount groups. For instance, bind mounting a directory in a
shared mount namespace, can result in hierarchy changes propagating
across the peer groups in that namespace. Such changes cannot be checked
for privilege escalation as the REFER check only covers the source and
the destination surfaced in the bind mount hook function. We do allow
recursive bind mounts. This is safe because, if a bind mount already
exists in some location, it must have passed the REFER check, so if its
parent passes the REFER check with respect to a new mount point, by
definition the child bind mount also passes the REFER check with respect
to that new mount point, and so it can be safely cloned to the new
location along with its predecessor bind mount.

If a bind mount request carries a --make-shared flag, or any flag other
than --make-private, the bind mount succeeds while the subsequent
propagation type change attempt fails. This is because, the kernel calls
the mount hook twice, first with the MS_BIND flag, and then with the
flag for the propagation type change. If the bind mount request carries
the --make-private flag, both the mount operation and the subsequent
propagation type change succeed. Any mount request with --make-private
or --make-rprivate flags are also allowed. Such requests operate on
existing mount points, changing the propagation type to private. In this
case any previously propagated mounts would continue to exist, but
additional bind mounts under the newly privatized mount point, would not
propagate anymore.

Finally, any existing mounts or bind mounts before the process enters a
LandLock domain remain as they are. Such mounts can be of the shared
propagation type, and they would continue to share updates with the rest
of their peer group. While this is an existing behavior, after this
patch such mounts can also be remounted as private, or be unmounted
after the process enters the sandbox. Existing mounts are outside the
scope of LandLock and should be considered before entering the sandbox.

Tests: Regular mount is rejected. Bind mount is rejected if either the
source parent or the mount point do not have the REFER rule, or if the
destination does not have the MOUNT rule. Bind mount is allowed only if
the REFER check passes, i.e.: no restrictions are dropped. Recursive
bind mounts are allowed, as long as they abide by the same rules. Bind
mounting a directory onto itself is always allowed. Mount calls with the
flags --make-private or --make-rprivate are always allowed. Unmounting
is always allowed. Link and rename functionality is unaffected. All
LandLock filesystem tests pass.

Link: https://github.com/landlock-lsm/linux/issues/14
Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
---

Changes since v2:
- Change the flag check conditions in the main mount hook function to
  allow any type of request carrying MS_BIND or MS_PRIVATE, as opposed
  to requests carrying only those flags, effectively allowing MS_REC
- Update the LandLock filesystem self tests to reflect the condition
  change that results in MS_PRIVATE | MS_REC succeeding
- Update the bind mount tests for readability
- Update the commit message to reflect the above updates

Changes since v1:
- Update the failing static assert in ruleset.h
- Update the bind mount hook function to include the newly added
  argument, recurse
---
 include/uapi/linux/landlock.h              |   1 +
 security/landlock/fs.c                     | 104 ++++++++++++-------
 security/landlock/limits.h                 |   2 +-
 security/landlock/ruleset.h                |   6 +-
 tools/testing/selftests/landlock/fs_test.c | 114 +++++++++++++++++++--
 5 files changed, 175 insertions(+), 52 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 33745642f787..10541a001f2f 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -259,6 +259,7 @@ struct landlock_net_port_attr {
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 #define LANDLOCK_ACCESS_FS_IOCTL_DEV			(1ULL << 15)
+#define LANDLOCK_ACCESS_FS_MOUNT			(1ULL << 16)
 /* clang-format on */
 
 /**
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index e31b97a9f175..df0a3094d90b 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -35,6 +35,7 @@
 #include <linux/workqueue.h>
 #include <uapi/linux/fiemap.h>
 #include <uapi/linux/landlock.h>
+#include <uapi/linux/mount.h>
 
 #include "common.h"
 #include "cred.h"
@@ -1033,34 +1034,36 @@ static bool collect_domain_accesses(
 }
 
 /**
- * current_check_refer_path - Check if a rename or link action is allowed
+ * current_check_reparent_path - Check if a reparent action is allowed
  *
- * @old_dentry: File or directory requested to be moved or linked.
- * @new_dir: Destination parent directory.
+ * @old_dentry: Source file or directory for move, link or bind mount.
+ * @new_dir: Destination parent directory for move and link, or destination
+ *     directory itself for bind mount (mount point).
  * @new_dentry: Destination file or directory.
  * @removable: Sets to true if it is a rename operation.
  * @exchange: Sets to true if it is a rename operation with RENAME_EXCHANGE.
+ * @bind_mount: Sets to true if it is a bind mount operation.
  *
  * Because of its unprivileged constraints, Landlock relies on file hierarchies
- * (and not only inodes) to tie access rights to files.  Being able to link or
- * rename a file hierarchy brings some challenges.  Indeed, moving or linking a
- * file (i.e. creating a new reference to an inode) can have an impact on the
- * actions allowed for a set of files if it would change its parent directory
- * (i.e. reparenting).
+ * (and not only inodes) to tie access rights to files.  Being able to link,
+ * rename or bind mount a file hierarchy brings some challenges.  Indeed, these
+ * operations create a new reference to an inode, which can have an impact on
+ * the actions allowed for a set of files if it would change its parent
+ * directory (i.e. reparenting).
  *
  * To avoid trivial access right bypasses, Landlock first checks if the file or
  * directory requested to be moved would gain new access rights inherited from
  * its new hierarchy.  Before returning any error, Landlock then checks that
  * the parent source hierarchy and the destination hierarchy would allow the
- * link or rename action.  If it is not the case, an error with EACCES is
- * returned to inform user space that there is no way to remove or create the
- * requested source file type.  If it should be allowed but the new inherited
- * access rights would be greater than the source access rights, then the
- * kernel returns an error with EXDEV.  Prioritizing EACCES over EXDEV enables
+ * link, rename or bind mount action.  If it is not the case, an error with
+ * EACCES is returned to inform user space that there is no way to remove or
+ * create the requested source file type.  If it should be allowed but the new
+ * inherited access rights would be greater than the source access rights, then
+ * the kernel returns an error with EXDEV or EPERM.  Prioritizing EACCES enables
  * user space to abort the whole operation if there is no way to do it, or to
  * manually copy the source to the destination if this remains allowed, e.g.
  * because file creation is allowed on the destination directory but not direct
- * linking.
+ * linking, or bind mounting.
  *
  * To achieve this goal, the kernel needs to compare two file hierarchies: the
  * one identifying the source file or directory (including itself), and the
@@ -1082,13 +1085,18 @@ static bool collect_domain_accesses(
  *
  * Returns:
  * - 0 if access is allowed;
- * - -EXDEV if @old_dentry would inherit new access rights from @new_dir;
+ * - -EXDEV if @old_dentry would inherit new access rights from @new_dir through
+ *       link or rename.
+ * - -EPERM if @old_dentry would inherit new access rights from @new_dir through
+ *       bind mount.
  * - -EACCES if file removal or creation is denied.
  */
-static int current_check_refer_path(struct dentry *const old_dentry,
-				    const struct path *const new_dir,
-				    struct dentry *const new_dentry,
-				    const bool removable, const bool exchange)
+static int current_check_reparent_path(struct dentry *const old_dentry,
+				       const struct path *const new_dir,
+				       struct dentry *const new_dentry,
+				       const bool removable,
+				       const bool exchange,
+				       const bool bind_mount)
 {
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 	bool allow_parent1, allow_parent2;
@@ -1120,7 +1128,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	}
 
 	/* The mount points are the same for old and new paths, cf. EXDEV. */
-	if (old_dentry->d_parent == new_dir->dentry) {
+	if ((bind_mount && old_dentry == new_dentry) ||
+	    (!bind_mount && old_dentry->d_parent == new_dentry->d_parent)) {
 		/*
 		 * The LANDLOCK_ACCESS_FS_REFER access right is not required
 		 * for same-directory referer (i.e. no reparenting).
@@ -1160,6 +1169,14 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	if (allow_parent1 && allow_parent2)
 		return 0;
 
+	if (bind_mount) {
+		if (!is_access_to_paths_allowed(
+			    dom, new_dir, LANDLOCK_ACCESS_FS_MOUNT,
+			    &layer_masks_parent2, NULL, 0, NULL, NULL)) {
+			return -EPERM;
+		}
+	}
+
 	/*
 	 * To be able to compare source and destination domain access rights,
 	 * take into account the @old_dentry access rights aggregated with its
@@ -1173,7 +1190,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		return 0;
 
 	/*
-	 * This prioritizes EACCES over EXDEV for all actions, including
+	 * This prioritizes EACCES over EXDEV/EPERM for all actions, including
 	 * renames with RENAME_EXCHANGE.
 	 */
 	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
@@ -1186,6 +1203,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
 	 * source or the destination.
 	 */
+	if (bind_mount)
+		return -EPERM;
 	return -EXDEV;
 }
 
@@ -1319,9 +1338,11 @@ static void hook_sb_delete(struct super_block *const sb)
  * topology (i.e. the mount namespace), changing it may grant access to files
  * not previously allowed.
  *
- * To make it simple, deny any filesystem topology modification by landlocked
- * processes.  Non-landlocked processes may still change the namespace of a
- * landlocked process, but this kind of threat must be handled by a system-wide
+ * Currently, we can safely handle private bind mounts, as the source and
+ * destination restrictions can be compared, however all other types of mount
+ * system calls are blocked, including shared bind mounts and device mounts.
+ * Non-landlocked processes may still change the namespace of a landlocked
+ * process, but this kind of threat must be handled by a system-wide
  * access-control security policy.
  *
  * This could be lifted in the future if Landlock can safely handle mount
@@ -1338,22 +1359,26 @@ static int hook_sb_mount(const char *const dev_name,
 {
 	if (!get_current_fs_domain())
 		return 0;
+
+	/*
+	 * Allow bind mount and make-private requests to proceed, including requests
+	 * with the MS_REC flag.  For bind mount requests, further security checks
+	 * are performed in the bind mount specific hook.
+	 */
+	if (flags && ((MS_BIND | MS_PRIVATE) & flags))
+		return 0;
 	return -EPERM;
 }
 
-static int hook_move_mount(const struct path *const from_path,
-			   const struct path *const to_path)
+static int hook_sb_bindmount(const struct path *const old_path,
+			     const struct path *const path, bool recurse)
 {
-	if (!get_current_fs_domain())
-		return 0;
-	return -EPERM;
+	return current_check_reparent_path(old_path->dentry, path, path->dentry,
+					   false, false, true);
 }
 
-/*
- * Removing a mount point may reveal a previously hidden file hierarchy, which
- * may then grant access to files, which may have previously been forbidden.
- */
-static int hook_sb_umount(struct vfsmount *const mnt, const int flags)
+static int hook_move_mount(const struct path *const from_path,
+			   const struct path *const to_path)
 {
 	if (!get_current_fs_domain())
 		return 0;
@@ -1389,8 +1414,8 @@ static int hook_path_link(struct dentry *const old_dentry,
 			  const struct path *const new_dir,
 			  struct dentry *const new_dentry)
 {
-	return current_check_refer_path(old_dentry, new_dir, new_dentry, false,
-					false);
+	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
+					   false, false, false);
 }
 
 static int hook_path_rename(const struct path *const old_dir,
@@ -1400,8 +1425,9 @@ static int hook_path_rename(const struct path *const old_dir,
 			    const unsigned int flags)
 {
 	/* old_dir refers to old_dentry->d_parent and new_dir->mnt */
-	return current_check_refer_path(old_dentry, new_dir, new_dentry, true,
-					!!(flags & RENAME_EXCHANGE));
+	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
+					   true, !!(flags & RENAME_EXCHANGE),
+					   false);
 }
 
 static int hook_path_mkdir(const struct path *const dir,
@@ -1652,8 +1678,8 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 
 	LSM_HOOK_INIT(sb_delete, hook_sb_delete),
 	LSM_HOOK_INIT(sb_mount, hook_sb_mount),
+	LSM_HOOK_INIT(sb_bindmount, hook_sb_bindmount),
 	LSM_HOOK_INIT(move_mount, hook_move_mount),
-	LSM_HOOK_INIT(sb_umount, hook_sb_umount),
 	LSM_HOOK_INIT(sb_remount, hook_sb_remount),
 	LSM_HOOK_INIT(sb_pivotroot, hook_sb_pivotroot),
 
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 15f7606066c8..b495b15df25d 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_MOUNT
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 631e24d4ffe9..62e1db3dd95a 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -31,7 +31,7 @@
 	LANDLOCK_ACCESS_FS_REFER)
 /* clang-format on */
 
-typedef u16 access_mask_t;
+typedef u32 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. */
@@ -50,11 +50,11 @@ struct access_masks {
 
 union access_masks_all {
 	struct access_masks masks;
-	u32 all;
+	u64 all;
 };
 
 /* Makes sure all fields are covered. */
-static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
+static_assert(sizeof(typeof_member(union access_masks_all, masks)) <=
 	      sizeof(typeof_member(union access_masks_all, all)));
 
 typedef u16 layer_mask_t;
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6788762188fe..ab59bea0125b 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -274,6 +274,11 @@ static int mount_opt(const struct mnt_opt *const mnt, const char *const target)
 		     mnt->data);
 }
 
+static int bindmount(const char *const source, const char *const target)
+{
+	return mount(source, target, NULL, MS_BIND, NULL);
+}
+
 static void prepare_layout_opt(struct __test_metadata *const _metadata,
 			       const struct mnt_opt *const mnt)
 {
@@ -558,7 +563,7 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
 	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_MOUNT
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -572,7 +577,8 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	LANDLOCK_ACCESS_FS_REFER)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_MOUNT)
 
 /* clang-format on */
 
@@ -1735,14 +1741,14 @@ TEST_F_FORK(layout1, topology_changes_with_net_only)
 	enforce_ruleset(_metadata, ruleset_fd);
 	ASSERT_EQ(0, close(ruleset_fd));
 
-	/* Mount, remount, move_mount, umount, and pivot_root checks. */
+	/* Mount, remount, move_mount, pivot_root and umount checks. */
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(0, mount_opt(&mnt_tmp, dir_s1d2));
 	ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC, NULL));
 	ASSERT_EQ(0, syscall(__NR_move_mount, AT_FDCWD, dir_s1d2, AT_FDCWD,
 			     dir_s2d2, 0));
-	ASSERT_EQ(0, umount(dir_s2d2));
 	ASSERT_EQ(0, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
+	ASSERT_EQ(0, umount(dir_s2d2));
 	ASSERT_EQ(0, chdir("/"));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 }
@@ -1763,19 +1769,17 @@ TEST_F_FORK(layout1, topology_changes_with_net_and_fs)
 	enforce_ruleset(_metadata, ruleset_fd);
 	ASSERT_EQ(0, close(ruleset_fd));
 
-	/* Mount, remount, move_mount, umount, and pivot_root checks. */
+	/* Mount, remount, move_mount, pivot_root and umount checks. */
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(-1, mount_opt(&mnt_tmp, dir_s1d2));
 	ASSERT_EQ(EPERM, errno);
-	ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC, NULL));
-	ASSERT_EQ(EPERM, errno);
 	ASSERT_EQ(-1, syscall(__NR_move_mount, AT_FDCWD, dir_s3d2, AT_FDCWD,
 			      dir_s2d2, 0));
 	ASSERT_EQ(EPERM, errno);
-	ASSERT_EQ(-1, umount(dir_s3d2));
-	ASSERT_EQ(EPERM, errno);
 	ASSERT_EQ(-1, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
 	ASSERT_EQ(EPERM, errno);
+	ASSERT_EQ(0, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC, NULL));
+	ASSERT_EQ(0, umount(dir_s3d2));
 	clear_cap(_metadata, CAP_SYS_ADMIN);
 }
 
@@ -3029,6 +3033,98 @@ TEST_F_FORK(layout1, reparent_remove)
 	ASSERT_EQ(EACCES, errno);
 }
 
+TEST_F_FORK(layout1, reparent_bindmount_deny)
+{
+	const struct rule layer1[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_READ_DIR,
+		},
+		{
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_READ_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR,
+		},
+		{
+			.path = dir_s3d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_READ_DIR |
+				  LANDLOCK_ACCESS_FS_MAKE_DIR,
+		},
+		{},
+	};
+	const int ruleset_fd =
+		create_ruleset(_metadata, layer1[1].access, layer1);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* Privilege escalation (FS_MAKE_DIR). */
+	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s2d2));
+	ASSERT_EQ(EPERM, errno);
+
+	/* Mount point missing FS_MOUNT. */
+	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s3d2));
+	ASSERT_EQ(EPERM, errno);
+
+	/* Mount point missing permissions other than FS_MOUNT. */
+	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s1d2));
+	ASSERT_EQ(EACCES, errno);
+
+	/* Missing permissions other than FS_MOUNT, for a self bind mount. */
+	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s1d2));
+	ASSERT_EQ(EACCES, errno);
+
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+TEST_F_FORK(layout1, reparent_bindmount_allow)
+{
+	const struct rule layer1[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT,
+		},
+		{
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+				  LANDLOCK_ACCESS_FS_MOUNT |
+				  LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{
+			.path = dir_s3d1,
+			.access = LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{},
+	};
+	const int ruleset_fd =
+		create_ruleset(_metadata, layer1[1].access, layer1);
+
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	set_cap(_metadata, CAP_SYS_ADMIN);
+
+	/* Mount point has more restrictions than the source. */
+	ASSERT_EQ(0, bindmount(dir_s2d2, dir_s1d2));
+	ASSERT_EQ(0, umount(dir_s1d2));
+
+	/* Bind mounting a directory on itself with no FS_MOUNT. */
+	ASSERT_EQ(0, bindmount(dir_s3d2, dir_s3d2));
+	ASSERT_EQ(0, umount(dir_s3d2));
+
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
 TEST_F_FORK(layout1, reparent_dom_superset)
 {
 	const struct rule layer1[] = {
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[Shervin Oloumi]

Thanks for the feedback Paul, the latest patch should include the
recommendations now.

On Thu, Jan 2, 2025 at 9:11 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi <enlightened@chromium.org> wrote:
> >
> > The main mount security hook (security_sb_mount) is called early in the
> > process before the mount type is determined and the arguments are
> > validated and converted to the appropriate format. Specifically, the
> > source path is surfaced as a string, which is not appropriate for
> > checking bind mount requests. For bind mounts the source should be
> > validated and passed as a path struct (same as destination), after the
> > mount type is determined. This allows the hook users to evaluate the
> > mount attributes without the need to perform any validations or
> > conversions out of band, which can introduce a TOCTOU race condition.
> >
> > The newly introduced hook is invoked only if the security_sb_mount hook
> > passes, and only if the MS_BIND flag is detected. At this point the
> > source of the mount has been successfully converted to a path struct
> > using the kernel's kern_path API. This allows LSMs to target bind mount
> > requests at the right stage, and evaluate the attributes in the right
> > format, based on the type of mount.
> >
> > This does not affect the functionality of the existing mount security
> > hooks, including security_sb_mount. The new hook, can be utilized as a
> > supplement to the main hook for further analyzing bind mount requests.
> > This means that there is still the option of only using the main hook
> > function, if all one wants to do is indiscriminately reject all bind
> > mount requests, regardless of the source and destination arguments.
> > However, if one needs to evaluate the source and destination of a bind
> > mount request before making a decision, this hook function should be
> > preferred. Of course, if a bind mount request does not make it past the
> > security_sb_mount check, the bind mount hook function is never invoked.
> >
> > Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> > ---
> >  fs/namespace.c                |  4 ++++
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  1 +
> >  security/security.c           | 16 ++++++++++++++++
> >  4 files changed, 22 insertions(+)
>
> Like Casey I'm not really excited about such a specific LSM hook, but
> unfortunately we can't simply call kern_path() in the existing
> security_sb_mount() callback as that could end up resolving to
> something different than the call in do_loopback() which would be bad.
> Moving the kern_path() call up into path_mount() just looks like a bad
> idea anyway you look at it.  Unfortunately I don't really see an
> alternative to what you're proposing, so I guess we're kinda stuck
> with this as a solution, unless someone can think of something better.
>
> I'm going to need to see an ACK from the VFS folks on this before I
> merge the new hook.
>
> I'd also stick with the security_sb_bindmount() name as opposed to the
> XXX_path() suggestion from Casey simply to help distinguish it from
> the pathname based LSM hooks.  Yes, this is operating on the pathname,
> but bind mounts are a bit of a special case.
>
> Unrelated, but I just noticed that we are calling security_sb_mount()
> *before* may_mount(); that's the opposite order for most LSM hook
> placements where we do the discretionary/capabilities checks first and
> the LSM checks.  That's something we should look at, perhaps there is
> a good reason for the ordering being different, perhaps it's a
> mistake.
>
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 23e81c2a1e3f..c902608c9759 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
> >         if (err)
> >                 return err;
> >
> > +       err = security_sb_bindmount(&old_path, path);
> > +       if (err)
> > +               goto out;
>
> I might make a mention in the commit description that the
> do_reconfigure_mnt() case (MS_REMOUNT|MS_BIND) should be able to be
> handled using the existing security_sb_mount() hook.
>
> >         err = -EINVAL;
> >         if (mnt_ns_loop(old_path.dentry))
> >                 goto out;
>
> ...
>
> > diff --git a/security/security.c b/security/security.c
> > index 09664e09fec9..bd7cb3df16f4 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path,
> >         return call_int_hook(sb_mount, dev_name, path, type, flags, data);
> >  }
> >
> > +/**
> > + * security_sb_bindmount() - Loopback/bind mount specific permission check
> > + * @old_path: source of loopback/bind mount
> > + * @path: mount point
> > + *
> > + * This check is performed in addition to security_sb_mount and only if the
> > + * mount type is determined to be loopback/bind mount (flags & MS_BIND).  It
> > + * surfaces the mount source as a path struct.
>
> I wouldn't mention security_sb_mount() above as that makes the comment
> somewhat fragile in the face of changing hooks.  I would suggest
> something along these lines:
>
> "Beyond any general mounting hooks, this check is performed on an
>  initial loopback/bind mount (MS_BIND) with the mount source presented
>  as a path struct in @old_path."
>
> > + * Return: Returns 0 if permission is granted.
> > + */
> > +int security_sb_bindmount(const struct path *old_path, const struct path *path)
> > +{
> > +       return call_int_hook(sb_bindmount, old_path, path);
> > +}
> > +
> >  /**
> >   * security_sb_umount() - Check permission for unmounting a filesystem
> >   * @mnt: mounted filesystem
> >
> > base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
>
> --
> paul-moore.com

Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

[Shervin Oloumi]

On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:
> > The main mount security hook (security_sb_mount) is called early in the
> > process before the mount type is determined and the arguments are
> > validated and converted to the appropriate format. Specifically, the
> > source path is surfaced as a string, which is not appropriate for
> > checking bind mount requests. For bind mounts the source should be
> > validated and passed as a path struct (same as destination), after the
> > mount type is determined. This allows the hook users to evaluate the
> > mount attributes without the need to perform any validations or
> > conversions out of band, which can introduce a TOCTOU race condition.
> >
> > The newly introduced hook is invoked only if the security_sb_mount hook
> > passes, and only if the MS_BIND flag is detected. At this point the
> > source of the mount has been successfully converted to a path struct
> > using the kernel's kern_path API. This allows LSMs to target bind mount
> > requests at the right stage, and evaluate the attributes in the right
> > format, based on the type of mount.
> >
> > This does not affect the functionality of the existing mount security
> > hooks, including security_sb_mount. The new hook, can be utilized as a
> > supplement to the main hook for further analyzing bind mount requests.
> > This means that there is still the option of only using the main hook
> > function, if all one wants to do is indiscriminately reject all bind
> > mount requests, regardless of the source and destination arguments.
> > However, if one needs to evaluate the source and destination of a bind
> > mount request before making a decision, this hook function should be
> > preferred. Of course, if a bind mount request does not make it past the
> > security_sb_mount check, the bind mount hook function is never invoked.
> >
> > Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
>
> Christian is much more experienced in this area than me but let me share my
> thoughts before he returns from vacation.
>
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 23e81c2a1e3f..c902608c9759 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
> >       if (err)
> >               return err;
> >
> > +     err = security_sb_bindmount(&old_path, path);
> > +     if (err)
> > +             goto out;
> > +
>
> So this gets triggered for the legacy mount API path (mount(2) syscall).
> For the new mount API, I can see open_tree() does not have any security
> hook. Is that intented? Are you catching equivalent changes done through
> the new mount API inside security_move_mount() hook?

I am not very familiar with the new API and when it is used, but LandLock does
listen to security_move_mount() and it rejects all such requests. It also
listens to and rejects remount and pivotroot. Are there cases where bind mount
requests go through open_tree() and this hook is bypassed?

> Also what caught my eye is that the LSM doesn't care at all whether this is
> a recursive bind mount (copying all mounts beneath the specified one) or
> just a normal one (copying only a single mount). Maybe that's fine but it
> seems a bit counter-intuitive to me as AFAIU it implicitly places a
> requirement on the policy that if doing some bind mount is forbidden, then
> doing bind mount of any predecessor must be forbidden as well (otherwise
> the policy will be inconsistent).

I need to double check this with Mickaël, but I think it is safe to allow
recursive bind mounts. If a bind mount was allowed, let's say /A -> /home/B,
then we already verified that the process did not gain more access (or even
dropped some access) through the new mount point, e.g. accessing /A/a through
/home/B/a. So with a recursive bind mount, let's say /home -> /C, once we check
for privilege escalation as usual, it should be safe to clone any existing sub
mounts in the new destination. Because if access through B <= A and C <= B then
access through C <= A. So back to your point, there should never exist an
illegal bind mount action that can implicitly happen through a legal recursive
bind mount of its predecessor. Regardless, I think it might be useful to know if
a mount is recursive for other use cases so I added a boolean for surfacing
MS_REC in the new patches.

>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v3 1/2] fs: add loopback/bind mount specific security hook

[Christian Brauner]

On Thu, Jan 09, 2025 at 06:10:07PM -0800, Shervin Oloumi wrote:
> The main mount security hook (security_sb_mount) is called early in the
> process before the mount type is determined and the arguments are
> validated and converted to the appropriate format. Specifically, the
> source path is surfaced as a string, which is not appropriate for
> checking bind mount requests. For bind mounts the source should be
> validated and passed as a path struct (same as destination), after the
> mount type is determined. This allows the hook users to evaluate the
> mount attributes without the need to perform any validations or
> conversions out of band, which can introduce a TOCTOU race condition.
> 
> The newly introduced hook is invoked only if the security_sb_mount hook
> passes, and only if the MS_BIND flag is detected. The
> do_reconfigure_mnt() case (MS_REMOUNT | MS_BIND) is still handled by the
> existing security_sb_mount() hook. When the new bind mount hook is
> called, the source of the mount has already been successfully converted
> to a path struct using the kernel's kern_path API. This allows LSMs to
> target bind mount requests at the right stage, and evaluate the
> attributes in the right format, based on the type of mount. This bind
> mount hook also signals the existence of MS_REC flag via a boolean.
> 
> This does not affect the functionality of the existing mount security
> hooks, including security_sb_mount. The new hook, can be utilized as a
> supplement to the main hook for further analyzing bind mount requests.
> This means that there is still the option of only using the main hook
> function, if all one wants to do is indiscriminately reject all bind
> mount requests, regardless of the source and destination arguments.
> However, if one needs to evaluate the source and destination of a bind
> mount request before making a decision, this hook function should be
> preferred. Of course, if a bind mount request does not make it past the
> security_sb_mount check, the bind mount hook function is never invoked.
> 
> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> ---
> 
> Changes since v2:
> - Reword the commit to include how do_reconfigure_mnt() is handled and
>   mention that MS_REC is also surfaced in the hook
> 
> Changes since v1:
> - Indicate whether the mount is recursive in the hook. This can be a
>   factor when deciding if a mount should be allowed
> - Add default capabilities function for the new hook in security.h. This
>   is required for some tests to pass
> - Reword the hook description to be more future proof
> ---
>  fs/namespace.c                |  4 ++++
>  include/linux/lsm_hook_defs.h |  2 ++
>  include/linux/security.h      |  7 +++++++
>  security/security.c           | 18 ++++++++++++++++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 23e81c2a1e3f..535a1841c200 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
>  	if (err)
>  		return err;
>  
> +	err = security_sb_bindmount(&old_path, path, recurse ? true : false);
> +	if (err)
> +		goto out;
> +
>  	err = -EINVAL;
>  	if (mnt_ns_loop(old_path.dentry))
>  		goto out;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index eb2937599cb0..94f5a3530b98 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -71,6 +71,8 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
>  LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
>  LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
>  	 const char *type, unsigned long flags, void *data)
> +LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path,
> +	 const struct path *path, bool recurse)
>  LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
>  LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
>  	 const struct path *new_path)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index cbdba435b798..d4a4c71865e3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
>  int security_sb_statfs(struct dentry *dentry);
>  int security_sb_mount(const char *dev_name, const struct path *path,
>  		      const char *type, unsigned long flags, void *data);
> +int security_sb_bindmount(const struct path *old_path, const struct path *path, bool recurse);
>  int security_sb_umount(struct vfsmount *mnt, int flags);
>  int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
>  int security_sb_set_mnt_opts(struct super_block *sb,
> @@ -801,6 +802,12 @@ static inline int security_sb_mount(const char *dev_name, const struct path *pat
>  	return 0;
>  }
>  
> +static inline int security_sb_bindmount(const struct path *old_path,
> +					const struct path *path, bool recurse)
> +{
> +	return 0;
> +}
> +
>  static inline int security_sb_umount(struct vfsmount *mnt, int flags)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 09664e09fec9..c115d8627e2d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1564,6 +1564,24 @@ int security_sb_mount(const char *dev_name, const struct path *path,
>  	return call_int_hook(sb_mount, dev_name, path, type, flags, data);
>  }
>  
> +/**
> + * security_sb_bindmount() - Loopback/bind mount permission check
> + * @old_path: source of loopback/bind mount
> + * @path: mount point
> + * @recurse: true if recursive (MS_REC)
> + *
> + * Beyond any general mounting hooks, this check is performed on an initial
> + * loopback/bind mount (MS_BIND) with the mount source presented as a path
> + * struct in @old_path.
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_sb_bindmount(const struct path *old_path, const struct path *path,
> +			  bool recurse)
> +{
> +	return call_int_hook(sb_bindmount, old_path, path, recurse);
> +}
> +
>  /**
>   * security_sb_umount() - Check permission for unmounting a filesystem
>   * @mnt: mounted filesystem
> 
> base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

On Thu, Jan 09, 2025 at 08:14:07PM -0800, Shervin Oloumi wrote:
> On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:
> > > The main mount security hook (security_sb_mount) is called early in the
> > > process before the mount type is determined and the arguments are
> > > validated and converted to the appropriate format. Specifically, the
> > > source path is surfaced as a string, which is not appropriate for
> > > checking bind mount requests. For bind mounts the source should be
> > > validated and passed as a path struct (same as destination), after the
> > > mount type is determined. This allows the hook users to evaluate the
> > > mount attributes without the need to perform any validations or
> > > conversions out of band, which can introduce a TOCTOU race condition.
> > >
> > > The newly introduced hook is invoked only if the security_sb_mount hook
> > > passes, and only if the MS_BIND flag is detected. At this point the
> > > source of the mount has been successfully converted to a path struct
> > > using the kernel's kern_path API. This allows LSMs to target bind mount
> > > requests at the right stage, and evaluate the attributes in the right
> > > format, based on the type of mount.
> > >
> > > This does not affect the functionality of the existing mount security
> > > hooks, including security_sb_mount. The new hook, can be utilized as a
> > > supplement to the main hook for further analyzing bind mount requests.
> > > This means that there is still the option of only using the main hook
> > > function, if all one wants to do is indiscriminately reject all bind
> > > mount requests, regardless of the source and destination arguments.
> > > However, if one needs to evaluate the source and destination of a bind
> > > mount request before making a decision, this hook function should be
> > > preferred. Of course, if a bind mount request does not make it past the
> > > security_sb_mount check, the bind mount hook function is never invoked.
> > >
> > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> >
> > Christian is much more experienced in this area than me but let me share my
> > thoughts before he returns from vacation.
> >
> > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > index 23e81c2a1e3f..c902608c9759 100644
> > > --- a/fs/namespace.c
> > > +++ b/fs/namespace.c
> > > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
> > >       if (err)
> > >               return err;
> > >
> > > +     err = security_sb_bindmount(&old_path, path);
> > > +     if (err)
> > > +             goto out;
> > > +
> >
> > So this gets triggered for the legacy mount API path (mount(2) syscall).
> > For the new mount API, I can see open_tree() does not have any security
> > hook. Is that intented? Are you catching equivalent changes done through
> > the new mount API inside security_move_mount() hook?
> 
> I am not very familiar with the new API and when it is used, but LandLock does
> listen to security_move_mount() and it rejects all such requests. It also
> listens to and rejects remount and pivotroot. Are there cases where bind mount
> requests go through open_tree() and this hook is bypassed?

Whether or not Landlock currently blocks security_move_mount()
unconditionally doesn't matter. Introducing this api will have to do it
for the old and the new mount api. First, because we don't implement new
features for the old mount api that aren't also available in the new
mount api. Second, because this asymmetry just begs for bugs when
security_move_mount() isn't blocked anymore.

And third, there seems to be a misconception here.
open_tree(OPEN_TREE_CLONE) gives you an unattached bind-mount.
move_mount() is just sugar on top to attach it to a mount namespace.

But a file descriptor from open_tree(OPEN_TREE_CLONE) can be interacted
with just fine, i.e., read, write, create, shared with other processes.
You could create a bind-mount that is never attached anywhere. So I'm
not sure what security guarantees it would give you to block MS_BIND but
not OPEN_TREE_CLONE.

It should be done for both.

> 
> > Also what caught my eye is that the LSM doesn't care at all whether this is
> > a recursive bind mount (copying all mounts beneath the specified one) or
> > just a normal one (copying only a single mount). Maybe that's fine but it
> > seems a bit counter-intuitive to me as AFAIU it implicitly places a
> > requirement on the policy that if doing some bind mount is forbidden, then
> > doing bind mount of any predecessor must be forbidden as well (otherwise
> > the policy will be inconsistent).
> 
> I need to double check this with Mickaël, but I think it is safe to allow
> recursive bind mounts. If a bind mount was allowed, let's say /A -> /home/B,
> then we already verified that the process did not gain more access (or even
> dropped some access) through the new mount point, e.g. accessing /A/a through
> /home/B/a. So with a recursive bind mount, let's say /home -> /C, once we check
> for privilege escalation as usual, it should be safe to clone any existing sub
> mounts in the new destination. Because if access through B <= A and C <= B then
> access through C <= A. So back to your point, there should never exist an
> illegal bind mount action that can implicitly happen through a legal recursive
> bind mount of its predecessor. Regardless, I think it might be useful to know if
> a mount is recursive for other use cases so I added a boolean for surfacing
> MS_REC in the new patches.

Say /home/hidden is covered by a bind-mount of /dev/null and you're
doing mount --bind /home /somewhere then /home/hidden will be uncovered
(There's cases where the kernel itself fuses mounts together or "locks"
them so things like this cannot happen e.g., when creating an
unprivileged mount namespace.). If your policy blocks unmounting
/home/hidden to protect the underlying file then a non-recursive
bind-mount would be able to circumvent that restriction.

Re: [PATCH v3 1/2] fs: add loopback/bind mount specific security hook

[Mickaël Salaün]

On Fri, Jan 10, 2025 at 04:42:19PM +0100, Christian Brauner wrote:

> On Thu, Jan 09, 2025 at 08:14:07PM -0800, Shervin Oloumi wrote:
> > On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:

> > > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > > index 23e81c2a1e3f..c902608c9759 100644
> > > > --- a/fs/namespace.c
> > > > +++ b/fs/namespace.c
> > > > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
> > > >       if (err)
> > > >               return err;
> > > >
> > > > +     err = security_sb_bindmount(&old_path, path);
> > > > +     if (err)
> > > > +             goto out;
> > > > +
> > >
> > > So this gets triggered for the legacy mount API path (mount(2) syscall).
> > > For the new mount API, I can see open_tree() does not have any security
> > > hook. Is that intented? Are you catching equivalent changes done through
> > > the new mount API inside security_move_mount() hook?
> > 
> > I am not very familiar with the new API and when it is used, but LandLock does
> > listen to security_move_mount() and it rejects all such requests. It also
> > listens to and rejects remount and pivotroot. Are there cases where bind mount
> > requests go through open_tree() and this hook is bypassed?
> 
> Whether or not Landlock currently blocks security_move_mount()
> unconditionally doesn't matter. Introducing this api will have to do it
> for the old and the new mount api. First, because we don't implement new
> features for the old mount api that aren't also available in the new
> mount api. Second, because this asymmetry just begs for bugs when
> security_move_mount() isn't blocked anymore.
> 
> And third, there seems to be a misconception here.
> open_tree(OPEN_TREE_CLONE) gives you an unattached bind-mount.
> move_mount() is just sugar on top to attach it to a mount namespace.
> 
> But a file descriptor from open_tree(OPEN_TREE_CLONE) can be interacted
> with just fine, i.e., read, write, create, shared with other processes.
> You could create a bind-mount that is never attached anywhere. So I'm
> not sure what security guarantees it would give you to block MS_BIND but
> not OPEN_TREE_CLONE.
> 
> It should be done for both.

Yes, the new hook should probably be called by attach_recursive_mnt().

Landlock tests should check with the legacy mount(2) and the new
move_mount(2) (e.g. with test variants).

> 
> > 
> > > Also what caught my eye is that the LSM doesn't care at all whether this is
> > > a recursive bind mount (copying all mounts beneath the specified one) or
> > > just a normal one (copying only a single mount). Maybe that's fine but it
> > > seems a bit counter-intuitive to me as AFAIU it implicitly places a
> > > requirement on the policy that if doing some bind mount is forbidden, then
> > > doing bind mount of any predecessor must be forbidden as well (otherwise
> > > the policy will be inconsistent).
> > 
> > I need to double check this with Mickaël, but I think it is safe to allow
> > recursive bind mounts. If a bind mount was allowed, let's say /A -> /home/B,
> > then we already verified that the process did not gain more access (or even
> > dropped some access) through the new mount point, e.g. accessing /A/a through
> > /home/B/a. So with a recursive bind mount, let's say /home -> /C, once we check
> > for privilege escalation as usual, it should be safe to clone any existing sub
> > mounts in the new destination. Because if access through B <= A and C <= B then
> > access through C <= A. So back to your point, there should never exist an
> > illegal bind mount action that can implicitly happen through a legal recursive
> > bind mount of its predecessor. Regardless, I think it might be useful to know if
> > a mount is recursive for other use cases so I added a boolean for surfacing
> > MS_REC in the new patches.
> 
> Say /home/hidden is covered by a bind-mount of /dev/null and you're
> doing mount --bind /home /somewhere then /home/hidden will be uncovered
> (There's cases where the kernel itself fuses mounts together or "locks"
> them so things like this cannot happen e.g., when creating an
> unprivileged mount namespace.). If your policy blocks unmounting
> /home/hidden to protect the underlying file then a non-recursive
> bind-mount would be able to circumvent that restriction.

That would be a valid attack scenario.  For Landlock, to make it simple,
non-recursive bind mounts should always be denied.

Shervin, please add this scenario in a comment for the Landlock
implementation of the hook.

Re: [PATCH v3 2/2] landlock: add support for private bind mount

[Mickaël Salaün]

On Thu, Jan 09, 2025 at 06:10:08PM -0800, Shervin Oloumi wrote:
> This patch introduces a new LandLock rule, LANDLOCK_ACCESS_FS_MOUNT for
> controlling bind mount operations. While this patch mostly focuses on
> bind mounts, the long-term goal is to utilize this rule for controlling
> regular device mounts as well. To perform a successful bind mount, both
> the source parent and the destination need to have the REFER rule and
> the destination needs to carry the MOUNT rule. Note that this does imply
> that the MOUNT rule needs to also exist in the source hierarchy, to
> avoid rejection due to privilege escalation. The same path access logic
> as REFER (used for rename/move and link) is used to check for any
> possible privilege escalations before allowing the bind mount.

Thanks for this patch series!

FYI, the bind mount is the tricky part, but it would make sense for
LANDLOCK_ACCESS_FS_MOUNT to handle any mount type the same way (e.g.
block device, tmpfs, proc...).  We should be able to incrementally go
there with a new rule type that can define a source of a mount (e.g. FS
type, major/minor block device...).  So, this design should be OK.

> 
> Additionally, only private bind mounts are allowed. This is to avoid
> filesystem hierarchy changes that happen through mount propagation
> between peer mount groups. For instance, bind mounting a directory in a
> shared mount namespace, can result in hierarchy changes propagating
> across the peer groups in that namespace. Such changes cannot be checked
> for privilege escalation as the REFER check only covers the source and
> the destination surfaced in the bind mount hook function. We do allow
> recursive bind mounts. This is safe because, if a bind mount already
> exists in some location, it must have passed the REFER check, so if its
> parent passes the REFER check with respect to a new mount point, by
> definition the child bind mount also passes the REFER check with respect
> to that new mount point, and so it can be safely cloned to the new
> location along with its predecessor bind mount.
> 
> If a bind mount request carries a --make-shared flag, or any flag other
> than --make-private, the bind mount succeeds while the subsequent
> propagation type change attempt fails. This is because, the kernel calls
> the mount hook twice, first with the MS_BIND flag, and then with the
> flag for the propagation type change. If the bind mount request carries
> the --make-private flag, both the mount operation and the subsequent
> propagation type change succeed. Any mount request with --make-private
> or --make-rprivate flags are also allowed. Such requests operate on
> existing mount points, changing the propagation type to private. In this
> case any previously propagated mounts would continue to exist, but
> additional bind mounts under the newly privatized mount point, would not
> propagate anymore.
> 
> Finally, any existing mounts or bind mounts before the process enters a
> LandLock domain remain as they are. Such mounts can be of the shared
> propagation type, and they would continue to share updates with the rest
> of their peer group. While this is an existing behavior, after this
> patch

> such mounts can also be remounted as private,

OK

> or be unmounted after the process enters the sandbox.

As Christian pointed out, being able to unmount pre-sandbox mount points
could give access to previously-hidden files.  For unmounts, we should
have a dedicated LANDLOCK_ACCESS_FS_UNMOUNT right and highlight in the
documentation the risk of unveiling hidden files.

> Existing mounts are outside the
> scope of LandLock and should be considered before entering the sandbox.
> 
> Tests: Regular mount is rejected. Bind mount is rejected if either the
> source parent or the mount point do not have the REFER rule, or if the
> destination does not have the MOUNT rule. Bind mount is allowed only if
> the REFER check passes, i.e.: no restrictions are dropped. Recursive
> bind mounts are allowed, as long as they abide by the same rules. Bind
> mounting a directory onto itself is always allowed. Mount calls with the
> flags --make-private or --make-rprivate are always allowed. Unmounting
> is always allowed. Link and rename functionality is unaffected. All
> LandLock filesystem tests pass.

Please add at least one dedicated patch for tests.  Existing tests
should all pass for each patch.

> 
> Link: https://github.com/landlock-lsm/linux/issues/14

This should be:
Closes: https://github.com/landlock-lsm/linux/issues/14

> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> ---
> 
> Changes since v2:
> - Change the flag check conditions in the main mount hook function to
>   allow any type of request carrying MS_BIND or MS_PRIVATE, as opposed
>   to requests carrying only those flags, effectively allowing MS_REC
> - Update the LandLock filesystem self tests to reflect the condition
>   change that results in MS_PRIVATE | MS_REC succeeding
> - Update the bind mount tests for readability
> - Update the commit message to reflect the above updates
> 
> Changes since v1:
> - Update the failing static assert in ruleset.h
> - Update the bind mount hook function to include the newly added
>   argument, recurse
> ---
>  include/uapi/linux/landlock.h              |   1 +
>  security/landlock/fs.c                     | 104 ++++++++++++-------
>  security/landlock/limits.h                 |   2 +-
>  security/landlock/ruleset.h                |   6 +-
>  tools/testing/selftests/landlock/fs_test.c | 114 +++++++++++++++++++--
>  5 files changed, 175 insertions(+), 52 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 33745642f787..10541a001f2f 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -259,6 +259,7 @@ struct landlock_net_port_attr {
>  #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>  #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>  #define LANDLOCK_ACCESS_FS_IOCTL_DEV			(1ULL << 15)
> +#define LANDLOCK_ACCESS_FS_MOUNT			(1ULL << 16)
>  /* clang-format on */
>  
>  /**
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index e31b97a9f175..df0a3094d90b 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -35,6 +35,7 @@
>  #include <linux/workqueue.h>
>  #include <uapi/linux/fiemap.h>
>  #include <uapi/linux/landlock.h>
> +#include <uapi/linux/mount.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -1033,34 +1034,36 @@ static bool collect_domain_accesses(
>  }
>  
>  /**
> - * current_check_refer_path - Check if a rename or link action is allowed
> + * current_check_reparent_path - Check if a reparent action is allowed

I think we should keep this function's name.  There is no "reparent"
access right.

>   *
> - * @old_dentry: File or directory requested to be moved or linked.
> - * @new_dir: Destination parent directory.
> + * @old_dentry: Source file or directory for move, link or bind mount.
> + * @new_dir: Destination parent directory for move and link, or destination
> + *     directory itself for bind mount (mount point).
>   * @new_dentry: Destination file or directory.
>   * @removable: Sets to true if it is a rename operation.
>   * @exchange: Sets to true if it is a rename operation with RENAME_EXCHANGE.
> + * @bind_mount: Sets to true if it is a bind mount operation.
>   *
>   * Because of its unprivileged constraints, Landlock relies on file hierarchies
> - * (and not only inodes) to tie access rights to files.  Being able to link or
> - * rename a file hierarchy brings some challenges.  Indeed, moving or linking a
> - * file (i.e. creating a new reference to an inode) can have an impact on the
> - * actions allowed for a set of files if it would change its parent directory
> - * (i.e. reparenting).
> + * (and not only inodes) to tie access rights to files.  Being able to link,
> + * rename or bind mount a file hierarchy brings some challenges.  Indeed, these
> + * operations create a new reference to an inode, which can have an impact on
> + * the actions allowed for a set of files if it would change its parent
> + * directory (i.e. reparenting).
>   *
>   * To avoid trivial access right bypasses, Landlock first checks if the file or
>   * directory requested to be moved would gain new access rights inherited from
>   * its new hierarchy.  Before returning any error, Landlock then checks that
>   * the parent source hierarchy and the destination hierarchy would allow the
> - * link or rename action.  If it is not the case, an error with EACCES is
> - * returned to inform user space that there is no way to remove or create the
> - * requested source file type.  If it should be allowed but the new inherited
> - * access rights would be greater than the source access rights, then the
> - * kernel returns an error with EXDEV.  Prioritizing EACCES over EXDEV enables
> + * link, rename or bind mount action.  If it is not the case, an error with
> + * EACCES is returned to inform user space that there is no way to remove or
> + * create the requested source file type.  If it should be allowed but the new
> + * inherited access rights would be greater than the source access rights, then
> + * the kernel returns an error with EXDEV or EPERM.  Prioritizing EACCES enables
>   * user space to abort the whole operation if there is no way to do it, or to
>   * manually copy the source to the destination if this remains allowed, e.g.
>   * because file creation is allowed on the destination directory but not direct
> - * linking.
> + * linking, or bind mounting.
>   *
>   * To achieve this goal, the kernel needs to compare two file hierarchies: the
>   * one identifying the source file or directory (including itself), and the
> @@ -1082,13 +1085,18 @@ static bool collect_domain_accesses(
>   *
>   * Returns:
>   * - 0 if access is allowed;
> - * - -EXDEV if @old_dentry would inherit new access rights from @new_dir;
> + * - -EXDEV if @old_dentry would inherit new access rights from @new_dir through
> + *       link or rename.
> + * - -EPERM if @old_dentry would inherit new access rights from @new_dir through
> + *       bind mount.
>   * - -EACCES if file removal or creation is denied.
>   */
> -static int current_check_refer_path(struct dentry *const old_dentry,
> -				    const struct path *const new_dir,
> -				    struct dentry *const new_dentry,
> -				    const bool removable, const bool exchange)
> +static int current_check_reparent_path(struct dentry *const old_dentry,
> +				       const struct path *const new_dir,
> +				       struct dentry *const new_dentry,
> +				       const bool removable,
> +				       const bool exchange,
> +				       const bool bind_mount)
>  {
>  	const struct landlock_ruleset *const dom = get_current_fs_domain();
>  	bool allow_parent1, allow_parent2;
> @@ -1120,7 +1128,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  	}
>  
>  	/* The mount points are the same for old and new paths, cf. EXDEV. */
> -	if (old_dentry->d_parent == new_dir->dentry) {
> +	if ((bind_mount && old_dentry == new_dentry) ||
> +	    (!bind_mount && old_dentry->d_parent == new_dentry->d_parent)) {
>  		/*
>  		 * The LANDLOCK_ACCESS_FS_REFER access right is not required
>  		 * for same-directory referer (i.e. no reparenting).
> @@ -1160,6 +1169,14 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  	if (allow_parent1 && allow_parent2)
>  		return 0;
>  
> +	if (bind_mount) {
> +		if (!is_access_to_paths_allowed(
> +			    dom, new_dir, LANDLOCK_ACCESS_FS_MOUNT,
> +			    &layer_masks_parent2, NULL, 0, NULL, NULL)) {
> +			return -EPERM;
> +		}
> +	}

Can this check be done in the bind_mount hook?

> +
>  	/*
>  	 * To be able to compare source and destination domain access rights,
>  	 * take into account the @old_dentry access rights aggregated with its
> @@ -1173,7 +1190,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  		return 0;
>  
>  	/*
> -	 * This prioritizes EACCES over EXDEV for all actions, including
> +	 * This prioritizes EACCES over EXDEV/EPERM for all actions, including
>  	 * renames with RENAME_EXCHANGE.
>  	 */
>  	if (likely(is_eacces(&layer_masks_parent1, access_request_parent1) ||
> @@ -1186,6 +1203,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  	 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
>  	 * source or the destination.
>  	 */
> +	if (bind_mount)
> +		return -EPERM;
>  	return -EXDEV;
>  }
>  
> @@ -1319,9 +1338,11 @@ static void hook_sb_delete(struct super_block *const sb)
>   * topology (i.e. the mount namespace), changing it may grant access to files
>   * not previously allowed.
>   *
> - * To make it simple, deny any filesystem topology modification by landlocked
> - * processes.  Non-landlocked processes may still change the namespace of a
> - * landlocked process, but this kind of threat must be handled by a system-wide
> + * Currently, we can safely handle private bind mounts, as the source and
> + * destination restrictions can be compared, however all other types of mount
> + * system calls are blocked, including shared bind mounts and device mounts.
> + * Non-landlocked processes may still change the namespace of a landlocked
> + * process, but this kind of threat must be handled by a system-wide
>   * access-control security policy.
>   *
>   * This could be lifted in the future if Landlock can safely handle mount
> @@ -1338,22 +1359,26 @@ static int hook_sb_mount(const char *const dev_name,
>  {
>  	if (!get_current_fs_domain())
>  		return 0;
> +
> +	/*
> +	 * Allow bind mount and make-private requests to proceed, including requests
> +	 * with the MS_REC flag.  For bind mount requests, further security checks
> +	 * are performed in the bind mount specific hook.
> +	 */
> +	if (flags && ((MS_BIND | MS_PRIVATE) & flags))

Why MS_PRIVATE?

> +		return 0;
>  	return -EPERM;
>  }
>  
> -static int hook_move_mount(const struct path *const from_path,
> -			   const struct path *const to_path)
> +static int hook_sb_bindmount(const struct path *const old_path,
> +			     const struct path *const path, bool recurse)
>  {
> -	if (!get_current_fs_domain())
> -		return 0;
> -	return -EPERM;
> +	return current_check_reparent_path(old_path->dentry, path, path->dentry,
> +					   false, false, true);
>  }
>  
> -/*
> - * Removing a mount point may reveal a previously hidden file hierarchy, which
> - * may then grant access to files, which may have previously been forbidden.
> - */
> -static int hook_sb_umount(struct vfsmount *const mnt, const int flags)

Why do you remove this hook?  Umounts should not be allowed by default.

> +static int hook_move_mount(const struct path *const from_path,
> +			   const struct path *const to_path)
>  {
>  	if (!get_current_fs_domain())
>  		return 0;
> @@ -1389,8 +1414,8 @@ static int hook_path_link(struct dentry *const old_dentry,
>  			  const struct path *const new_dir,
>  			  struct dentry *const new_dentry)
>  {
> -	return current_check_refer_path(old_dentry, new_dir, new_dentry, false,
> -					false);
> +	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
> +					   false, false, false);
>  }
>  
>  static int hook_path_rename(const struct path *const old_dir,
> @@ -1400,8 +1425,9 @@ static int hook_path_rename(const struct path *const old_dir,
>  			    const unsigned int flags)
>  {
>  	/* old_dir refers to old_dentry->d_parent and new_dir->mnt */
> -	return current_check_refer_path(old_dentry, new_dir, new_dentry, true,
> -					!!(flags & RENAME_EXCHANGE));
> +	return current_check_reparent_path(old_dentry, new_dir, new_dentry,
> +					   true, !!(flags & RENAME_EXCHANGE),
> +					   false);
>  }
>  
>  static int hook_path_mkdir(const struct path *const dir,
> @@ -1652,8 +1678,8 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  
>  	LSM_HOOK_INIT(sb_delete, hook_sb_delete),
>  	LSM_HOOK_INIT(sb_mount, hook_sb_mount),
> +	LSM_HOOK_INIT(sb_bindmount, hook_sb_bindmount),
>  	LSM_HOOK_INIT(move_mount, hook_move_mount),
> -	LSM_HOOK_INIT(sb_umount, hook_sb_umount),
>  	LSM_HOOK_INIT(sb_remount, hook_sb_remount),
>  	LSM_HOOK_INIT(sb_pivotroot, hook_sb_pivotroot),
>  
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 15f7606066c8..b495b15df25d 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>  #define LANDLOCK_MAX_NUM_LAYERS		16
>  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>  
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_MOUNT
>  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>  
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 631e24d4ffe9..62e1db3dd95a 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -31,7 +31,7 @@
>  	LANDLOCK_ACCESS_FS_REFER)
>  /* clang-format on */
>  
> -typedef u16 access_mask_t;
> +typedef u32 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. */
> @@ -50,11 +50,11 @@ struct access_masks {
>  
>  union access_masks_all {
>  	struct access_masks masks;
> -	u32 all;
> +	u64 all;

Why this change?

>  };
>  
>  /* Makes sure all fields are covered. */
> -static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
> +static_assert(sizeof(typeof_member(union access_masks_all, masks)) <=
>  	      sizeof(typeof_member(union access_masks_all, all)));
>  
>  typedef u16 layer_mask_t;
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 6788762188fe..ab59bea0125b 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -274,6 +274,11 @@ static int mount_opt(const struct mnt_opt *const mnt, const char *const target)
>  		     mnt->data);
>  }
>  
> +static int bindmount(const char *const source, const char *const target)
> +{
> +	return mount(source, target, NULL, MS_BIND, NULL);

I don't think this helper is useful.  Moreover we should use both
mount(2) and move_mount(2) e.g., one variant for each.

> +}
> +
>  static void prepare_layout_opt(struct __test_metadata *const _metadata,
>  			       const struct mnt_opt *const mnt)
>  {
> @@ -558,7 +563,7 @@ TEST_F_FORK(layout1, inval)
>  	LANDLOCK_ACCESS_FS_TRUNCATE | \
>  	LANDLOCK_ACCESS_FS_IOCTL_DEV)
>  
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_MOUNT
>  
>  #define ACCESS_ALL ( \
>  	ACCESS_FILE | \
> @@ -572,7 +577,8 @@ TEST_F_FORK(layout1, inval)
>  	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
>  	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
>  	LANDLOCK_ACCESS_FS_MAKE_SYM | \
> -	LANDLOCK_ACCESS_FS_REFER)
> +	LANDLOCK_ACCESS_FS_REFER | \
> +	LANDLOCK_ACCESS_FS_MOUNT)
>  
>  /* clang-format on */
>  
> @@ -1735,14 +1741,14 @@ TEST_F_FORK(layout1, topology_changes_with_net_only)
>  	enforce_ruleset(_metadata, ruleset_fd);
>  	ASSERT_EQ(0, close(ruleset_fd));
>  
> -	/* Mount, remount, move_mount, umount, and pivot_root checks. */
> +	/* Mount, remount, move_mount, pivot_root and umount checks. */

We should not remove tests for a new feature.

>  	set_cap(_metadata, CAP_SYS_ADMIN);
>  	ASSERT_EQ(0, mount_opt(&mnt_tmp, dir_s1d2));
>  	ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC, NULL));
>  	ASSERT_EQ(0, syscall(__NR_move_mount, AT_FDCWD, dir_s1d2, AT_FDCWD,
>  			     dir_s2d2, 0));
> -	ASSERT_EQ(0, umount(dir_s2d2));

Why?

>  	ASSERT_EQ(0, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
> +	ASSERT_EQ(0, umount(dir_s2d2));
>  	ASSERT_EQ(0, chdir("/"));
>  	clear_cap(_metadata, CAP_SYS_ADMIN);
>  }
> @@ -1763,19 +1769,17 @@ TEST_F_FORK(layout1, topology_changes_with_net_and_fs)
>  	enforce_ruleset(_metadata, ruleset_fd);
>  	ASSERT_EQ(0, close(ruleset_fd));
>  
> -	/* Mount, remount, move_mount, umount, and pivot_root checks. */
> +	/* Mount, remount, move_mount, pivot_root and umount checks. */
>  	set_cap(_metadata, CAP_SYS_ADMIN);
>  	ASSERT_EQ(-1, mount_opt(&mnt_tmp, dir_s1d2));
>  	ASSERT_EQ(EPERM, errno);
> -	ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC, NULL));
> -	ASSERT_EQ(EPERM, errno);

All these tests should be kept as is, they ensure that the current
restrictions are not changed with new kernel changes.

>  	ASSERT_EQ(-1, syscall(__NR_move_mount, AT_FDCWD, dir_s3d2, AT_FDCWD,
>  			      dir_s2d2, 0));
>  	ASSERT_EQ(EPERM, errno);
> -	ASSERT_EQ(-1, umount(dir_s3d2));
> -	ASSERT_EQ(EPERM, errno);

ditto

>  	ASSERT_EQ(-1, syscall(__NR_pivot_root, dir_s3d2, dir_s3d3));
>  	ASSERT_EQ(EPERM, errno);
> +	ASSERT_EQ(0, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC, NULL));
> +	ASSERT_EQ(0, umount(dir_s3d2));

ditto

>  	clear_cap(_metadata, CAP_SYS_ADMIN);
>  }
>  
> @@ -3029,6 +3033,98 @@ TEST_F_FORK(layout1, reparent_remove)
>  	ASSERT_EQ(EACCES, errno);
>  }
>  
> +TEST_F_FORK(layout1, reparent_bindmount_deny)
> +{
> +	const struct rule layer1[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MOUNT |
> +				  LANDLOCK_ACCESS_FS_READ_DIR,
> +		},
> +		{
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MOUNT |
> +				  LANDLOCK_ACCESS_FS_READ_DIR |
> +				  LANDLOCK_ACCESS_FS_MAKE_DIR,
> +		},
> +		{
> +			.path = dir_s3d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_READ_DIR |
> +				  LANDLOCK_ACCESS_FS_MAKE_DIR,
> +		},
> +		{},
> +	};
> +	const int ruleset_fd =
> +		create_ruleset(_metadata, layer1[1].access, layer1);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	/* Privilege escalation (FS_MAKE_DIR). */
> +	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s2d2));
> +	ASSERT_EQ(EPERM, errno);
> +
> +	/* Mount point missing FS_MOUNT. */
> +	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s3d2));
> +	ASSERT_EQ(EPERM, errno);
> +
> +	/* Mount point missing permissions other than FS_MOUNT. */
> +	ASSERT_EQ(-1, bindmount(dir_s2d2, dir_s1d2));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	/* Missing permissions other than FS_MOUNT, for a self bind mount. */
> +	ASSERT_EQ(-1, bindmount(dir_s1d2, dir_s1d2));
> +	ASSERT_EQ(EACCES, errno);
> +
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +}

These new tests look good.

> +
> +TEST_F_FORK(layout1, reparent_bindmount_allow)
> +{
> +	const struct rule layer1[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MOUNT,
> +		},
> +		{
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER |
> +				  LANDLOCK_ACCESS_FS_MOUNT |
> +				  LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{
> +			.path = dir_s3d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +	const int ruleset_fd =
> +		create_ruleset(_metadata, layer1[1].access, layer1);
> +
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	set_cap(_metadata, CAP_SYS_ADMIN);
> +
> +	/* Mount point has more restrictions than the source. */
> +	ASSERT_EQ(0, bindmount(dir_s2d2, dir_s1d2));
> +	ASSERT_EQ(0, umount(dir_s1d2));

We should have a specific access right for that.

> +
> +	/* Bind mounting a directory on itself with no FS_MOUNT. */
> +	ASSERT_EQ(0, bindmount(dir_s3d2, dir_s3d2));

Why should this be allowed?

> +	ASSERT_EQ(0, umount(dir_s3d2));
> +
> +	clear_cap(_metadata, CAP_SYS_ADMIN);
> +}

We also need tests to check mount of full filesystems (e.g. tmpfs) or
block devices.

> +
>  TEST_F_FORK(layout1, reparent_dom_superset)
>  {
>  	const struct rule layer1[] = {
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 
> 

Re: [PATCH v3 2/2] landlock: add support for private bind mount

[Mickaël Salaün]

On Thu, Jan 23, 2025 at 09:34:50PM +0100, Mickaël Salaün wrote:
> On Thu, Jan 09, 2025 at 06:10:08PM -0800, Shervin Oloumi wrote:

> > Finally, any existing mounts or bind mounts before the process enters a
> > LandLock domain remain as they are. Such mounts can be of the shared
> > propagation type, and they would continue to share updates with the rest
> > of their peer group. While this is an existing behavior, after this
> > patch
> 
> > such mounts can also be remounted as private,
> 
> OK
> 
> > or be unmounted after the process enters the sandbox.
> 
> As Christian pointed out, being able to unmount pre-sandbox mount points
> could give access to previously-hidden files.  For unmounts, we should
> have a dedicated LANDLOCK_ACCESS_FS_UNMOUNT right and highlight in the
> documentation the risk of unveiling hidden files.

Instead of a new access right, a better approach would be to require the
LANDLOCK_ACCESS_FS_MOUNT and that the mount point was created by the
task trying to unmount it (or one with less privileges).  This could be
done by recording the mount task's credential in struct
landlock_superblock_security and then checking that the task requesting
the unmount can ptrace this (mount) credential.

> 
> > Existing mounts are outside the
> > scope of LandLock and should be considered before entering the sandbox.

Re: [PATCH v3 2/2] landlock: add support for private bind mount

[Mickaël Salaün]

On Thu, Jan 23, 2025 at 10:08:30PM +0100, Mickaël Salaün wrote:
> On Thu, Jan 23, 2025 at 09:34:50PM +0100, Mickaël Salaün wrote:
> > On Thu, Jan 09, 2025 at 06:10:08PM -0800, Shervin Oloumi wrote:
> 
> > > Finally, any existing mounts or bind mounts before the process enters a
> > > LandLock domain remain as they are. Such mounts can be of the shared
> > > propagation type, and they would continue to share updates with the rest
> > > of their peer group. While this is an existing behavior, after this
> > > patch
> > 
> > > such mounts can also be remounted as private,
> > 
> > OK
> > 
> > > or be unmounted after the process enters the sandbox.
> > 
> > As Christian pointed out, being able to unmount pre-sandbox mount points
> > could give access to previously-hidden files.  For unmounts, we should
> > have a dedicated LANDLOCK_ACCESS_FS_UNMOUNT right and highlight in the
> > documentation the risk of unveiling hidden files.
> 
> Instead of a new access right, a better approach would be to require the
> LANDLOCK_ACCESS_FS_MOUNT and that the mount point was created by the
> task trying to unmount it (or one with less privileges).  This could be
> done by recording the mount task's credential in struct
> landlock_superblock_security and then checking that the task requesting
> the unmount can ptrace this (mount) credential.

The superblock cannot be used here, we'll need a new security blob,
probably in struct vfsmount.

Christian, would that be OK with you?

> 
> > 
> > > Existing mounts are outside the
> > > scope of LandLock and should be considered before entering the sandbox.