[PATCH 0/3] landlock: Restrict renameat2 with RENAME_WHITEOUT

[PATCH 0/3] landlock: Restrict renameat2 with RENAME_WHITEOUT

[Günther Noack]

Hello!

As discussed in [1], the renameat2() syscall's RENAME_WHITEOUT flag allows
the creation of chardev directory entries with major=minor=0 as "whiteout
objects" in the location of the rename source file [2].

This functionality is available even without having any OverlayFS mounted
and can be invoked with the regular renameat2(2) syscall [3].


Motivation
==========

The RENAME_WHITEOUT flag side-steps Landlock's LANDLOCK_ACCESS_FS_MAKE_CHAR
right, which is designed to restrict the creation of chardev device files.

This patch set fixes that by adding a check in Landlock's path_rename hook.


Tradeoffs considered in the implementation
==========================================

Q: Should we guard it with a dedicated LANDLOCK_ACCESS_FS_MAKE_WHITEOUT
   right?

   Pros:
   * This would be the fully backwards compatible solution,
     and Linux always strives for full backward compatibility.

   Cons:
   * Complicates the Landlock API surface for a very minor use case.

     In Debian Code search, the only use of RENAME_WHITEOUT from userspace
     seems to be for fuse-overlayfs.  It is used there for the same purpose
     as in the kernel OverlayFS and it likely does not run in a Landlock
     domain.

   The tradeoff does not seem worth it to me.  The chances that we break
   anyone with this seem very low, and I'm inclined to treat it as a bugfix
   for the existing LANDLOCK_ACCESS_FS_MAKE_CHAR right.


Q: Should we add a Landlock erratum for this?

   I punted on it for now, but we can do it if needed.

Q: Should the access right check be merged into the longer
   current_check_refer_path() function?

   I am leaning towards keeping it as a special case earlier.  This means
   that we traverse the source path twice, but as we have seen in Debian
   Code Search, there are apparently no legitimate callers of renameat2()
   with RENAME_WHITEOUT who are calling this from within a Landlock domain.
   (fuse-overlayfs is legitimate, but is not landlocked)

   It doesn't seem worth complicating our common rename code for a corner
   case that doesn't happen in practice.


[1] https://lore.kernel.org/all/adUBCQXrt7kmgqJT@google.com/
[2] https://docs.kernel.org/filesystems/overlayfs.html#whiteouts-and-opaque-directories
[3] https://man7.org/linux/man-pages/man2/renameat2.2.html#DESCRIPTION
[4] https://codesearch.debian.net/search?q=rename.*RENAME_WHITEOUT&literal=0


Günther Noack (3):
  landlock: Require LANDLOCK_ACCESS_FS_MAKE_CHAR for RENAME_WHITEOUT
  selftests/landlock: Add test for RENAME_WHITEOUT denial
  selftests/landlock: Test OverlayFS renames w/o
    LANDLOCK_ACCESS_FS_MAKE_CHAR

 security/landlock/fs.c                     | 16 ++++++++
 tools/testing/selftests/landlock/fs_test.c | 45 ++++++++++++++++++++++
 2 files changed, 61 insertions(+)

-- 
2.54.0.rc0.605.g598a273b03-goog

[PATCH 1/3] landlock: Require LANDLOCK_ACCESS_FS_MAKE_CHAR for RENAME_WHITEOUT

[Günther Noack]

renameat2(2) with the RENAME_WHITEOUT flag places a whiteout character
device file in the source file location in place of the moved file,
bypassing the LANDLOCK_ACCESS_FS_MAKE_CHAR right.

Fix this by checking for LANDLOCK_ACCESS_FS_MAKE_CHAR if RENAME_WHITEOUT is
passed.

This does not affect normal renames within layered OverlayFS mounts: When
OverlayFS invokes rename with RENAME_WHITEOUT as part of a "normal" rename
operation, it does so in ovl_rename() using the credentials that were set
at the time of mounting the OverlayFS.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 security/landlock/fs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c1ecfe239032..2b84a229e4d8 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1519,6 +1519,19 @@ 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 */
+	if (flags & RENAME_WHITEOUT) {
+		int err;
+
+		/*
+		 * This check would better be done together with other path
+		 * walks which are already happening for the normal rename check
+		 * in current_check_refer_path().
+		 */
+		err = current_check_access_path(old_dir, LANDLOCK_ACCESS_FS_MAKE_CHAR);
+		if (err)
+			return err;
+	}
+
 	return current_check_refer_path(old_dentry, new_dir, new_dentry, true,
 					!!(flags & RENAME_EXCHANGE));
 }
-- 
2.54.0.rc0.605.g598a273b03-goog

[PATCH 2/3] selftests/landlock: Add test for RENAME_WHITEOUT denial

[Günther Noack]

Add a test to check that renames with RENAME_WHITEOUT are guarded by
LANDLOCK_ACCESS_FS_MAKE_CHAR.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 tools/testing/selftests/landlock/fs_test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index cdb47fc1fc0a..d867016e3fd3 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -2247,6 +2247,19 @@ TEST_F_FORK(layout1, rename_file)
 			       RENAME_EXCHANGE));
 }
 
+TEST_F_FORK(layout1, rename_whiteout_denied)
+{
+	enforce_fs(_metadata, LANDLOCK_ACCESS_FS_MAKE_CHAR, NULL);
+
+	/*
+	 * Try to rename a file with RENAME_WHITEOUT.
+	 * file1_s3d3 is in dir_s3d2 (tmpfs), so it supports RENAME_WHITEOUT.
+	 */
+	EXPECT_EQ(-1, renameat2(AT_FDCWD, file1_s3d3, AT_FDCWD,
+				TMP_DIR "/s3d1/s3d2/s3d3/f2", RENAME_WHITEOUT));
+	EXPECT_EQ(EACCES, errno);
+}
+
 TEST_F_FORK(layout1, rename_dir)
 {
 	const struct rule rules[] = {
@@ -6949,6 +6962,7 @@ TEST_F_FORK(layout2_overlay, same_content_different_file)
 	}
 }
 
+
 FIXTURE(layout3_fs)
 {
 	bool has_created_dir;
-- 
2.54.0.rc0.605.g598a273b03-goog

[PATCH 3/3] selftests/landlock: Test OverlayFS renames w/o LANDLOCK_ACCESS_FS_MAKE_CHAR

[Günther Noack]

Even though OverlayFS uses vfs_rename() with RENAME_WHITEOUT under the
hood, and even though RENAME_WHITEOUT requires
LANDLOCK_ACCESS_FS_MAKE_CHAR, a process that renames files in an OverlayFS
can do so without having the LANDLOCK_ACCESS_FS_MAKE_CHAR right in that
location.  This works because OverlayFS uses the credentials determined at
mount time for the internal vfs_rename() operation.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 security/landlock/fs.c                     | 11 +++++---
 tools/testing/selftests/landlock/fs_test.c | 31 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 2b84a229e4d8..9b49f6c3e5da 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1523,11 +1523,14 @@ static int hook_path_rename(const struct path *const old_dir,
 		int err;
 
 		/*
-		 * This check would better be done together with other path
-		 * walks which are already happening for the normal rename check
-		 * in current_check_refer_path().
+		 * Rename with RENAME_WHITEOUT creates a whiteout object
+		 * (character device file with major=minor=0) in the old
+		 * location, so we check the access right for creating that.
+		 *
+		 * See Documentation/filesystems/overlayfs.rst and renameat2(2).
 		 */
-		err = current_check_access_path(old_dir, LANDLOCK_ACCESS_FS_MAKE_CHAR);
+		err = current_check_access_path(old_dir,
+						LANDLOCK_ACCESS_FS_MAKE_CHAR);
 		if (err)
 			return err;
 	}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d867016e3fd3..4cf6fc0bcb71 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -6962,6 +6962,37 @@ TEST_F_FORK(layout2_overlay, same_content_different_file)
 	}
 }
 
+TEST_F_FORK(layout2_overlay, rename_in_overlay_without_make_char)
+{
+	struct stat st;
+	const char *merge_fl1_renamed = MERGE_DATA "/fl1_renamed";
+
+	if (self->skip_test)
+		SKIP(return, "overlayfs is not supported (test)");
+
+	enforce_fs(_metadata, LANDLOCK_ACCESS_FS_MAKE_CHAR, NULL);
+
+	/*
+	 * Execute a regular file rename within OverlayFS.
+	 * merge_fl1 originates from lower layer, so this triggers a copy-up
+	 * and creation of a whiteout in the upper layer.
+	 */
+	EXPECT_EQ(0, rename(merge_fl1, merge_fl1_renamed));
+
+	/* Check that the rename worked. */
+	EXPECT_EQ(0, stat(merge_fl1_renamed, &st));
+	EXPECT_EQ(-1, stat(merge_fl1, &st));
+	EXPECT_EQ(ENOENT, errno);
+
+	/*
+	 * Check that the whiteout object on the underlying "upper" filesystem
+	 * exists after the rename.  This is OK because it was done with the
+	 * credentials of the OverlayFS.
+	 */
+	EXPECT_EQ(0, stat(UPPER_DATA "/fl1", &st));
+	EXPECT_TRUE(S_ISCHR(st.st_mode));
+	EXPECT_EQ(0, st.st_rdev);
+}
 
 FIXTURE(layout3_fs)
 {
-- 
2.54.0.rc0.605.g598a273b03-goog