[PATCH v3 0/5] Landlock: IOCTL support

[PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hello!

These patches add simple ioctl(2) support to Landlock.

Objective
~~~~~~~~~

Make ioctl(2) requests restrictable with Landlock,
in a way that is useful for real-world applications.

Proposed approach
~~~~~~~~~~~~~~~~~

Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
of ioctl(2) on file descriptors.

We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.

We make an exception for the common and known-harmless IOCTL commands FIOCLEX,
FIONCLEX, FIONBIO, FIOASYNC and FIONREAD.  These IOCTL commands are always
permitted.  The functionality of the first four is already available through
fcntl(2), and FIONREAD only returns the number of ready-to-read bytes.

I believe that this approach works for the majority of use cases, and
offers a good trade-off between Landlock API and implementation
complexity and flexibility when the feature is used.

Current limitations
~~~~~~~~~~~~~~~~~~~

With this patch set, ioctl(2) requests can *not* be filtered based on
file type, device number (dev_t) or on the ioctl(2) request number.

On the initial RFC patch set [1], we have reached consensus to start
with this simpler coarse-grained approach, and build additional IOCTL
restriction capabilities on top in subsequent steps.

[1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/

Notable implications of this approach
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* Existing inherited file descriptors stay unaffected
  when a program enables Landlock.

  This means in particular that in common scenarios,
  the terminal's IOCTLs (ioctl_tty(2)) continue to work.

* ioctl(2) continues to be available for file descriptors acquired
  through means other than open(2).  Example: Network sockets,
  memfd_create(2), file descriptors that are already open before the
  Landlock ruleset is enabled.

Examples
~~~~~~~~

Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:

  LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash

The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights
here, so we expect that newly opened files outside of $HOME don't work
with ioctl(2).

  * "stty" works: It probes terminal properties

  * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is
    denied.

  * "eject" fails: ioctls to use CD-ROM drive are denied.

  * "ls /dev" works: It uses ioctl to get the terminal size for
    columnar layout

  * The text editors "vim" and "mg" work.  (GNU Emacs fails because it
    attempts to reopen /dev/tty.)

How we arrived at the list of always-permitted IOCTL commands
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To decide which IOCTL commands should be blanket-permitted I went through the
list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
to understand what they are about.  The following list is my conclusion from
that.

We should always allow the following IOCTL commands:

 * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
   close-on-exec flag
 * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
   and async flags
 * FIONREAD - get the number of bytes available for reading (the implementation
   is defined per file type)

The first four are also available through fcntl with the F_SETFD and F_SETFL
commands.

The following commands mentioned in fs/ioctl.c should be guarded by the
LANDLOCK_ACCESS_FS_IOCTL access right, the same as the other ioctl commands,
because they are nontrivial:

 * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
   system. Requires CAP_SYS_ADMIN.
 * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage
   between multiple files.  These only work on some file systems, by design.
 * Commands that read file system internals:
   * FS_IOC_FIEMAP - get information about file extent mapping
     (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)
   * FIBMAP - get a file's file system block number
   * FIGETBSZ - get file system blocksize
 * Accessing file attributes:
   * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
   * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes
 * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
   FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation
   syscalls which predate fallocate(2).

Related Work
~~~~~~~~~~~~

OpenBSD's pledge(2) [2] restricts ioctl(2) independent of the file
descriptor which is used.  The implementers maintain multiple
allow-lists of predefined ioctl(2) operations required for different
application domains such as "audio", "bpf", "tty" and "inet".

OpenBSD does not guarantee ABI backwards compatibility to the same
extent as Linux does, so it's easier for them to update these lists in
later versions.  It might not be a feasible approach for Linux though.

[2] https://man.openbsd.org/OpenBSD-7.3/pledge.2

Changes
~~~~~~~

V3:
 * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and
   FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL
 * increment ABI version in the same commit where the feature is introduced
 * testing changes
   * use FIOQSIZE instead of TTY IOCTL commands
     (FIOQSIZE works with regular files, directories and memfds)
   * run the memfd test with both Landlock enabled and disabled
   * add a test for the always-permitted IOCTL commands

V2:
 * rebased on mic-next
 * added documentation
 * exercise ioctl(2) in the memfd test
 * test: Use layout0 for the test

---

V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/
V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/

Günther Noack (5):
  landlock: Add ioctl access right
  selftests/landlock: Test ioctl support
  selftests/landlock: Test ioctl with memfds
  samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL
  landlock: Document ioctl support

 Documentation/userspace-api/landlock.rst     |  74 ++++++++---
 include/uapi/linux/landlock.h                |  31 +++--
 samples/landlock/sandboxer.c                 |  12 +-
 security/landlock/fs.c                       |  38 +++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 133 +++++++++++++++++--
 8 files changed, 249 insertions(+), 45 deletions(-)


base-commit: 35ca4239929737bdc021ee923f97ebe7aff8fcc4
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 1/5] landlock: Add ioctl access right

[Günther Noack]

Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
and increments the Landlock ABI version to 4.

Like the truncate right, these rights are associated with a file
descriptor at the time of open(2), and get respected even when the
file descriptor is used outside of the thread which it was originally
opened in.

A newly enabled Landlock policy therefore does not apply to file
descriptors which are already open.

If a file was opened without the LANDLOCK_ACCESS_FS_IOCTL right, IOCTL
attempts on the opened file will fail, except for a small number of
common and harmless IOCTL commands (see documentation).

Noteworthy scenarios which require special attention:

TTY devices support IOCTLs like TIOCSTI and TIOCLINUX, which can be
used to control shell processes on the same terminal which run at
different privilege levels, which may make it possible to escape a
sandbox.  Because stdin, stdout and stderr are normally inherited
rather than newly opened, IOCTLs are usually permitted on them even
after the Landlock policy is enforced.

Some legitimate file system features, like setting up fscrypt, are
exposed as IOCTL commands on regular files and directories -- users of
Landlock are advised to double check that the sandboxed process does
not require these IOCTLs.

Known limitations:

The LANDLOCK_ACCESS_FS_IOCTL access right is a coarse-grained control
over IOCTL commands.  Future work will enable a more fine-grained
access control for IOCTLs.

In the meantime, Landlock users may use path-based restrictions in
combination with their knowledge about the file system layout to
control what IOCTLs can be done.  Mounting file systems with the nodev
option can help to distinguish regular files and devices, and give
guarantees about the affected files, which Landlock alone can not give
yet.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 include/uapi/linux/landlock.h                | 31 +++++++++++-----
 security/landlock/fs.c                       | 38 ++++++++++++++++++--
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  5 +--
 6 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 81d09ef9aa50..3c1d4f1e084d 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -92,7 +92,7 @@ struct landlock_path_beneath_attr {
  * files and directories.  Files or directories opened before the sandboxing
  * are not subject to these restrictions.
  *
- * A file can only receive these access rights:
+ * The following access rights apply only to files:
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
  * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
@@ -102,12 +102,13 @@ struct landlock_path_beneath_attr {
  * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
  * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
  *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
- *   ``O_TRUNC``. Whether an opened file can be truncated with
- *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
- *   same way as read and write permissions are checked during
- *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
- *   %LANDLOCK_ACCESS_FS_WRITE_FILE. This access right is available since the
- *   third version of the Landlock ABI.
+ *   ``O_TRUNC``.  This access right is available since the third version of the
+ *   Landlock ABI.
+ *
+ * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used
+ * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as
+ * read and write permissions are checked during :manpage:`open(2)` using
+ * %LANDLOCK_ACCESS_FS_READ_FILE and %LANDLOCK_ACCESS_FS_WRITE_FILE.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -162,13 +163,26 @@ struct landlock_path_beneath_attr {
  *   If multiple requirements are not met, the ``EACCES`` error code takes
  *   precedence over ``EXDEV``.
  *
+ * The following access right applies both to files and directories:
+ *
+ * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` commands on an opened
+ *   file or directory.
+ *
+ *   This access right applies to all :manpage:`ioctl(2)` commands, except of
+ *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC`` and ``FIONREAD``.
+ *   These commands continue to be invokable independent of the
+ *   %LANDLOCK_ACCESS_FS_IOCTL access right.
+ *
+ *   This access right is available since the fourth version of the Landlock
+ *   ABI.
+ *
  * .. warning::
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
  *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
  *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -187,6 +201,7 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
+#define LANDLOCK_ACCESS_FS_IOCTL			(1ULL << 15)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 1c0c198f6fdb..3b4a6263f5a9 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -7,6 +7,7 @@
  * Copyright © 2021-2022 Microsoft Corporation
  */
 
+#include <asm/ioctls.h>
 #include <linux/atomic.h>
 #include <linux/bitops.h>
 #include <linux/bits.h>
@@ -147,7 +148,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 /* clang-format on */
 
 /*
@@ -1207,7 +1209,8 @@ static int hook_file_open(struct file *const file)
 {
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
 	access_mask_t open_access_request, full_access_request, allowed_access;
-	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
+	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
+					      LANDLOCK_ACCESS_FS_IOCTL;
 	const struct landlock_ruleset *const dom =
 		landlock_get_current_domain();
 
@@ -1280,6 +1283,36 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+static int hook_file_ioctl(struct file *file, unsigned int cmd,
+			   unsigned long arg)
+{
+	/*
+	 * These IOCTL commands are generally permitted with Landlock: FIOCLEX,
+	 * FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's close-on-exec and
+	 * the file's buffered-IO and async flags.  These operations are also
+	 * available through fcntl(2).  FIONREAD returns the number of
+	 * immediately writable bytes.
+	 */
+	switch (cmd) {
+	case FIOCLEX:
+	case FIONCLEX:
+	case FIONBIO:
+	case FIOASYNC:
+	case FIONREAD:
+		return 0;
+	}
+
+	/*
+	 * It is the access rights at the time of opening the file which
+	 * determine whether ioctl can be used on the opened file later.
+	 *
+	 * The access right is attached to the opened file in hook_file_open().
+	 */
+	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_IOCTL)
+		return 0;
+	return -EACCES;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
 
@@ -1302,6 +1335,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
 	LSM_HOOK_INIT(file_open, hook_file_open),
 	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
+	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
 };
 
 __init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 82288f0e9e5e..40d8f17698b6 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_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL
 #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/syscalls.c b/security/landlock/syscalls.c
index 245cc650a4dc..c70fc9e6fe9e 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 3
+#define LANDLOCK_ABI_VERSION 4
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 792c3f0a59b4..646f778dfb1e 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 83d565569512..09dd1eaac8a9 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -523,9 +523,10 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 2/5] selftests/landlock: Test ioctl support

[Günther Noack]

Exercises Landlock's IOCTL feature: If the LANDLOCK_ACCESS_FS_IOCTL
right is restricted, the use of IOCTL fails with a freshly opened
file.

Irrespective of the LANDLOCK_ACCESS_FS_IOCTL right, IOCTL continues to
work with a selected set of known harmless IOCTL commands.

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 09dd1eaac8a9..456bd681091d 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3329,7 +3329,7 @@ TEST_F_FORK(layout1, truncate_unhandled)
 			      LANDLOCK_ACCESS_FS_WRITE_FILE;
 	int ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, handled, rules);
 
 	ASSERT_LE(0, ruleset_fd);
@@ -3412,7 +3412,7 @@ TEST_F_FORK(layout1, truncate)
 			      LANDLOCK_ACCESS_FS_TRUNCATE;
 	int ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, handled, rules);
 
 	ASSERT_LE(0, ruleset_fd);
@@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
 	};
 	int fd, ruleset_fd;
 
-	/* Enable Landlock. */
+	/* Enables Landlock. */
 	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
 	ASSERT_LE(0, ruleset_fd);
 	enforce_ruleset(_metadata, ruleset_fd);
@@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
 	ASSERT_EQ(0, close(fd));
 }
 
+/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
+static int test_fioqsize_ioctl(int fd)
+{
+	loff_t size;
+
+	if (ioctl(fd, FIOQSIZE, &size) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * Attempt ioctls on regular files, with file descriptors opened before and
+ * after landlocking.
+ */
+TEST_F_FORK(layout1, ioctl)
+{
+	const struct rule rules[] = {
+		{
+			.path = file1_s1d1,
+			.access = LANDLOCK_ACCESS_FS_IOCTL,
+		},
+		{
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_IOCTL,
+		},
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
+	int ruleset_fd;
+	int dir_s1d1_fd, file1_s1d1_fd, dir_s2d1_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	dir_s1d1_fd = open(dir_s1d1, O_RDONLY);
+	ASSERT_LE(0, dir_s1d1_fd);
+	file1_s1d1_fd = open(file1_s1d1, O_RDONLY);
+	ASSERT_LE(0, file1_s1d1_fd);
+	dir_s2d1_fd = open(dir_s2d1, O_RDONLY);
+	ASSERT_LE(0, dir_s2d1_fd);
+
+	/*
+	 * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is
+	 * permitted.
+	 */
+	EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd));
+	EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd));
+	EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd));
+
+	/* Closes all file descriptors. */
+	ASSERT_EQ(0, close(dir_s1d1_fd));
+	ASSERT_EQ(0, close(file1_s1d1_fd));
+	ASSERT_EQ(0, close(dir_s2d1_fd));
+}
+
+TEST_F_FORK(layout1, ioctl_always_allowed)
+{
+	struct landlock_ruleset_attr attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL,
+	};
+	int ruleset_fd, fd;
+	int flag = 0;
+	int n;
+
+	/* Enables Landlock. */
+	ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	fd = open(file1_s1d1, O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	/* Checks that the restrictable FIOQSIZE is restricted. */
+	EXPECT_EQ(EACCES, test_fioqsize_ioctl(fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
+	EXPECT_EQ(0, ioctl(fd, FIONREAD, &n));
+	EXPECT_EQ(0, n);
+
+	ASSERT_EQ(0, close(fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 3/5] selftests/landlock: Test ioctl with memfds

[Günther Noack]

Because the ioctl right is associated with the opened file,
we expect that it will work with files which are opened by means
other than open(2).

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 456bd681091d..4eb989d5ff39 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3716,22 +3716,6 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
 	ASSERT_EQ(0, close(socket_fds[1]));
 }
 
-TEST(memfd_ftruncate)
-{
-	int fd;
-
-	fd = memfd_create("name", MFD_CLOEXEC);
-	ASSERT_LE(0, fd);
-
-	/*
-	 * Checks that ftruncate is permitted on file descriptors that are
-	 * created in ways other than open(2).
-	 */
-	EXPECT_EQ(0, test_ftruncate(fd));
-
-	ASSERT_EQ(0, close(fd));
-}
-
 /* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
 static int test_fioqsize_ioctl(int fd)
 {
@@ -3742,6 +3726,40 @@ static int test_fioqsize_ioctl(int fd)
 	return 0;
 }
 
+TEST(memfd_ftruncate_and_ioctl)
+{
+	struct landlock_ruleset_attr attr = {
+		.handled_access_fs = ACCESS_ALL,
+	};
+	int ruleset_fd, fd, i;
+
+	/*
+	 * We exercise the same test both with and without Landlock enabled, to
+	 * ensure that it behaves the same in both cases.
+	 */
+	for (i = 0; i < 2; i++) {
+		/* Creates a new memfd. */
+		fd = memfd_create("name", MFD_CLOEXEC);
+		ASSERT_LE(0, fd);
+
+		/*
+		 * Checks that operations associated with the opened file
+		 * (ftruncate, ioctl) are permitted on file descriptors that are
+		 * created in ways other than open(2).
+		 */
+		EXPECT_EQ(0, test_ftruncate(fd));
+		EXPECT_EQ(0, test_fioqsize_ioctl(fd));
+
+		ASSERT_EQ(0, close(fd));
+
+		/* Enables Landlock. */
+		ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
+		ASSERT_LE(0, ruleset_fd)
+		enforce_ruleset(_metadata, ruleset_fd);
+		ASSERT_EQ(0, close(ruleset_fd));
+	}
+}
+
 /*
  * Attempt ioctls on regular files, with file descriptors opened before and
  * after landlocking.
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 4/5] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

[Günther Noack]

Add ioctl support to the Landlock sample tool.

The ioctl right is grouped with the read-write rights in the sample
tool, as some ioctl requests provide features that mutate state.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 samples/landlock/sandboxer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e2056c8b902c..c70d96d15c70 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -77,7 +77,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
 /* clang-format on */
 
@@ -162,11 +163,12 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
 	LANDLOCK_ACCESS_FS_REFER | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 3
+#define LANDLOCK_ABI_LAST 4
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -255,6 +257,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	case 2:
 		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+		__attribute__((fallthrough));
+	case 3:
+		/* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 4 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
 
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
-- 
2.41.0.694.ge786442a9b-goog

[PATCH v3 5/5] landlock: Document ioctl support

[Günther Noack]

In the paragraph above the fallback logic, use the shorter phrasing
from the landlock(7) man page.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 Documentation/userspace-api/landlock.rst | 74 ++++++++++++++++++------
 1 file changed, 57 insertions(+), 17 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index d8cd8cd9ce25..e0e35e474307 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -61,18 +61,17 @@ the need to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_FS_MAKE_BLOCK |
             LANDLOCK_ACCESS_FS_MAKE_SYM |
             LANDLOCK_ACCESS_FS_REFER |
-            LANDLOCK_ACCESS_FS_TRUNCATE,
+            LANDLOCK_ACCESS_FS_TRUNCATE |
+            LANDLOCK_ACCESS_FS_IOCTL,
     };
 
 Because we may not know on which kernel version an application will be
 executed, it is safer to follow a best-effort security approach.  Indeed, we
 should try to protect users as much as possible whatever the kernel they are
-using.  To avoid binary enforcement (i.e. either all security features or
-none), we can leverage a dedicated Landlock command to get the current version
-of the Landlock ABI and adapt the handled accesses.  Let's check if we should
-remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE``
-access rights, which are only supported starting with the second and third
-version of the ABI.
+using.
+
+To be compatible with older Linux versions, we detect the available Landlock ABI
+version, and only use the available subset of access rights:
 
 .. code-block:: c
 
@@ -92,6 +91,9 @@ version of the ABI.
     case 2:
         /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
         ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+    case 3:
+        /* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 4 */
+        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -190,6 +192,7 @@ access rights per directory enables to change the location of such directory
 without relying on the destination directory access rights (except those that
 are required for this operation, see ``LANDLOCK_ACCESS_FS_REFER``
 documentation).
+
 Having self-sufficient hierarchies also helps to tighten the required access
 rights to the minimal set of data.  This also helps avoid sinkhole directories,
 i.e.  directories where data can be linked to but not linked from.  However,
@@ -283,18 +286,24 @@ It should also be noted that truncating files does not require the
 system call, this can also be done through :manpage:`open(2)` with the flags
 ``O_RDONLY | O_TRUNC``.
 
-When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
-right is associated with the newly created file descriptor and will be used for
-subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
-similar to opening a file for reading or writing, where permissions are checked
-during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
+The truncate right is associated with the opened file (see below).
+
+Rights associated with file descriptors
+---------------------------------------
+
+When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` and
+``LANDLOCK_ACCESS_FS_IOCTL`` rights is associated with the newly created file
+descriptor and will be used for subsequent truncation and ioctl attempts using
+:manpage:`ftruncate(2)` and :manpage:`ioctl(2)`.  The behavior is similar to
+opening a file for reading or writing, where permissions are checked during
+:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
 :manpage:`write(2)` calls.
 
-As a consequence, it is possible to have multiple open file descriptors for the
-same file, where one grants the right to truncate the file and the other does
-not.  It is also possible to pass such file descriptors between processes,
-keeping their Landlock properties, even when these processes do not have an
-enforced Landlock ruleset.
+As a consequence, it is possible to have multiple open file descriptors
+referring to the same file, where one grants the truncate or ioctl right and the
+other does not.  It is also possible to pass such file descriptors between
+processes, keeping their Landlock properties, even when these processes do not
+have an enforced Landlock ruleset.
 
 Compatibility
 =============
@@ -422,6 +431,27 @@ Memory usage
 Kernel memory allocated to create rulesets is accounted and can be restricted
 by the Documentation/admin-guide/cgroup-v1/memory.rst.
 
+IOCTL support
+-------------
+
+The ``LANDLOCK_ACCESS_FS_IOCTL`` access right restricts the use of
+:manpage:`ioctl(2)`, but it only applies to newly opened files.  This means
+specifically that pre-existing file descriptors like STDIN, STDOUT and STDERR
+are unaffected.
+
+Users should be aware that TTY devices have traditionally permitted to control
+other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL
+commands.  It is therefore recommended to close inherited TTY file descriptors.
+The :manpage:`isatty(3)` function checks whether a given file descriptor is a
+TTY.
+
+Landlock's IOCTL support is coarse-grained at the moment, but may become more
+fine-grained in the future.  Until then, users are advised to establish the
+guarantees that they need through the file hierarchy, by only permitting the
+``LANDLOCK_ACCESS_FS_IOCTL`` right on files where it is really harmless.  In
+cases where you can control the mounts, the ``nodev`` mount option can help to
+rule out that device files can be accessed.
+
 Previous limitations
 ====================
 
@@ -451,6 +481,16 @@ always allowed when using a kernel that only supports the first or second ABI.
 Starting with the Landlock ABI version 3, it is now possible to securely control
 truncation thanks to the new ``LANDLOCK_ACCESS_FS_TRUNCATE`` access right.
 
+Ioctl (ABI < 4)
+---------------
+
+IOCTL operations could not be denied before the fourth Landlock ABI, so
+:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an
+earlier ABI.
+
+Starting with the Landlock ABI version 4, it is possible to restrict the use of
+:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL`` access right.
+
 .. _kernel_support:
 
 Kernel support
-- 
2.41.0.694.ge786442a9b-goog

Re: [PATCH v3 1/5] landlock: Add ioctl access right

[Günther Noack]

Hi!

On Mon, Aug 14, 2023 at 07:28:12PM +0200, Günther Noack wrote:
> @@ -1207,7 +1209,8 @@ static int hook_file_open(struct file *const file)
>  {
>  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>  	access_mask_t open_access_request, full_access_request, allowed_access;
> -	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> +	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
> +					      LANDLOCK_ACCESS_FS_IOCTL;
>  	const struct landlock_ruleset *const dom =
>  		landlock_get_current_domain();
>  
> @@ -1280,6 +1283,36 @@ static int hook_file_truncate(struct file *const file)
>  	return -EACCES;
>  }

About the error code:

The ioctl(2) man page documents ENOTTY as "The specified request does not apply
to this kind of object".  It does not document EACCES.  EACCES would be slightly
more appropriate semantically, but existing programs might be more well-equipped
to handle ENOTTY.

Do you think we should return ENOTTY here?

—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> Hello!
> 
> These patches add simple ioctl(2) support to Landlock.
> 
> Objective
> ~~~~~~~~~
> 
> Make ioctl(2) requests restrictable with Landlock,
> in a way that is useful for real-world applications.
> 
> Proposed approach
> ~~~~~~~~~~~~~~~~~
> 
> Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
> of ioctl(2) on file descriptors.
> 
> We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
> descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.
> 
> We make an exception for the common and known-harmless IOCTL commands FIOCLEX,
> FIONCLEX, FIONBIO, FIOASYNC and FIONREAD.  These IOCTL commands are always
> permitted.  The functionality of the first four is already available through
> fcntl(2), and FIONREAD only returns the number of ready-to-read bytes.
> 
> I believe that this approach works for the majority of use cases, and
> offers a good trade-off between Landlock API and implementation
> complexity and flexibility when the feature is used.
> 
> Current limitations
> ~~~~~~~~~~~~~~~~~~~
> 
> With this patch set, ioctl(2) requests can *not* be filtered based on
> file type, device number (dev_t) or on the ioctl(2) request number.
> 
> On the initial RFC patch set [1], we have reached consensus to start
> with this simpler coarse-grained approach, and build additional IOCTL
> restriction capabilities on top in subsequent steps.
> 
> [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/
> 
> Notable implications of this approach
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> * Existing inherited file descriptors stay unaffected
>   when a program enables Landlock.
> 
>   This means in particular that in common scenarios,
>   the terminal's IOCTLs (ioctl_tty(2)) continue to work.
> 
> * ioctl(2) continues to be available for file descriptors acquired
>   through means other than open(2).  Example: Network sockets,
>   memfd_create(2), file descriptors that are already open before the
>   Landlock ruleset is enabled.
> 
> Examples
> ~~~~~~~~
> 
> Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:
> 
>   LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash
> 
> The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights
> here, so we expect that newly opened files outside of $HOME don't work
> with ioctl(2).
> 
>   * "stty" works: It probes terminal properties
> 
>   * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is
>     denied.
> 
>   * "eject" fails: ioctls to use CD-ROM drive are denied.
> 
>   * "ls /dev" works: It uses ioctl to get the terminal size for
>     columnar layout
> 
>   * The text editors "vim" and "mg" work.  (GNU Emacs fails because it
>     attempts to reopen /dev/tty.)
> 
> How we arrived at the list of always-permitted IOCTL commands
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> To decide which IOCTL commands should be blanket-permitted I went through the
> list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> to understand what they are about.  The following list is my conclusion from
> that.
> 
> We should always allow the following IOCTL commands:
> 
>  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
>    close-on-exec flag

I agree that FIOCLEX and FIONCLEX should always be allowed.

>  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
>    and async flags

About FIONBIO and FIOASYNC, I think it's OK because it is already
allowed thanks to fcntl. We could combine these commands with
LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE but it may not be related to only
regular files.

I found that there is an inconsistency between the fcntl's SETFL and the
FIONBIO/FIOASYNC IOCTLs. The first one call filp->f_op->check_flags()
while the second one doesn't. This should enable to bypass such
check_flags() checks. This is unrelated to Landlock or this patch series
though, and it should be OK because only NFS seems to implement
check_flags() and only O_DIRECT|O_APPEND are checked.

Cc Christian

>  * FIONREAD - get the number of bytes available for reading (the implementation
>    is defined per file type)

About FIONREAD, I'm convinced we should only allow this command
according to LANDLOCK_ACCESS_FS_READ. As for the VFS implementation,
it should also depend on the file being a regular file (otherwise any
driver could implement another semantic).

To make it forward compatible (with the same semantic), I think we
should handle specific IOCTLs this way: if the complementary access
right (e.g. LANDLOCK_ACCESS_FS_READ_FILE) is not handled by the ruleset
(which is not the same as allowed by a rule), then the related IOCTLs
(e.g. FIONREAD) are denied, otherwise the related IOCTLs are only
allowed if the complementary access is explicitly allowed for this FD.
This way of delegating enables to extend the access control to IOCTL
commands while preserving the access (rights) semantic.

This will enable for instance to restrict FS_IOC_GETFLAGS according to a
potential future LANDLOCK_ACCESS_FS_READ_METADATA, while keeping the
same IOCTL restriction semantic.

We could also follow this same semantic for synthetic access rights
grouping IOCTLs commands.


> 
> The first four are also available through fcntl with the F_SETFD and F_SETFL
> commands.
> 
> The following commands mentioned in fs/ioctl.c should be guarded by the
> LANDLOCK_ACCESS_FS_IOCTL access right, the same as the other ioctl commands,
> because they are nontrivial:
> 
>  * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
>    system. Requires CAP_SYS_ADMIN.
>  * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage
>    between multiple files.  These only work on some file systems, by design.
>  * Commands that read file system internals:
>    * FS_IOC_FIEMAP - get information about file extent mapping
>      (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)
>    * FIBMAP - get a file's file system block number
>    * FIGETBSZ - get file system blocksize
>  * Accessing file attributes:
>    * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
>    * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes
>  * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
>    FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation
>    syscalls which predate fallocate(2).
> 
> Related Work
> ~~~~~~~~~~~~
> 
> OpenBSD's pledge(2) [2] restricts ioctl(2) independent of the file
> descriptor which is used.  The implementers maintain multiple
> allow-lists of predefined ioctl(2) operations required for different
> application domains such as "audio", "bpf", "tty" and "inet".
> 
> OpenBSD does not guarantee ABI backwards compatibility to the same
> extent as Linux does, so it's easier for them to update these lists in
> later versions.  It might not be a feasible approach for Linux though.
> 
> [2] https://man.openbsd.org/OpenBSD-7.3/pledge.2
> 
> Changes
> ~~~~~~~
> 
> V3:
>  * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and
>    FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL
>  * increment ABI version in the same commit where the feature is introduced
>  * testing changes
>    * use FIOQSIZE instead of TTY IOCTL commands
>      (FIOQSIZE works with regular files, directories and memfds)
>    * run the memfd test with both Landlock enabled and disabled
>    * add a test for the always-permitted IOCTL commands
> 
> V2:
>  * rebased on mic-next
>  * added documentation
>  * exercise ioctl(2) in the memfd test
>  * test: Use layout0 for the test
> 
> ---
> 
> V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/
> V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/
> 
> Günther Noack (5):
>   landlock: Add ioctl access right
>   selftests/landlock: Test ioctl support
>   selftests/landlock: Test ioctl with memfds
>   samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL
>   landlock: Document ioctl support
> 
>  Documentation/userspace-api/landlock.rst     |  74 ++++++++---
>  include/uapi/linux/landlock.h                |  31 +++--
>  samples/landlock/sandboxer.c                 |  12 +-
>  security/landlock/fs.c                       |  38 +++++-
>  security/landlock/limits.h                   |   2 +-
>  security/landlock/syscalls.c                 |   2 +-
>  tools/testing/selftests/landlock/base_test.c |   2 +-
>  tools/testing/selftests/landlock/fs_test.c   | 133 +++++++++++++++++--
>  8 files changed, 249 insertions(+), 45 deletions(-)
> 
> 
> base-commit: 35ca4239929737bdc021ee923f97ebe7aff8fcc4
> -- 
> 2.41.0.694.ge786442a9b-goog
> 

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> Hello!
> 
> These patches add simple ioctl(2) support to Landlock.
> 

[...]

> How we arrived at the list of always-permitted IOCTL commands
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> To decide which IOCTL commands should be blanket-permitted I went through the
> list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> to understand what they are about.  The following list is my conclusion from
> that.
> 
> We should always allow the following IOCTL commands:
> 
>  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
>    close-on-exec flag
>  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
>    and async flags
>  * FIONREAD - get the number of bytes available for reading (the implementation
>    is defined per file type)

I think we should treat FIOQSIZE like FIONREAD, i.e. check for
LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
Tests should then rely on something else.

[...]

> Changes
> ~~~~~~~
> 
> V3:
>  * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and
>    FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL
>  * increment ABI version in the same commit where the feature is introduced
>  * testing changes
>    * use FIOQSIZE instead of TTY IOCTL commands
>      (FIOQSIZE works with regular files, directories and memfds)
>    * run the memfd test with both Landlock enabled and disabled
>    * add a test for the always-permitted IOCTL commands

Re: [PATCH v3 5/5] landlock: Document ioctl support

[Mickaël Salaün]

This looks good!

On Mon, Aug 14, 2023 at 07:28:16PM +0200, Günther Noack wrote:
> In the paragraph above the fallback logic, use the shorter phrasing
> from the landlock(7) man page.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  Documentation/userspace-api/landlock.rst | 74 ++++++++++++++++++------
>  1 file changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index d8cd8cd9ce25..e0e35e474307 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -61,18 +61,17 @@ the need to be explicit about the denied-by-default access rights.
>              LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>              LANDLOCK_ACCESS_FS_MAKE_SYM |
>              LANDLOCK_ACCESS_FS_REFER |
> -            LANDLOCK_ACCESS_FS_TRUNCATE,
> +            LANDLOCK_ACCESS_FS_TRUNCATE |
> +            LANDLOCK_ACCESS_FS_IOCTL,
>      };
>  
>  Because we may not know on which kernel version an application will be
>  executed, it is safer to follow a best-effort security approach.  Indeed, we
>  should try to protect users as much as possible whatever the kernel they are
> -using.  To avoid binary enforcement (i.e. either all security features or
> -none), we can leverage a dedicated Landlock command to get the current version
> -of the Landlock ABI and adapt the handled accesses.  Let's check if we should
> -remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE``
> -access rights, which are only supported starting with the second and third
> -version of the ABI.
> +using.
> +
> +To be compatible with older Linux versions, we detect the available Landlock ABI
> +version, and only use the available subset of access rights:
>  
>  .. code-block:: c
>  
> @@ -92,6 +91,9 @@ version of the ABI.
>      case 2:
>          /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
>          ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
> +    case 3:
> +        /* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 4 */
> +        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
>      }
>  
>  This enables to create an inclusive ruleset that will contain our rules.
> @@ -190,6 +192,7 @@ access rights per directory enables to change the location of such directory
>  without relying on the destination directory access rights (except those that
>  are required for this operation, see ``LANDLOCK_ACCESS_FS_REFER``
>  documentation).
> +
>  Having self-sufficient hierarchies also helps to tighten the required access
>  rights to the minimal set of data.  This also helps avoid sinkhole directories,
>  i.e.  directories where data can be linked to but not linked from.  However,
> @@ -283,18 +286,24 @@ It should also be noted that truncating files does not require the
>  system call, this can also be done through :manpage:`open(2)` with the flags
>  ``O_RDONLY | O_TRUNC``.
>  
> -When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
> -right is associated with the newly created file descriptor and will be used for
> -subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
> -similar to opening a file for reading or writing, where permissions are checked
> -during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
> +The truncate right is associated with the opened file (see below).
> +
> +Rights associated with file descriptors
> +---------------------------------------
> +
> +When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` and
> +``LANDLOCK_ACCESS_FS_IOCTL`` rights is associated with the newly created file
> +descriptor and will be used for subsequent truncation and ioctl attempts using
> +:manpage:`ftruncate(2)` and :manpage:`ioctl(2)`.  The behavior is similar to
> +opening a file for reading or writing, where permissions are checked during
> +:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
>  :manpage:`write(2)` calls.
>  
> -As a consequence, it is possible to have multiple open file descriptors for the
> -same file, where one grants the right to truncate the file and the other does
> -not.  It is also possible to pass such file descriptors between processes,
> -keeping their Landlock properties, even when these processes do not have an
> -enforced Landlock ruleset.
> +As a consequence, it is possible to have multiple open file descriptors
> +referring to the same file, where one grants the truncate or ioctl right and the
> +other does not.  It is also possible to pass such file descriptors between
> +processes, keeping their Landlock properties, even when these processes do not
> +have an enforced Landlock ruleset.
>  
>  Compatibility
>  =============
> @@ -422,6 +431,27 @@ Memory usage
>  Kernel memory allocated to create rulesets is accounted and can be restricted
>  by the Documentation/admin-guide/cgroup-v1/memory.rst.
>  
> +IOCTL support
> +-------------
> +
> +The ``LANDLOCK_ACCESS_FS_IOCTL`` access right restricts the use of
> +:manpage:`ioctl(2)`, but it only applies to newly opened files.  This means
> +specifically that pre-existing file descriptors like STDIN, STDOUT and STDERR

According to man pages (and unlike IOCTL commands) we should not
capitalize stdin, stdout and stderr.

> +are unaffected.
> +
> +Users should be aware that TTY devices have traditionally permitted to control
> +other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL
> +commands.  It is therefore recommended to close inherited TTY file descriptors.

Good to see such warnings in the documentation.

We could also propose a simple solution to still uses stdin, stdout and
stderr without complex TTY proxying: re-opening the TTY, or replacing
related FD thanks to /proc/self/fd/*

For instance, with shell scripts it would look like this:
exec </proc/self/fd/0
exec >/proc/self/fd/1
exec 2>/proc/self/fd/2

Because of TIOCGWINSZ and TCGETS, an interactive shell may not work as
expected though.

> +The :manpage:`isatty(3)` function checks whether a given file descriptor is a
> +TTY.
> +
> +Landlock's IOCTL support is coarse-grained at the moment, but may become more
> +fine-grained in the future.  Until then, users are advised to establish the
> +guarantees that they need through the file hierarchy, by only permitting the
> +``LANDLOCK_ACCESS_FS_IOCTL`` right on files where it is really harmless.  In
> +cases where you can control the mounts, the ``nodev`` mount option can help to
> +rule out that device files can be accessed.
> +
>  Previous limitations
>  ====================
>  
> @@ -451,6 +481,16 @@ always allowed when using a kernel that only supports the first or second ABI.
>  Starting with the Landlock ABI version 3, it is now possible to securely control
>  truncation thanks to the new ``LANDLOCK_ACCESS_FS_TRUNCATE`` access right.
>  
> +Ioctl (ABI < 4)
> +---------------
> +
> +IOCTL operations could not be denied before the fourth Landlock ABI, so
> +:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an
> +earlier ABI.
> +
> +Starting with the Landlock ABI version 4, it is possible to restrict the use of
> +:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL`` access right.
> +
>  .. _kernel_support:
>  
>  Kernel support
> -- 
> 2.41.0.694.ge786442a9b-goog
> 

Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

[Mickaël Salaün]

On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> Exercises Landlock's IOCTL feature: If the LANDLOCK_ACCESS_FS_IOCTL
> right is restricted, the use of IOCTL fails with a freshly opened
> file.
> 
> Irrespective of the LANDLOCK_ACCESS_FS_IOCTL right, IOCTL continues to
> work with a selected set of known harmless IOCTL commands.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 96 +++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 09dd1eaac8a9..456bd681091d 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3329,7 +3329,7 @@ TEST_F_FORK(layout1, truncate_unhandled)
>  			      LANDLOCK_ACCESS_FS_WRITE_FILE;
>  	int ruleset_fd;
>  
> -	/* Enable Landlock. */
> +	/* Enables Landlock. */
>  	ruleset_fd = create_ruleset(_metadata, handled, rules);
>  
>  	ASSERT_LE(0, ruleset_fd);
> @@ -3412,7 +3412,7 @@ TEST_F_FORK(layout1, truncate)
>  			      LANDLOCK_ACCESS_FS_TRUNCATE;
>  	int ruleset_fd;
>  
> -	/* Enable Landlock. */
> +	/* Enables Landlock. */
>  	ruleset_fd = create_ruleset(_metadata, handled, rules);
>  
>  	ASSERT_LE(0, ruleset_fd);
> @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
>  	};
>  	int fd, ruleset_fd;
>  
> -	/* Enable Landlock. */
> +	/* Enables Landlock. */
>  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
>  	ASSERT_LE(0, ruleset_fd);
>  	enforce_ruleset(_metadata, ruleset_fd);
> @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
>  	ASSERT_EQ(0, close(fd));
>  }

We should also check with O_PATH to make sure the correct error is
returned (and not EACCES).

>  
> +/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
> +static int test_fioqsize_ioctl(int fd)
> +{
> +	loff_t size;
> +
> +	if (ioctl(fd, FIOQSIZE, &size) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Attempt ioctls on regular files, with file descriptors opened before and
> + * after landlocking.
> + */
> +TEST_F_FORK(layout1, ioctl)
> +{
> +	const struct rule rules[] = {
> +		{
> +			.path = file1_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> +		},
> +		{
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> +		},
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
> +	int ruleset_fd;
> +	int dir_s1d1_fd, file1_s1d1_fd, dir_s2d1_fd;
> +
> +	/* Enables Landlock. */
> +	ruleset_fd = create_ruleset(_metadata, handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	dir_s1d1_fd = open(dir_s1d1, O_RDONLY);

You can use O_CLOEXEC everywhere.

> +	ASSERT_LE(0, dir_s1d1_fd);
> +	file1_s1d1_fd = open(file1_s1d1, O_RDONLY);
> +	ASSERT_LE(0, file1_s1d1_fd);
> +	dir_s2d1_fd = open(dir_s2d1, O_RDONLY);
> +	ASSERT_LE(0, dir_s2d1_fd);
> +
> +	/*
> +	 * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is
> +	 * permitted.
> +	 */
> +	EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd));
> +	EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd));
> +	EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd));
> +
> +	/* Closes all file descriptors. */
> +	ASSERT_EQ(0, close(dir_s1d1_fd));
> +	ASSERT_EQ(0, close(file1_s1d1_fd));
> +	ASSERT_EQ(0, close(dir_s2d1_fd));
> +}
> +
> +TEST_F_FORK(layout1, ioctl_always_allowed)
> +{
> +	struct landlock_ruleset_attr attr = {

const struct landlock_ruleset_attr attr = {

> +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL,
> +	};
> +	int ruleset_fd, fd;
> +	int flag = 0;
> +	int n;

const int flag = 0;
int ruleset_fd, test_fd, n;


> +
> +	/* Enables Landlock. */
> +	ruleset_fd = landlock_create_ruleset(&attr, sizeof(attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	fd = open(file1_s1d1, O_RDONLY);
> +	ASSERT_LE(0, fd);
> +
> +	/* Checks that the restrictable FIOQSIZE is restricted. */
> +	EXPECT_EQ(EACCES, test_fioqsize_ioctl(fd));
> +
> +	/* Checks that unrestrictable commands are unrestricted. */
> +	EXPECT_EQ(0, ioctl(fd, FIOCLEX));
> +	EXPECT_EQ(0, ioctl(fd, FIONCLEX));
> +	EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
> +	EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
> +	EXPECT_EQ(0, ioctl(fd, FIONREAD, &n));
> +	EXPECT_EQ(0, n);
> +
> +	ASSERT_EQ(0, close(fd));
> +}
> +
>  /* clang-format off */
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
> -- 
> 2.41.0.694.ge786442a9b-goog
> 

Re: [PATCH v3 0/5] Landlock: IOCTL support - TTY restrictions RFC

[Mickaël Salaün]

Here is a proposal to restrict TTY with Landlock, complementary to this
patch series (which should land before any other IOCTL-related
features).

CCing folks part of TIOCSTI discussions, as a complementary approach to
https://lore.kernel.org/all/ZN+X6o3cDWcLoviq@google.com/

On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> Hello!
> 
> These patches add simple ioctl(2) support to Landlock.
> 
> Objective
> ~~~~~~~~~
> 
> Make ioctl(2) requests restrictable with Landlock,
> in a way that is useful for real-world applications.
> 
> Proposed approach
> ~~~~~~~~~~~~~~~~~
> 
> Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
> of ioctl(2) on file descriptors.
> 
> We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
> descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.
> 
> We make an exception for the common and known-harmless IOCTL commands FIOCLEX,
> FIONCLEX, FIONBIO, FIOASYNC and FIONREAD.  These IOCTL commands are always
> permitted.  The functionality of the first four is already available through
> fcntl(2), and FIONREAD only returns the number of ready-to-read bytes.
> 
> I believe that this approach works for the majority of use cases, and
> offers a good trade-off between Landlock API and implementation
> complexity and flexibility when the feature is used.
> 
> Current limitations
> ~~~~~~~~~~~~~~~~~~~
> 
> With this patch set, ioctl(2) requests can *not* be filtered based on
> file type, device number (dev_t) or on the ioctl(2) request number.
> 
> On the initial RFC patch set [1], we have reached consensus to start
> with this simpler coarse-grained approach, and build additional IOCTL
> restriction capabilities on top in subsequent steps.
> 
> [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/
> 
> Notable implications of this approach
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> * Existing inherited file descriptors stay unaffected
>   when a program enables Landlock.
> 
>   This means in particular that in common scenarios,
>   the terminal's IOCTLs (ioctl_tty(2)) continue to work.
> 
> * ioctl(2) continues to be available for file descriptors acquired
>   through means other than open(2).  Example: Network sockets,
>   memfd_create(2), file descriptors that are already open before the
>   Landlock ruleset is enabled.

Digging through all potential malicious use of TTYs and because TTYs are
part of process management (e.g. signaling) and mediated by the kernel,
I changed my mind and I think we should investigate on protecting shared
TTY thanks to Landlock, but not without context and not only against
TIOCSTI and TIOCLINUX IOCTLs.

TIOCSTI is abused since a few decades [1] [2], and as Günther said, it
is still the case [3] as with TIOCLINUX [4] [5]. Since Linux 6.2,
CONFIG_LEGACY_TIOCSTI (and the corresponding sysctl knob) can be set to
deny all use of TIOCSTI. This kernel configuration is a good step
forward but it may not be enabled everywhere because it is a
system-wide restriction. Moreover, it is not a sandboxing feature which
means developers cannot safely protect users from their applications
without impacting the whole system. Making Landlock able to protect
against this kind of attack and other TTY-based ones (e.g. snoop
keystrokes [6]) is definitely something worth it.

The behavior change should only affect a TTY which is shared (same
session or not) with a set of processes containing at least one
sandboxed process.

The simplest and more generic solution I came up with is to tie the TTY
(e.g. PTY slave) with the Landlock domain (if any) of the
*first process* to be a session leader with this TTY. For all sandboxed
processes, if the TTY's domain is more privileged than the process's
domain, then any TIOCSTI should be denied.

For the snooping protection, I think we could enforce that only the
(current) session leader can read the TTY. Same goes for writing to the
TTY (but this should already be covered).

Basically, all IOCTLs that enable, one way or another, to fool a user
should be restricted as TIOCSTI. This includes copy/paste requests
(TIOCLINUX subcommands), but also potentially font change (e.g.
PIO_FONT), keyboard mapping change, all CAP_SYS_TTY_CONFIG-checked
IOCTLs, and probably more. The goal is not to protect against
potentially annoying features such as keyboard light changes though, but
really to protect integrity and confidentiality of data going through
the TTY.

The goal is to enforce Landlock security boundaries across TTY's
clients. In a nutshell, if a process is sandboxed, only allow read,
write and most IOCTL requests if the TTY's domain would be ptracable
(see security/landlock/ptrace.c), otherwise deny such action. I think
this algorithm would fit well:
* if the current process is not sandboxed, then allow
* else if the TTY's domain is the same or a child of the current
  process's domain, then allow (including the TIOCSTI IOCTL)
* else if the current process is the session leader of this TTY, then
  allow read/write/non-TIOCSTI-IOCTLs
* else deny

The challenge would be to make these checks efficient, especially for
the read and write syscalls.

When setting the session leader, we could update the TTY's domain with
the highest-privileged one, or the NULL domain (i.e. the
root/unsandboxed). However, this would mean that previous TIOCSTI
requests could have been allowed and could now impact the current
(higher privileged) session leader. I think this cannot be properly
mitigated solely at the access control level. I'd prefer to properly
document this limitation for which I don't see any valid use case.
We should test if this theory works in practice with real-world
applications though. The question is: are they any programs that pass a
TTY FD to a (potentially malicious but sandboxed) process, and *then*
switch for the first time to a session leader with this TTY?

We might also not want to return EPERM for all kind of requests but EIO
instead.

Because of compatibility reasons, and different use cases, these
restrictions should only be enforced between Landlock domains that
opt-in for this feature thanks to a new ruleset's flag, something like
LANDLOCK_RULESET_SCOPE_TTY. So all mentions of Landlock domains should
in fact only refer to TTY-restricted-domains. As for the upcoming
Landlock network restrictions, these TTY restrictions should be
independent from any FS-related actions (e.g. mount).

BTW, the TIOCSTI would be useful to test (cf. kselftest) this kind of
restrictions.

What do you think?

[1] https://isopenbsdsecu.re/mitigations/tiocsti/
[2] https://jdebp.uk/FGA/TIOCSTI-is-a-kernel-problem.html
[3] https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=TIOCSTI
[4] https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=TIOCLINUX
[5] https://lore.kernel.org/all/ZN+X6o3cDWcLoviq@google.com/
[6] https://gist.github.com/thejh/e163071dfe4c96a9f9b589b7a2c24fc6

Re: [PATCH v3 5/5] landlock: Document ioctl support

[Günther Noack]

Hi!

On Fri, Aug 18, 2023 at 06:28:41PM +0200, Mickaël Salaün wrote:
> This looks good!
> 
> On Mon, Aug 14, 2023 at 07:28:16PM +0200, Günther Noack wrote:
> > +IOCTL support
> > +-------------
> > +
> > +The ``LANDLOCK_ACCESS_FS_IOCTL`` access right restricts the use of
> > +:manpage:`ioctl(2)`, but it only applies to newly opened files.  This means
> > +specifically that pre-existing file descriptors like STDIN, STDOUT and STDERR
> 
> According to man pages (and unlike IOCTL commands) we should not
> capitalize stdin, stdout and stderr.

Done.

> > +are unaffected.
> > +
> > +Users should be aware that TTY devices have traditionally permitted to control
> > +other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL
> > +commands.  It is therefore recommended to close inherited TTY file descriptors.
> 
> Good to see such warnings in the documentation.
> 
> We could also propose a simple solution to still uses stdin, stdout and
> stderr without complex TTY proxying: re-opening the TTY, or replacing
> related FD thanks to /proc/self/fd/*
> 
> For instance, with shell scripts it would look like this:
> exec </proc/self/fd/0
> exec >/proc/self/fd/1
> exec 2>/proc/self/fd/2
> 
> Because of TIOCGWINSZ and TCGETS, an interactive shell may not work as
> expected though.

I inserted the phrase

  ..., or to reopen them from ``/proc/self/fd/*`` without the
  ``LANDLOCK_ACCESS_FS_IOCTL`` right, if possible.

to hint at that approach.

I added "if possible" there, because there are thorny edge cases that make it
difficult as a general approach -- the open(2) can fail if a stacked Landlock
policy exists.

—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hi!

On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote:
> On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> > These patches add simple ioctl(2) support to Landlock.
> 
> [...]
> 
> > How we arrived at the list of always-permitted IOCTL commands
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > To decide which IOCTL commands should be blanket-permitted I went through the
> > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> > to understand what they are about.  The following list is my conclusion from
> > that.
> > 
> > We should always allow the following IOCTL commands:
> > 
> >  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
> >    close-on-exec flag
> >  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
> >    and async flags
> >  * FIONREAD - get the number of bytes available for reading (the implementation
> >    is defined per file type)
> 
> I think we should treat FIOQSIZE like FIONREAD, i.e. check for
> LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
> Tests should then rely on something else.

OK, I rewrote the tests to use FS_IOC_GETFLAGS.

Some thoughts on these two IOCTLs:

FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
only useful when the file is open for reading.  However, do you think that we
should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
right for that file.  In case (b), it would only work if you also opened the
file for reading.)

FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
reading case is obvious, but for writers it's also useful if you want to seek
around in the file, and make sure that the position that you seek to already
exists.  (I'm not sure whether that use case is relevant in practical
applications though.) -- Why would FIOQSIZE only be useful for readers?

(In fact, it seems to me almost like FIOQSIZE might rather be missing a security
hook check for one of the "getting file attribute" hooks?)

So basically, the implementation that I currently ended up with is:

switch (cmd) {
  case FIOCLEX:
  case FIONCLEX:
  case FIONBIO:
  case FIOASYNC:
  case FIOQSIZE:
    return 0;
  case FIONREAD:
    if (file->f_mode & FMODE_READ)
      return 0;
}

(with some comments in the source code, of course...)

Does that look reasonable to you?

—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

[Günther Noack]

Hello!

On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> >  	};
> >  	int fd, ruleset_fd;
> >  
> > -	/* Enable Landlock. */
> > +	/* Enables Landlock. */
> >  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> >  	ASSERT_LE(0, ruleset_fd);
> >  	enforce_ruleset(_metadata, ruleset_fd);
> > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> >  	ASSERT_EQ(0, close(fd));
> >  }
> 
> We should also check with O_PATH to make sure the correct error is
> returned (and not EACCES).

Is this remark referring to the code before it or after it?

My interpretation is that you are asking to test that test_fioqsize_ioctl() will
return errnos correctly?  Do I understand that correctly?  (I think that would
be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
all, which is below the threshold where it can be verified by staring at it for
a bit. :))

> > +/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
> > +static int test_fioqsize_ioctl(int fd)
> > +{
> > +	loff_t size;
> > +
> > +	if (ioctl(fd, FIOQSIZE, &size) < 0)
> > +		return errno;
> > +	return 0;
> > +}



> > +	dir_s1d1_fd = open(dir_s1d1, O_RDONLY);
> 
> You can use O_CLOEXEC everywhere.

Done.


> > +	ASSERT_LE(0, dir_s1d1_fd);
> > +	file1_s1d1_fd = open(file1_s1d1, O_RDONLY);
> > +	ASSERT_LE(0, file1_s1d1_fd);
> > +	dir_s2d1_fd = open(dir_s2d1, O_RDONLY);
> > +	ASSERT_LE(0, dir_s2d1_fd);
> > +
> > +	/*
> > +	 * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is
> > +	 * permitted.
> > +	 */
> > +	EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd));
> > +	EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd));
> > +	EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd));
> > +
> > +	/* Closes all file descriptors. */
> > +	ASSERT_EQ(0, close(dir_s1d1_fd));
> > +	ASSERT_EQ(0, close(file1_s1d1_fd));
> > +	ASSERT_EQ(0, close(dir_s2d1_fd));
> > +}
> > +
> > +TEST_F_FORK(layout1, ioctl_always_allowed)
> > +{
> > +	struct landlock_ruleset_attr attr = {
> 
> const struct landlock_ruleset_attr attr = {

Done.

I am personally unsure whether "const" is worth it for local variables, but I am
happy to abide by whatever the dominant style is.  (The kernel style guide
doesn't seem to mention it though.)

BTW, it's somewhat inconsistent within this file already -- we should maybe
clean this up.


> > +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL,
> > +	};
> > +	int ruleset_fd, fd;
> > +	int flag = 0;
> > +	int n;
> 
> const int flag = 0;
> int ruleset_fd, test_fd, n;

Done.

Thanks for the review!
—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> Hi!
> 
> On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote:
> > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> > > These patches add simple ioctl(2) support to Landlock.
> > 
> > [...]
> > 
> > > How we arrived at the list of always-permitted IOCTL commands
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > To decide which IOCTL commands should be blanket-permitted I went through the
> > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> > > to understand what they are about.  The following list is my conclusion from
> > > that.
> > > 
> > > We should always allow the following IOCTL commands:
> > > 
> > >  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
> > >    close-on-exec flag
> > >  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
> > >    and async flags
> > >  * FIONREAD - get the number of bytes available for reading (the implementation
> > >    is defined per file type)
> > 
> > I think we should treat FIOQSIZE like FIONREAD, i.e. check for
> > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
> > Tests should then rely on something else.
> 
> OK, I rewrote the tests to use FS_IOC_GETFLAGS.
> 
> Some thoughts on these two IOCTLs:
> 
> FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> only useful when the file is open for reading.  However, do you think that we
> should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> right for that file.  In case (b), it would only work if you also opened the
> file for reading.)

I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.

f->f_mode & FMODE_READ would make sense but it should have been handled
by the kernel; Landlock should not try to fix this inconsistency.

These are good test cases though.

It should be noted that SELinux considers FIONREAD as tied to metadata
reading (FILE__GETATTR). It think it makes more sense to tie it to the
read right (LANDLOCK_ACCESS_FS_READ_FILE) because it might be
(legitimately) required to properly read a file and FIONREAD is tied to
the content of a file, not file attributes.

> 
> FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> reading case is obvious, but for writers it's also useful if you want to seek
> around in the file, and make sure that the position that you seek to already
> exists.  (I'm not sure whether that use case is relevant in practical
> applications though.) -- Why would FIOQSIZE only be useful for readers?

Good point! The use case you define for writing is interesting. However,
would it make sense to seek at a specific offset without being able to
know/read the content? I guest not in theory, but in practice we might
want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
they only require to write (at a specific offset), or to deal with write
errors. Anyway, I guess that this information can be inferred by trying
to seek at a specific offset.  The only limitation that this approach
would bring is that it seems that we can seek into an FD even without
read nor write right, and there is no specific (LSM) access control for
this operation (and nobody seems to care about being able to read the
size of a symlink once opened). If this is correct, I think we should
indeed always allow FIOQSIZE. Being able to open a file requires
LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
check and test with O_PATH though.

> 
> (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> hook check for one of the "getting file attribute" hooks?)
> 
> So basically, the implementation that I currently ended up with is:
> 

Before checking these commands, we first need to check that the original
domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
bit and replace the file's allowed_access field (see
landlock_add_fs_access_mask() and similar helpers in the network patch
series that does a similar thing but for ruleset's handle access
rights), but here is the idea:

if (!landlock_file_handles_ioctl(file))
	return 0;

> switch (cmd) {
	/*
	 * Allows file descriptor and file description operations (see
	 * fcntl commands).
	 */
>   case FIOCLEX:
>   case FIONCLEX:
>   case FIONBIO:
>   case FIOASYNC:

>   case FIOQSIZE:
>     return 0;
>   case FIONREAD:
>     if (file->f_mode & FMODE_READ)

We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
FMODE_READ.

>       return 0;
> }
> 
> (with some comments in the source code, of course...)
> 
> Does that look reasonable to you?
> 
> —Günther
> 
> -- 
> Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

[Mickaël Salaün]

On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote:
> Hello!
> 
> On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> > >  	};
> > >  	int fd, ruleset_fd;
> > >  
> > > -	/* Enable Landlock. */
> > > +	/* Enables Landlock. */
> > >  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > >  	ASSERT_LE(0, ruleset_fd);
> > >  	enforce_ruleset(_metadata, ruleset_fd);
> > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> > >  	ASSERT_EQ(0, close(fd));
> > >  }
> > 
> > We should also check with O_PATH to make sure the correct error is
> > returned (and not EACCES).
> 
> Is this remark referring to the code before it or after it?
> 
> My interpretation is that you are asking to test that test_fioqsize_ioctl() will
> return errnos correctly?  Do I understand that correctly?  (I think that would
> be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
> all, which is below the threshold where it can be verified by staring at it for
> a bit. :))

I was refering to the previous memfd_ftruncate test, which is changed
with a next patch. We should check the access rights tied (and checkd)
to FD (i.e. truncate and ioctl) opened with O_PATH.

> 
> > > +/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */
> > > +static int test_fioqsize_ioctl(int fd)
> > > +{
> > > +	loff_t size;
> > > +
> > > +	if (ioctl(fd, FIOQSIZE, &size) < 0)
> > > +		return errno;
> > > +	return 0;
> > > +}
> 
> 
> 
> > > +	dir_s1d1_fd = open(dir_s1d1, O_RDONLY);
> > 
> > You can use O_CLOEXEC everywhere.
> 
> Done.
> 
> 
> > > +	ASSERT_LE(0, dir_s1d1_fd);
> > > +	file1_s1d1_fd = open(file1_s1d1, O_RDONLY);
> > > +	ASSERT_LE(0, file1_s1d1_fd);
> > > +	dir_s2d1_fd = open(dir_s2d1, O_RDONLY);
> > > +	ASSERT_LE(0, dir_s2d1_fd);
> > > +
> > > +	/*
> > > +	 * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is
> > > +	 * permitted.
> > > +	 */
> > > +	EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd));
> > > +	EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd));
> > > +	EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd));
> > > +
> > > +	/* Closes all file descriptors. */
> > > +	ASSERT_EQ(0, close(dir_s1d1_fd));
> > > +	ASSERT_EQ(0, close(file1_s1d1_fd));
> > > +	ASSERT_EQ(0, close(dir_s2d1_fd));
> > > +}
> > > +
> > > +TEST_F_FORK(layout1, ioctl_always_allowed)
> > > +{
> > > +	struct landlock_ruleset_attr attr = {
> > 
> > const struct landlock_ruleset_attr attr = {
> 
> Done.
> 
> I am personally unsure whether "const" is worth it for local variables, but I am
> happy to abide by whatever the dominant style is.  (The kernel style guide
> doesn't seem to mention it though.)

I prefer to constify as much as possible to be notified when a write
will be needed for a patch. From a security point of view, it's always
good to have as much as possible read-only data, at least in theory (it
might not always be enforced in memory). It's also useful as
documentation.

> 
> BTW, it's somewhat inconsistent within this file already -- we should maybe
> clean this up.

I probably missed some, more constification would be good, but not with
this patch series.

> 
> 
> > > +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL,
> > > +	};
> > > +	int ruleset_fd, fd;
> > > +	int flag = 0;
> > > +	int n;
> > 
> > const int flag = 0;
> > int ruleset_fd, test_fd, n;
> 
> Done.
> 
> Thanks for the review!
> —Günther
> 
> -- 
> Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > Hi!
> > 
> > On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote:
> > > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> > > > These patches add simple ioctl(2) support to Landlock.
> > > 
> > > [...]
> > > 
> > > > How we arrived at the list of always-permitted IOCTL commands
> > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > To decide which IOCTL commands should be blanket-permitted I went through the
> > > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> > > > to understand what they are about.  The following list is my conclusion from
> > > > that.
> > > > 
> > > > We should always allow the following IOCTL commands:
> > > > 
> > > >  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
> > > >    close-on-exec flag
> > > >  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
> > > >    and async flags
> > > >  * FIONREAD - get the number of bytes available for reading (the implementation
> > > >    is defined per file type)
> > > 
> > > I think we should treat FIOQSIZE like FIONREAD, i.e. check for
> > > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
> > > Tests should then rely on something else.
> > 
> > OK, I rewrote the tests to use FS_IOC_GETFLAGS.
> > 
> > Some thoughts on these two IOCTLs:
> > 
> > FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> > only useful when the file is open for reading.  However, do you think that we
> > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> > f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> > right for that file.  In case (b), it would only work if you also opened the
> > file for reading.)
> 
> I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
> and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.
> 
> f->f_mode & FMODE_READ would make sense but it should have been handled
> by the kernel; Landlock should not try to fix this inconsistency.
> 
> These are good test cases though.
> 
> It should be noted that SELinux considers FIONREAD as tied to metadata
> reading (FILE__GETATTR). It think it makes more sense to tie it to the
> read right (LANDLOCK_ACCESS_FS_READ_FILE) because it might be
> (legitimately) required to properly read a file and FIONREAD is tied to
> the content of a file, not file attributes.
> 
> > 
> > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> > reading case is obvious, but for writers it's also useful if you want to seek
> > around in the file, and make sure that the position that you seek to already
> > exists.  (I'm not sure whether that use case is relevant in practical
> > applications though.) -- Why would FIOQSIZE only be useful for readers?
> 
> Good point! The use case you define for writing is interesting. However,
> would it make sense to seek at a specific offset without being able to
> know/read the content? I guest not in theory, but in practice we might
> want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
> they only require to write (at a specific offset), or to deal with write
> errors. Anyway, I guess that this information can be inferred by trying
> to seek at a specific offset.  The only limitation that this approach
> would bring is that it seems that we can seek into an FD even without
> read nor write right, and there is no specific (LSM) access control for
> this operation (and nobody seems to care about being able to read the
> size of a symlink once opened). If this is correct, I think we should
> indeed always allow FIOQSIZE. Being able to open a file requires
> LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
> check and test with O_PATH though.

FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
nor write mode (i.e. O_PATH), so it could be an issue for landlocked
applications but they can explicitly allow IOCTL for this case. When
we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
should also tie FIOQSIZE to this access right, and we'll be able to
fully handle all the use cases without fully allowing all other IOCTLs.

> 
> > 
> > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > hook check for one of the "getting file attribute" hooks?)
> > 
> > So basically, the implementation that I currently ended up with is:
> > 
> 
> Before checking these commands, we first need to check that the original
> domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> bit and replace the file's allowed_access field (see
> landlock_add_fs_access_mask() and similar helpers in the network patch
> series that does a similar thing but for ruleset's handle access
> rights), but here is the idea:
> 
> if (!landlock_file_handles_ioctl(file))
> 	return 0;
> 
> > switch (cmd) {
> 	/*
> 	 * Allows file descriptor and file description operations (see
> 	 * fcntl commands).
> 	 */
> >   case FIOCLEX:
> >   case FIONCLEX:
> >   case FIONBIO:
> >   case FIOASYNC:
> 
> >   case FIOQSIZE:

We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
checks. A kselftest test should check that ENOTTY is returned according
to the file type and the access rights.

> >     return 0;
> >   case FIONREAD:
> >     if (file->f_mode & FMODE_READ)
> 
> We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
> FMODE_READ.
> 
> >       return 0;
> > }
> > 
> > (with some comments in the source code, of course...)
> > 
> > Does that look reasonable to you?
> > 
> > —Günther
> > 
> > -- 
> > Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

[Günther Noack]

Hello!

On Fri, Aug 25, 2023 at 07:07:01PM +0200, Mickaël Salaün wrote:
> On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote:
> > Hello!
> > 
> > On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> > > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> > > >  	};
> > > >  	int fd, ruleset_fd;
> > > >  
> > > > -	/* Enable Landlock. */
> > > > +	/* Enables Landlock. */
> > > >  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > > >  	ASSERT_LE(0, ruleset_fd);
> > > >  	enforce_ruleset(_metadata, ruleset_fd);
> > > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> > > >  	ASSERT_EQ(0, close(fd));
> > > >  }
> > > 
> > > We should also check with O_PATH to make sure the correct error is
> > > returned (and not EACCES).
> > 
> > Is this remark referring to the code before it or after it?
> > 
> > My interpretation is that you are asking to test that test_fioqsize_ioctl() will
> > return errnos correctly?  Do I understand that correctly?  (I think that would
> > be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
> > all, which is below the threshold where it can be verified by staring at it for
> > a bit. :))
> 
> I was refering to the previous memfd_ftruncate test, which is changed
> with a next patch. We should check the access rights tied (and checkd)
> to FD (i.e. truncate and ioctl) opened with O_PATH.

OK, I added a test that checks ioctl(2) and ftruncate(2) on files that
were opened with O_PATH, both before and after enabling Landlock.
ftruncate() and ioctl() always give an EBADF error, both before and
after enabling Landlock (as described in open(2) in the section about
O_PATH).

A bit outside of the IOCTL path set scope:

I was surprised that it is even possible to successfully open a file
with O_PATH, even after Landlock is enabled and restricts all it can
in that file hierarchy.  This lets you detect that a file exists, even
when that file is in a directory whose contents you are otherwise not
permitted to list due to Landlock.

The logic for that is in the get_required_file_open_access() function.
Should we add a "LANDLOCK_ACCESS_FS_PATH_FILE" right, which would work
similar to LANDLOCK_ACCESS_FS_READ_FILE and
LANDLOCK_ACCESS_FS_WRITE_FILE, so that this can be restricted?

—Günther

Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

[Mickaël Salaün]

On Fri, Sep 01, 2023 at 03:35:59PM +0200, Günther Noack wrote:
> Hello!
> 
> On Fri, Aug 25, 2023 at 07:07:01PM +0200, Mickaël Salaün wrote:
> > On Fri, Aug 25, 2023 at 05:51:09PM +0200, Günther Noack wrote:
> > > Hello!
> > > 
> > > On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote:
> > > > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote:
> > > > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
> > > > >  	};
> > > > >  	int fd, ruleset_fd;
> > > > >  
> > > > > -	/* Enable Landlock. */
> > > > > +	/* Enables Landlock. */
> > > > >  	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
> > > > >  	ASSERT_LE(0, ruleset_fd);
> > > > >  	enforce_ruleset(_metadata, ruleset_fd);
> > > > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate)
> > > > >  	ASSERT_EQ(0, close(fd));
> > > > >  }
> > > > 
> > > > We should also check with O_PATH to make sure the correct error is
> > > > returned (and not EACCES).
> > > 
> > > Is this remark referring to the code before it or after it?
> > > 
> > > My interpretation is that you are asking to test that test_fioqsize_ioctl() will
> > > return errnos correctly?  Do I understand that correctly?  (I think that would
> > > be a little bit overdone, IMHO - it's just a test utility of ~10 lines after
> > > all, which is below the threshold where it can be verified by staring at it for
> > > a bit. :))
> > 
> > I was refering to the previous memfd_ftruncate test, which is changed
> > with a next patch. We should check the access rights tied (and checkd)
> > to FD (i.e. truncate and ioctl) opened with O_PATH.
> 
> OK, I added a test that checks ioctl(2) and ftruncate(2) on files that
> were opened with O_PATH, both before and after enabling Landlock.
> ftruncate() and ioctl() always give an EBADF error, both before and
> after enabling Landlock (as described in open(2) in the section about
> O_PATH).

Good!

> 
> A bit outside of the IOCTL path set scope:
> 
> I was surprised that it is even possible to successfully open a file
> with O_PATH, even after Landlock is enabled and restricts all it can
> in that file hierarchy.  This lets you detect that a file exists, even
> when that file is in a directory whose contents you are otherwise not
> permitted to list due to Landlock.

This is indeed intended and tested with the effective_access test.
O_PATH is handled the same way as chdir (and similar path-based
syscalls) and always allowed. However, I just realized that the O_PATH
case is not explicitly mentioned in the documentation.

> 
> The logic for that is in the get_required_file_open_access() function.
> Should we add a "LANDLOCK_ACCESS_FS_PATH_FILE" right, which would work
> similar to LANDLOCK_ACCESS_FS_READ_FILE and
> LANDLOCK_ACCESS_FS_WRITE_FILE, so that this can be restricted?

Having a file descriptor opened with O_PATH should not give any specific
access (hence the need to test with IOCTLs). O_PATH is designed to get an
explicit reference to the filesystem (without the issues of paths) and
use the related FD with *at() syscalls, including landlock_add_rule()
with a path_beneath rule.  FDs opened with O_PATH should not be an issue
by themselves for a sandbox, but the real issue is to discover paths,
i.e. the directory's execute access right.  This is why I think it would
make more sense to have something like a LANDLOCK_ACCESS_FS_WALK right
instead. This would enable to definitely deny any access to a file
hierarchy.

For chdir-like syscalls, we could rely on path_permission(), but for a
more holistic approach we need to properly handle the execute permission
on directories. See Christian's comment on chdir/path_permission and the
limit of what an LSM can infer from paths:
https://lore.kernel.org/r/20230530-mietfrei-zynisch-8b63a8566f66@brauner

We could rely on the last walked (leaf) dentry to allow or deny such
walk, but that would still enable malicious processes to infer path
because of intermediate walk that may return ENOENT. We could also
return ENOENT instead of EACCES, but this would not handle the case of
other access controls returning EACCES.

With this in mind, I think the better solution is to properly check each
intermediate directory during a path walk. This is not
currently possible with path-based LSM hooks but I think that we could
add a new LSM hook in filename_lookup(), close to the audit_inode()
call, to get the necessary information about the currently walked
directory.

Simply using this new hook with Landlock would add a significant
performance impact because of the way Landlock identifies paths (i.e.
walk back). An interesting approach to efficiently check Landlock access
right would be to store the collected access rights of a path in a
cache, and use it when checking a "child" of this path. According to
task_struct, there is only one path walk at a time per thread, which
would enable us to have only one cached path per task and
opportunistically use it in Landlock's is_access_to_paths_allowed() as a
backstop to end the walk. With this trick, I think in most cases no walk
back would be required, which would be great for performance. The main
challenge would be to efficiently handle the ".." directories.
See an initial approach of caching for Landlock:
https://lore.kernel.org/r/20210630224856.1313928-1-mic@digikod.net

Being able to control path walks would be a way to use (most?)
inode-based LSM hooks for Landlock, especially the
security_inode_permission(). Indeed. we could then tie an inode to a
path (resolution) thanks to the cache (and potentially other caches
according to the number of checked inodes).  This would be great for new
access rights such as LANDLOCK_ACCESS_FS_{READ,WRITE}_METADATA.

Xiu, that would be a good opportunity to continue your work, and
probably without waiting for IMA to be converted to a proper LSM. What
do you think?

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hello!

Thanks for the detailed review again!  The FIONREAD implementation that you
suggested works.  With FIOQSIZE I ran into some surprises - I believe the check
is a noop - more details below.

On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > > FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> > > only useful when the file is open for reading.  However, do you think that we
> > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> > > f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> > > right for that file.  In case (b), it would only work if you also opened the
> > > file for reading.)
> > 
> > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
> > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.

Just paraphrasing for later reference, because I almost misunderstood it:

FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*,
which is lingo for "listed in the ruleset_attr.handled_access_fs".
When it is listed there, that means that the Landlock policy does not
grant the LANDLOCK_ACCESS_FS_IOCTL right.

So we treat FIONREAD as blanket-permitted independent of the
LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have
LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do.


> > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> > > reading case is obvious, but for writers it's also useful if you want to seek
> > > around in the file, and make sure that the position that you seek to already
> > > exists.  (I'm not sure whether that use case is relevant in practical
> > > applications though.) -- Why would FIOQSIZE only be useful for readers?
> > 
> > Good point! The use case you define for writing is interesting. However,
> > would it make sense to seek at a specific offset without being able to
> > know/read the content? I guest not in theory, but in practice we might
> > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
> > they only require to write (at a specific offset), or to deal with write
> > errors. Anyway, I guess that this information can be inferred by trying
> > to seek at a specific offset.  The only limitation that this approach
> > would bring is that it seems that we can seek into an FD even without
> > read nor write right, and there is no specific (LSM) access control for
> > this operation (and nobody seems to care about being able to read the
> > size of a symlink once opened). If this is correct, I think we should
> > indeed always allow FIOQSIZE. Being able to open a file requires
> > LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
> > check and test with O_PATH though.
> 
> FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
> LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
> and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
> nor write mode (i.e. O_PATH), so it could be an issue for landlocked
> applications but they can explicitly allow IOCTL for this case. When
> we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
> should also tie FIOQSIZE to this access right, and we'll be able to
> fully handle all the use cases without fully allowing all other IOCTLs.

I implemented this check for the Landlock access rights in the ioctl hook, but
when testing it I realized that I could not ever get it to fail in practice:

ioctl(2) generally returns EBADF when the file was opened with O_PATH.  Early in
the ioctl(2) syscall implementation, it returns EBADF when the struct fd does
not have the ->file attribute set.  (This is even true for the commands to
manipulate the Close-on-exec flag, which don't strictly need that. But they
might work through fcntl.)

In my understanding from the open(2) man page, the only ways to open files are
with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH:

- O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2).
- O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2).
- O_RDWR: we had both of these two rights at the time of open(2).
- O_PATH: any ioctl(2) attempt returns EBADF early on

So at the time that the ioctl security hook gets called, we already know that
the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or
LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly
necessary?

Am I missing something here?  (In particular, am I overlooking additional ways
to call open(2) where the read and write rights are not necessary, other than
O_PATH?)

I'd propose this path forward: Let's keep the check for the rights as you
suggested, but I would just keep it as an additional safety net there, for
Landlock's internal consistency, and in case that future Linux versions
introduce new ways to open files.  I believe that at the moment, that check is
equivalent to always permitting the FIOQSIZE command in that hook (with the same
logic as for FIOCLEX, FIONCLEX etc).


> > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > hook check for one of the "getting file attribute" hooks?)
> > > 
> > > So basically, the implementation that I currently ended up with is:
> > > 
> > 
> > Before checking these commands, we first need to check that the original
> > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > bit and replace the file's allowed_access field (see
> > landlock_add_fs_access_mask() and similar helpers in the network patch
> > series that does a similar thing but for ruleset's handle access
> > rights), but here is the idea:
> > 
> > if (!landlock_file_handles_ioctl(file))
> > 	return 0;
> > 
> > > switch (cmd) {
> > 	/*
> > 	 * Allows file descriptor and file description operations (see
> > 	 * fcntl commands).
> > 	 */
> > >   case FIOCLEX:
> > >   case FIONCLEX:
> > >   case FIONBIO:
> > >   case FIOASYNC:
> > 
> > >   case FIOQSIZE:
> 
> We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> checks. A kselftest test should check that ENOTTY is returned according
> to the file type and the access rights.

It's not clear to me why we would need to add the same i_mode checks for
S_ISDIR() || S_ISREG() || S_ISLNK() there?  If these checks in do_vfs_ioctl()
fail, it returns -ENOTTY.  Is that not an appropriate error already?


> > >     return 0;
> > >   case FIONREAD:
> > >     if (file->f_mode & FMODE_READ)
> > 
> > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
> > FMODE_READ.

Done.


—Günther

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote:
> Hello!
> 
> Thanks for the detailed review again!  The FIONREAD implementation that you
> suggested works.  With FIOQSIZE I ran into some surprises - I believe the check
> is a noop - more details below.
> 
> On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > > > FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> > > > only useful when the file is open for reading.  However, do you think that we
> > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> > > > f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> > > > right for that file.  In case (b), it would only work if you also opened the
> > > > file for reading.)
> > > 
> > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
> > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.
> 
> Just paraphrasing for later reference, because I almost misunderstood it:
> 
> FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*,
> which is lingo for "listed in the ruleset_attr.handled_access_fs".
> When it is listed there, that means that the Landlock policy does not
> grant the LANDLOCK_ACCESS_FS_IOCTL right.
> 
> So we treat FIONREAD as blanket-permitted independent of the
> LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have
> LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do.

We are almost on the same line but here is the explicit algorithm:

if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
	allow FIONREAD
} else {
	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
			allow FIONREAD
		} else {
			deny FIONREAD
		}
	} else {
		if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
			allow FIONREAD
		} else {
			deny FIONREAD
		}
	}
}

The notation "FD's ruleset" refers to the Landlock domain that was being
evaluated when the FD was being opened, not necessarely the current
process's domain.

The same logic should apply for all IOCTL delegations, and the tests
should check this behavior. We may want to create a new helper to ease
this IOCTL delegation and future ones.

BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This
should probably be fixed. Any though Christian?

> 
> 
> > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> > > > reading case is obvious, but for writers it's also useful if you want to seek
> > > > around in the file, and make sure that the position that you seek to already
> > > > exists.  (I'm not sure whether that use case is relevant in practical
> > > > applications though.) -- Why would FIOQSIZE only be useful for readers?
> > > 
> > > Good point! The use case you define for writing is interesting. However,
> > > would it make sense to seek at a specific offset without being able to
> > > know/read the content? I guest not in theory, but in practice we might
> > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
> > > they only require to write (at a specific offset), or to deal with write
> > > errors. Anyway, I guess that this information can be inferred by trying
> > > to seek at a specific offset.  The only limitation that this approach
> > > would bring is that it seems that we can seek into an FD even without
> > > read nor write right, and there is no specific (LSM) access control for
> > > this operation (and nobody seems to care about being able to read the
> > > size of a symlink once opened). If this is correct, I think we should
> > > indeed always allow FIOQSIZE. Being able to open a file requires
> > > LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
> > > check and test with O_PATH though.
> > 
> > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
> > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
> > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
> > nor write mode (i.e. O_PATH), so it could be an issue for landlocked
> > applications but they can explicitly allow IOCTL for this case. When
> > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
> > should also tie FIOQSIZE to this access right, and we'll be able to
> > fully handle all the use cases without fully allowing all other IOCTLs.
> 
> I implemented this check for the Landlock access rights in the ioctl hook, but
> when testing it I realized that I could not ever get it to fail in practice:
> 
> ioctl(2) generally returns EBADF when the file was opened with O_PATH.  Early in
> the ioctl(2) syscall implementation, it returns EBADF when the struct fd does
> not have the ->file attribute set.  (This is even true for the commands to
> manipulate the Close-on-exec flag, which don't strictly need that. But they
> might work through fcntl.)

Yes, this is expected, but I'd like to keep these tests to guarantee
this behavior with all future kernel versions as well.

> 
> In my understanding from the open(2) man page, the only ways to open files are
> with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH:
> 
> - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2).
> - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2).
> - O_RDWR: we had both of these two rights at the time of open(2).
> - O_PATH: any ioctl(2) attempt returns EBADF early on
> 
> So at the time that the ioctl security hook gets called, we already know that
> the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or
> LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly
> necessary?

This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not
handled by the ruleset, see the subtle difference between not handled
and explicitly allowed. Here is the corresponding explicit algorithm:

if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
	allow FIOQSIZE
} else {
	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
			allow FIOQSIZE
		} else {
			if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
				if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
					allow FIOQSIZE
				} else {
					deny FIOQSIZE
				}
			} else {
				if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
					allow FIOQSIZE
				} else {
					deny FIOQSIZE
				}
			}
		}
	} else {
		if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
			if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
				allow FIOQSIZE
			} else {
				deny FIOQSIZE
			}
		} else {
			if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
				allow FIOQSIZE
			} else {
				deny FIOQSIZE
			}
		}
	}
}

> 
> Am I missing something here?  (In particular, am I overlooking additional ways
> to call open(2) where the read and write rights are not necessary, other than
> O_PATH?)

You're correct about the file mode and IOCTL returning EBADF for O_PATH,
but you didn't take into account the fact that (for whatever reason)
rulesets may not handle read/write file access rights.

> 
> I'd propose this path forward: Let's keep the check for the rights as you
> suggested, but I would just keep it as an additional safety net there, for
> Landlock's internal consistency, and in case that future Linux versions
> introduce new ways to open files.

Yes, you're correct, this is the right approach. Even if the kernel
doesn't need additional checks for now, we should still give
guarantees/promises that we can keep (i.e. part of Landlock's code), and
be consistent with Landlock's internals and code documentation.

> I believe that at the moment, that check is
> equivalent to always permitting the FIOQSIZE command in that hook (with the same
> logic as for FIOCLEX, FIONCLEX etc).

Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are
not allowed.

> 
> 
> > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > > hook check for one of the "getting file attribute" hooks?)
> > > > 
> > > > So basically, the implementation that I currently ended up with is:
> > > > 
> > > 
> > > Before checking these commands, we first need to check that the original
> > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > > bit and replace the file's allowed_access field (see
> > > landlock_add_fs_access_mask() and similar helpers in the network patch
> > > series that does a similar thing but for ruleset's handle access
> > > rights), but here is the idea:
> > > 
> > > if (!landlock_file_handles_ioctl(file))
> > > 	return 0;
> > > 
> > > > switch (cmd) {
> > > 	/*
> > > 	 * Allows file descriptor and file description operations (see
> > > 	 * fcntl commands).
> > > 	 */
> > > >   case FIOCLEX:
> > > >   case FIONCLEX:
> > > >   case FIONBIO:
> > > >   case FIOASYNC:
> > > 
> > > >   case FIOQSIZE:
> > 
> > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> > checks. A kselftest test should check that ENOTTY is returned according
> > to the file type and the access rights.
> 
> It's not clear to me why we would need to add the same i_mode checks for
> S_ISDIR() || S_ISREG() || S_ISLNK() there?  If these checks in do_vfs_ioctl()
> fail, it returns -ENOTTY.  Is that not an appropriate error already?

The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that
Landlock should not change the error codes according to the error types
(i.e. return ENOTTY when the inode is something else than a directory,
regular file, or symlink). Indeed, I think it's valuable to not blindly
return EACCES when the IOCTL doesn't make sense for this file type. This
should help user space handle meaningful error messages, inconsistent
requests, and fallback mechanisms. Tests should check these return
codes, see the network patch series (and especially the latest reviews
and changes) that takes care of this kind of error codes compatibility.

We could also return 0 (i.e. accept the request) and rely on the
do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky
from an access control point of view. Let's directly return ENOTTY as a
safeguard (which BTW also avoids some useless CPU instructions) and test
this behavior.

I think an access control mechanism, and especially Landlock, should be
as discreet as possible, and help developers quickly debug issues (and
avoid going through the access control layer if it doesn't make sense).
I think error ordering like this could be useful but I'd like to get
other point of views.

> 
> 
> > > >     return 0;
> > > >   case FIONREAD:
> > > >     if (file->f_mode & FMODE_READ)
> > > 
> > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
> > > FMODE_READ.
> 
> Done.

We could also mimic the do_vfs_ioctl() checks according to the file
being a regular file or not, but I think the FIONREAD semantic is well
defined and delegating this command to underlying VFS implementations
should not be an issue and be controlled the same way. It seems that
only the pipe FS also implements this IOCTL and we should treat it the
same way as for regular files. We should keep an eye on new
implementations of this IOCTL though, but I guess the limit of our
review stops at the FUSE boundary.

We should also allow all IOCTLs implemented by pipefifo_fops. They are
unrelated to the underlying filesystem (and then don't store data) but
they can still be found on any of them, they can only have an impact on
the related IPC (not directly system-wide like char/block devices), and
these kind of files may be swapped from a FD unrelated to the filesystem
to a named pipe according to application configuration (e.g. pipe
redirection). IOCTLs on unix named socket should also be allowed, but
anyway, they cannot be opened with open(2) (ENXIO is returned), so
socket FDs should never get any restriction because of a path_beneath
rule, so we can simply ignore this case (but still document and test
it).

Thinking more about the IOCTL control, I think we should help as much as
possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right
because that would mask their intent and it would grant a lot of
potentially-undefined accesses. Delegating at least most VFS IOCTLs
(i.e. standardized/generic IOCTLs) to regular Landlock access rights
should then be encouraged.

Even if new VFS IOCTLs should be scarce and new VFS syscalls should be
encouraged instead, we will still be able to delegate future VFS IOCTLs
to existing Landlock access rights according to their semantic.

Taking the list of VFS IOCTLs you extracted, here is my updated point of
view:
>  * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
>    system. Requires CAP_SYS_ADMIN.

These acts on an entire filesystem, not a file, and this is an
administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to
control these ones for now.

>  * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage
>    between multiple files.  These only work on some file systems, by design.

For these IOCTLs, the kernel already check file permission with the
remap_verify_area() and generic_file_rw_checks() calls. We should then
follow the same logic. However, we should not check if the FD has the
read/write Landlock access rights in the IOCTL hook (to only check once,
and avoid TOCTOU), but only check if they are handled. The effective
checks will be done by the VFS code, and we can then keep the current
error ordering.

FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE.
The other FD, extracted from IOCTL argument, is checked against the
read permission, but this should only be part of a comment (in our IOCTL
hook implementation).

FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE

>  * Commands that read file system internals:
>    * FS_IOC_FIEMAP - get information about file extent mapping
>      (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)

For FS_IOCT_FIEMAP, there is no file permission check. This should be
delegated to LANDLOCK_ACCESS_FS_READ_FILE or
LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE.

I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that
should not be an issue for now.

>    * FIBMAP - get a file's file system block number

There is no file permission check for FIBMAP (only the process's
capabilities).  I think the Landlock checks should be the same as for
FS_IOCT_FIEMAP.

>    * FIGETBSZ - get file system blocksize

I guess this also enables to optimize file storage. It would probably
make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or
follow the FIOQSIZE logic?

>  * Accessing file attributes:
>    * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))

These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA,
so only LANDLOCK_ACCESS_FS_IOCTL for now.

>    * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes

These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA,
so only LANDLOCK_ACCESS_FS_IOCTL for now.

>  * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
>    FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation
>    syscalls which predate fallocate(2).

ioctl_preallocate()-related IOCTLs should also be controlled like
FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is
in fact already a check with the vfs_fallocate/security_file_permission
call.

We should also check an IOCTL from an unrestricted special filesystems,
e.g. NS_GET_NSTYPE.

What do you think?

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hello!

On Mon, Sep 04, 2023 at 08:08:29PM +0200, Micka�l Sala�n wrote:
> On Sat, Sep 02, 2023 at 01:53:57PM +0200, G�nther Noack wrote:
> > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Micka�l Sala�n wrote:
> > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Micka�l Sala�n wrote:
> > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, G�nther Noack wrote:
> > > > > FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> > > > > only useful when the file is open for reading.  However, do you think that we
> > > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> > > > > f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> > > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> > > > > right for that file.  In case (b), it would only work if you also opened the
> > > > > file for reading.)
> > > > 
> > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
> > > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.
> > 
> > Just paraphrasing for later reference, because I almost misunderstood it:
> > 
> > FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*,
> > which is lingo for "listed in the ruleset_attr.handled_access_fs".
> > When it is listed there, that means that the Landlock policy does not
> > grant the LANDLOCK_ACCESS_FS_IOCTL right.
> > 
> > So we treat FIONREAD as blanket-permitted independent of the
> > LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have
> > LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do.
> 
> We are almost on the same line but here is the explicit algorithm:
> 
> if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
> 	allow FIONREAD
> } else {
> 	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
> 		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
> 			allow FIONREAD
> 		} else {
> 			deny FIONREAD
> 		}
> 	} else {
> 		if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> 			allow FIONREAD
> 		} else {
> 			deny FIONREAD
> 		}
> 	}
> }
> 
> The notation "FD's ruleset" refers to the Landlock domain that was being
> evaluated when the FD was being opened, not necessarely the current
> process's domain.
> 
> The same logic should apply for all IOCTL delegations, and the tests
> should check this behavior. We may want to create a new helper to ease
> this IOCTL delegation and future ones.

Thank you for making the algorithm that explicit -- that helps to trace down the
differences.  I can follow the logic now, but I still don't understand what your
underlying rationale for that is?

I believe that fundamentally, a core difference is:

For an access right R and a file F, for these two cases:

 (a) the access right R is unhandled  (nothing gets restricted)
 (b) the access right R is handled, but R is granted for F in a rule.

I believe that accesses in case (a) and (b) to the file F should have the same
results.

This is at least how the existing Landlock implementation works, as far as I can
tell.

("Refer" is an exceptional case, but we have documented that it was always
"implicitly handled" in ABI V1, which makes it consistent again.)


When I expand your code above to a boolean table, I end up with the following
decisions, depending on whether IOCTL and READ are handled or not, and whether
they are explicitly permitted for the file through a rule:


Micka�l's        IOCTL      IOCTL      IOCTL
suggestion       handled,   handled,   unhandled
2023-09-04       file       file not
                 permitted  permitted
--------------------------------------------------
READ handled,
file permitted   allow      allow      allow

READ handled,
f not permitted  deny       deny       allow

READ unhandled   allow      deny       allow


In patch set V3, this is different: Because I think that cases (a) and (b) from
above should always behave the same, the first and third column and row must be
symmetric and have the same entries.  So, in patch set V3, it is sufficient if
*one of* the two rights IOCTL and READ_FILE are present, in order to use the
FIONREAD IOCTL:


G�nther's        IOCTL      IOCTL      IOCTL
patch set V3     handled,   handled,   unhandled
2023-08-14       file       file not
                 permitted  permitted
--------------------------------------------------
READ handled,
file permitted   allow      allow      allow

READ handled,
f not permitted  allow      deny       allow

READ unhandled   allow      allow      allow


I have not spelled out the boolean logic tables for FIOQSIZE in the same way
(they would also have three dimensions, strictly speaking), but I assume that
the fundamental difference is the same -- you're also mentioning the "subtle
difference between not handled and explicitly allowed" below.

I honestly don't think that these cases should be different - it complicates
both the contract with the callers and our internal implementation if we need to
differentiate between three different configurations (handled + permitted,
handled + not permitted, unhandled), rather than collapsing these into two
(permitted, not permitted) as we currently do.

I would like to encourage users of Landlock to "handle" as many rights as
possible, and then poke holes into these restrictions for the files they need.
But that is only an easy sell if we can actually guarantee that it behaves the
same as if the right was unhandled.


> BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This
> should probably be fixed. Any though Christian?
> 
> > 
> > 
> > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> > > > > reading case is obvious, but for writers it's also useful if you want to seek
> > > > > around in the file, and make sure that the position that you seek to already
> > > > > exists.  (I'm not sure whether that use case is relevant in practical
> > > > > applications though.) -- Why would FIOQSIZE only be useful for readers?
> > > > 
> > > > Good point! The use case you define for writing is interesting. However,
> > > > would it make sense to seek at a specific offset without being able to
> > > > know/read the content? I guest not in theory, but in practice we might
> > > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
> > > > they only require to write (at a specific offset), or to deal with write
> > > > errors. Anyway, I guess that this information can be inferred by trying
> > > > to seek at a specific offset.  The only limitation that this approach
> > > > would bring is that it seems that we can seek into an FD even without
> > > > read nor write right, and there is no specific (LSM) access control for
> > > > this operation (and nobody seems to care about being able to read the
> > > > size of a symlink once opened). If this is correct, I think we should
> > > > indeed always allow FIOQSIZE. Being able to open a file requires
> > > > LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
> > > > check and test with O_PATH though.
> > > 
> > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
> > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
> > > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
> > > nor write mode (i.e. O_PATH), so it could be an issue for landlocked
> > > applications but they can explicitly allow IOCTL for this case. When
> > > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
> > > should also tie FIOQSIZE to this access right, and we'll be able to
> > > fully handle all the use cases without fully allowing all other IOCTLs.
> > 
> > I implemented this check for the Landlock access rights in the ioctl hook, but
> > when testing it I realized that I could not ever get it to fail in practice:
> > 
> > ioctl(2) generally returns EBADF when the file was opened with O_PATH.  Early in
> > the ioctl(2) syscall implementation, it returns EBADF when the struct fd does
> > not have the ->file attribute set.  (This is even true for the commands to
> > manipulate the Close-on-exec flag, which don't strictly need that. But they
> > might work through fcntl.)
> 
> Yes, this is expected, but I'd like to keep these tests to guarantee
> this behavior with all future kernel versions as well.
> 
> > 
> > In my understanding from the open(2) man page, the only ways to open files are
> > with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH:
> > 
> > - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2).
> > - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2).
> > - O_RDWR: we had both of these two rights at the time of open(2).
> > - O_PATH: any ioctl(2) attempt returns EBADF early on
> > 
> > So at the time that the ioctl security hook gets called, we already know that
> > the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or
> > LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly
> > necessary?
> 
> This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not
> handled by the ruleset, see the subtle difference between not handled
> and explicitly allowed. Here is the corresponding explicit algorithm:
> 
> if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
> 	allow FIOQSIZE
> } else {
> 	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
> 		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
> 			allow FIOQSIZE
> 		} else {
> 			if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
> 				if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
> 					allow FIOQSIZE
> 				} else {
> 					deny FIOQSIZE
> 				}
> 			} else {
> 				if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> 					allow FIOQSIZE
> 				} else {
> 					deny FIOQSIZE
> 				}
> 			}
> 		}
> 	} else {
> 		if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
> 			if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
> 				allow FIOQSIZE
> 			} else {
> 				deny FIOQSIZE
> 			}
> 		} else {
> 			if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> 				allow FIOQSIZE
> 			} else {
> 				deny FIOQSIZE
> 			}
> 		}
> 	}
> }
> 
> > 
> > Am I missing something here?  (In particular, am I overlooking additional ways
> > to call open(2) where the read and write rights are not necessary, other than
> > O_PATH?)
> 
> You're correct about the file mode and IOCTL returning EBADF for O_PATH,
> but you didn't take into account the fact that (for whatever reason)
> rulesets may not handle read/write file access rights.
> 
> > 
> > I'd propose this path forward: Let's keep the check for the rights as you
> > suggested, but I would just keep it as an additional safety net there, for
> > Landlock's internal consistency, and in case that future Linux versions
> > introduce new ways to open files.
> 
> Yes, you're correct, this is the right approach. Even if the kernel
> doesn't need additional checks for now, we should still give
> guarantees/promises that we can keep (i.e. part of Landlock's code), and
> be consistent with Landlock's internals and code documentation.
> 
> > I believe that at the moment, that check is
> > equivalent to always permitting the FIOQSIZE command in that hook (with the same
> > logic as for FIOCLEX, FIONCLEX etc).
> 
> Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are
> not allowed.
> 
> > 
> > 
> > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > > > hook check for one of the "getting file attribute" hooks?)
> > > > > 
> > > > > So basically, the implementation that I currently ended up with is:
> > > > > 
> > > > 
> > > > Before checking these commands, we first need to check that the original
> > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > > > bit and replace the file's allowed_access field (see
> > > > landlock_add_fs_access_mask() and similar helpers in the network patch
> > > > series that does a similar thing but for ruleset's handle access
> > > > rights), but here is the idea:
> > > > 
> > > > if (!landlock_file_handles_ioctl(file))
> > > > 	return 0;
> > > > 
> > > > > switch (cmd) {
> > > > 	/*
> > > > 	 * Allows file descriptor and file description operations (see
> > > > 	 * fcntl commands).
> > > > 	 */
> > > > >   case FIOCLEX:
> > > > >   case FIONCLEX:
> > > > >   case FIONBIO:
> > > > >   case FIOASYNC:
> > > > 
> > > > >   case FIOQSIZE:
> > > 
> > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> > > checks. A kselftest test should check that ENOTTY is returned according
> > > to the file type and the access rights.
> > 
> > It's not clear to me why we would need to add the same i_mode checks for
> > S_ISDIR() || S_ISREG() || S_ISLNK() there?  If these checks in do_vfs_ioctl()
> > fail, it returns -ENOTTY.  Is that not an appropriate error already?
> 
> The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that
> Landlock should not change the error codes according to the error types
> (i.e. return ENOTTY when the inode is something else than a directory,
> regular file, or symlink). Indeed, I think it's valuable to not blindly
> return EACCES when the IOCTL doesn't make sense for this file type. This
> should help user space handle meaningful error messages, inconsistent
> requests, and fallback mechanisms. Tests should check these return
> codes, see the network patch series (and especially the latest reviews
> and changes) that takes care of this kind of error codes compatibility.
> 
> We could also return 0 (i.e. accept the request) and rely on the
> do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky
> from an access control point of view. Let's directly return ENOTTY as a
> safeguard (which BTW also avoids some useless CPU instructions) and test
> this behavior.
> 
> I think an access control mechanism, and especially Landlock, should be
> as discreet as possible, and help developers quickly debug issues (and
> avoid going through the access control layer if it doesn't make sense).
> I think error ordering like this could be useful but I'd like to get
> other point of views.

I see what you mean now, OK.

Another option, btw, would be to return ENOTTY generally when Landlock denies an
IOCTL attempt, instead of EACCES, as I have previously suggested in
https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just
do that instead?

I believe that users of ioctl(2) should be better equipped to deal with ENOTTY,
because that is an error that ioctl(2) can return in general, whereas EACCES can
only be returned if one of the specific subcommands returns it.

According to the man page, ENOTTY is the error that ioctl(2) returns if the
given "request does not apply to the kind of object that the file descriptor fd
references".

That is not 100% what is happening here, but from the errors listed in ioctl(2),
this seems to be the closest, semantically.

What do you think?

> 
> > 
> > 
> > > > >     return 0;
> > > > >   case FIONREAD:
> > > > >     if (file->f_mode & FMODE_READ)
> > > > 
> > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
> > > > FMODE_READ.
> > 
> > Done.
> 
> We could also mimic the do_vfs_ioctl() checks according to the file
> being a regular file or not, but I think the FIONREAD semantic is well
> defined and delegating this command to underlying VFS implementations
> should not be an issue and be controlled the same way. It seems that
> only the pipe FS also implements this IOCTL and we should treat it the
> same way as for regular files. We should keep an eye on new
> implementations of this IOCTL though, but I guess the limit of our
> review stops at the FUSE boundary.
> 
> We should also allow all IOCTLs implemented by pipefifo_fops. They are
> unrelated to the underlying filesystem (and then don't store data) but
> they can still be found on any of them, they can only have an impact on
> the related IPC (not directly system-wide like char/block devices), and
> these kind of files may be swapped from a FD unrelated to the filesystem
> to a named pipe according to application configuration (e.g. pipe
> redirection). IOCTLs on unix named socket should also be allowed, but
> anyway, they cannot be opened with open(2) (ENXIO is returned), so
> socket FDs should never get any restriction because of a path_beneath
> rule, so we can simply ignore this case (but still document and test
> it).

Thanks, that's a good observation!  Adding named pipes and named unix sockets to
the TODO list for the next iteration.  I had not thought of these.


> Thinking more about the IOCTL control, I think we should help as much as
> possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right
> because that would mask their intent and it would grant a lot of
> potentially-undefined accesses. Delegating at least most VFS IOCTLs
> (i.e. standardized/generic IOCTLs) to regular Landlock access rights
> should then be encouraged.
> 
> Even if new VFS IOCTLs should be scarce and new VFS syscalls should be
> encouraged instead, we will still be able to delegate future VFS IOCTLs
> to existing Landlock access rights according to their semantic.

OK, the approach makes sense.  I'll respond on these individual items as I get
around to implement them when we have clarified the unhandled/handled+permitted
question above.


> Taking the list of VFS IOCTLs you extracted, here is my updated point of
> view:
> >  * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
> >    system. Requires CAP_SYS_ADMIN.
> 
> These acts on an entire filesystem, not a file, and this is an
> administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to
> control these ones for now.
> 
> >  * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage
> >    between multiple files.  These only work on some file systems, by design.
> 
> For these IOCTLs, the kernel already check file permission with the
> remap_verify_area() and generic_file_rw_checks() calls. We should then
> follow the same logic. However, we should not check if the FD has the
> read/write Landlock access rights in the IOCTL hook (to only check once,
> and avoid TOCTOU), but only check if they are handled. The effective
> checks will be done by the VFS code, and we can then keep the current
> error ordering.
> 
> FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE.
> The other FD, extracted from IOCTL argument, is checked against the
> read permission, but this should only be part of a comment (in our IOCTL
> hook implementation).
> 
> FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE
> 
> >  * Commands that read file system internals:
> >    * FS_IOC_FIEMAP - get information about file extent mapping
> >      (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)
> 
> For FS_IOCT_FIEMAP, there is no file permission check. This should be
> delegated to LANDLOCK_ACCESS_FS_READ_FILE or
> LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE.
> 
> I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that
> should not be an issue for now.
> 
> >    * FIBMAP - get a file's file system block number
> 
> There is no file permission check for FIBMAP (only the process's
> capabilities).  I think the Landlock checks should be the same as for
> FS_IOCT_FIEMAP.
> 
> >    * FIGETBSZ - get file system blocksize
> 
> I guess this also enables to optimize file storage. It would probably
> make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or
> follow the FIOQSIZE logic?
> 
> >  * Accessing file attributes:
> >    * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
> 
> These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA,
> so only LANDLOCK_ACCESS_FS_IOCTL for now.
> 
> >    * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes
> 
> These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA,
> so only LANDLOCK_ACCESS_FS_IOCTL for now.
> 
> >  * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
> >    FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation
> >    syscalls which predate fallocate(2).
> 
> ioctl_preallocate()-related IOCTLs should also be controlled like
> FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is
> in fact already a check with the vfs_fallocate/security_file_permission
> call.
> 
> We should also check an IOCTL from an unrestricted special filesystems,
> e.g. NS_GET_NSTYPE.
> 
> What do you think?

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> Hello!
> 
> On Mon, Sep 04, 2023 at 08:08:29PM +0200, Mickaël Salaün wrote:
> > On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote:
> > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > > > > > FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> > > > > > only useful when the file is open for reading.  However, do you think that we
> > > > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> > > > > > f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> > > > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> > > > > > right for that file.  In case (b), it would only work if you also opened the
> > > > > > file for reading.)
> > > > > 
> > > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
> > > > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.
> > > 
> > > Just paraphrasing for later reference, because I almost misunderstood it:
> > > 
> > > FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*,
> > > which is lingo for "listed in the ruleset_attr.handled_access_fs".
> > > When it is listed there, that means that the Landlock policy does not
> > > grant the LANDLOCK_ACCESS_FS_IOCTL right.
> > > 
> > > So we treat FIONREAD as blanket-permitted independent of the
> > > LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have
> > > LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do.
> > 
> > We are almost on the same line but here is the explicit algorithm:
> > 
> > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
> > 	allow FIONREAD
> > } else {
> > 	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
> > 		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
> > 			allow FIONREAD
> > 		} else {
> > 			deny FIONREAD
> > 		}
> > 	} else {
> > 		if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> > 			allow FIONREAD
> > 		} else {
> > 			deny FIONREAD
> > 		}
> > 	}
> > }
> > 
> > The notation "FD's ruleset" refers to the Landlock domain that was being
> > evaluated when the FD was being opened, not necessarely the current
> > process's domain.
> > 
> > The same logic should apply for all IOCTL delegations, and the tests
> > should check this behavior. We may want to create a new helper to ease
> > this IOCTL delegation and future ones.
> 
> Thank you for making the algorithm that explicit -- that helps to trace down the
> differences.  I can follow the logic now, but I still don't understand what your
> underlying rationale for that is?
> 
> I believe that fundamentally, a core difference is:
> 
> For an access right R and a file F, for these two cases:
> 
>  (a) the access right R is unhandled  (nothing gets restricted)
>  (b) the access right R is handled, but R is granted for F in a rule.
> 
> I believe that accesses in case (a) and (b) to the file F should have the same
> results.
> 
> This is at least how the existing Landlock implementation works, as far as I can
> tell.
> 
> ("Refer" is an exceptional case, but we have documented that it was always
> "implicitly handled" in ABI V1, which makes it consistent again.)
> 
> 
> When I expand your code above to a boolean table, I end up with the following
> decisions, depending on whether IOCTL and READ are handled or not, and whether
> they are explicitly permitted for the file through a rule:
> 
> 
> Mickaël's        IOCTL      IOCTL      IOCTL
> suggestion       handled,   handled,   unhandled
> 2023-09-04       file       file not
>                  permitted  permitted
> --------------------------------------------------
> READ handled,
> file permitted   allow      allow      allow
> 
> READ handled,
> f not permitted  deny       deny       allow
> 
> READ unhandled   allow      deny       allow
> 
> 
> In patch set V3, this is different: Because I think that cases (a) and (b) from
> above should always behave the same, the first and third column and row must be
> symmetric and have the same entries.  So, in patch set V3, it is sufficient if
> *one of* the two rights IOCTL and READ_FILE are present, in order to use the
> FIONREAD IOCTL:
> 
> 
> Günther's        IOCTL      IOCTL      IOCTL
> patch set V3     handled,   handled,   unhandled
> 2023-08-14       file       file not
>                  permitted  permitted
> --------------------------------------------------
> READ handled,
> file permitted   allow      allow      allow
> 
> READ handled,
> f not permitted  allow      deny       allow
> 
> READ unhandled   allow      allow      allow

A first difference is about (READ unhandled) AND (IOCTL handled +
file not permitted). It will not be possible to follow the same logic
with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA
that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it
consistent.

A second difference is about (READ handled + f not permitted) AND
(IOCTL handled + file permitted). The reasoning was to avoid giving too
much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access
rights are implemented. This looks quite similar to the CAP_SYS_ADMIN
right that can basically do anything, and new capabilites are mainly a
subset of this one. My proposal was to incrementally downgrade the power
given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the
I was thinking that, if we make a requirement to have the "new correct"
access right, the application update might drop the IOCTL access right.
I now think this reasoning is flawed.

Indeed, this comparaison doesn't fit well because IOCTLs are not a
superset of all access rights, and because nothing can force developers
that already gave access to all IOCTLs for a file to not just add
another access right (instead of swapping them).

Instead, I think user space libraries should manage this kind of access
right swapping when possible and have a fallback mechanism relying on
the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they
may be updated before the (stable system) kernel, and this would be
easier for developers to manage.

In a nutshell, it is about giving control for an action (e.g. FIONREAD)
to either a unique access right or to a set of access rights. At first,
I would have preferred to have a unique access right to control an
action, because it is simpler (e.g. for audit/debug). On the other hand,
we need to handle access rights that allow the same action (e.g. file
read OR write for FIOQSIZE). I now think your approach (i.e. set of
access rights to control an action) could make more sense. Another good
point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which
could in fact be difficult to understand for users. Nested Landlock
domains should also be easier to manage with this logic.

> 
> 
> I have not spelled out the boolean logic tables for FIOQSIZE in the same way
> (they would also have three dimensions, strictly speaking), but I assume that
> the fundamental difference is the same -- you're also mentioning the "subtle
> difference between not handled and explicitly allowed" below.
> 
> I honestly don't think that these cases should be different - it complicates
> both the contract with the callers and our internal implementation if we need to
> differentiate between three different configurations (handled + permitted,
> handled + not permitted, unhandled), rather than collapsing these into two
> (permitted, not permitted) as we currently do.
> 
> I would like to encourage users of Landlock to "handle" as many rights as
> possible, and then poke holes into these restrictions for the files they need.
> But that is only an easy sell if we can actually guarantee that it behaves the
> same as if the right was unhandled.

I agree with that, but there are compatibility limitations, hence the
first difference. This case should be rare though, i.e. app not
supporting an existing access right but still willing to support a new
one (IOCTL).

> 
> 
> > BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This
> > should probably be fixed. Any though Christian?
> > 
> > > 
> > > 
> > > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> > > > > > reading case is obvious, but for writers it's also useful if you want to seek
> > > > > > around in the file, and make sure that the position that you seek to already
> > > > > > exists.  (I'm not sure whether that use case is relevant in practical
> > > > > > applications though.) -- Why would FIOQSIZE only be useful for readers?
> > > > > 
> > > > > Good point! The use case you define for writing is interesting. However,
> > > > > would it make sense to seek at a specific offset without being able to
> > > > > know/read the content? I guest not in theory, but in practice we might
> > > > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
> > > > > they only require to write (at a specific offset), or to deal with write
> > > > > errors. Anyway, I guess that this information can be inferred by trying
> > > > > to seek at a specific offset.  The only limitation that this approach
> > > > > would bring is that it seems that we can seek into an FD even without
> > > > > read nor write right, and there is no specific (LSM) access control for
> > > > > this operation (and nobody seems to care about being able to read the
> > > > > size of a symlink once opened). If this is correct, I think we should
> > > > > indeed always allow FIOQSIZE. Being able to open a file requires
> > > > > LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
> > > > > check and test with O_PATH though.
> > > > 
> > > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
> > > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
> > > > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
> > > > nor write mode (i.e. O_PATH), so it could be an issue for landlocked
> > > > applications but they can explicitly allow IOCTL for this case. When
> > > > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
> > > > should also tie FIOQSIZE to this access right, and we'll be able to
> > > > fully handle all the use cases without fully allowing all other IOCTLs.
> > > 
> > > I implemented this check for the Landlock access rights in the ioctl hook, but
> > > when testing it I realized that I could not ever get it to fail in practice:
> > > 
> > > ioctl(2) generally returns EBADF when the file was opened with O_PATH.  Early in
> > > the ioctl(2) syscall implementation, it returns EBADF when the struct fd does
> > > not have the ->file attribute set.  (This is even true for the commands to
> > > manipulate the Close-on-exec flag, which don't strictly need that. But they
> > > might work through fcntl.)
> > 
> > Yes, this is expected, but I'd like to keep these tests to guarantee
> > this behavior with all future kernel versions as well.
> > 
> > > 
> > > In my understanding from the open(2) man page, the only ways to open files are
> > > with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH:
> > > 
> > > - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2).
> > > - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2).
> > > - O_RDWR: we had both of these two rights at the time of open(2).
> > > - O_PATH: any ioctl(2) attempt returns EBADF early on
> > > 
> > > So at the time that the ioctl security hook gets called, we already know that
> > > the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or
> > > LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly
> > > necessary?
> > 
> > This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not
> > handled by the ruleset, see the subtle difference between not handled
> > and explicitly allowed. Here is the corresponding explicit algorithm:
> > 
> > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) {
> > 	allow FIOQSIZE
> > } else {
> > 	if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) {
> > 		if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) {
> > 			allow FIOQSIZE
> > 		} else {
> > 			if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
> > 				if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
> > 					allow FIOQSIZE
> > 				} else {
> > 					deny FIOQSIZE
> > 				}
> > 			} else {
> > 				if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> > 					allow FIOQSIZE
> > 				} else {
> > 					deny FIOQSIZE
> > 				}
> > 			}
> > 		}
> > 	} else {
> > 		if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) {
> > 			if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) {
> > 				allow FIOQSIZE
> > 			} else {
> > 				deny FIOQSIZE
> > 			}
> > 		} else {
> > 			if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) {
> > 				allow FIOQSIZE
> > 			} else {
> > 				deny FIOQSIZE
> > 			}
> > 		}
> > 	}
> > }
> > 
> > > 
> > > Am I missing something here?  (In particular, am I overlooking additional ways
> > > to call open(2) where the read and write rights are not necessary, other than
> > > O_PATH?)
> > 
> > You're correct about the file mode and IOCTL returning EBADF for O_PATH,
> > but you didn't take into account the fact that (for whatever reason)
> > rulesets may not handle read/write file access rights.
> > 
> > > 
> > > I'd propose this path forward: Let's keep the check for the rights as you
> > > suggested, but I would just keep it as an additional safety net there, for
> > > Landlock's internal consistency, and in case that future Linux versions
> > > introduce new ways to open files.
> > 
> > Yes, you're correct, this is the right approach. Even if the kernel
> > doesn't need additional checks for now, we should still give
> > guarantees/promises that we can keep (i.e. part of Landlock's code), and
> > be consistent with Landlock's internals and code documentation.
> > 
> > > I believe that at the moment, that check is
> > > equivalent to always permitting the FIOQSIZE command in that hook (with the same
> > > logic as for FIOCLEX, FIONCLEX etc).
> > 
> > Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are
> > not allowed.
> > 
> > > 
> > > 
> > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > > > > hook check for one of the "getting file attribute" hooks?)
> > > > > > 
> > > > > > So basically, the implementation that I currently ended up with is:
> > > > > > 
> > > > > 
> > > > > Before checking these commands, we first need to check that the original
> > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > > > > bit and replace the file's allowed_access field (see
> > > > > landlock_add_fs_access_mask() and similar helpers in the network patch
> > > > > series that does a similar thing but for ruleset's handle access
> > > > > rights), but here is the idea:
> > > > > 
> > > > > if (!landlock_file_handles_ioctl(file))
> > > > > 	return 0;
> > > > > 
> > > > > > switch (cmd) {
> > > > > 	/*
> > > > > 	 * Allows file descriptor and file description operations (see
> > > > > 	 * fcntl commands).
> > > > > 	 */
> > > > > >   case FIOCLEX:
> > > > > >   case FIONCLEX:
> > > > > >   case FIONBIO:
> > > > > >   case FIOASYNC:
> > > > > 
> > > > > >   case FIOQSIZE:
> > > > 
> > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> > > > checks. A kselftest test should check that ENOTTY is returned according
> > > > to the file type and the access rights.
> > > 
> > > It's not clear to me why we would need to add the same i_mode checks for
> > > S_ISDIR() || S_ISREG() || S_ISLNK() there?  If these checks in do_vfs_ioctl()
> > > fail, it returns -ENOTTY.  Is that not an appropriate error already?
> > 
> > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that
> > Landlock should not change the error codes according to the error types
> > (i.e. return ENOTTY when the inode is something else than a directory,
> > regular file, or symlink). Indeed, I think it's valuable to not blindly
> > return EACCES when the IOCTL doesn't make sense for this file type. This
> > should help user space handle meaningful error messages, inconsistent
> > requests, and fallback mechanisms. Tests should check these return
> > codes, see the network patch series (and especially the latest reviews
> > and changes) that takes care of this kind of error codes compatibility.
> > 
> > We could also return 0 (i.e. accept the request) and rely on the
> > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky
> > from an access control point of view. Let's directly return ENOTTY as a
> > safeguard (which BTW also avoids some useless CPU instructions) and test
> > this behavior.
> > 
> > I think an access control mechanism, and especially Landlock, should be
> > as discreet as possible, and help developers quickly debug issues (and
> > avoid going through the access control layer if it doesn't make sense).
> > I think error ordering like this could be useful but I'd like to get
> > other point of views.
> 
> I see what you mean now, OK.
> 
> Another option, btw, would be to return ENOTTY generally when Landlock denies an
> IOCTL attempt, instead of EACCES, as I have previously suggested in
> https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just
> do that instead?
> 
> I believe that users of ioctl(2) should be better equipped to deal with ENOTTY,
> because that is an error that ioctl(2) can return in general, whereas EACCES can
> only be returned if one of the specific subcommands returns it.
> 
> According to the man page, ENOTTY is the error that ioctl(2) returns if the
> given "request does not apply to the kind of object that the file descriptor fd
> references".
> 
> That is not 100% what is happening here, but from the errors listed in ioctl(2),
> this seems to be the closest, semantically.
> 
> What do you think?

ENOTTY has a (kinda) well-defined semantic, which should not depend on
an access control. Other LSMs already return EACCES for denied IOCTLs,
so the man page should be updated with this information instead. ;)

> 
> > 
> > > 
> > > 
> > > > > >     return 0;
> > > > > >   case FIONREAD:
> > > > > >     if (file->f_mode & FMODE_READ)
> > > > > 
> > > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
> > > > > FMODE_READ.
> > > 
> > > Done.
> > 
> > We could also mimic the do_vfs_ioctl() checks according to the file
> > being a regular file or not, but I think the FIONREAD semantic is well
> > defined and delegating this command to underlying VFS implementations
> > should not be an issue and be controlled the same way. It seems that
> > only the pipe FS also implements this IOCTL and we should treat it the
> > same way as for regular files. We should keep an eye on new
> > implementations of this IOCTL though, but I guess the limit of our
> > review stops at the FUSE boundary.
> > 
> > We should also allow all IOCTLs implemented by pipefifo_fops. They are
> > unrelated to the underlying filesystem (and then don't store data) but
> > they can still be found on any of them, they can only have an impact on
> > the related IPC (not directly system-wide like char/block devices), and
> > these kind of files may be swapped from a FD unrelated to the filesystem
> > to a named pipe according to application configuration (e.g. pipe
> > redirection). IOCTLs on unix named socket should also be allowed, but
> > anyway, they cannot be opened with open(2) (ENXIO is returned), so
> > socket FDs should never get any restriction because of a path_beneath
> > rule, so we can simply ignore this case (but still document and test
> > it).
> 
> Thanks, that's a good observation!  Adding named pipes and named unix sockets to
> the TODO list for the next iteration.  I had not thought of these.
> 
> 
> > Thinking more about the IOCTL control, I think we should help as much as
> > possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right
> > because that would mask their intent and it would grant a lot of
> > potentially-undefined accesses. Delegating at least most VFS IOCTLs
> > (i.e. standardized/generic IOCTLs) to regular Landlock access rights
> > should then be encouraged.
> > 
> > Even if new VFS IOCTLs should be scarce and new VFS syscalls should be
> > encouraged instead, we will still be able to delegate future VFS IOCTLs
> > to existing Landlock access rights according to their semantic.
> 
> OK, the approach makes sense.  I'll respond on these individual items as I get
> around to implement them when we have clarified the unhandled/handled+permitted
> question above.
> 
> 
> > Taking the list of VFS IOCTLs you extracted, here is my updated point of
> > view:
> > >  * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
> > >    system. Requires CAP_SYS_ADMIN.
> > 
> > These acts on an entire filesystem, not a file, and this is an
> > administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to
> > control these ones for now.
> > 
> > >  * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage
> > >    between multiple files.  These only work on some file systems, by design.
> > 
> > For these IOCTLs, the kernel already check file permission with the
> > remap_verify_area() and generic_file_rw_checks() calls. We should then
> > follow the same logic. However, we should not check if the FD has the
> > read/write Landlock access rights in the IOCTL hook (to only check once,
> > and avoid TOCTOU), but only check if they are handled. The effective
> > checks will be done by the VFS code, and we can then keep the current
> > error ordering.
> > 
> > FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE.
> > The other FD, extracted from IOCTL argument, is checked against the
> > read permission, but this should only be part of a comment (in our IOCTL
> > hook implementation).
> > 
> > FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE
> > 
> > >  * Commands that read file system internals:
> > >    * FS_IOC_FIEMAP - get information about file extent mapping
> > >      (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)
> > 
> > For FS_IOCT_FIEMAP, there is no file permission check. This should be
> > delegated to LANDLOCK_ACCESS_FS_READ_FILE or
> > LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE.
> > 
> > I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that
> > should not be an issue for now.
> > 
> > >    * FIBMAP - get a file's file system block number
> > 
> > There is no file permission check for FIBMAP (only the process's
> > capabilities).  I think the Landlock checks should be the same as for
> > FS_IOCT_FIEMAP.
> > 
> > >    * FIGETBSZ - get file system blocksize
> > 
> > I guess this also enables to optimize file storage. It would probably
> > make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or
> > follow the FIOQSIZE logic?
> > 
> > >  * Accessing file attributes:
> > >    * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
> > 
> > These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA,
> > so only LANDLOCK_ACCESS_FS_IOCTL for now.
> > 
> > >    * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes
> > 
> > These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA,
> > so only LANDLOCK_ACCESS_FS_IOCTL for now.
> > 
> > >  * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
> > >    FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation
> > >    syscalls which predate fallocate(2).
> > 
> > ioctl_preallocate()-related IOCTLs should also be controlled like
> > FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is
> > in fact already a check with the vfs_fallocate/security_file_permission
> > call.
> > 
> > We should also check an IOCTL from an unrestricted special filesystems,
> > e.g. NS_GET_NSTYPE.
> > 
> > What do you think?

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Mon, Sep 11, 2023 at 05:25:34PM +0200, Mickaël Salaün wrote:
> On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> > Hello!
> > 
> > On Mon, Sep 04, 2023 at 08:08:29PM +0200, Mickaël Salaün wrote:
> > > On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote:
> > > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> > > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:

> > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > > > > > hook check for one of the "getting file attribute" hooks?)
> > > > > > > 
> > > > > > > So basically, the implementation that I currently ended up with is:
> > > > > > > 
> > > > > > 
> > > > > > Before checking these commands, we first need to check that the original
> > > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > > > > > bit and replace the file's allowed_access field (see
> > > > > > landlock_add_fs_access_mask() and similar helpers in the network patch
> > > > > > series that does a similar thing but for ruleset's handle access
> > > > > > rights), but here is the idea:
> > > > > > 
> > > > > > if (!landlock_file_handles_ioctl(file))
> > > > > > 	return 0;
> > > > > > 
> > > > > > > switch (cmd) {
> > > > > > 	/*
> > > > > > 	 * Allows file descriptor and file description operations (see
> > > > > > 	 * fcntl commands).
> > > > > > 	 */
> > > > > > >   case FIOCLEX:
> > > > > > >   case FIONCLEX:
> > > > > > >   case FIONBIO:
> > > > > > >   case FIOASYNC:
> > > > > > 
> > > > > > >   case FIOQSIZE:
> > > > > 
> > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> > > > > checks. A kselftest test should check that ENOTTY is returned according
> > > > > to the file type and the access rights.
> > > > 
> > > > It's not clear to me why we would need to add the same i_mode checks for
> > > > S_ISDIR() || S_ISREG() || S_ISLNK() there?  If these checks in do_vfs_ioctl()
> > > > fail, it returns -ENOTTY.  Is that not an appropriate error already?
> > > 
> > > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that
> > > Landlock should not change the error codes according to the error types
> > > (i.e. return ENOTTY when the inode is something else than a directory,
> > > regular file, or symlink). Indeed, I think it's valuable to not blindly
> > > return EACCES when the IOCTL doesn't make sense for this file type. This
> > > should help user space handle meaningful error messages, inconsistent
> > > requests, and fallback mechanisms. Tests should check these return
> > > codes, see the network patch series (and especially the latest reviews
> > > and changes) that takes care of this kind of error codes compatibility.
> > > 
> > > We could also return 0 (i.e. accept the request) and rely on the
> > > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky
> > > from an access control point of view. Let's directly return ENOTTY as a
> > > safeguard (which BTW also avoids some useless CPU instructions) and test
> > > this behavior.
> > > 
> > > I think an access control mechanism, and especially Landlock, should be
> > > as discreet as possible, and help developers quickly debug issues (and
> > > avoid going through the access control layer if it doesn't make sense).
> > > I think error ordering like this could be useful but I'd like to get
> > > other point of views.
> > 
> > I see what you mean now, OK.
> > 
> > Another option, btw, would be to return ENOTTY generally when Landlock denies an
> > IOCTL attempt, instead of EACCES, as I have previously suggested in
> > https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just
> > do that instead?
> > 
> > I believe that users of ioctl(2) should be better equipped to deal with ENOTTY,
> > because that is an error that ioctl(2) can return in general, whereas EACCES can
> > only be returned if one of the specific subcommands returns it.
> > 
> > According to the man page, ENOTTY is the error that ioctl(2) returns if the
> > given "request does not apply to the kind of object that the file descriptor fd
> > references".
> > 
> > That is not 100% what is happening here, but from the errors listed in ioctl(2),
> > this seems to be the closest, semantically.
> > 
> > What do you think?
> 
> ENOTTY has a (kinda) well-defined semantic, which should not depend on
> an access control. Other LSMs already return EACCES for denied IOCTLs,
> so the man page should be updated with this information instead. ;)

Well, thinking about this again, for the sake of consistency with other
IOCTLs, we should not specifically handle IOCTL error codes, but instead
return either 0 or -EACCES. The network error cases are special because
the LSM is called before some user-provided data are interpreted by the
network stack, and in this case we need to interpret these data ourself.
But in the case of IOCTLs, we may only need to handle the cases where an
IOCTL command may be interpreted by different implementations (e.g. VFS
or FS implementation), but even that is not a good idea, see below.

For FIOQSIZE, I don't think anymore that we should try to mimic the
do_vfs_ioctl() implementation. In fact, this approach could end up
confusing developers and leaking metadata (see FIGETBSZ). Even with
FIONREAD, the FS (i.e. vfs_ioctl() call) should follow the same
semantic as the VFS (i.e. do_vfs_ioctl() code), so we should treat them
the same and keep it simple: either return 0 or -EACCES.

About the file_ioctl() IOCTLs, we should check that there are no
overlapping with other IOCTLs (with different name). I think we should
trust the FS implementation to implement the same semantic, but much
less the device drivers. The main issue is with VFS and FS code
returning -ENOIOCTLCMD because in this case it is forwarded to any
implementation.

However, in the case of delegation, and as a safeguard, what we could do
to avoid driver IOCTL command overlaps is to check if the file is a
block or character device. In this case, we just don't delegate any
access right but make LANDLOCK_ACCESS_FS_IOCTL handle them (we need to
manage VFS's IOCTLs that deal with block/char device though). Again, we
should make sure that -ENOIOCTLCMD will not trick us (for the delegate
cases). I'm not sure how the link between drivers and the FS storing the
related block/char device is managed though. Does that make sense?

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hello!

On Mon, Sep 11, 2023 at 05:25:31PM +0200, Mickaël Salaün wrote:
> On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> > Thank you for making the algorithm that explicit -- that helps to trace down the
> > differences.  I can follow the logic now, but I still don't understand what your
> > underlying rationale for that is?
> > 
> > I believe that fundamentally, a core difference is:
> > 
> > For an access right R and a file F, for these two cases:
> > 
> >  (a) the access right R is unhandled  (nothing gets restricted)
> >  (b) the access right R is handled, but R is granted for F in a rule.
> > 
> > I believe that accesses in case (a) and (b) to the file F should have the same
> > results.
> > 
> > This is at least how the existing Landlock implementation works, as far as I can
> > tell.
> > 
> > ("Refer" is an exceptional case, but we have documented that it was always
> > "implicitly handled" in ABI V1, which makes it consistent again.)
> > 
> > 
> > When I expand your code above to a boolean table, I end up with the following
> > decisions, depending on whether IOCTL and READ are handled or not, and whether
> > they are explicitly permitted for the file through a rule:
> > 
> > 
> > Mickaël's        IOCTL      IOCTL      IOCTL
> > suggestion       handled,   handled,   unhandled
> > 2023-09-04       file       file not
> >                  permitted  permitted
> > --------------------------------------------------
> > READ handled,
> > file permitted   allow      allow      allow
> > 
> > READ handled,
> > f not permitted  deny       deny       allow
> > 
> > READ unhandled   allow      deny       allow
> > 
> > 
> > In patch set V3, this is different: Because I think that cases (a) and (b) from
> > above should always behave the same, the first and third column and row must be
> > symmetric and have the same entries.  So, in patch set V3, it is sufficient if
> > *one of* the two rights IOCTL and READ_FILE are present, in order to use the
> > FIONREAD IOCTL:
> > 
> > 
> > Günther's        IOCTL      IOCTL      IOCTL
> > patch set V3     handled,   handled,   unhandled
> > 2023-08-14       file       file not
> >                  permitted  permitted
> > --------------------------------------------------
> > READ handled,
> > file permitted   allow      allow      allow
> > 
> > READ handled,
> > f not permitted  allow      deny       allow
> > 
> > READ unhandled   allow      allow      allow
> 
> A first difference is about (READ unhandled) AND (IOCTL handled +
> file not permitted). It will not be possible to follow the same logic
> with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA
> that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it
> consistent.
> 
> A second difference is about (READ handled + f not permitted) AND
> (IOCTL handled + file permitted). The reasoning was to avoid giving too
> much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access
> rights are implemented. This looks quite similar to the CAP_SYS_ADMIN
> right that can basically do anything, and new capabilites are mainly a
> subset of this one. My proposal was to incrementally downgrade the power
> given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the
> I was thinking that, if we make a requirement to have the "new correct"
> access right, the application update might drop the IOCTL access right.
> I now think this reasoning is flawed.
> 
> Indeed, this comparaison doesn't fit well because IOCTLs are not a
> superset of all access rights, and because nothing can force developers
> that already gave access to all IOCTLs for a file to not just add
> another access right (instead of swapping them).
> 
> Instead, I think user space libraries should manage this kind of access
> right swapping when possible and have a fallback mechanism relying on
> the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they
> may be updated before the (stable system) kernel, and this would be
> easier for developers to manage.
> 
> In a nutshell, it is about giving control for an action (e.g. FIONREAD)
> to either a unique access right or to a set of access rights. At first,
> I would have preferred to have a unique access right to control an
> action, because it is simpler (e.g. for audit/debug). On the other hand,
> we need to handle access rights that allow the same action (e.g. file
> read OR write for FIOQSIZE). I now think your approach (i.e. set of
> access rights to control an action) could make more sense. Another good
> point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which
> could in fact be difficult to understand for users. Nested Landlock
> domains should also be easier to manage with this logic.

After we discussed this difficult topic briefly off-list, let me try to
summarize my takeaways and write it up here for reference.

I think the requirements for the logic of the IOCTL right are as follows:

 (1) In the future, if a new FOO access right is introduced, this right should
     implicitly give access to FOO-related IOCTLs on the affected same files,
     *without requiring the LANDLOCK_ACCESS_FS_IOCTL right*.

     Example: If in Landlock version 10, we introduce LANDLOCK_ACCESS_FS_GFX for
     graphics-related functionality, this access right should potentially give
     access to graphics-related ioctl commands.  I'll use the "GFX" example
     below as a stand-in for a generic future access right which should give
     access to a set of IOCTL commands.

and then the ones which are a bit more obvious:

 (2) When stacking additional Landlock layers, the thread's available access can
     only be restricted further (it should not accidentally be able to do more
     than before).

 (3) Landlock usages need to stay compatible across kernel versions.
     The Landlock usages that are in use today need to do the same thing
     in future kernel versions.

I had indeed overlooked requirement (1) and did not realize that my proposal was
going to be at odds with that.



## Some counterexamples for approaches that don't work

So: Counterexample for why my earlier proposal (OR-combination) does not work:

  In my proposal, a GFX-related IOCTL would be permitted when *either one* of
  the ..._GFX or the ..._IOCTL rights are available for the file.  (The READ
  right in the tables above should work the same as the GFX or FOO rights from
  requirement (1), for consistency).

  So a user who today uses

    handled: LANDLOCK_ACCESS_FS_IOCTL
    allowed: (nothing)

  will expect that GFX-related IOCTL operations are forbidden.  (We do not know
  yet whether the "GFX" access right will ever exist, therefore it is covered by
  LANDLOCK_ACCESS_FS_IOCTL.)

  Now we introduce the LANDLOCK_ACCESS_FS_GFX right, and suddenly, GFX-related
  IOCTL commands are checked with a new logic: You *either* need to have the
  LANDLOCK_ACCESS_FS_IOCTL right, *or* the LANDLOCK_ACCESS_FS_GFX right.  So
  when the user again uses

    handled: LANDLOCK_ACCESS_FS_IOCTL
    allowed: (nothing)

  the user would according to the new logic suddenly *have* the
  LANDLOCK_ACCESS_FS_GFX right, and these IOCTL commands would be permitted.

  This is a change of how Landlock behaves compared to the earlier version,
  and that is at odds with rule (3).


The other obvious bitwise combination (AND) does not work either -- that one
would violate requirement (1).



## A new proposal

We have discussed above that one option would be to start distinguishing between
the case where a right is "not handled" and the case where the right is
"handled, but allowed on the file".

This is not very nice, because it would be inconsistent with the semantics which
we had before for all other rights.

After thinking a bit more about it, one way to look at it is that we are using
the "handled" flags to control how the IOCTLs are grouped.  I agree that we have
to control the IOCTL grouping, but I am not sure whether the "handled" flags are
the right place to do that. -- We could just as well pass instructions about the
IOCTL grouping out of band, and I think it might make that logic clearer:

To put forward something concrete, how about this:

* LANDLOCK_ACCESS_FS_IOCTL: This access right controls the invocation of IOCTL
  commands, unless these commands are controlled by another access right.

  In every layer, each IOCTL command is only controlled through one access right.

* LANDLOCK_ACCESS_FS_READ_FILE: This access right controls opening files for
  reading, and additionally the use of the FIONREAD ioctl command.

* We introduce a flag in struct landlock_ruleset_attr which controls whether the
  graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
  access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.

  (This could potentially also be put in the "flags" argument to
  landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
  think, as it influences the interpretation of the logic.  But I'm open to
  suggestions.)


Example: Without the flag, the IOCTL groups will be:

  These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
  LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
  LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands

but when users set the flag, the IOCTL groups will be:

  These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
  LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
  LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
  LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands


Implementation-wise, I think it would actually look very similar to what would
be needed for your proposal of having a new special meaning for "handled".  It
would have the slight advantage that the new flag is actually only needed at the
time when we introduce a new way of grouping the IOCTL commands, so we would
only burden users with the additional complexity when it's actually required.

One implementation approach that I find reasonable to think about is to create
"synthetic" access rights when rulesets are enabled.  That is, we introduce
LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
private to the kernel.

* *At ruleset enablement time*, we populate the bit for this access right either
  from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
  same access_mask_t, depending on the IOCTL grouping which the ruleset is
  configured with.

* *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
  for the GFX-related IOCTL commands.

I'm in favor of using the synthetic access rights, because I find it clearer to
understand that the effective access rights for a file from different layers are
just combined with a bitwise AND, and will give the right results.  We could
probably also make these path walk helpers aware of the special cases and only
have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
these helpers even further.


Sorry for the long mail, I hope that the examples clarify it a bit. :)

In summary, it seems conceptually cleaner to me to control every IOCTL command
with only one access right, and let users control which one that should be with
a separate flag, so that "handled" keeps its original semantics.  It would also
have the upside that we can delay that implementation until the time where we
actually introduce new IOCTL-aware access rights on top of the current patch st.

I'd be interested to hear your thoughts on it.
—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote:
> Hello!
> 
> On Mon, Sep 11, 2023 at 05:25:31PM +0200, Mickaël Salaün wrote:
> > On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> > > Thank you for making the algorithm that explicit -- that helps to trace down the
> > > differences.  I can follow the logic now, but I still don't understand what your
> > > underlying rationale for that is?
> > > 
> > > I believe that fundamentally, a core difference is:
> > > 
> > > For an access right R and a file F, for these two cases:
> > > 
> > >  (a) the access right R is unhandled  (nothing gets restricted)
> > >  (b) the access right R is handled, but R is granted for F in a rule.
> > > 
> > > I believe that accesses in case (a) and (b) to the file F should have the same
> > > results.
> > > 
> > > This is at least how the existing Landlock implementation works, as far as I can
> > > tell.
> > > 
> > > ("Refer" is an exceptional case, but we have documented that it was always
> > > "implicitly handled" in ABI V1, which makes it consistent again.)
> > > 
> > > 
> > > When I expand your code above to a boolean table, I end up with the following
> > > decisions, depending on whether IOCTL and READ are handled or not, and whether
> > > they are explicitly permitted for the file through a rule:
> > > 
> > > 
> > > Mickaël's        IOCTL      IOCTL      IOCTL
> > > suggestion       handled,   handled,   unhandled
> > > 2023-09-04       file       file not
> > >                  permitted  permitted
> > > --------------------------------------------------
> > > READ handled,
> > > file permitted   allow      allow      allow
> > > 
> > > READ handled,
> > > f not permitted  deny       deny       allow
> > > 
> > > READ unhandled   allow      deny       allow
> > > 
> > > 
> > > In patch set V3, this is different: Because I think that cases (a) and (b) from
> > > above should always behave the same, the first and third column and row must be
> > > symmetric and have the same entries.  So, in patch set V3, it is sufficient if
> > > *one of* the two rights IOCTL and READ_FILE are present, in order to use the
> > > FIONREAD IOCTL:
> > > 
> > > 
> > > Günther's        IOCTL      IOCTL      IOCTL
> > > patch set V3     handled,   handled,   unhandled
> > > 2023-08-14       file       file not
> > >                  permitted  permitted
> > > --------------------------------------------------
> > > READ handled,
> > > file permitted   allow      allow      allow
> > > 
> > > READ handled,
> > > f not permitted  allow      deny       allow
> > > 
> > > READ unhandled   allow      allow      allow
> > 
> > A first difference is about (READ unhandled) AND (IOCTL handled +
> > file not permitted). It will not be possible to follow the same logic
> > with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA
> > that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it
> > consistent.
> > 
> > A second difference is about (READ handled + f not permitted) AND
> > (IOCTL handled + file permitted). The reasoning was to avoid giving too
> > much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access
> > rights are implemented. This looks quite similar to the CAP_SYS_ADMIN
> > right that can basically do anything, and new capabilites are mainly a
> > subset of this one. My proposal was to incrementally downgrade the power
> > given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the
> > I was thinking that, if we make a requirement to have the "new correct"
> > access right, the application update might drop the IOCTL access right.
> > I now think this reasoning is flawed.
> > 
> > Indeed, this comparaison doesn't fit well because IOCTLs are not a
> > superset of all access rights, and because nothing can force developers
> > that already gave access to all IOCTLs for a file to not just add
> > another access right (instead of swapping them).
> > 
> > Instead, I think user space libraries should manage this kind of access
> > right swapping when possible and have a fallback mechanism relying on
> > the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they
> > may be updated before the (stable system) kernel, and this would be
> > easier for developers to manage.
> > 
> > In a nutshell, it is about giving control for an action (e.g. FIONREAD)
> > to either a unique access right or to a set of access rights. At first,
> > I would have preferred to have a unique access right to control an
> > action, because it is simpler (e.g. for audit/debug). On the other hand,
> > we need to handle access rights that allow the same action (e.g. file
> > read OR write for FIOQSIZE). I now think your approach (i.e. set of
> > access rights to control an action) could make more sense. Another good
> > point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which
> > could in fact be difficult to understand for users. Nested Landlock
> > domains should also be easier to manage with this logic.

Hmm, in fact I think my initial reasoning was better. I still agree with
my suggestion from 2023-09-04, and I think you're now proposing the
same. Handling access right in an exclusive way (i.e. a specific IOCTL
command is always handle by only one access rigth, but this one may
depend on the handle access rights) is indeed better because it will
force developers to not use the generic IOCTL access right instead of
e.g. the GFX one once they want to use it. In practice, it can reduce
the set of IOCTL commands allowed for LANDLOCK_ACCESS_FS_IOCTL-only file
hierarchies if users only allow the new LANDLOCK_ACCESS_FS_GFX to the
appropriate (e.g. /dev/dri) files.

To summarize, the reasoning is as follow: when READ is handled, the
related IOCTL commands (e.g. FIONREAD) are delegated to the READ access
right (both for the ruleset and the related rules), and the IOCTL access
right doesn't handle at all FIONREAD. If READ is not handled, then the
IOCTL access right handles FIONREAD as well. The same logic applies to
any current or future access rights, like LANDLOCK_ACCESS_FS_GFX.

> 
> After we discussed this difficult topic briefly off-list, let me try to
> summarize my takeaways and write it up here for reference.
> 
> I think the requirements for the logic of the IOCTL right are as follows:
> 
>  (1) In the future, if a new FOO access right is introduced, this right should
>      implicitly give access to FOO-related IOCTLs on the affected same files,
>      *without requiring the LANDLOCK_ACCESS_FS_IOCTL right*.
> 
>      Example: If in Landlock version 10, we introduce LANDLOCK_ACCESS_FS_GFX for
>      graphics-related functionality, this access right should potentially give
>      access to graphics-related ioctl commands.  I'll use the "GFX" example
>      below as a stand-in for a generic future access right which should give
>      access to a set of IOCTL commands.
> 
> and then the ones which are a bit more obvious:
> 
>  (2) When stacking additional Landlock layers, the thread's available access can
>      only be restricted further (it should not accidentally be able to do more
>      than before).
> 
>  (3) Landlock usages need to stay compatible across kernel versions.
>      The Landlock usages that are in use today need to do the same thing
>      in future kernel versions.
> 
> I had indeed overlooked requirement (1) and did not realize that my proposal was
> going to be at odds with that.
> 
> 
> 
> ## Some counterexamples for approaches that don't work
> 
> So: Counterexample for why my earlier proposal (OR-combination) does not work:
> 
>   In my proposal, a GFX-related IOCTL would be permitted when *either one* of
>   the ..._GFX or the ..._IOCTL rights are available for the file.  (The READ
>   right in the tables above should work the same as the GFX or FOO rights from
>   requirement (1), for consistency).
> 
>   So a user who today uses
> 
>     handled: LANDLOCK_ACCESS_FS_IOCTL
>     allowed: (nothing)
> 
>   will expect that GFX-related IOCTL operations are forbidden.  (We do not know
>   yet whether the "GFX" access right will ever exist, therefore it is covered by
>   LANDLOCK_ACCESS_FS_IOCTL.)
> 
>   Now we introduce the LANDLOCK_ACCESS_FS_GFX right, and suddenly, GFX-related
>   IOCTL commands are checked with a new logic: You *either* need to have the
>   LANDLOCK_ACCESS_FS_IOCTL right, *or* the LANDLOCK_ACCESS_FS_GFX right.  So
>   when the user again uses
> 
>     handled: LANDLOCK_ACCESS_FS_IOCTL
>     allowed: (nothing)
> 
>   the user would according to the new logic suddenly *have* the
>   LANDLOCK_ACCESS_FS_GFX right, and these IOCTL commands would be permitted.
> 
>   This is a change of how Landlock behaves compared to the earlier version,
>   and that is at odds with rule (3).
> 
> 
> The other obvious bitwise combination (AND) does not work either -- that one
> would violate requirement (1).
> 

Good summary, thanks!

> 
> ## A new proposal
> 
> We have discussed above that one option would be to start distinguishing between
> the case where a right is "not handled" and the case where the right is
> "handled, but allowed on the file".
> 
> This is not very nice, because it would be inconsistent with the semantics which
> we had before for all other rights.
> 
> After thinking a bit more about it, one way to look at it is that we are using
> the "handled" flags to control how the IOCTLs are grouped.  I agree that we have
> to control the IOCTL grouping, but I am not sure whether the "handled" flags are
> the right place to do that. -- We could just as well pass instructions about the
> IOCTL grouping out of band, and I think it might make that logic clearer:
> 
> To put forward something concrete, how about this:
> 
> * LANDLOCK_ACCESS_FS_IOCTL: This access right controls the invocation of IOCTL
>   commands, unless these commands are controlled by another access right.
> 
>   In every layer, each IOCTL command is only controlled through one access right.

Yes, I agree with that, see the reasoning about handling access right in
an exclusive way above.

> 
> * LANDLOCK_ACCESS_FS_READ_FILE: This access right controls opening files for
>   reading, and additionally the use of the FIONREAD ioctl command.

Yes

> 
> * We introduce a flag in struct landlock_ruleset_attr which controls whether the
>   graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
>   access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.
> 
>   (This could potentially also be put in the "flags" argument to
>   landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
>   think, as it influences the interpretation of the logic.  But I'm open to
>   suggestions.)
> 

What would be the difference with creating a
LANDLOCK_ACCESS_FS_GFX_IOCTL access right?

The main issue with this approach is that it complexifies the usage of
Landlock, and users would need to tweak more knobs to configure a
ruleset.

What about keeping my proposal (mainly the IOCTL handling and delegation
logic) for the user interface, and translate that for kernel internals
to your proposal? See the below example.


> 
> Example: Without the flag, the IOCTL groups will be:
> 
>   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
>   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
>   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> 
> but when users set the flag, the IOCTL groups will be:
> 
>   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
>   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
>   LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
>   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> 

Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag
would not allow GFX-related IOCTL commands? Thit would be inconsistent
with the way LANDLOCK_ACCESS_FS_READ_FILE is handled.

Would this flag works with non-GFX access rights as well? Would there be
potentially one new flag per new access right?

> 
> Implementation-wise, I think it would actually look very similar to what would
> be needed for your proposal of having a new special meaning for "handled".  It
> would have the slight advantage that the new flag is actually only needed at the
> time when we introduce a new way of grouping the IOCTL commands, so we would
> only burden users with the additional complexity when it's actually required.

Indeed, and burdening users with more flags would increase the cost of
(properly) using Landlock.

I'm definitely in favor to make the Landlock interface as simple as
possible, taking into account the inherent compatibilty complexity, and
pushing most of this complexity handling to user space libraries, and if
it not possible, pushing the rest of the complexity into the kernel.

> 
> One implementation approach that I find reasonable to think about is to create
> "synthetic" access rights when rulesets are enabled.  That is, we introduce
> LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
> private to the kernel.
> 
> * *At ruleset enablement time*, we populate the bit for this access right either
>   from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
>   same access_mask_t, depending on the IOCTL grouping which the ruleset is
>   configured with.
> 
> * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
>   for the GFX-related IOCTL commands.
> 
> I'm in favor of using the synthetic access rights, because I find it clearer to
> understand that the effective access rights for a file from different layers are
> just combined with a bitwise AND, and will give the right results.  We could
> probably also make these path walk helpers aware of the special cases and only
> have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
> these helpers even further.

I like this synthetic access right approach, but what worries me is that
it will potentially double the number of access rights. This is not an
issue for the handled access right (i.e. per ruleset layer), but we
should avoid that for allowed accesses (i.e. rules). Indeed, the
layer_masks[] size is proportional to the number of potential allowed
access rights, and increasing this array could increase the kernel stack
size (see is_access_to_paths_allowed).  It would not be an issue for now
though, we have a lot of room, it is just something to keep in mind.

Because of the way we need to compare file hierarchies (cf. FS_REFER),
it seems to be safer to only rely on (synthetic) access rights. So I
think it is the right approach.

> 
> 
> Sorry for the long mail, I hope that the examples clarify it a bit. :)
> 
> In summary, it seems conceptually cleaner to me to control every IOCTL command
> with only one access right, and let users control which one that should be with
> a separate flag, so that "handled" keeps its original semantics.  It would also
> have the upside that we can delay that implementation until the time where we
> actually introduce new IOCTL-aware access rights on top of the current patch st.

I don't see how we'll not get an inconsistent logic: a first one with
old/current access rights, and another one for future access rights
(e.g. GFX).

> 
> I'd be interested to hear your thoughts on it.

Thanks for this detailed explanation, that is useful.

I'm in favor of the synthetic access right, but I'd like to not add
optional flags to the user API.  What do you think about the kernel
doing the translation to the synthetic access rights?

To make the reasoning easier for the kernel implementation, following
the synthetic access rights idea, we can create these groups:

* IOCTL_CMD_G1: FIOQSIZE
* IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ
* IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE
* IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64
  | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE

Existing (and future) access rights would automatically get the related
IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled:
* LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4
* LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3
* LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1

This works with the ruleset handled access rights and the related rules
allowed accesses by simply ORing the access rights.

We should also keep in mind that some IOCTL commands may only be related
to some specific file types or filesystems, either now or in the future
(see the GFX example).

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

On Fri, Oct 20, 2023 at 04:57:39PM +0200, Mickaël Salaün wrote:
> On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote:
> > * We introduce a flag in struct landlock_ruleset_attr which controls whether the
> >   graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
> >   access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.
> > 
> >   (This could potentially also be put in the "flags" argument to
> >   landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
> >   think, as it influences the interpretation of the logic.  But I'm open to
> >   suggestions.)
> > 
> 
> What would be the difference with creating a
> LANDLOCK_ACCESS_FS_GFX_IOCTL access right?
> 
> The main issue with this approach is that it complexifies the usage of
> Landlock, and users would need to tweak more knobs to configure a
> ruleset.
> 
> What about keeping my proposal (mainly the IOCTL handling and delegation
> logic) for the user interface, and translate that for kernel internals
> to your proposal? See the below example.

Yes!

I have pondered this for about a day now, and tried to break the example in
various ways, but I believe you are right with this -- I think we can actually
use the "handled" flags to control the IOCTL grouping, and then translate all of
it quickly to synthetic access rights for the internal logic.  When doing the
translation only once during ruleset enablement time, we can keep using the
existing logic for the synthetic rights and it'll obviously work correctly when
layers are stacked.  (I paraphrase it in more detail at the end, to make sure we
are on the same page. -- But I think we are.)


> > Example: Without the flag, the IOCTL groups will be:
> > 
> >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > 
> > but when users set the flag, the IOCTL groups will be:
> > 
> >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> >   LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
> >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > 
> 
> Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag
> would not allow GFX-related IOCTL commands? Thit would be inconsistent
> with the way LANDLOCK_ACCESS_FS_READ_FILE is handled.

Yes, that is how I had imagined that.  It's true that it's slightly inconsistent
in usage, and you are right that it creates some new concepts in the API which
are maybe avoidable.  Let's try it the way you proposed and control it with the
"handled" flags.


> Would this flag works with non-GFX access rights as well? Would there be
> potentially one new flag per new access right?
> 
> > 
> > Implementation-wise, I think it would actually look very similar to what would
> > be needed for your proposal of having a new special meaning for "handled".  It
> > would have the slight advantage that the new flag is actually only needed at the
> > time when we introduce a new way of grouping the IOCTL commands, so we would
> > only burden users with the additional complexity when it's actually required.
> 
> Indeed, and burdening users with more flags would increase the cost of
> (properly) using Landlock.
> 
> I'm definitely in favor to make the Landlock interface as simple as
> possible, taking into account the inherent compatibilty complexity, and
> pushing most of this complexity handling to user space libraries, and if
> it not possible, pushing the rest of the complexity into the kernel.

Ack, sounds good.


> > One implementation approach that I find reasonable to think about is to create
> > "synthetic" access rights when rulesets are enabled.  That is, we introduce
> > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
> > private to the kernel.
> > 
> > * *At ruleset enablement time*, we populate the bit for this access right either
> >   from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
> >   same access_mask_t, depending on the IOCTL grouping which the ruleset is
> >   configured with.
> > 
> > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
> >   for the GFX-related IOCTL commands.
> > 
> > I'm in favor of using the synthetic access rights, because I find it clearer to
> > understand that the effective access rights for a file from different layers are
> > just combined with a bitwise AND, and will give the right results.  We could
> > probably also make these path walk helpers aware of the special cases and only
> > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
> > these helpers even further.
> 
> I like this synthetic access right approach, but what worries me is that
> it will potentially double the number of access rights. This is not an
> issue for the handled access right (i.e. per ruleset layer), but we
> should avoid that for allowed accesses (i.e. rules). Indeed, the
> layer_masks[] size is proportional to the number of potential allowed
> access rights, and increasing this array could increase the kernel stack
> size (see is_access_to_paths_allowed).  It would not be an issue for now
> though, we have a lot of room, it is just something to keep in mind.

Yes, acknowledged.

FWIW, LANDLOCK_ACCESS_FS_IOCTL is already 1 << 15, so adding the synthetic
rights will indeed make access_mask_t go up to 32 bit.  (This was already done
in the patch for the metadata access, but that one was not merged yet.)  I also
feel that the stack usage is the case where this is most likely to be an issue.


> Because of the way we need to compare file hierarchies (cf. FS_REFER),
> it seems to be safer to only rely on (synthetic) access rights. So I
> think it is the right approach.
> 
> > 
> > 
> > Sorry for the long mail, I hope that the examples clarify it a bit. :)
> > 
> > In summary, it seems conceptually cleaner to me to control every IOCTL command
> > with only one access right, and let users control which one that should be with
> > a separate flag, so that "handled" keeps its original semantics.  It would also
> > have the upside that we can delay that implementation until the time where we
> > actually introduce new IOCTL-aware access rights on top of the current patch st.
> 
> I don't see how we'll not get an inconsistent logic: a first one with
> old/current access rights, and another one for future access rights
> (e.g. GFX).
> 
> > 
> > I'd be interested to hear your thoughts on it.
> 
> Thanks for this detailed explanation, that is useful.
> 
> I'm in favor of the synthetic access right, but I'd like to not add
> optional flags to the user API.  What do you think about the kernel
> doing the translation to the synthetic access rights?
> 
> To make the reasoning easier for the kernel implementation, following
> the synthetic access rights idea, we can create these groups:
> 
> * IOCTL_CMD_G1: FIOQSIZE
> * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ
> * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE
> * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64
>   | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE
> 
> Existing (and future) access rights would automatically get the related
> IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled:
> * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4
> * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3
> * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1
> 
> This works with the ruleset handled access rights and the related rules
> allowed accesses by simply ORing the access rights.
> 
> We should also keep in mind that some IOCTL commands may only be related
> to some specific file types or filesystems, either now or in the future
> (see the GFX example).

I am coming around to your approach with using "handled" bits to determine the
grouping.  Let me paraphrase some key concepts to make sure we are on the same
page:

* The IOCTL groups are modeled as synthetic access rights, IOCTL_CMD_G1...G4 in
  your example.  Each IOCTL command maps to exactly one of these groups.

  Because the presence of these groups is an implementation detail in the
  kernel, we can adapt it later and make it more fine-grained if needed.

* We use "handled" bits like LANDLOCK_ACCESS_FS_WRITE_FILE to determine the
  synthetic access rights.

  We can populate the synthetic IOCTL_CMD_G1...G4 groups depending on how the
  "handled" bits are populated.

  In my understanding, the logic could roughly be this:

  static access_mask_t expand_ioctl(access_mask_t handled, access_mask_t am,
                                    access_mask_t src, access_mask_t dst)
  {
    if (handled & src) {
      /* If "src" access right is handled, populate "dst" from "src". */
      return am | ((am & src) ? dst : 0);
    } else {
      /* Otherwise, populate "dst" flag from "ioctl" flag. */
      return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0);
    }
  }

  static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am)
  {
    am = expand_ioctl(handled, am,
                      LANDLOCK_ACCESS_FS_WRITE_FILE,
		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4);
    am = expand_ioctl(handled, am,
                      LANDLOCK_ACCESS_FS_READ_FILE,
		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3);
    am = expand_ioctl(handled, am,
                      LANDLOCK_ACCESS_FS_READ_DIR,
		      IOCTL_CMD_G1);
    return am;
  }

  and then during the installing of a ruleset, we'd call
  expand_all_ioctl(handled, access) for each specified file access, and
  expand_all_ioctl(handled, handled) for the handled access rights,
  to populate the synthetic IOCTL_CMD_G* access rights.

  In expand_ioctl() above, if "src" is *not* handled, we populate the associated
  synthetic access rights "dst" from the value in LANDLOCK_ACCESS_FS_IOCTL.
  With that, when enabling a ruleset, we map everything to the most specific
  grouping which is available, and later on, the LSM hook can just ignore that
  different grouping configurations are possible.

* In the ioctl LSM hook, each possible cmd is controlled by exactly one access
  right.  The ones that you have listed are all controlled by one of the
  IOCTL_CMD_G1...G4 access rights, and all others by LANDLOCK_ACCESS_FS_IOCTL.

I was previously concerned that the usage of "handled" to control the grouping
would be at odds with the layer composition logic, but with this logic, we are
now mapping these to the synthetic access rights at enablement time, and all the
ruleset composition logic can stay working as it is (at least until we run out
of bits in access_mask_t).

I've also been concerned before that we would break compatibility across
versions, but this also seems less likely now that we've discussed this in all
this detail %-)

I suspect that the normal upgrade path from one Landlock version to the next
will be for most users to always use the full set of "handled" flags that their
library knows about.  When we add the hypothetical "GFX" flag to that set, this
will change the IOCTL grouping a bit, so that files which were previously listed
as having the LANDLOCK_ACCESS_FS_IOCTL right, might now not be enabled for GFX
ioctls.  But that is (A) probably correct anyway in most cases, and (B) users
upgrading from one Landlock ABI version to the next have a chance to read their
library changelog as part of that upgrade.

I think this is a reasonable approach.  If you agree, I'm willing to give it a
shot and adapt the patch set to implement that.

—Günther

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Thu, Oct 26, 2023 at 12:07:28AM +0200, Günther Noack wrote:
> On Fri, Oct 20, 2023 at 04:57:39PM +0200, Mickaël Salaün wrote:
> > On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote:
> > > * We introduce a flag in struct landlock_ruleset_attr which controls whether the
> > >   graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
> > >   access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.
> > > 
> > >   (This could potentially also be put in the "flags" argument to
> > >   landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
> > >   think, as it influences the interpretation of the logic.  But I'm open to
> > >   suggestions.)
> > > 
> > 
> > What would be the difference with creating a
> > LANDLOCK_ACCESS_FS_GFX_IOCTL access right?
> > 
> > The main issue with this approach is that it complexifies the usage of
> > Landlock, and users would need to tweak more knobs to configure a
> > ruleset.
> > 
> > What about keeping my proposal (mainly the IOCTL handling and delegation
> > logic) for the user interface, and translate that for kernel internals
> > to your proposal? See the below example.
> 
> Yes!
> 
> I have pondered this for about a day now, and tried to break the example in
> various ways, but I believe you are right with this -- I think we can actually
> use the "handled" flags to control the IOCTL grouping, and then translate all of
> it quickly to synthetic access rights for the internal logic.  When doing the
> translation only once during ruleset enablement time, we can keep using the
> existing logic for the synthetic rights and it'll obviously work correctly when
> layers are stacked.  (I paraphrase it in more detail at the end, to make sure we
> are on the same page. -- But I think we are.)
> 
> 
> > > Example: Without the flag, the IOCTL groups will be:
> > > 
> > >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> > >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> > >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > > 
> > > but when users set the flag, the IOCTL groups will be:
> > > 
> > >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> > >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> > >   LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
> > >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > > 
> > 
> > Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag
> > would not allow GFX-related IOCTL commands? Thit would be inconsistent
> > with the way LANDLOCK_ACCESS_FS_READ_FILE is handled.
> 
> Yes, that is how I had imagined that.  It's true that it's slightly inconsistent
> in usage, and you are right that it creates some new concepts in the API which
> are maybe avoidable.  Let's try it the way you proposed and control it with the
> "handled" flags.
> 
> 
> > Would this flag works with non-GFX access rights as well? Would there be
> > potentially one new flag per new access right?
> > 
> > > 
> > > Implementation-wise, I think it would actually look very similar to what would
> > > be needed for your proposal of having a new special meaning for "handled".  It
> > > would have the slight advantage that the new flag is actually only needed at the
> > > time when we introduce a new way of grouping the IOCTL commands, so we would
> > > only burden users with the additional complexity when it's actually required.
> > 
> > Indeed, and burdening users with more flags would increase the cost of
> > (properly) using Landlock.
> > 
> > I'm definitely in favor to make the Landlock interface as simple as
> > possible, taking into account the inherent compatibilty complexity, and
> > pushing most of this complexity handling to user space libraries, and if
> > it not possible, pushing the rest of the complexity into the kernel.
> 
> Ack, sounds good.
> 
> 
> > > One implementation approach that I find reasonable to think about is to create
> > > "synthetic" access rights when rulesets are enabled.  That is, we introduce
> > > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
> > > private to the kernel.
> > > 
> > > * *At ruleset enablement time*, we populate the bit for this access right either
> > >   from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
> > >   same access_mask_t, depending on the IOCTL grouping which the ruleset is
> > >   configured with.
> > > 
> > > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
> > >   for the GFX-related IOCTL commands.
> > > 
> > > I'm in favor of using the synthetic access rights, because I find it clearer to
> > > understand that the effective access rights for a file from different layers are
> > > just combined with a bitwise AND, and will give the right results.  We could
> > > probably also make these path walk helpers aware of the special cases and only
> > > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
> > > these helpers even further.
> > 
> > I like this synthetic access right approach, but what worries me is that
> > it will potentially double the number of access rights. This is not an
> > issue for the handled access right (i.e. per ruleset layer), but we
> > should avoid that for allowed accesses (i.e. rules). Indeed, the
> > layer_masks[] size is proportional to the number of potential allowed
> > access rights, and increasing this array could increase the kernel stack
> > size (see is_access_to_paths_allowed).  It would not be an issue for now
> > though, we have a lot of room, it is just something to keep in mind.
> 
> Yes, acknowledged.
> 
> FWIW, LANDLOCK_ACCESS_FS_IOCTL is already 1 << 15, so adding the synthetic
> rights will indeed make access_mask_t go up to 32 bit.  (This was already done
> in the patch for the metadata access, but that one was not merged yet.)  I also
> feel that the stack usage is the case where this is most likely to be an issue.
> 
> 
> > Because of the way we need to compare file hierarchies (cf. FS_REFER),
> > it seems to be safer to only rely on (synthetic) access rights. So I
> > think it is the right approach.
> > 
> > > 
> > > 
> > > Sorry for the long mail, I hope that the examples clarify it a bit. :)
> > > 
> > > In summary, it seems conceptually cleaner to me to control every IOCTL command
> > > with only one access right, and let users control which one that should be with
> > > a separate flag, so that "handled" keeps its original semantics.  It would also
> > > have the upside that we can delay that implementation until the time where we
> > > actually introduce new IOCTL-aware access rights on top of the current patch st.
> > 
> > I don't see how we'll not get an inconsistent logic: a first one with
> > old/current access rights, and another one for future access rights
> > (e.g. GFX).
> > 
> > > 
> > > I'd be interested to hear your thoughts on it.
> > 
> > Thanks for this detailed explanation, that is useful.
> > 
> > I'm in favor of the synthetic access right, but I'd like to not add
> > optional flags to the user API.  What do you think about the kernel
> > doing the translation to the synthetic access rights?
> > 
> > To make the reasoning easier for the kernel implementation, following
> > the synthetic access rights idea, we can create these groups:
> > 
> > * IOCTL_CMD_G1: FIOQSIZE
> > * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ
> > * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE
> > * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64
> >   | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE
> > 
> > Existing (and future) access rights would automatically get the related
> > IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled:
> > * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4
> > * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3
> > * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1
> > 
> > This works with the ruleset handled access rights and the related rules
> > allowed accesses by simply ORing the access rights.
> > 
> > We should also keep in mind that some IOCTL commands may only be related
> > to some specific file types or filesystems, either now or in the future
> > (see the GFX example).
> 
> I am coming around to your approach with using "handled" bits to determine the
> grouping.  Let me paraphrase some key concepts to make sure we are on the same
> page:
> 
> * The IOCTL groups are modeled as synthetic access rights, IOCTL_CMD_G1...G4 in
>   your example.  Each IOCTL command maps to exactly one of these groups.
> 
>   Because the presence of these groups is an implementation detail in the
>   kernel, we can adapt it later and make it more fine-grained if needed.
> 
> * We use "handled" bits like LANDLOCK_ACCESS_FS_WRITE_FILE to determine the
>   synthetic access rights.
> 
>   We can populate the synthetic IOCTL_CMD_G1...G4 groups depending on how the
>   "handled" bits are populated.
> 
>   In my understanding, the logic could roughly be this:
> 
>   static access_mask_t expand_ioctl(access_mask_t handled, access_mask_t am,
>                                     access_mask_t src, access_mask_t dst)
>   {

The third column "IOCTL unhandled" is not reflected here. What about
this patch?

if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) {
  return am | dst;
}

>     if (handled & src) {
>       /* If "src" access right is handled, populate "dst" from "src". */
>       return am | ((am & src) ? dst : 0);
>     } else {
>       /* Otherwise, populate "dst" flag from "ioctl" flag. */
>       return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0);
>     }
>   }
> 
>   static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am)
>   {

Instead of reapeating "am | " in expand_ioctl() and assigning am several
times in expand_all_ioctl(), you could simply do something like that:

return am |
	expand_ioctl(handled, am, ...) |
	expand_ioctl(handled, am, ...) |
	expand_ioctl(handled, am, ...);

>     am = expand_ioctl(handled, am,
>                       LANDLOCK_ACCESS_FS_WRITE_FILE,
> 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4);
>     am = expand_ioctl(handled, am,
>                       LANDLOCK_ACCESS_FS_READ_FILE,
> 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3);
>     am = expand_ioctl(handled, am,
>                       LANDLOCK_ACCESS_FS_READ_DIR,
> 		      IOCTL_CMD_G1);
>     return am;
>   }
> 
>   and then during the installing of a ruleset, we'd call
>   expand_all_ioctl(handled, access) for each specified file access, and
>   expand_all_ioctl(handled, handled) for the handled access rights,
>   to populate the synthetic IOCTL_CMD_G* access rights.

We can do these transformations directly in the new
landlock_add_fs_access_mask() and landlock_append_fs_rule().

Please base the next series on
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
This branch might be rebased from time to time, but only minor changes
will get there.

> 
>   In expand_ioctl() above, if "src" is *not* handled, we populate the associated
>   synthetic access rights "dst" from the value in LANDLOCK_ACCESS_FS_IOCTL.
>   With that, when enabling a ruleset, we map everything to the most specific
>   grouping which is available, and later on, the LSM hook can just ignore that
>   different grouping configurations are possible.
> 
> * In the ioctl LSM hook, each possible cmd is controlled by exactly one access
>   right.  The ones that you have listed are all controlled by one of the
>   IOCTL_CMD_G1...G4 access rights, and all others by LANDLOCK_ACCESS_FS_IOCTL.
> 
> I was previously concerned that the usage of "handled" to control the grouping
> would be at odds with the layer composition logic, but with this logic, we are
> now mapping these to the synthetic access rights at enablement time, and all the
> ruleset composition logic can stay working as it is (at least until we run out
> of bits in access_mask_t).
> 
> I've also been concerned before that we would break compatibility across
> versions, but this also seems less likely now that we've discussed this in all
> this detail %-)
> 
> I suspect that the normal upgrade path from one Landlock version to the next
> will be for most users to always use the full set of "handled" flags that their
> library knows about.  When we add the hypothetical "GFX" flag to that set, this
> will change the IOCTL grouping a bit, so that files which were previously listed
> as having the LANDLOCK_ACCESS_FS_IOCTL right, might now not be enabled for GFX
> ioctls.  But that is (A) probably correct anyway in most cases, and (B) users
> upgrading from one Landlock ABI version to the next have a chance to read their
> library changelog as part of that upgrade.

Yes, explicitly adding a new flag to a function argument should indeed
lead to read the related documentation, and hopefully test the code in
an up-to-date sandbox environment!

This strategy should help avoid long-term use of the generic
LANDLOCK_ACCESS_FS_IOCTL but converge to new dedicated access rights
instead.

> 
> I think this is a reasonable approach.  If you agree, I'm willing to give it a
> shot and adapt the patch set to implement that.

This looks great!

> 
> —Günther

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Günther Noack]

Hello Mickaël!

Thanks for the review!

On Thu, Oct 26, 2023 at 04:55:30PM +0200, Mickaël Salaün wrote:
> The third column "IOCTL unhandled" is not reflected here. What about
> this patch?
> 
> if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) {
>   return am | dst;
> }

You are right that this needs special treatment.  The reasoning is the scenario
where a user creates a ruleset where LANDLOCK_ACCESS_FS_READ_FILE is handled,
but LANDLOCK_ACCESS_FS_IOCTL is not.  In that case, when a file is opened for
which we do not have the READ_FILE access right, without your additional check,
the IOCTLs associated with READ_FILE would be forbidden.  But this is also a
Landlock usage that was possible before the introduction of the IOCTL handling,
and so all IOCTLs should work in that case.

> 
> >     if (handled & src) {
> >       /* If "src" access right is handled, populate "dst" from "src". */
> >       return am | ((am & src) ? dst : 0);
> >     } else {
> >       /* Otherwise, populate "dst" flag from "ioctl" flag. */
> >       return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0);
> >     }
> >   }
> > 
> >   static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am)
> >   {
> 
> Instead of reapeating "am | " in expand_ioctl() and assigning am several
> times in expand_all_ioctl(), you could simply do something like that:
> 
> return am |
> 	expand_ioctl(handled, am, ...) |
> 	expand_ioctl(handled, am, ...) |
> 	expand_ioctl(handled, am, ...);

Agreed, this is more elegant.  Will do.


> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_WRITE_FILE,
> > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4);
> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_READ_FILE,
> > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3);
> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_READ_DIR,
> > 		      IOCTL_CMD_G1);
> >     return am;
> >   }
> > 
> >   and then during the installing of a ruleset, we'd call
> >   expand_all_ioctl(handled, access) for each specified file access, and
> >   expand_all_ioctl(handled, handled) for the handled access rights,
> >   to populate the synthetic IOCTL_CMD_G* access rights.
> 
> We can do these transformations directly in the new
> landlock_add_fs_access_mask() and landlock_append_fs_rule().

Working on these changes, the location of these transformations is one of the
last outstanding problems that I don't like yet.

I have added the expansion code to landlock_add_fs_access_mask() and
landlock_append_fs_rule() as you suggested.

This works, but as a result, this (somewhat complicated) expansion logic is now
part of the ruleset.o module, where it seems a bit too FS-specific.  I think
that maybe we can pull this out further, but I'll probably send you a patch set
with the current status before doing that, so that we are on the same page.


> Please base the next series on
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> This branch might be rebased from time to time, but only minor changes
> will get there.

OK, will do.


In summary, I'll send a patch soon.

FYI, some open questions I still have are:

* Logic
  * How will userspace libraries handle best-effort fallback,
    when expanded IOCTL access rights come into play?
    (Still need to think about this more.)
* Internal code layout
  * Move expansion logic out of ruleset.o module into syscalls.o?
  * Find more appropriate names for IOCTL_CMD_G1,...,IOCTL_CMD_G4

but we can discuss these in the context of the next patch set.

—Günther

Re: [PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 02:06:53PM +0100, Günther Noack wrote:
> Hello Mickaël!
> 
> Thanks for the review!
> 
> On Thu, Oct 26, 2023 at 04:55:30PM +0200, Mickaël Salaün wrote:
> > The third column "IOCTL unhandled" is not reflected here. What about
> > this patch?
> > 
> > if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) {
> >   return am | dst;
> > }
> 
> You are right that this needs special treatment.  The reasoning is the scenario
> where a user creates a ruleset where LANDLOCK_ACCESS_FS_READ_FILE is handled,
> but LANDLOCK_ACCESS_FS_IOCTL is not.  In that case, when a file is opened for
> which we do not have the READ_FILE access right, without your additional check,
> the IOCTLs associated with READ_FILE would be forbidden.  But this is also a
> Landlock usage that was possible before the introduction of the IOCTL handling,
> and so all IOCTLs should work in that case.
> 
> > 
> > >     if (handled & src) {
> > >       /* If "src" access right is handled, populate "dst" from "src". */
> > >       return am | ((am & src) ? dst : 0);
> > >     } else {
> > >       /* Otherwise, populate "dst" flag from "ioctl" flag. */
> > >       return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0);
> > >     }
> > >   }
> > > 
> > >   static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am)
> > >   {
> > 
> > Instead of reapeating "am | " in expand_ioctl() and assigning am several
> > times in expand_all_ioctl(), you could simply do something like that:
> > 
> > return am |
> > 	expand_ioctl(handled, am, ...) |
> > 	expand_ioctl(handled, am, ...) |
> > 	expand_ioctl(handled, am, ...);
> 
> Agreed, this is more elegant.  Will do.
> 
> 
> > >     am = expand_ioctl(handled, am,
> > >                       LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4);
> > >     am = expand_ioctl(handled, am,
> > >                       LANDLOCK_ACCESS_FS_READ_FILE,
> > > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3);
> > >     am = expand_ioctl(handled, am,
> > >                       LANDLOCK_ACCESS_FS_READ_DIR,
> > > 		      IOCTL_CMD_G1);
> > >     return am;
> > >   }
> > > 
> > >   and then during the installing of a ruleset, we'd call
> > >   expand_all_ioctl(handled, access) for each specified file access, and
> > >   expand_all_ioctl(handled, handled) for the handled access rights,
> > >   to populate the synthetic IOCTL_CMD_G* access rights.
> > 
> > We can do these transformations directly in the new
> > landlock_add_fs_access_mask() and landlock_append_fs_rule().
> 
> Working on these changes, the location of these transformations is one of the
> last outstanding problems that I don't like yet.
> 
> I have added the expansion code to landlock_add_fs_access_mask() and
> landlock_append_fs_rule() as you suggested.
> 
> This works, but as a result, this (somewhat complicated) expansion logic is now
> part of the ruleset.o module, where it seems a bit too FS-specific.  I think
> that maybe we can pull this out further, but I'll probably send you a patch set
> with the current status before doing that, so that we are on the same page.

I guess we can put the expand functions in fs.c .

But at that point we need an actual patch to discuss such details.

> 
> 
> > Please base the next series on
> > https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> > This branch might be rebased from time to time, but only minor changes
> > will get there.
> 
> OK, will do.
> 
> 
> In summary, I'll send a patch soon.
> 
> FYI, some open questions I still have are:
> 
> * Logic
>   * How will userspace libraries handle best-effort fallback,
>     when expanded IOCTL access rights come into play?
>     (Still need to think about this more.)

If users set the GFX right, the library should fallback to the IOCTL
right if GFX is not supported.

> * Internal code layout
>   * Move expansion logic out of ruleset.o module into syscalls.o?
>   * Find more appropriate names for IOCTL_CMD_G1,...,IOCTL_CMD_G4

Actually, I think these groups should be static const variables defined
in the function that uses them, so the naming would not change much.
Maybe something like ioctl_groupN?

> 
> but we can discuss these in the context of the next patch set.

Definitely

> 
> —Günther