[PATCH v8 0/9] Landlock: IOCTL support

[PATCH v8 0/9] 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 IOCTL access rights to opened file descriptors, as we
already do for LANDLOCK_ACCESS_FS_TRUNCATE.

If LANDLOCK_ACCESS_FS_IOCTL is handled (restricted in the ruleset),
the LANDLOCK_ACCESS_FS_IOCTL access right governs the use of all IOCTL
commands.

We make an exception for the common and known-harmless IOCTL commands
FIOCLEX, FIONCLEX, FIONBIO and FIONREAD.  These IOCTL commands are
always permitted.  Their functionality is already available through
fcntl(2).

If additionally(!), the access rights LANDLOCK_ACCESS_FS_READ_FILE,
LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are
handled, these access rights also unlock some IOCTL commands which are
considered safe for use with files opened in these ways.

As soon as these access rights are handled, the affected IOCTL
commands can not be permitted through LANDLOCK_ACCESS_FS_IOCTL any
more, but only be permitted through the respective more specific
access rights.  A full list of these access rights is listed below in
this cover letter and in the documentation.

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 most IOCTL commands.

  * "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.)

IOCTL groups
~~~~~~~~~~~~

To decide which IOCTL commands should be blanket-permitted we 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 for reference.

We should always allow the following IOCTL commands, which are also
available through fcntl(2) with the F_SETFD and F_SETFL 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

The following command is guarded and enabled by either of
LANDLOCK_ACCESS_FS_WRITE_FILE, LANDLOCK_ACCESS_FS_READ_FILE or
LANDLOCK_ACCESS_FS_READ_DIR (G2), once one of them is handled
(otherwise by LANDLOCK_ACCESS_FS_IOCTL):

 * FIOQSIZE - get the size of the opened file

The following commands are guarded and enabled by either of
LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_FILE (G2),
once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL):

These are 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

The following commands are guarded and enabled by
LANDLOCK_ACCESS_FS_READ_FILE (G3), if it is handled (otherwise by
LANDLOCK_ACCESS_FS_IOCTL):

 * FIONREAD - get the number of bytes available for reading (the
   implementation is defined per file type)
 * FIDEDUPRANGE - manipulating shared physical storage between files.

The following commands are guarded and enabled by
LANDLOCK_ACCESS_FS_WRITE_FILE (G4), if it is handled (otherwise by
LANDLOCK_ACCESS_FS_IOCTL):

 * FICLONE, FICLONERANGE - making files share physical storage between
   multiple files.  These only work on some file systems, by design.
 * 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).

The following commands are also mentioned in fs/ioctl.c, but are not
handled specially and are managed by LANDLOCK_ACCESS_FS_IOCTL together
with all other remaining IOCTL commands:

 * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
   system. Requires CAP_SYS_ADMIN.
 * Accessing file attributes:
   * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
   * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes

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
~~~~~~~

V8:
 * Documentation changes
   * userspace-api/landlock.rst:
     * Add an extra paragraph about how the IOCTL right combines
       when used with other access rights.
     * Explain better the circumstances under which passing of
       file descriptors between different Landlock domains can happen
   * limits.h: Add comment to explain public vs internal FS access rights
   * Add a paragraph in the commit to explain better why the IOCTL
     right works as it does

V7:
 * in “landlock: Add IOCTL access right”:
   * Make IOCTL_GROUPS a #define so that static_assert works even on
     old compilers (bug reported by Intel about PowerPC GCC9 config)
   * Adapt indentation of IOCTL_GROUPS definition
   * Add missing dots in kernel-doc comments.
 * in “landlock: Remove remaining "inline" modifiers in .c files”:
   * explain reasoning in commit message

V6:
 * Implementation:
   * Check that only publicly visible access rights can be used when adding a
     rule (rather than the synthetic ones).  Thanks Mickaël for spotting that!
   * Move all functionality related to IOCTL groups and synthetic access rights
     into the same place at the top of fs.c
   * Move kernel doc to the .c file in one instance
   * Smaller code style issues (upcase IOCTL, vardecl at block start)
   * Remove inline modifier from functions in .c files
 * Tests:
   * use SKIP
   * Rename 'fd' to dir_fd and file_fd where appropriate
   * Remove duplicate "ioctl" mentions from test names
   * Rename "permitted" to "allowed", in ioctl and ftruncate tests
   * Do not add rules if access is 0, in test helper

V5:
 * Implementation:
   * move IOCTL group expansion logic into fs.c (implementation suggested by
     mic)
   * rename IOCTL_CMD_G* constants to LANDLOCK_ACCESS_FS_IOCTL_GROUP*
   * fs.c: create ioctl_groups constant
   * add "const" to some variables
 * Formatting and docstring fixes (including wrong kernel-doc format)
 * samples/landlock: fix ABI version and fallback attribute (mic)
 * Documentation
   * move header documentation changes into the implementation commit
   * spell out how FIFREEZE, FITHAW and attribute-manipulation ioctls from
     fs/ioctl.c are handled
   * change ABI 4 to ABI 5 in some missing places
   
V4:
 * use "synthetic" IOCTL access rights, as previously discussed
 * testing changes
   * use a large fixture-based test, for more exhaustive coverage,
     and replace some of the earlier tests with it
 * rebased on mic-next

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/
V3: https://lore.kernel.org/linux-security-module/20230814172816.3907299-1-gnoack@google.com/
V4: https://lore.kernel.org/linux-security-module/20231103155717.78042-1-gnoack@google.com/
V5: https://lore.kernel.org/linux-security-module/20231117154920.1706371-1-gnoack@google.com/
V6: https://lore.kernel.org/linux-security-module/20231124173026.3257122-1-gnoack@google.com/
V7: https://lore.kernel.org/linux-security-module/20231201143042.3276833-1-gnoack@google.com/

Günther Noack (9):
  landlock: Remove remaining "inline" modifiers in .c files
  selftests/landlock: Rename "permitted" to "allowed" in ftruncate tests
  landlock: Optimize the number of calls to get_access_mask slightly
  landlock: Add IOCTL access right
  selftests/landlock: Test IOCTL support
  selftests/landlock: Test IOCTL with memfds
  selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH)
  samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL
  landlock: Document IOCTL support

 Documentation/userspace-api/landlock.rst     | 119 ++++-
 include/uapi/linux/landlock.h                |  58 +-
 samples/landlock/sandboxer.c                 |  13 +-
 security/landlock/fs.c                       | 202 ++++++-
 security/landlock/fs.h                       |   2 +
 security/landlock/limits.h                   |  11 +-
 security/landlock/ruleset.c                  |   7 +-
 security/landlock/ruleset.h                  |   2 +-
 security/landlock/syscalls.c                 |  19 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 523 ++++++++++++++++++-
 11 files changed, 884 insertions(+), 74 deletions(-)


base-commit: 413e638fb4dfee94933e28fd9a6a8ed294447279
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 1/9] landlock: Remove remaining "inline" modifiers in .c files

[Günther Noack]

For module-internal static functions, compilers are already in a good
position to decide whether to inline them or not.

Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 security/landlock/fs.c      | 26 +++++++++++++-------------
 security/landlock/ruleset.c |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index bc7c126deea2..9ba989ef46a5 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -193,7 +193,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
  *
  * Returns NULL if no rule is found or if @dentry is negative.
  */
-static inline const struct landlock_rule *
+static const struct landlock_rule *
 find_rule(const struct landlock_ruleset *const domain,
 	  const struct dentry *const dentry)
 {
@@ -220,7 +220,7 @@ find_rule(const struct landlock_ruleset *const domain,
  * sockfs, pipefs), but can still be reachable through
  * /proc/<pid>/fd/<file-descriptor>
  */
-static inline bool is_nouser_or_private(const struct dentry *dentry)
+static bool is_nouser_or_private(const struct dentry *dentry)
 {
 	return (dentry->d_sb->s_flags & SB_NOUSER) ||
 	       (d_is_positive(dentry) &&
@@ -264,7 +264,7 @@ static const struct landlock_ruleset *get_current_fs_domain(void)
  *
  * @layer_masks_child2: Optional child masks.
  */
-static inline bool no_more_access(
+static bool no_more_access(
 	const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS],
 	const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS],
 	const bool child1_is_directory,
@@ -316,7 +316,7 @@ static inline bool no_more_access(
  *
  * Returns true if the request is allowed, false otherwise.
  */
-static inline bool
+static bool
 scope_to_request(const access_mask_t access_request,
 		 layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
 {
@@ -335,7 +335,7 @@ scope_to_request(const access_mask_t access_request,
  * Returns true if there is at least one access right different than
  * LANDLOCK_ACCESS_FS_REFER.
  */
-static inline bool
+static bool
 is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
 	  const access_mask_t access_request)
 {
@@ -551,9 +551,9 @@ static bool is_access_to_paths_allowed(
 	return allowed_parent1 && allowed_parent2;
 }
 
-static inline int check_access_path(const struct landlock_ruleset *const domain,
-				    const struct path *const path,
-				    access_mask_t access_request)
+static int check_access_path(const struct landlock_ruleset *const domain,
+			     const struct path *const path,
+			     access_mask_t access_request)
 {
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
 
@@ -565,8 +565,8 @@ static inline int check_access_path(const struct landlock_ruleset *const domain,
 	return -EACCES;
 }
 
-static inline int current_check_access_path(const struct path *const path,
-					    const access_mask_t access_request)
+static int current_check_access_path(const struct path *const path,
+				     const access_mask_t access_request)
 {
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 
@@ -575,7 +575,7 @@ static inline int current_check_access_path(const struct path *const path,
 	return check_access_path(dom, path, access_request);
 }
 
-static inline access_mask_t get_mode_access(const umode_t mode)
+static access_mask_t get_mode_access(const umode_t mode)
 {
 	switch (mode & S_IFMT) {
 	case S_IFLNK:
@@ -600,7 +600,7 @@ static inline access_mask_t get_mode_access(const umode_t mode)
 	}
 }
 
-static inline access_mask_t maybe_remove(const struct dentry *const dentry)
+static access_mask_t maybe_remove(const struct dentry *const dentry)
 {
 	if (d_is_negative(dentry))
 		return 0;
@@ -1086,7 +1086,7 @@ static int hook_path_truncate(const struct path *const path)
  * Returns the access rights that are required for opening the given file,
  * depending on the file type and open mode.
  */
-static inline access_mask_t
+static access_mask_t
 get_required_file_open_access(const struct file *const file)
 {
 	access_mask_t access = 0;
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index ffedc99f2b68..789c81b26a50 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -305,7 +305,7 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
 	return insert_rule(ruleset, id, &layers, ARRAY_SIZE(layers));
 }
 
-static inline void get_hierarchy(struct landlock_hierarchy *const hierarchy)
+static void get_hierarchy(struct landlock_hierarchy *const hierarchy)
 {
 	if (hierarchy)
 		refcount_inc(&hierarchy->usage);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 2/9] selftests/landlock: Rename "permitted" to "allowed" in ftruncate tests

[Günther Noack]

Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 tools/testing/selftests/landlock/fs_test.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index a1d17ab527ae..50818904397c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3688,7 +3688,7 @@ FIXTURE_TEARDOWN(ftruncate)
 FIXTURE_VARIANT(ftruncate)
 {
 	const __u64 handled;
-	const __u64 permitted;
+	const __u64 allowed;
 	const int expected_open_result;
 	const int expected_ftruncate_result;
 };
@@ -3697,7 +3697,7 @@ FIXTURE_VARIANT(ftruncate)
 FIXTURE_VARIANT_ADD(ftruncate, w_w) {
 	/* clang-format on */
 	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE,
-	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE,
 	.expected_open_result = 0,
 	.expected_ftruncate_result = 0,
 };
@@ -3706,7 +3706,7 @@ FIXTURE_VARIANT_ADD(ftruncate, w_w) {
 FIXTURE_VARIANT_ADD(ftruncate, t_t) {
 	/* clang-format on */
 	.handled = LANDLOCK_ACCESS_FS_TRUNCATE,
-	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
+	.allowed = LANDLOCK_ACCESS_FS_TRUNCATE,
 	.expected_open_result = 0,
 	.expected_ftruncate_result = 0,
 };
@@ -3715,7 +3715,7 @@ FIXTURE_VARIANT_ADD(ftruncate, t_t) {
 FIXTURE_VARIANT_ADD(ftruncate, wt_w) {
 	/* clang-format on */
 	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
-	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE,
 	.expected_open_result = 0,
 	.expected_ftruncate_result = EACCES,
 };
@@ -3724,8 +3724,7 @@ FIXTURE_VARIANT_ADD(ftruncate, wt_w) {
 FIXTURE_VARIANT_ADD(ftruncate, wt_wt) {
 	/* clang-format on */
 	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
-	.permitted = LANDLOCK_ACCESS_FS_WRITE_FILE |
-		     LANDLOCK_ACCESS_FS_TRUNCATE,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
 	.expected_open_result = 0,
 	.expected_ftruncate_result = 0,
 };
@@ -3734,7 +3733,7 @@ FIXTURE_VARIANT_ADD(ftruncate, wt_wt) {
 FIXTURE_VARIANT_ADD(ftruncate, wt_t) {
 	/* clang-format on */
 	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_TRUNCATE,
-	.permitted = LANDLOCK_ACCESS_FS_TRUNCATE,
+	.allowed = LANDLOCK_ACCESS_FS_TRUNCATE,
 	.expected_open_result = EACCES,
 };
 
@@ -3744,7 +3743,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate)
 	const struct rule rules[] = {
 		{
 			.path = path,
-			.access = variant->permitted,
+			.access = variant->allowed,
 		},
 		{},
 	};
@@ -3785,7 +3784,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
 		const struct rule rules[] = {
 			{
 				.path = path,
-				.access = variant->permitted,
+				.access = variant->allowed,
 			},
 			{},
 		};
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 3/9] landlock: Optimize the number of calls to get_access_mask slightly

[Günther Noack]

This call is now going through a function pointer,
and it is not as obvious any more that it will be inlined.

Signed-off-by: Günther Noack <gnoack@google.com>
---
 security/landlock/ruleset.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 789c81b26a50..e0a5fbf9201a 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -723,11 +723,12 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
 	/* Saves all handled accesses per layer. */
 	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
 		const unsigned long access_req = access_request;
+		const access_mask_t access_mask =
+			get_access_mask(domain, layer_level);
 		unsigned long access_bit;
 
 		for_each_set_bit(access_bit, &access_req, num_access) {
-			if (BIT_ULL(access_bit) &
-			    get_access_mask(domain, layer_level)) {
+			if (BIT_ULL(access_bit) & access_mask) {
 				(*layer_masks)[access_bit] |=
 					BIT_ULL(layer_level);
 				handled_accesses |= BIT_ULL(access_bit);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 4/9] landlock: Add IOCTL access right

[Günther Noack]

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

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 the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
of safe IOCTL commands will be permitted on newly opened files.  The
permitted IOCTLs can be configured through the ruleset in limited ways
now.  (See documentation for details.)

Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
right on a file or directory will *not* permit to do all IOCTL
commands, but only influence the IOCTL commands which are not already
handled through other access rights.  The intent is to keep the groups
of IOCTL commands more fine-grained.

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 need to invoke 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                |  58 +++++-
 security/landlock/fs.c                       | 176 ++++++++++++++++++-
 security/landlock/fs.h                       |   2 +
 security/landlock/limits.h                   |  11 +-
 security/landlock/ruleset.h                  |   2 +-
 security/landlock/syscalls.c                 |  19 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 253 insertions(+), 22 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677539..578f268b084b 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -128,7 +128,7 @@ struct landlock_net_port_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
@@ -138,12 +138,13 @@ struct landlock_net_port_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
@@ -198,13 +199,53 @@ struct landlock_net_port_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`` and ``FIOASYNC``.  These commands
+ *   continue to be invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL
+ *   access right.
+ *
+ *   When certain other access rights are handled in the ruleset, in addition to
+ *   %LANDLOCK_ACCESS_FS_IOCTL, granting these access rights will unlock access
+ *   to additional groups of IOCTL commands, on the affected files:
+ *
+ *   * %LANDLOCK_ACCESS_FS_READ_FILE unlocks access to ``FIOQSIZE``,
+ *     ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``, ``FIONREAD``,
+ *     ``FIDEDUPRANGE``.
+ *
+ *   * %LANDLOCK_ACCESS_FS_WRITE_FILE unlocks access to ``FIOQSIZE``,
+ *     ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``, ``FICLONE``,
+ *     ``FICLONERANGE``, ``FS_IOC_RESVSP``, ``FS_IOC_RESVSP64``,
+ *     ``FS_IOC_UNRESVSP``, ``FS_IOC_UNRESVSP64``, ``FS_IOC_ZERO_RANGE``.
+ *
+ *   * %LANDLOCK_ACCESS_FS_READ_DIR unlocks access to ``FIOQSIZE``,
+ *     ``FS_IOC_FIEMAP``, ``FIBMAP``, ``FIGETBSZ``.
+ *
+ *   When these access rights are handled in the ruleset, the availability of
+ *   the affected IOCTL commands is not governed by %LANDLOCK_ACCESS_FS_IOCTL
+ *   any more, but by the respective access right.
+ *
+ *   All other IOCTL commands are not handled specially, and are governed by
+ *   %LANDLOCK_ACCESS_FS_IOCTL.  This includes %FS_IOC_GETFLAGS and
+ *   %FS_IOC_SETFLAGS for manipulating inode flags (:manpage:`ioctl_iflags(2)`),
+ *   %FS_IOC_FSFETXATTR and %FS_IOC_FSSETXATTR for manipulating extended
+ *   attributes, as well as %FIFREEZE and %FITHAW for freezing and thawing file
+ *   systems.
+ *
+ *   This access right is available since the fifth 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 */
@@ -223,6 +264,7 @@ struct landlock_net_port_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 */
 
 /**
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 9ba989ef46a5..81ce41e9e6db 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -7,12 +7,14 @@
  * Copyright © 2021-2022 Microsoft Corporation
  */
 
+#include <asm/ioctls.h>
 #include <linux/atomic.h>
 #include <linux/bitops.h>
 #include <linux/bits.h>
 #include <linux/compiler_types.h>
 #include <linux/dcache.h>
 #include <linux/err.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -28,6 +30,7 @@
 #include <linux/types.h>
 #include <linux/wait_bit.h>
 #include <linux/workqueue.h>
+#include <uapi/linux/fiemap.h>
 #include <uapi/linux/landlock.h>
 
 #include "common.h"
@@ -83,6 +86,145 @@ static const struct landlock_object_underops landlock_fs_underops = {
 	.release = release_inode
 };
 
+/* IOCTL helpers */
+
+/*
+ * These are synthetic access rights, which are only used within the kernel, but
+ * not exposed to callers in userspace.  The mapping between these access rights
+ * and IOCTL commands is defined in the required_ioctl_access() helper function.
+ */
+#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
+#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
+#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
+#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
+
+/* ioctl_groups - all synthetic access rights for IOCTL command groups */
+/* clang-format off */
+#define IOCTL_GROUPS (			  \
+	LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \
+	LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \
+	LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \
+	LANDLOCK_ACCESS_FS_IOCTL_GROUP4)
+/* clang-format on */
+
+static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
+
+/**
+ * required_ioctl_access(): Determine required IOCTL access rights.
+ *
+ * @cmd: The IOCTL command that is supposed to be run.
+ *
+ * Returns: The access rights that must be granted on an opened file in order to
+ * use the given @cmd.
+ */
+static access_mask_t required_ioctl_access(unsigned int cmd)
+{
+	switch (cmd) {
+	case FIOCLEX:
+	case FIONCLEX:
+	case FIONBIO:
+	case FIOASYNC:
+		/*
+		 * 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),
+		 * and are unconditionally permitted in Landlock.
+		 */
+		return 0;
+	case FIOQSIZE:
+		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
+	case FS_IOC_FIEMAP:
+	case FIBMAP:
+	case FIGETBSZ:
+		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
+	case FIONREAD:
+	case FIDEDUPERANGE:
+		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
+	case FICLONE:
+	case FICLONERANGE:
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+	case FS_IOC_UNRESVSP:
+	case FS_IOC_UNRESVSP64:
+	case FS_IOC_ZERO_RANGE:
+		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
+	default:
+		/*
+		 * Other commands are guarded by the catch-all access right.
+		 */
+		return LANDLOCK_ACCESS_FS_IOCTL;
+	}
+}
+
+/**
+ * expand_ioctl() - Return the dst flags from either the src flag or the
+ * %LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
+ * %LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
+ *
+ * @handled: Handled access rights.
+ * @access: The access mask to copy values from.
+ * @src: A single access right to copy from in @access.
+ * @dst: One or more access rights to copy to.
+ *
+ * Returns: @dst, or 0.
+ */
+static access_mask_t expand_ioctl(const access_mask_t handled,
+				  const access_mask_t access,
+				  const access_mask_t src,
+				  const access_mask_t dst)
+{
+	access_mask_t copy_from;
+
+	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
+		return 0;
+
+	copy_from = (handled & src) ? src : LANDLOCK_ACCESS_FS_IOCTL;
+	if (access & copy_from)
+		return dst;
+
+	return 0;
+}
+
+/**
+ * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
+ * flags enabled if necessary.
+ *
+ * @handled: Handled FS access rights.
+ * @access: FS access rights to expand.
+ *
+ * Returns: @access expanded by the necessary flags for the synthetic IOCTL
+ * access rights.
+ */
+static access_mask_t landlock_expand_access_fs(const access_mask_t handled,
+					       const access_mask_t access)
+{
+	return access |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
+			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
+				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
+				    LANDLOCK_ACCESS_FS_IOCTL_GROUP4) |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
+			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
+				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
+				    LANDLOCK_ACCESS_FS_IOCTL_GROUP3) |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
+			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1);
+}
+
+/**
+ * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
+ * access mask of handled accesses.
+ *
+ * @handled: The handled accesses of a ruleset that is being created.
+ *
+ * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
+ * if %LANDLOCK_ACCESS_FS_IOCTL is handled.
+ */
+access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled)
+{
+	return landlock_expand_access_fs(handled, handled);
+}
+
 /* Ruleset management */
 
 static struct landlock_object *get_inode_object(struct inode *const inode)
@@ -147,7 +289,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 */
 
 /*
@@ -157,6 +300,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
 			    const struct path *const path,
 			    access_mask_t access_rights)
 {
+	access_mask_t handled;
 	int err;
 	struct landlock_id id = {
 		.type = LANDLOCK_KEY_INODE,
@@ -169,9 +313,11 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
 	if (WARN_ON_ONCE(ruleset->num_layers != 1))
 		return -EINVAL;
 
+	handled = landlock_get_fs_access_mask(ruleset, 0);
+	/* Expands the synthetic IOCTL groups. */
+	access_rights |= landlock_expand_access_fs(handled, access_rights);
 	/* Transforms relative access rights to absolute ones. */
-	access_rights |= LANDLOCK_MASK_ACCESS_FS &
-			 ~landlock_get_fs_access_mask(ruleset, 0);
+	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~handled;
 	id.key.object = get_inode_object(d_backing_inode(path->dentry));
 	if (IS_ERR(id.key.object))
 		return PTR_ERR(id.key.object);
@@ -1123,7 +1269,9 @@ 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 |
+					      IOCTL_GROUPS;
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 
 	if (!dom)
@@ -1196,6 +1344,25 @@ 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)
+{
+	const access_mask_t required_access = required_ioctl_access(cmd);
+	const access_mask_t allowed_access =
+		landlock_file(file)->allowed_access;
+
+	/*
+	 * 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 ((allowed_access & required_access) == required_access)
+		return 0;
+
+	return -EACCES;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
 
@@ -1218,6 +1385,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/fs.h b/security/landlock/fs.h
index 488e4813680a..c88fe7bda37b 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h
@@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
 			    const struct path *const path,
 			    access_mask_t access_hierarchy);
 
+access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled);
+
 #endif /* _SECURITY_LANDLOCK_FS_H */
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 93c9c6f91556..296795f8a5c1 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,16 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
+/*
+ * For file system access rights, Landlock distinguishes between the publicly
+ * visible access rights (1 to LANDLOCK_LAST_PUBLIC_ACCESS_FS) and the private
+ * ones which are not exposed to userspace (LANDLOCK_LAST_PUBLIC_ACCESS_FS + 1
+ * to LANDLOCK_LAST_ACCESS_FS).  The private access rights are defined in fs.c.
+ */
+#define LANDLOCK_LAST_PUBLIC_ACCESS_FS	LANDLOCK_ACCESS_FS_IOCTL
+#define LANDLOCK_MASK_PUBLIC_ACCESS_FS	((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1)
+
+#define LANDLOCK_LAST_ACCESS_FS		(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 #define LANDLOCK_SHIFT_ACCESS_FS	0
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index c7f1526784fd..5a28ea8e1c3d 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -30,7 +30,7 @@
 	LANDLOCK_ACCESS_FS_REFER)
 /* clang-format on */
 
-typedef u16 access_mask_t;
+typedef u32 access_mask_t;
 /* Makes sure all filesystem access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
 /* Makes sure all network access rights can be stored. */
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 898358f57fa0..f0bc50003b46 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -137,7 +137,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 4
+#define LANDLOCK_ABI_VERSION 5
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
@@ -192,8 +192,8 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 		return err;
 
 	/* Checks content (and 32-bits cast). */
-	if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
-	    LANDLOCK_MASK_ACCESS_FS)
+	if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_PUBLIC_ACCESS_FS) !=
+	    LANDLOCK_MASK_PUBLIC_ACCESS_FS)
 		return -EINVAL;
 
 	/* Checks network content (and 32-bits cast). */
@@ -201,6 +201,10 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	    LANDLOCK_MASK_ACCESS_NET)
 		return -EINVAL;
 
+	/* Expands synthetic IOCTL groups. */
+	ruleset_attr.handled_access_fs = landlock_expand_handled_access_fs(
+		ruleset_attr.handled_access_fs);
+
 	/* Checks arguments and transforms to kernel struct. */
 	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
 					  ruleset_attr.handled_access_net);
@@ -309,8 +313,13 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
 	if (!path_beneath_attr.allowed_access)
 		return -ENOMSG;
 
-	/* Checks that allowed_access matches the @ruleset constraints. */
-	mask = landlock_get_raw_fs_access_mask(ruleset, 0);
+	/*
+	 * Checks that allowed_access matches the @ruleset constraints and only
+	 * consists of publicly visible access rights (as opposed to synthetic
+	 * ones).
+	 */
+	mask = landlock_get_raw_fs_access_mask(ruleset, 0) &
+	       LANDLOCK_MASK_PUBLIC_ACCESS_FS;
 	if ((path_beneath_attr.allowed_access | mask) != mask)
 		return -EINVAL;
 
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 646f778dfb1e..d292b419ccba 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(4, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(5, 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 50818904397c..192608c3e840 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -525,9 +525,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.43.0.472.g3155946c3a-goog

[PATCH v8 5/9] selftests/landlock: Test IOCTL support

[Günther Noack]

Exercises Landlock's IOCTL feature in different combinations of
handling and permitting the rights LANDLOCK_ACCESS_FS_IOCTL,
LANDLOCK_ACCESS_FS_READ_FILE, LANDLOCK_ACCESS_FS_WRITE_FILE and
LANDLOCK_ACCESS_FS_READ_DIR, and in different combinations of using
files and directories.

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 192608c3e840..054779ef4527 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -9,6 +9,7 @@
 
 #define _GNU_SOURCE
 #include <fcntl.h>
+#include <linux/fs.h>
 #include <linux/landlock.h>
 #include <linux/magic.h>
 #include <sched.h>
@@ -733,6 +734,9 @@ static int create_ruleset(struct __test_metadata *const _metadata,
 	}
 
 	for (i = 0; rules[i].path; i++) {
+		if (!rules[i].access)
+			continue;
+
 		add_path_beneath(_metadata, ruleset_fd, rules[i].access,
 				 rules[i].path);
 	}
@@ -3441,7 +3445,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);
@@ -3524,7 +3528,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);
@@ -3750,7 +3754,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);
@@ -3827,6 +3831,16 @@ TEST_F_FORK(ftruncate, open_and_ftruncate_in_different_processes)
 	ASSERT_EQ(0, close(socket_fds[1]));
 }
 
+/* Invokes the FS_IOC_GETFLAGS IOCTL and returns its errno or 0. */
+static int test_fs_ioc_getflags_ioctl(int fd)
+{
+	uint32_t flags;
+
+	if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0)
+		return errno;
+	return 0;
+}
+
 TEST(memfd_ftruncate)
 {
 	int fd;
@@ -3843,6 +3857,417 @@ TEST(memfd_ftruncate)
 	ASSERT_EQ(0, close(fd));
 }
 
+/* clang-format off */
+FIXTURE(ioctl) {};
+/* clang-format on */
+
+FIXTURE_SETUP(ioctl)
+{
+	prepare_layout(_metadata);
+	create_file(_metadata, file1_s1d1);
+}
+
+FIXTURE_TEARDOWN(ioctl)
+{
+	EXPECT_EQ(0, remove_path(file1_s1d1));
+	cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(ioctl)
+{
+	const __u64 handled;
+	const __u64 allowed;
+	const mode_t open_mode;
+	/*
+	 * These are the expected IOCTL results for a representative IOCTL from
+	 * each of the IOCTL groups.  We only distinguish the 0 and EACCES
+	 * results here, and treat other errors as 0.
+	 */
+	const int expected_fioqsize_result; /* G1 */
+	const int expected_fibmap_result; /* G2 */
+	const int expected_fionread_result; /* G3 */
+	const int expected_fs_ioc_zero_range_result; /* G4 */
+	const int expected_fs_ioc_getflags_result; /* other */
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_none) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = 0,
+	.open_mode = O_RDWR,
+	.expected_fioqsize_result = EACCES,
+	.expected_fibmap_result = EACCES,
+	.expected_fionread_result = EACCES,
+	.expected_fs_ioc_zero_range_result = EACCES,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_i) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_IOCTL,
+	.open_mode = O_RDWR,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, unhandled) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_EXECUTE,
+	.allowed = LANDLOCK_ACCESS_FS_EXECUTE,
+	.open_mode = O_RDWR,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwd_allowed_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_READ_DIR,
+	.allowed = LANDLOCK_ACCESS_FS_READ_FILE,
+	.open_mode = O_RDONLY,
+	/* If LANDLOCK_ACCESS_FS_IOCTL is not handled, all IOCTLs work. */
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwd_allowed_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_READ_DIR,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.open_mode = O_WRONLY,
+	/* If LANDLOCK_ACCESS_FS_IOCTL is not handled, all IOCTLs work. */
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_ri_allowed_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_READ_FILE,
+	.open_mode = O_RDONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = EACCES,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_wi_allowed_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.open_mode = O_WRONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = EACCES,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_di_allowed_d) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_READ_DIR,
+	.open_mode = O_RDWR,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = EACCES,
+	.expected_fionread_result = EACCES,
+	.expected_fs_ioc_zero_range_result = EACCES,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwi_allowed_rw) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.open_mode = O_RDWR,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwi_allowed_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_READ_FILE,
+	.open_mode = O_RDONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = EACCES,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwi_allowed_ri) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.open_mode = O_RDONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = 0,
+	.expected_fs_ioc_zero_range_result = EACCES,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwi_allowed_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE,
+	.open_mode = O_WRONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = EACCES,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_rwi_allowed_wi) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.allowed = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.open_mode = O_WRONLY,
+	.expected_fioqsize_result = 0,
+	.expected_fibmap_result = 0,
+	.expected_fionread_result = EACCES,
+	.expected_fs_ioc_zero_range_result = 0,
+	.expected_fs_ioc_getflags_result = 0,
+};
+
+static int test_fioqsize_ioctl(int fd)
+{
+	size_t sz;
+
+	if (ioctl(fd, FIOQSIZE, &sz) < 0)
+		return errno;
+	return 0;
+}
+
+static int test_fibmap_ioctl(int fd)
+{
+	int blk = 0;
+
+	/*
+	 * We only want to distinguish here whether Landlock already caught it,
+	 * so we treat anything but EACCESS as success.  (It commonly returns
+	 * EPERM when missing CAP_SYS_RAWIO.)
+	 */
+	if (ioctl(fd, FIBMAP, &blk) < 0 && errno == EACCES)
+		return errno;
+	return 0;
+}
+
+static int test_fionread_ioctl(int fd)
+{
+	size_t sz = 0;
+
+	if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
+		return errno;
+	return 0;
+}
+
+#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv)
+
+static int test_fs_ioc_zero_range_ioctl(int fd)
+{
+	struct space_resv {
+		__s16 l_type;
+		__s16 l_whence;
+		__s64 l_start;
+		__s64 l_len; /* len == 0 means until end of file */
+		__s32 l_sysid;
+		__u32 l_pid;
+		__s32 l_pad[4]; /* reserved area */
+	} reservation = {};
+	/*
+	 * This can fail for various reasons, but we only want to distinguish
+	 * here whether Landlock already caught it, so we treat anything but
+	 * EACCES as success.
+	 */
+	if (ioctl(fd, FS_IOC_ZERO_RANGE, &reservation) < 0 && errno == EACCES)
+		return errno;
+	return 0;
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_file)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = dir_s1d1,
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open(file1_s1d1, variant->open_mode);
+	ASSERT_LE(0, file_fd);
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result,
+		  test_fioqsize_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_dir)
+{
+	const char *const path = dir_s1d1;
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = path,
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int dir_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/*
+	 * Ignore variant->open_mode for this test, as we intend to open a
+	 * directory.  If the directory can not be opened, the variant is
+	 * infeasible to test with an opened directory.
+	 */
+	dir_fd = open(path, O_RDONLY);
+	if (dir_fd < 0)
+		return;
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result,
+		  test_fioqsize_ioctl(dir_fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(dir_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(dir_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(dir_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(dir_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(dir_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(dir_fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(dir_fd));
+}
+
+TEST_F_FORK(ioctl, handle_file_access_file)
+{
+	const char *const path = file1_s1d1;
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = path,
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
+		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
+			     "can not be granted on files");
+	}
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open(path, variant->open_mode);
+	ASSERT_LE(0, file_fd);
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result,
+		  test_fioqsize_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 6/9] selftests/landlock: Test IOCTL with memfds

[Günther Noack]

Because the LANDLOCK_ACCESS_FS_IOCTL right is associated with the
opened file during open(2), IOCTLs are supposed to 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 | 36 ++++++++++++++++------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 054779ef4527..dcc8ed6cc076 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3841,20 +3841,38 @@ static int test_fs_ioc_getflags_ioctl(int fd)
 	return 0;
 }
 
-TEST(memfd_ftruncate)
+TEST(memfd_ftruncate_and_ioctl)
 {
-	int fd;
-
-	fd = memfd_create("name", MFD_CLOEXEC);
-	ASSERT_LE(0, fd);
+	const struct landlock_ruleset_attr attr = {
+		.handled_access_fs = ACCESS_ALL,
+	};
+	int ruleset_fd, fd, i;
 
 	/*
-	 * Checks that ftruncate is permitted on file descriptors that are
-	 * created in ways other than open(2).
+	 * We exercise the same test both with and without Landlock enabled, to
+	 * ensure that it behaves the same in both cases.
 	 */
-	EXPECT_EQ(0, test_ftruncate(fd));
+	for (i = 0; i < 2; i++) {
+		/* Creates a new memfd. */
+		fd = memfd_create("name", MFD_CLOEXEC);
+		ASSERT_LE(0, fd);
 
-	ASSERT_EQ(0, close(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_fs_ioc_getflags_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));
+	}
 }
 
 /* clang-format off */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 7/9] selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH)

[Günther Noack]

ioctl(2) and ftruncate(2) operations on files opened with O_PATH
should always return EBADF, independent of the
LANDLOCK_ACCESS_FS_TRUNCATE and LANDLOCK_ACCESS_FS_IOCTL access rights
in that file hierarchy.

Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 tools/testing/selftests/landlock/fs_test.c | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index dcc8ed6cc076..89d1e4af6fb2 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3875,6 +3875,46 @@ TEST(memfd_ftruncate_and_ioctl)
 	}
 }
 
+TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
+{
+	const struct landlock_ruleset_attr attr = {
+		.handled_access_fs = ACCESS_ALL,
+	};
+	int ruleset_fd, fd;
+
+	/*
+	 * Checks that for files opened with O_PATH, both ioctl(2) and
+	 * ftruncate(2) yield EBADF, as it is documented in open(2) for the
+	 * O_PATH flag.
+	 */
+	fd = open(dir_s1d1, O_PATH | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+
+	EXPECT_EQ(EBADF, test_ftruncate(fd));
+	EXPECT_EQ(EBADF, test_fs_ioc_getflags_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));
+
+	/*
+	 * Checks that after enabling Landlock,
+	 * - the file can still be opened with O_PATH
+	 * - both ioctl and truncate still yield EBADF (not EACCES).
+	 */
+	fd = open(dir_s1d1, O_PATH | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+
+	EXPECT_EQ(EBADF, test_ftruncate(fd));
+	EXPECT_EQ(EBADF, test_fs_ioc_getflags_ioctl(fd));
+
+	ASSERT_EQ(0, close(fd));
+}
+
 /* clang-format off */
 FIXTURE(ioctl) {};
 /* clang-format on */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 8/9] 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 | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 08596c0ef070..d7323e5526be 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -81,7 +81,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 */
 
@@ -199,11 +200,12 @@ static int populate_ruleset_net(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 4
+#define LANDLOCK_ABI_LAST 5
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -317,6 +319,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		ruleset_attr.handled_access_net &=
 			~(LANDLOCK_ACCESS_NET_BIND_TCP |
 			  LANDLOCK_ACCESS_NET_CONNECT_TCP);
+		__attribute__((fallthrough));
+	case 4:
+		/* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 5 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
+
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v8 9/9] 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 | 119 ++++++++++++++++++++---
 1 file changed, 104 insertions(+), 15 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 2e3822677061..8398851964e6 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -75,7 +75,8 @@ 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,
         .handled_access_net =
             LANDLOCK_ACCESS_NET_BIND_TCP |
             LANDLOCK_ACCESS_NET_CONNECT_TCP,
@@ -84,10 +85,10 @@ to be explicit about the denied-by-default access rights.
 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 access rights which are only supported in higher versions 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
 
@@ -113,6 +114,10 @@ remove access rights which are only supported in higher versions of the ABI.
         ruleset_attr.handled_access_net &=
             ~(LANDLOCK_ACCESS_NET_BIND_TCP |
               LANDLOCK_ACCESS_NET_CONNECT_TCP);
+        __attribute__((fallthrough));
+    case 4:
+        /* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 5 */
+        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -224,6 +229,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,
@@ -317,18 +323,69 @@ 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 that a process has multiple open file
+descriptors referring to the same file, but Landlock enforces different things
+when operating with these file descriptors.  This can happen when a Landlock
+ruleset gets enforced and the process keeps file descriptors which were opened
+both before and after the enforcement.  It is also possible to pass such file
+descriptors between processes, keeping their Landlock properties, even when some
+of the involved processes do not have an enforced Landlock ruleset.
+
+Restricting IOCTL commands
+--------------------------
+
+When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will
+restrict the invocation of IOCTL commands.  However, to *permit* these IOCTL
+commands again, some of these IOCTL commands are then granted through other,
+preexisting access rights.
+
+For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and
+``LANDLOCK_ACCESS_FS_READ_FILE``.  The program *permits*
+``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``.
+
+By virtue of granting this access on the ``foo.log`` file, it is now possible to
+use common and harmless IOCTL commands which are useful when reading files, such
+as ``FIONREAD``.
+
+On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on
+another file, ``FIONREAD`` will not work on that file when it is opened.  As
+soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL
+commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL``
+any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``.
+
+The following table illustrates how IOCTL attempts for ``FIONREAD`` are
+filtered, depending on how a Landlock ruleset handles and permits the
+``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights:
+
++------------------------+-------------+-------------------+-------------------+
+|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |
+|                        | not handled | and permitted     | and not permitted |
++------------------------+-------------+-------------------+-------------------+
+| ``READ_FILE`` not      | allow       | allow             | deny              |
+| handled                |             |                   |                   |
++------------------------+             +-------------------+-------------------+
+| ``READ_FILE`` handled  |             | allow                                 |
+| and permitted          |             |                                       |
++------------------------+             +-------------------+-------------------+
+| ``READ_FILE`` handled  |             | deny                                  |
+| and not permitted      |             |                                       |
++------------------------+-------------+-------------------+-------------------+
+
+The full list of IOCTL commands and the access rights which affect them is
+documented below.
 
 Compatibility
 =============
@@ -457,6 +514,28 @@ 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,
+or to reopen them from ``/proc/self/fd/*`` without the
+``LANDLOCK_ACCESS_FS_IOCTL`` right, if possible.  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
 ====================
 
@@ -494,6 +573,16 @@ bind and connect actions to only a set of allowed ports thanks to the new
 ``LANDLOCK_ACCESS_NET_BIND_TCP`` and ``LANDLOCK_ACCESS_NET_CONNECT_TCP``
 access rights.
 
+IOCTL (ABI < 5)
+---------------
+
+IOCTL operations could not be denied before the fifth 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 5, 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.43.0.472.g3155946c3a-goog

Re: [PATCH v8 9/9] landlock: Document IOCTL support

[Mickaël Salaün]

On Fri, Dec 08, 2023 at 04:51:21PM +0100, 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 | 119 ++++++++++++++++++++---
>  1 file changed, 104 insertions(+), 15 deletions(-)
> 

> +Restricting IOCTL commands
> +--------------------------
> +
> +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will
> +restrict the invocation of IOCTL commands.  However, to *permit* these IOCTL
> +commands again, some of these IOCTL commands are then granted through other,
> +preexisting access rights.
> +
> +For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and
> +``LANDLOCK_ACCESS_FS_READ_FILE``.  The program *permits*
> +``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``.
> +
> +By virtue of granting this access on the ``foo.log`` file, it is now possible to
> +use common and harmless IOCTL commands which are useful when reading files, such
> +as ``FIONREAD``.
> +
> +On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on
> +another file, ``FIONREAD`` will not work on that file when it is opened.  As
> +soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL
> +commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL``
> +any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``.
> +
> +The following table illustrates how IOCTL attempts for ``FIONREAD`` are
> +filtered, depending on how a Landlock ruleset handles and permits the
> +``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights:
> +
> ++------------------------+-------------+-------------------+-------------------+
> +|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |
> +|                        | not handled | and permitted     | and not permitted |
> ++------------------------+-------------+-------------------+-------------------+
> +| ``READ_FILE`` not      | allow       | allow             | deny              |
> +| handled                |             |                   |                   |
> ++------------------------+             +-------------------+-------------------+
> +| ``READ_FILE`` handled  |             | allow                                 |
> +| and permitted          |             |                                       |
> ++------------------------+             +-------------------+-------------------+
> +| ``READ_FILE`` handled  |             | deny                                  |
> +| and not permitted      |             |                                       |
> ++------------------------+-------------+-------------------+-------------------+

Great! Could you please format this table with the flat-table syntax?
See https://docs.kernel.org/doc-guide/sphinx.html#tables

> +
> +The full list of IOCTL commands and the access rights which affect them is
> +documented below.

Re: [PATCH v8 9/9] landlock: Document IOCTL support

[Günther Noack]

Hello Mickaël!

Thanks for the review!

On Mon, Dec 11, 2023 at 08:04:33AM +0100, Mickaël Salaün wrote:
> On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote:
> > ++------------------------+-------------+-------------------+-------------------+
> > +|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |
> > +|                        | not handled | and permitted     | and not permitted |
> > ++------------------------+-------------+-------------------+-------------------+
> > +| ``READ_FILE`` not      | allow       | allow             | deny              |
> > +| handled                |             |                   |                   |
> > ++------------------------+             +-------------------+-------------------+
> > +| ``READ_FILE`` handled  |             | allow                                 |
> > +| and permitted          |             |                                       |
> > ++------------------------+             +-------------------+-------------------+
> > +| ``READ_FILE`` handled  |             | deny                                  |
> > +| and not permitted      |             |                                       |
> > ++------------------------+-------------+-------------------+-------------------+
> 
> Great! Could you please format this table with the flat-table syntax?
> See https://docs.kernel.org/doc-guide/sphinx.html#tables

This link actually says that “Kernel style for tables is to prefer simple table
syntax or grid table syntax” (instead of the flat-table syntax).

This "visual" style is more cumbersome to edit, but editing documentation
happens less than reading it, so further edits are less likely.  I also find it
easier to reason about what the cell sizes are that way, rather than having to
wrap my head around special :rspan: and :cspan: syntax.

If you are not strongly opposed to it, I'd prefer to keep the existing style,
but we can do it either way if you feel strongly about it.  Let me know how
important this is to you.

Thanks,
—Günther

Re: [PATCH v8 9/9] landlock: Document IOCTL support

[Mickaël Salaün]

On Mon, Dec 11, 2023 at 09:49:14AM +0100, Günther Noack wrote:
> Hello Mickaël!
> 
> Thanks for the review!
> 
> On Mon, Dec 11, 2023 at 08:04:33AM +0100, Mickaël Salaün wrote:
> > On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote:
> > > ++------------------------+-------------+-------------------+-------------------+
> > > +|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |
> > > +|                        | not handled | and permitted     | and not permitted |
> > > ++------------------------+-------------+-------------------+-------------------+
> > > +| ``READ_FILE`` not      | allow       | allow             | deny              |
> > > +| handled                |             |                   |                   |
> > > ++------------------------+             +-------------------+-------------------+
> > > +| ``READ_FILE`` handled  |             | allow                                 |
> > > +| and permitted          |             |                                       |
> > > ++------------------------+             +-------------------+-------------------+
> > > +| ``READ_FILE`` handled  |             | deny                                  |
> > > +| and not permitted      |             |                                       |
> > > ++------------------------+-------------+-------------------+-------------------+
> > 
> > Great! Could you please format this table with the flat-table syntax?
> > See https://docs.kernel.org/doc-guide/sphinx.html#tables
> 
> This link actually says that “Kernel style for tables is to prefer simple table
> syntax or grid table syntax” (instead of the flat-table syntax).
> 
> This "visual" style is more cumbersome to edit, but editing documentation
> happens less than reading it, so further edits are less likely.  I also find it
> easier to reason about what the cell sizes are that way, rather than having to
> wrap my head around special :rspan: and :cspan: syntax.

Indeed, let's keep this ascii art.

> 
> If you are not strongly opposed to it, I'd prefer to keep the existing style,
> but we can do it either way if you feel strongly about it.  Let me know how
> important this is to you.
> 
> Thanks,
> —Günther
> 

Re: [PATCH v8 9/9] landlock: Document IOCTL support

[Mickaël Salaün]

On Fri, Dec 08, 2023 at 04:51:21PM +0100, 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 | 119 ++++++++++++++++++++---
>  1 file changed, 104 insertions(+), 15 deletions(-)
> 

> +Restricting IOCTL commands
> +--------------------------
> +
> +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will

I only use "right" (instead of "access right") when LANDLOCK_ACCESS_*
precede to avoid repetition.

> +restrict the invocation of IOCTL commands.  However, to *permit* these IOCTL

This patch introduces the "permit*" wording instead of the currently
used "allowed", which is inconsistent.

> +commands again, some of these IOCTL commands are then granted through other,
> +preexisting access rights.
> +
> +For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and
> +``LANDLOCK_ACCESS_FS_READ_FILE``.  The program *permits*
> +``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``.
> +
> +By virtue of granting this access on the ``foo.log`` file, it is now possible to
> +use common and harmless IOCTL commands which are useful when reading files, such
> +as ``FIONREAD``.
> +
> +On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on
> +another file, ``FIONREAD`` will not work on that file when it is opened.  As
> +soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL
> +commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL``
> +any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``.
> +
> +The following table illustrates how IOCTL attempts for ``FIONREAD`` are
> +filtered, depending on how a Landlock ruleset handles and permits the
> +``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights:
> +
> ++------------------------+-------------+-------------------+-------------------+
> +|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |

I was a bit confused at first read, wondering why IOCTL was quoted, then
I realized that it was in fact LANDLOCK_ACCESS_FS_IOCTL. Maybe using the
"FS_" prefix would avoid this kind of misreading (same for READ_FILE)?

> +|                        | not handled | and permitted     | and not permitted |
> ++------------------------+-------------+-------------------+-------------------+
> +| ``READ_FILE`` not      | allow       | allow             | deny              |
> +| handled                |             |                   |                   |
> ++------------------------+             +-------------------+-------------------+
> +| ``READ_FILE`` handled  |             | allow                                 |
> +| and permitted          |             |                                       |
> ++------------------------+             +-------------------+-------------------+
> +| ``READ_FILE`` handled  |             | deny                                  |
> +| and not permitted      |             |                                       |

If it makes the raw text easier to read, it should be OK to extend this
table to 100 columns (I guess checkpatch.pl will not complain).

> ++------------------------+-------------+-------------------+-------------------+
> +
> +The full list of IOCTL commands and the access rights which affect them is
> +documented below.
>  
>  Compatibility
>  =============
> @@ -457,6 +514,28 @@ 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,
> +or to reopen them from ``/proc/self/fd/*`` without the
> +``LANDLOCK_ACCESS_FS_IOCTL`` right, if possible.  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.
> +

Re: [PATCH v8 4/9] landlock: Add IOCTL access right

[Mickaël Salaün]

On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:
> Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
> and increments the Landlock ABI version to 5.
> 
> 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 the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
> of safe IOCTL commands will be permitted on newly opened files.  The
> permitted IOCTLs can be configured through the ruleset in limited ways
> now.  (See documentation for details.)
> 
> Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
> right on a file or directory will *not* permit to do all IOCTL
> commands, but only influence the IOCTL commands which are not already
> handled through other access rights.  The intent is to keep the groups
> of IOCTL commands more fine-grained.
> 
> 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 need to invoke 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                |  58 +++++-
>  security/landlock/fs.c                       | 176 ++++++++++++++++++-
>  security/landlock/fs.h                       |   2 +
>  security/landlock/limits.h                   |  11 +-
>  security/landlock/ruleset.h                  |   2 +-
>  security/landlock/syscalls.c                 |  19 +-
>  tools/testing/selftests/landlock/base_test.c |   2 +-
>  tools/testing/selftests/landlock/fs_test.c   |   5 +-
>  8 files changed, 253 insertions(+), 22 deletions(-)
> 

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 9ba989ef46a5..81ce41e9e6db 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -7,12 +7,14 @@
>   * Copyright © 2021-2022 Microsoft Corporation
>   */
>  
> +#include <asm/ioctls.h>
>  #include <linux/atomic.h>
>  #include <linux/bitops.h>
>  #include <linux/bits.h>
>  #include <linux/compiler_types.h>
>  #include <linux/dcache.h>
>  #include <linux/err.h>
> +#include <linux/falloc.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -28,6 +30,7 @@
>  #include <linux/types.h>
>  #include <linux/wait_bit.h>
>  #include <linux/workqueue.h>
> +#include <uapi/linux/fiemap.h>
>  #include <uapi/linux/landlock.h>
>  
>  #include "common.h"
> @@ -83,6 +86,145 @@ static const struct landlock_object_underops landlock_fs_underops = {
>  	.release = release_inode
>  };
>  
> +/* IOCTL helpers */
> +
> +/*
> + * These are synthetic access rights, which are only used within the kernel, but
> + * not exposed to callers in userspace.  The mapping between these access rights
> + * and IOCTL commands is defined in the required_ioctl_access() helper function.
> + */
> +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> +
> +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> +/* clang-format off */
> +#define IOCTL_GROUPS (			  \
> +	LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \
> +	LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \
> +	LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \
> +	LANDLOCK_ACCESS_FS_IOCTL_GROUP4)
> +/* clang-format on */
> +
> +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
> +
> +/**
> + * required_ioctl_access(): Determine required IOCTL access rights.
> + *
> + * @cmd: The IOCTL command that is supposed to be run.
> + *
> + * Returns: The access rights that must be granted on an opened file in order to
> + * use the given @cmd.
> + */
> +static access_mask_t required_ioctl_access(unsigned int cmd)

You can add __attribute_const__ after "static", and also constify cmd.

> +{
> +	switch (cmd) {
> +	case FIOCLEX:
> +	case FIONCLEX:
> +	case FIONBIO:
> +	case FIOASYNC:
> +		/*
> +		 * 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),
> +		 * and are unconditionally permitted in Landlock.
> +		 */
> +		return 0;
> +	case FIOQSIZE:
> +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> +	case FS_IOC_FIEMAP:
> +	case FIBMAP:
> +	case FIGETBSZ:
> +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> +	case FIONREAD:
> +	case FIDEDUPERANGE:
> +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> +	case FICLONE:
> +	case FICLONERANGE:
> +	case FS_IOC_RESVSP:
> +	case FS_IOC_RESVSP64:
> +	case FS_IOC_UNRESVSP:
> +	case FS_IOC_UNRESVSP64:
> +	case FS_IOC_ZERO_RANGE:
> +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> +	default:
> +		/*
> +		 * Other commands are guarded by the catch-all access right.
> +		 */
> +		return LANDLOCK_ACCESS_FS_IOCTL;
> +	}
> +}
> +
> +/**
> + * expand_ioctl() - Return the dst flags from either the src flag or the
> + * %LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
> + * %LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
> + *
> + * @handled: Handled access rights.
> + * @access: The access mask to copy values from.
> + * @src: A single access right to copy from in @access.
> + * @dst: One or more access rights to copy to.
> + *
> + * Returns: @dst, or 0.
> + */
> +static access_mask_t expand_ioctl(const access_mask_t handled,

static __attribute_const__

> +				  const access_mask_t access,
> +				  const access_mask_t src,
> +				  const access_mask_t dst)
> +{
> +	access_mask_t copy_from;
> +
> +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> +		return 0;
> +
> +	copy_from = (handled & src) ? src : LANDLOCK_ACCESS_FS_IOCTL;
> +	if (access & copy_from)
> +		return dst;
> +
> +	return 0;
> +}
> +
> +/**
> + * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
> + * flags enabled if necessary.
> + *
> + * @handled: Handled FS access rights.
> + * @access: FS access rights to expand.
> + *
> + * Returns: @access expanded by the necessary flags for the synthetic IOCTL
> + * access rights.
> + */
> +static access_mask_t landlock_expand_access_fs(const access_mask_t handled,

static __attribute_const__

> +					       const access_mask_t access)
> +{
> +	return access |
> +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
> +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP4) |
> +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
> +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP3) |
> +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
> +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1);
> +}
> +
> +/**
> + * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
> + * access mask of handled accesses.
> + *
> + * @handled: The handled accesses of a ruleset that is being created.
> + *
> + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> + * if %LANDLOCK_ACCESS_FS_IOCTL is handled.
> + */
> +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled)

__attribute_const__ access_mask_t

> +{
> +	return landlock_expand_access_fs(handled, handled);
> +}
> +

> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index 488e4813680a..c88fe7bda37b 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>  			    const struct path *const path,
>  			    access_mask_t access_hierarchy);
>  
> +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled);

__attribute_const__ access_mask_t

> +
>  #endif /* _SECURITY_LANDLOCK_FS_H */

Re: [PATCH v8 4/9] landlock: Add IOCTL access right

[Mickaël Salaün]

On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote:
> On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:
> > Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
> > and increments the Landlock ABI version to 5.
> > 
> > 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 the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
> > of safe IOCTL commands will be permitted on newly opened files.  The
> > permitted IOCTLs can be configured through the ruleset in limited ways
> > now.  (See documentation for details.)
> > 
> > Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
> > right on a file or directory will *not* permit to do all IOCTL
> > commands, but only influence the IOCTL commands which are not already
> > handled through other access rights.  The intent is to keep the groups
> > of IOCTL commands more fine-grained.
> > 
> > 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 need to invoke 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                |  58 +++++-
> >  security/landlock/fs.c                       | 176 ++++++++++++++++++-
> >  security/landlock/fs.h                       |   2 +
> >  security/landlock/limits.h                   |  11 +-
> >  security/landlock/ruleset.h                  |   2 +-
> >  security/landlock/syscalls.c                 |  19 +-
> >  tools/testing/selftests/landlock/base_test.c |   2 +-
> >  tools/testing/selftests/landlock/fs_test.c   |   5 +-
> >  8 files changed, 253 insertions(+), 22 deletions(-)
> > 
> 
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 9ba989ef46a5..81ce41e9e6db 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -7,12 +7,14 @@
> >   * Copyright © 2021-2022 Microsoft Corporation
> >   */
> >  
> > +#include <asm/ioctls.h>
> >  #include <linux/atomic.h>
> >  #include <linux/bitops.h>
> >  #include <linux/bits.h>
> >  #include <linux/compiler_types.h>
> >  #include <linux/dcache.h>
> >  #include <linux/err.h>
> > +#include <linux/falloc.h>
> >  #include <linux/fs.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> > @@ -28,6 +30,7 @@
> >  #include <linux/types.h>
> >  #include <linux/wait_bit.h>
> >  #include <linux/workqueue.h>
> > +#include <uapi/linux/fiemap.h>
> >  #include <uapi/linux/landlock.h>
> >  
> >  #include "common.h"
> > @@ -83,6 +86,145 @@ static const struct landlock_object_underops landlock_fs_underops = {
> >  	.release = release_inode
> >  };
> >  
> > +/* IOCTL helpers */
> > +
> > +/*
> > + * These are synthetic access rights, which are only used within the kernel, but
> > + * not exposed to callers in userspace.  The mapping between these access rights
> > + * and IOCTL commands is defined in the required_ioctl_access() helper function.
> > + */
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> > +
> > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> > +/* clang-format off */
> > +#define IOCTL_GROUPS (			  \
> > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \
> > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \
> > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \
> > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP4)
> > +/* clang-format on */
> > +
> > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
> > +
> > +/**
> > + * required_ioctl_access(): Determine required IOCTL access rights.
> > + *
> > + * @cmd: The IOCTL command that is supposed to be run.
> > + *
> > + * Returns: The access rights that must be granted on an opened file in order to
> > + * use the given @cmd.
> > + */
> > +static access_mask_t required_ioctl_access(unsigned int cmd)

Please use a verb for functions, something like
get_required_ioctl_access().

> 
> You can add __attribute_const__ after "static", and also constify cmd.
> 
> > +{
> > +	switch (cmd) {
> > +	case FIOCLEX:
> > +	case FIONCLEX:
> > +	case FIONBIO:
> > +	case FIOASYNC:
> > +		/*
> > +		 * 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),
> > +		 * and are unconditionally permitted in Landlock.
> > +		 */
> > +		return 0;
> > +	case FIOQSIZE:
> > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> > +	case FS_IOC_FIEMAP:
> > +	case FIBMAP:
> > +	case FIGETBSZ:
> > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> > +	case FIONREAD:
> > +	case FIDEDUPERANGE:
> > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> > +	case FICLONE:
> > +	case FICLONERANGE:
> > +	case FS_IOC_RESVSP:
> > +	case FS_IOC_RESVSP64:
> > +	case FS_IOC_UNRESVSP:
> > +	case FS_IOC_UNRESVSP64:
> > +	case FS_IOC_ZERO_RANGE:
> > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> > +	default:
> > +		/*
> > +		 * Other commands are guarded by the catch-all access right.
> > +		 */
> > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > +	}
> > +}
> > +
> > +/**
> > + * expand_ioctl() - Return the dst flags from either the src flag or the
> > + * %LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
> > + * %LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
> > + *
> > + * @handled: Handled access rights.
> > + * @access: The access mask to copy values from.
> > + * @src: A single access right to copy from in @access.
> > + * @dst: One or more access rights to copy to.
> > + *
> > + * Returns: @dst, or 0.
> > + */
> > +static access_mask_t expand_ioctl(const access_mask_t handled,
> 
> static __attribute_const__
> 
> > +				  const access_mask_t access,
> > +				  const access_mask_t src,
> > +				  const access_mask_t dst)
> > +{
> > +	access_mask_t copy_from;
> > +
> > +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> > +		return 0;
> > +
> > +	copy_from = (handled & src) ? src : LANDLOCK_ACCESS_FS_IOCTL;
> > +	if (access & copy_from)
> > +		return dst;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
> > + * flags enabled if necessary.
> > + *
> > + * @handled: Handled FS access rights.
> > + * @access: FS access rights to expand.
> > + *
> > + * Returns: @access expanded by the necessary flags for the synthetic IOCTL
> > + * access rights.
> > + */
> > +static access_mask_t landlock_expand_access_fs(const access_mask_t handled,
> 
> static __attribute_const__
> 
> > +					       const access_mask_t access)
> > +{
> > +	return access |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP4) |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP3) |
> > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
> > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1);
> > +}
> > +
> > +/**
> > + * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
> > + * access mask of handled accesses.
> > + *
> > + * @handled: The handled accesses of a ruleset that is being created.
> > + *
> > + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> > + * if %LANDLOCK_ACCESS_FS_IOCTL is handled.
> > + */
> > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled)
> 
> __attribute_const__ access_mask_t
> 
> > +{
> > +	return landlock_expand_access_fs(handled, handled);
> > +}
> > +
> 
> > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > index 488e4813680a..c88fe7bda37b 100644
> > --- a/security/landlock/fs.h
> > +++ b/security/landlock/fs.h
> > @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> >  			    const struct path *const path,
> >  			    access_mask_t access_hierarchy);
> >  
> > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled);
> 
> __attribute_const__ access_mask_t
> 
> > +
> >  #endif /* _SECURITY_LANDLOCK_FS_H */

Re: [PATCH v8 4/9] landlock: Add IOCTL access right

[Mickaël Salaün]

Christian, what do you think about the following IOCTL groups?

On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote:
> On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote:
> > On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:
> > > Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
> > > and increments the Landlock ABI version to 5.
> > > 
> > > 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 the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
> > > of safe IOCTL commands will be permitted on newly opened files.  The
> > > permitted IOCTLs can be configured through the ruleset in limited ways
> > > now.  (See documentation for details.)
> > > 
> > > Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
> > > right on a file or directory will *not* permit to do all IOCTL
> > > commands, but only influence the IOCTL commands which are not already
> > > handled through other access rights.  The intent is to keep the groups
> > > of IOCTL commands more fine-grained.
> > > 
> > > 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 need to invoke 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                |  58 +++++-
> > >  security/landlock/fs.c                       | 176 ++++++++++++++++++-
> > >  security/landlock/fs.h                       |   2 +
> > >  security/landlock/limits.h                   |  11 +-
> > >  security/landlock/ruleset.h                  |   2 +-
> > >  security/landlock/syscalls.c                 |  19 +-
> > >  tools/testing/selftests/landlock/base_test.c |   2 +-
> > >  tools/testing/selftests/landlock/fs_test.c   |   5 +-
> > >  8 files changed, 253 insertions(+), 22 deletions(-)
> > > 
> > 
> > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > index 9ba989ef46a5..81ce41e9e6db 100644
> > > --- a/security/landlock/fs.c
> > > +++ b/security/landlock/fs.c
> > > @@ -7,12 +7,14 @@
> > >   * Copyright © 2021-2022 Microsoft Corporation
> > >   */
> > >  
> > > +#include <asm/ioctls.h>
> > >  #include <linux/atomic.h>
> > >  #include <linux/bitops.h>
> > >  #include <linux/bits.h>
> > >  #include <linux/compiler_types.h>
> > >  #include <linux/dcache.h>
> > >  #include <linux/err.h>
> > > +#include <linux/falloc.h>
> > >  #include <linux/fs.h>
> > >  #include <linux/init.h>
> > >  #include <linux/kernel.h>
> > > @@ -28,6 +30,7 @@
> > >  #include <linux/types.h>
> > >  #include <linux/wait_bit.h>
> > >  #include <linux/workqueue.h>
> > > +#include <uapi/linux/fiemap.h>
> > >  #include <uapi/linux/landlock.h>
> > >  
> > >  #include "common.h"
> > > @@ -83,6 +86,145 @@ static const struct landlock_object_underops landlock_fs_underops = {
> > >  	.release = release_inode
> > >  };
> > >  
> > > +/* IOCTL helpers */
> > > +
> > > +/*
> > > + * These are synthetic access rights, which are only used within the kernel, but
> > > + * not exposed to callers in userspace.  The mapping between these access rights
> > > + * and IOCTL commands is defined in the required_ioctl_access() helper function.
> > > + */
> > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> > > +
> > > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> > > +/* clang-format off */
> > > +#define IOCTL_GROUPS (			  \
> > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \
> > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \
> > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \
> > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP4)
> > > +/* clang-format on */
> > > +
> > > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
> > > +
> > > +/**
> > > + * required_ioctl_access(): Determine required IOCTL access rights.
> > > + *
> > > + * @cmd: The IOCTL command that is supposed to be run.
> > > + *
> > > + * Returns: The access rights that must be granted on an opened file in order to
> > > + * use the given @cmd.
> > > + */
> > > +static access_mask_t required_ioctl_access(unsigned int cmd)
> 
> Please use a verb for functions, something like
> get_required_ioctl_access().
> 
> > 
> > You can add __attribute_const__ after "static", and also constify cmd.
> > 
> > > +{
> > > +	switch (cmd) {
> > > +	case FIOCLEX:
> > > +	case FIONCLEX:
> > > +	case FIONBIO:
> > > +	case FIOASYNC:
> > > +		/*
> > > +		 * 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),
> > > +		 * and are unconditionally permitted in Landlock.
> > > +		 */
> > > +		return 0;

Could you please add comments for the following IOCTL commands
explaining why they make sense for the related file/dir read/write
mapping? We discussed about that in the ML but it would be much easier
to put that doc here for future changes, and for reviewers to understand
the rationale. Some of this doc is already in the cover letter.

To make this easier to follow, what about renaming the IOCTL groups to
something like this:
* LANDLOCK_ACCESS_FS_IOCTL_GROUP1:
  LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE
* LANDLOCK_ACCESS_FS_IOCTL_GROUP2:
  LANDLOCK_ACCESS_FS_IOCTL_GET_INNER
* LANDLOCK_ACCESS_FS_IOCTL_GROUP3:
  LANDLOCK_ACCESS_FS_IOCTL_READ_FILE
* LANDLOCK_ACCESS_FS_IOCTL_GROUP4:
  LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE

> > > +	case FIOQSIZE:
> > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> > > +	case FS_IOC_FIEMAP:
> > > +	case FIBMAP:
> > > +	case FIGETBSZ:

Does it make sense to not include FIGETBSZ in
LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously
explained but I'd like to get confirmation:
https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net

> > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> > > +	case FIONREAD:
> > > +	case FIDEDUPERANGE:
> > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> > > +	case FICLONE:
> > > +	case FICLONERANGE:

The FICLONE* commands seems to already check read/write permissions with
generic_file_rw_checks(). Always allowing them should then be OK (and
the current tests should still pass), but we can still keep them here to
make the required access right explicit and test with and without
Landlock restrictions to make sure this is consistent with the VFS
access checks. See
https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
If this is correct, a new test should check that Landlock restrictions
are the same as the VFS checks and then don't impact such IOCTLs.

> > > +	case FS_IOC_RESVSP:
> > > +	case FS_IOC_RESVSP64:
> > > +	case FS_IOC_UNRESVSP:
> > > +	case FS_IOC_UNRESVSP64:
> > > +	case FS_IOC_ZERO_RANGE:
> > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> > > +	default:
> > > +		/*
> > > +		 * Other commands are guarded by the catch-all access right.
> > > +		 */
> > > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > > +	}
> > > +}

We previously talked about allowing all IOCTLs on unix sockets and named
pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com

I think the remaining issue with this grouping is that if the VFS
implementation returns -ENOIOCTLCMD, then the IOCTL command can be
forwarded to the device driver (for character or block devices).
For instance, FIONREAD on a character device could translate to unknown
action (on this device), which should then be considered dangerous and
denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not
any IOCTL_GROUP*).

For instance, FIONREAD on /dev/null should return -ENOTTY, which should
then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if
LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for
file_ioctl()'s commands.

One solution to implement this logic would be to add an additional check
in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe
exceptions) and IOCTL commands.

Christian, is it correct to say that device drivers are not "required"
to follow the same semantic as the VFS's IOCTLs and that (for whatever
reason) collisions may occur? I guess this is not the case for
filesystems, which should implement similar semantic for the same
IOCTLs.

> > > +
> > > +/**
> > > + * expand_ioctl() - Return the dst flags from either the src flag or the
> > > + * %LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
> > > + * %LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
> > > + *
> > > + * @handled: Handled access rights.
> > > + * @access: The access mask to copy values from.
> > > + * @src: A single access right to copy from in @access.
> > > + * @dst: One or more access rights to copy to.
> > > + *
> > > + * Returns: @dst, or 0.
> > > + */
> > > +static access_mask_t expand_ioctl(const access_mask_t handled,
> > 
> > static __attribute_const__
> > 
> > > +				  const access_mask_t access,
> > > +				  const access_mask_t src,
> > > +				  const access_mask_t dst)
> > > +{
> > > +	access_mask_t copy_from;
> > > +
> > > +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> > > +		return 0;
> > > +
> > > +	copy_from = (handled & src) ? src : LANDLOCK_ACCESS_FS_IOCTL;
> > > +	if (access & copy_from)
> > > +		return dst;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
> > > + * flags enabled if necessary.
> > > + *
> > > + * @handled: Handled FS access rights.
> > > + * @access: FS access rights to expand.
> > > + *
> > > + * Returns: @access expanded by the necessary flags for the synthetic IOCTL
> > > + * access rights.
> > > + */
> > > +static access_mask_t landlock_expand_access_fs(const access_mask_t handled,
> > 
> > static __attribute_const__
> > 
> > > +					       const access_mask_t access)
> > > +{
> > > +	return access |
> > > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP4) |
> > > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
> > > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1 |
> > > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP2 |
> > > +				    LANDLOCK_ACCESS_FS_IOCTL_GROUP3) |
> > > +	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
> > > +			    LANDLOCK_ACCESS_FS_IOCTL_GROUP1);
> > > +}
> > > +
> > > +/**
> > > + * landlock_expand_handled_access_fs() - add synthetic IOCTL access rights to an
> > > + * access mask of handled accesses.
> > > + *
> > > + * @handled: The handled accesses of a ruleset that is being created.
> > > + *
> > > + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> > > + * if %LANDLOCK_ACCESS_FS_IOCTL is handled.
> > > + */
> > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled)
> > 
> > __attribute_const__ access_mask_t
> > 
> > > +{
> > > +	return landlock_expand_access_fs(handled, handled);
> > > +}
> > > +
> > 
> > > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > > index 488e4813680a..c88fe7bda37b 100644
> > > --- a/security/landlock/fs.h
> > > +++ b/security/landlock/fs.h
> > > @@ -92,4 +92,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> > >  			    const struct path *const path,
> > >  			    access_mask_t access_hierarchy);
> > >  
> > > +access_mask_t landlock_expand_handled_access_fs(const access_mask_t handled);
> > 
> > __attribute_const__ access_mask_t
> > 
> > > +
> > >  #endif /* _SECURITY_LANDLOCK_FS_H */

Re: [PATCH v8 5/9] selftests/landlock: Test IOCTL support

[Aishwarya TCV]

On 08/12/2023 15:51, Günther Noack wrote:
> Exercises Landlock's IOCTL feature in different combinations of
> handling and permitting the rights LANDLOCK_ACCESS_FS_IOCTL,
> LANDLOCK_ACCESS_FS_READ_FILE, LANDLOCK_ACCESS_FS_WRITE_FILE and
> LANDLOCK_ACCESS_FS_READ_DIR, and in different combinations of using
> files and directories.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 431 ++++++++++++++++++++-

Hi Günther,

When building kselftest against next-master the below build error is
observed. A bisect (full log
below) identified this patch as introducing the failure.

Full log from a failure:
https://storage.kernelci.org/next/master/next-20231215/arm64/defconfig+kselftest/gcc-10/logs/kselftest.log

-----
make[4]: Entering directory
'/tmp/kci/linux/tools/testing/selftests/landlock'
aarch64-linux-gnu-gcc -Wall -O2 -isystem
/tmp/kci/linux/build/usr/include     base_test.c -lcap -o
/tmp/kci/linux/build/kselftest/landlock/base_test
aarch64-linux-gnu-gcc -Wall -O2 -isystem
/tmp/kci/linux/build/usr/include     fs_test.c -lcap -o
/tmp/kci/linux/build/kselftest/landlock/fs_test
In file included from /tmp/kci/linux/build/usr/include/linux/fs.h:19,
                 from fs_test.c:12:
/usr/include/aarch64-linux-gnu/sys/mount.h:35:3: error: expected
identifier before numeric constant
   35 |   MS_RDONLY = 1,  /* Mount read-only.  */
      |   ^~~~~~~~~
In file included from common.h:19,
                 from fs_test.c:27:
fs_test.c: In function ‘prepare_layout_opt’:
fs_test.c:281:42: error: ‘MS_PRIVATE’ undeclared (first use in this
function)
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                          ^~~~~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c:281:42: note: each undeclared identifier is reported only once
for each function it appears in
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                          ^~~~~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c:281:55: error: ‘MS_REC’ undeclared (first use in this function)
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                                       ^~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
  281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c: In function ‘layout1_mount_and_pivot_child’:
fs_test.c:1653:44: error: ‘MS_RDONLY’ undeclared (first use in this
function)
 1653 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_RDONLY, NULL));
      |                                            ^~~~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:1653:2: note: in expansion of macro ‘ASSERT_EQ’
 1653 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_RDONLY, NULL));
      |  ^~~~~~~~~
fs_test.c: In function ‘layout1_topology_changes_with_net_only_child’:
fs_test.c:1712:43: error: ‘MS_PRIVATE’ undeclared (first use in this
function)
 1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                           ^~~~~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:1712:2: note: in expansion of macro ‘ASSERT_EQ’
 1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c:1712:56: error: ‘MS_REC’ undeclared (first use in this function)
 1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                                        ^~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:1712:2: note: in expansion of macro ‘ASSERT_EQ’
 1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c: In function ‘layout1_topology_changes_with_net_and_fs_child’:
fs_test.c:1741:44: error: ‘MS_PRIVATE’ undeclared (first use in this
function)
 1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                            ^~~~~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:1741:2: note: in expansion of macro ‘ASSERT_EQ’
 1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c:1741:57: error: ‘MS_REC’ undeclared (first use in this function)
 1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |                                                         ^~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:1741:2: note: in expansion of macro ‘ASSERT_EQ’
 1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
NULL));
      |  ^~~~~~~~~
fs_test.c: In function ‘layout1_bind_setup’:
fs_test.c:4340:47: error: ‘MS_BIND’ undeclared (first use in this function)
 4340 |  ASSERT_EQ(0, mount(dir_s1d2, dir_s2d2, NULL, MS_BIND, NULL));
      |                                               ^~~~~~~
../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
  707 |  __typeof__(_seen) __seen = (_seen); \
      |             ^~~~~
fs_test.c:4340:2: note: in expansion of macro ‘ASSERT_EQ’
 4340 |  ASSERT_EQ(0, mount(dir_s1d2, dir_s2d2, NULL, MS_BIND, NULL));
      |  ^~~~~~~~~
In file included from fs_test.c:19:
fs_test.c: At top level:
fs_test.c:5155:12: error: ‘MS_BIND’ undeclared here (not in a function)
 5155 |   .flags = MS_BIND,
      |            ^~~~~~~
make[4]: *** [../lib.mk:147:
/tmp/kci/linux/build/kselftest/landlock/fs_test] Error 1
make[4]: Leaving directory '/tmp/kci/linux/tools/testing/selftests/landlock'
-----


Bisect log:

-----
git bisect start
# good: [a39b6ac3781d46ba18193c9dbb2110f31e9bffe9] Linux 6.7-rc5
git bisect good a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
# bad: [11651f8cb2e88372d4ed523d909514dc9a613ea3] Add linux-next
specific files for 20231214
git bisect bad 11651f8cb2e88372d4ed523d909514dc9a613ea3
# good: [436cc0377e881784e5d12a863db037ad7d56b700] Merge branch 'main'
of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 436cc0377e881784e5d12a863db037ad7d56b700
# good: [4acaf686fcfee1d2ce0770a1d7505cd0e66400f0] Merge branch 'next'
of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
git bisect good 4acaf686fcfee1d2ce0770a1d7505cd0e66400f0
# good: [81d6c0949c93b9fb46ddd53819bc1dd69b161fb5] Merge branch
'tty-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
git bisect good 81d6c0949c93b9fb46ddd53819bc1dd69b161fb5
# good: [21298ae90dfc30823d4b3e8c28b536b94816a625] Merge branch
'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
git bisect good 21298ae90dfc30823d4b3e8c28b536b94816a625
# good: [f2cd1cb9acacb72cab0f90d2d648659fda209f75] Merge branch 'kunit'
of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect good f2cd1cb9acacb72cab0f90d2d648659fda209f75
# good: [a3cd576f9a3d15f7697764a9439b91fd1acb603c] Merge branch
'slab/for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git
git bisect good a3cd576f9a3d15f7697764a9439b91fd1acb603c
# bad: [79b6e5e0cf1a746e40d87053db55dce76d1fd718] Merge branch
'for-next/kspp' of
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
git bisect bad 79b6e5e0cf1a746e40d87053db55dce76d1fd718
# bad: [7098a5baeb1014c676b9e86025afd274807900a7] Merge branch
'sysctl-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
git bisect bad 7098a5baeb1014c676b9e86025afd274807900a7
# bad: [9b4e8cb962dfcc7d5919b0ca383ff3df7f88f7cb] Merge branch 'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
git bisect bad 9b4e8cb962dfcc7d5919b0ca383ff3df7f88f7cb
# good: [2d2016fefb8edd11a87053caab3a9044dbd7093e] landlock: Add IOCTL
access right
git bisect good 2d2016fefb8edd11a87053caab3a9044dbd7093e
# bad: [86d25e41081ec6359c75e2e873b085de03f3cd34] selftests/landlock:
Test ioctl(2) and ftruncate(2) with open(O_PATH)
git bisect bad 86d25e41081ec6359c75e2e873b085de03f3cd34
# bad: [a725134eca88b930bc2c5947297ccf72238a8149] selftests/landlock:
Test IOCTL with memfds
git bisect bad a725134eca88b930bc2c5947297ccf72238a8149
# bad: [e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2] selftests/landlock:
Test IOCTL support
git bisect bad e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2
# first bad commit: [e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2]
selftests/landlock: Test IOCTL support
-----

Thanks,
Aishwarya

Re: [PATCH v8 3/9] landlock: Optimize the number of calls to get_access_mask slightly

[Mickaël Salaün]

I'll send these first three patches for the next merge window (next
week). You can remove them in the next series.

Thanks!

On Fri, Dec 08, 2023 at 04:51:15PM +0100, Günther Noack wrote:
> This call is now going through a function pointer,
> and it is not as obvious any more that it will be inlined.
> 
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  security/landlock/ruleset.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 789c81b26a50..e0a5fbf9201a 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -723,11 +723,12 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
>  	/* Saves all handled accesses per layer. */
>  	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
>  		const unsigned long access_req = access_request;
> +		const access_mask_t access_mask =
> +			get_access_mask(domain, layer_level);
>  		unsigned long access_bit;
>  
>  		for_each_set_bit(access_bit, &access_req, num_access) {
> -			if (BIT_ULL(access_bit) &
> -			    get_access_mask(domain, layer_level)) {
> +			if (BIT_ULL(access_bit) & access_mask) {
>  				(*layer_masks)[access_bit] |=
>  					BIT_ULL(layer_level);
>  				handled_accesses |= BIT_ULL(access_bit);
> -- 
> 2.43.0.472.g3155946c3a-goog
> 
> 

Re: [PATCH v8 9/9] landlock: Document IOCTL support

[Günther Noack]

On Wed, Dec 13, 2023 at 12:25:15PM +0100, Mickaël Salaün wrote:
> On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote:
> >  Documentation/userspace-api/landlock.rst | 119 ++++++++++++++++++++---
> >  1 file changed, 104 insertions(+), 15 deletions(-)
> > 
> 
> > +Restricting IOCTL commands
> > +--------------------------
> > +
> > +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will
> 
> I only use "right" (instead of "access right") when LANDLOCK_ACCESS_*
> precede to avoid repetition.

Done.

> > +restrict the invocation of IOCTL commands.  However, to *permit* these IOCTL
> 
> This patch introduces the "permit*" wording instead of the currently
> used "allowed", which is inconsistent.

Done.


> > ++------------------------+-------------+-------------------+-------------------+
> > +|                        | ``IOCTL``   | ``IOCTL`` handled | ``IOCTL`` handled |
> 
> I was a bit confused at first read, wondering why IOCTL was quoted, then
> I realized that it was in fact LANDLOCK_ACCESS_FS_IOCTL. Maybe using the
> "FS_" prefix would avoid this kind of misreading (same for READ_FILE)?

Done.


> > +|                        | not handled | and permitted     | and not permitted |
> > ++------------------------+-------------+-------------------+-------------------+
> > +| ``READ_FILE`` not      | allow       | allow             | deny              |
> > +| handled                |             |                   |                   |
> > ++------------------------+             +-------------------+-------------------+
> > +| ``READ_FILE`` handled  |             | allow                                 |
> > +| and permitted          |             |                                       |
> > ++------------------------+             +-------------------+-------------------+
> > +| ``READ_FILE`` handled  |             | deny                                  |
> > +| and not permitted      |             |                                       |
> 
> If it makes the raw text easier to read, it should be OK to extend this
> table to 100 columns (I guess checkpatch.pl will not complain).

I got it down to 72 columns and it still reads reasonably well.
(Emacs has support for editing ASCII tables. :))

—Günther

Re: [PATCH v8 5/9] selftests/landlock: Test IOCTL support

[Günther Noack]

Hello Aishwarya!

Thanks for the bug report!

I tried this with the aarch64-linux-gnu-gcc-13 cross compiler on Debian, on
next-20231215, but I can not reproduce it.

On Fri, Dec 15, 2023 at 12:52:19PM +0000, Aishwarya TCV wrote:
> On 08/12/2023 15:51, Günther Noack wrote:
> > Exercises Landlock's IOCTL feature in different combinations of
> > handling and permitting the rights LANDLOCK_ACCESS_FS_IOCTL,
> > LANDLOCK_ACCESS_FS_READ_FILE, LANDLOCK_ACCESS_FS_WRITE_FILE and
> > LANDLOCK_ACCESS_FS_READ_DIR, and in different combinations of using
> > files and directories.
> > 
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> >  tools/testing/selftests/landlock/fs_test.c | 431 ++++++++++++++++++++-
> 
> Hi Günther,
> 
> When building kselftest against next-master the below build error is
> observed. A bisect (full log
> below) identified this patch as introducing the failure.
> 
> Full log from a failure:
> https://storage.kernelci.org/next/master/next-20231215/arm64/defconfig+kselftest/gcc-10/logs/kselftest.log
> 
> -----
> make[4]: Entering directory
> '/tmp/kci/linux/tools/testing/selftests/landlock'
> aarch64-linux-gnu-gcc -Wall -O2 -isystem
> /tmp/kci/linux/build/usr/include     base_test.c -lcap -o
> /tmp/kci/linux/build/kselftest/landlock/base_test
> aarch64-linux-gnu-gcc -Wall -O2 -isystem
> /tmp/kci/linux/build/usr/include     fs_test.c -lcap -o
> /tmp/kci/linux/build/kselftest/landlock/fs_test
> In file included from /tmp/kci/linux/build/usr/include/linux/fs.h:19,
>                  from fs_test.c:12:
> /usr/include/aarch64-linux-gnu/sys/mount.h:35:3: error: expected

The IOCTL patch set has introduced an "#include <linux/fs.h>" at the top of
selftests/landlock/fs_test.c (that file includes some IOCTL command numbers),
but that should in my mind normally be safe to include?

I'm surprised that according to the log, fs.h line 19 is including sys/mount.h
instead of linux/mount.h...?  This line says “#include <linux/mount.h>”?

> identifier before numeric constant
>    35 |   MS_RDONLY = 1,  /* Mount read-only.  */
>       |   ^~~~~~~~~
> In file included from common.h:19,
>                  from fs_test.c:27:

If you have more leads or more concrete reproduction steps, I'd be interested to
know.  Otherwise, I'd have to just hope that it'll work better on the next
attempt...?

Thanks,
—Günther

Re: [PATCH v8 5/9] selftests/landlock: Test IOCTL support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

On Fri, Jan 12, 2024 at 06:31:37PM +0100, Günther Noack wrote:

> I tried this with the aarch64-linux-gnu-gcc-13 cross compiler on Debian, on
> next-20231215, but I can not reproduce it.

The KernelCI environment for building this is in a Docker container
kernelci/gcc-10_arm64 which is on hub.docker.com - it's just Debian
oldstable with the default toolchain.

> > Full log from a failure:
> > https://storage.kernelci.org/next/master/next-20231215/arm64/defconfig+kselftest/gcc-10/logs/kselftest.log

Today's -next log is at:

   https://storage.kernelci.org/next/master/next-20240112/arm64/defconfig/gcc-10/logs/kselftest.log

which looks about the same.

> > /tmp/kci/linux/build/usr/include     fs_test.c -lcap -o
> > /tmp/kci/linux/build/kselftest/landlock/fs_test
> > In file included from /tmp/kci/linux/build/usr/include/linux/fs.h:19,
> >                  from fs_test.c:12:
> > /usr/include/aarch64-linux-gnu/sys/mount.h:35:3: error: expected

> The IOCTL patch set has introduced an "#include <linux/fs.h>" at the top of
> selftests/landlock/fs_test.c (that file includes some IOCTL command numbers),
> but that should in my mind normally be safe to include?

I'd have expected so, and nothing in the header looks like it has
implicit dependencies or anything.

> I'm surprised that according to the log, fs.h line 19 is including sys/mount.h
> instead of linux/mount.h...?  This line says “#include <linux/mount.h>”?

I'm seeing sys/mount.h in -next 20240112 (ie, today):

  https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/tools/testing/selftests/landlock/fs_test.c

AFAICT it appears to have been that way since the original commit?

> > identifier before numeric constant
> >    35 |   MS_RDONLY = 1,  /* Mount read-only.  */
> >       |   ^~~~~~~~~
> > In file included from common.h:19,
> >                  from fs_test.c:27:

> If you have more leads or more concrete reproduction steps, I'd be interested to
> know.  Otherwise, I'd have to just hope that it'll work better on the next
> attempt...?

The make command at the top of the logs linked above:

   make KBUILD_BUILD_USER=KernelCI FORMAT=.xz ARCH=arm64 HOSTCC=gcc CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- CC="ccache aarch64-linux-gnu-gcc" O=/tmp/kci/linux/build -C/tmp/kci/linux -j10 kselftest-gen_tar

will hopefully DTRT in the above Docker container though I didn't
actually try locally.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v8 5/9] selftests/landlock: Test IOCTL support

[Günther Noack]

Side remark, this other patch on the LSM list seems to about a related bug:
https://lore.kernel.org/linux-security-module/20240112074059.29673-1-hu.yadi@h3c.com/T/#u

On Fri, Dec 15, 2023 at 12:52:19PM +0000, Aishwarya TCV wrote:
> 
> 
> On 08/12/2023 15:51, Günther Noack wrote:
> > Exercises Landlock's IOCTL feature in different combinations of
> > handling and permitting the rights LANDLOCK_ACCESS_FS_IOCTL,
> > LANDLOCK_ACCESS_FS_READ_FILE, LANDLOCK_ACCESS_FS_WRITE_FILE and
> > LANDLOCK_ACCESS_FS_READ_DIR, and in different combinations of using
> > files and directories.
> > 
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> >  tools/testing/selftests/landlock/fs_test.c | 431 ++++++++++++++++++++-
> 
> Hi Günther,
> 
> When building kselftest against next-master the below build error is
> observed. A bisect (full log
> below) identified this patch as introducing the failure.
> 
> Full log from a failure:
> https://storage.kernelci.org/next/master/next-20231215/arm64/defconfig+kselftest/gcc-10/logs/kselftest.log
> 
> -----
> make[4]: Entering directory
> '/tmp/kci/linux/tools/testing/selftests/landlock'
> aarch64-linux-gnu-gcc -Wall -O2 -isystem
> /tmp/kci/linux/build/usr/include     base_test.c -lcap -o
> /tmp/kci/linux/build/kselftest/landlock/base_test
> aarch64-linux-gnu-gcc -Wall -O2 -isystem
> /tmp/kci/linux/build/usr/include     fs_test.c -lcap -o
> /tmp/kci/linux/build/kselftest/landlock/fs_test
> In file included from /tmp/kci/linux/build/usr/include/linux/fs.h:19,
>                  from fs_test.c:12:
> /usr/include/aarch64-linux-gnu/sys/mount.h:35:3: error: expected
> identifier before numeric constant
>    35 |   MS_RDONLY = 1,  /* Mount read-only.  */
>       |   ^~~~~~~~~
> In file included from common.h:19,
>                  from fs_test.c:27:
> fs_test.c: In function ‘prepare_layout_opt’:
> fs_test.c:281:42: error: ‘MS_PRIVATE’ undeclared (first use in this
> function)
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                          ^~~~~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c:281:42: note: each undeclared identifier is reported only once
> for each function it appears in
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                          ^~~~~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c:281:55: error: ‘MS_REC’ undeclared (first use in this function)
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                                       ^~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:281:2: note: in expansion of macro ‘ASSERT_EQ’
>   281 |  ASSERT_EQ(0, mount(NULL, TMP_DIR, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c: In function ‘layout1_mount_and_pivot_child’:
> fs_test.c:1653:44: error: ‘MS_RDONLY’ undeclared (first use in this
> function)
>  1653 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_RDONLY, NULL));
>       |                                            ^~~~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:1653:2: note: in expansion of macro ‘ASSERT_EQ’
>  1653 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_RDONLY, NULL));
>       |  ^~~~~~~~~
> fs_test.c: In function ‘layout1_topology_changes_with_net_only_child’:
> fs_test.c:1712:43: error: ‘MS_PRIVATE’ undeclared (first use in this
> function)
>  1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                           ^~~~~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:1712:2: note: in expansion of macro ‘ASSERT_EQ’
>  1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c:1712:56: error: ‘MS_REC’ undeclared (first use in this function)
>  1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                                        ^~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:1712:2: note: in expansion of macro ‘ASSERT_EQ’
>  1712 |  ASSERT_EQ(0, mount(NULL, dir_s1d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c: In function ‘layout1_topology_changes_with_net_and_fs_child’:
> fs_test.c:1741:44: error: ‘MS_PRIVATE’ undeclared (first use in this
> function)
>  1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                            ^~~~~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:1741:2: note: in expansion of macro ‘ASSERT_EQ’
>  1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c:1741:57: error: ‘MS_REC’ undeclared (first use in this function)
>  1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |                                                         ^~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:1741:2: note: in expansion of macro ‘ASSERT_EQ’
>  1741 |  ASSERT_EQ(-1, mount(NULL, dir_s3d2, NULL, MS_PRIVATE | MS_REC,
> NULL));
>       |  ^~~~~~~~~
> fs_test.c: In function ‘layout1_bind_setup’:
> fs_test.c:4340:47: error: ‘MS_BIND’ undeclared (first use in this function)
>  4340 |  ASSERT_EQ(0, mount(dir_s1d2, dir_s2d2, NULL, MS_BIND, NULL));
>       |                                               ^~~~~~~
> ../kselftest_harness.h:707:13: note: in definition of macro ‘__EXPECT’
>   707 |  __typeof__(_seen) __seen = (_seen); \
>       |             ^~~~~
> fs_test.c:4340:2: note: in expansion of macro ‘ASSERT_EQ’
>  4340 |  ASSERT_EQ(0, mount(dir_s1d2, dir_s2d2, NULL, MS_BIND, NULL));
>       |  ^~~~~~~~~
> In file included from fs_test.c:19:
> fs_test.c: At top level:
> fs_test.c:5155:12: error: ‘MS_BIND’ undeclared here (not in a function)
>  5155 |   .flags = MS_BIND,
>       |            ^~~~~~~
> make[4]: *** [../lib.mk:147:
> /tmp/kci/linux/build/kselftest/landlock/fs_test] Error 1
> make[4]: Leaving directory '/tmp/kci/linux/tools/testing/selftests/landlock'
> -----
> 
> 
> Bisect log:
> 
> -----
> git bisect start
> # good: [a39b6ac3781d46ba18193c9dbb2110f31e9bffe9] Linux 6.7-rc5
> git bisect good a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
> # bad: [11651f8cb2e88372d4ed523d909514dc9a613ea3] Add linux-next
> specific files for 20231214
> git bisect bad 11651f8cb2e88372d4ed523d909514dc9a613ea3
> # good: [436cc0377e881784e5d12a863db037ad7d56b700] Merge branch 'main'
> of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect good 436cc0377e881784e5d12a863db037ad7d56b700
> # good: [4acaf686fcfee1d2ce0770a1d7505cd0e66400f0] Merge branch 'next'
> of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
> git bisect good 4acaf686fcfee1d2ce0770a1d7505cd0e66400f0
> # good: [81d6c0949c93b9fb46ddd53819bc1dd69b161fb5] Merge branch
> 'tty-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> git bisect good 81d6c0949c93b9fb46ddd53819bc1dd69b161fb5
> # good: [21298ae90dfc30823d4b3e8c28b536b94816a625] Merge branch
> 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> git bisect good 21298ae90dfc30823d4b3e8c28b536b94816a625
> # good: [f2cd1cb9acacb72cab0f90d2d648659fda209f75] Merge branch 'kunit'
> of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
> git bisect good f2cd1cb9acacb72cab0f90d2d648659fda209f75
> # good: [a3cd576f9a3d15f7697764a9439b91fd1acb603c] Merge branch
> 'slab/for-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git
> git bisect good a3cd576f9a3d15f7697764a9439b91fd1acb603c
> # bad: [79b6e5e0cf1a746e40d87053db55dce76d1fd718] Merge branch
> 'for-next/kspp' of
> git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
> git bisect bad 79b6e5e0cf1a746e40d87053db55dce76d1fd718
> # bad: [7098a5baeb1014c676b9e86025afd274807900a7] Merge branch
> 'sysctl-next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git
> git bisect bad 7098a5baeb1014c676b9e86025afd274807900a7
> # bad: [9b4e8cb962dfcc7d5919b0ca383ff3df7f88f7cb] Merge branch 'next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
> git bisect bad 9b4e8cb962dfcc7d5919b0ca383ff3df7f88f7cb
> # good: [2d2016fefb8edd11a87053caab3a9044dbd7093e] landlock: Add IOCTL
> access right
> git bisect good 2d2016fefb8edd11a87053caab3a9044dbd7093e
> # bad: [86d25e41081ec6359c75e2e873b085de03f3cd34] selftests/landlock:
> Test ioctl(2) and ftruncate(2) with open(O_PATH)
> git bisect bad 86d25e41081ec6359c75e2e873b085de03f3cd34
> # bad: [a725134eca88b930bc2c5947297ccf72238a8149] selftests/landlock:
> Test IOCTL with memfds
> git bisect bad a725134eca88b930bc2c5947297ccf72238a8149
> # bad: [e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2] selftests/landlock:
> Test IOCTL support
> git bisect bad e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2
> # first bad commit: [e0bf2e60f9c35ab3fa13ff33fb3e0088fe2248c2]
> selftests/landlock: Test IOCTL support
> -----
> 
> Thanks,
> Aishwarya

Re: [PATCH v8 4/9] landlock: Add IOCTL access right

[Günther Noack]

Hello!

On Thu, Dec 14, 2023 at 03:28:10PM +0100, Mickaël Salaün wrote:
> Christian, what do you think about the following IOCTL groups?
> 
> On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote:
> > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote:
> > > On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:
> > > > Introduces the LANDLOCK_ACCESS_FS_IOCTL access right
> > > > and increments the Landlock ABI version to 5.
> > > > 
> > > > 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 the LANDLOCK_ACCESS_FS_IOCTL right is handled, only a small number
> > > > of safe IOCTL commands will be permitted on newly opened files.  The
> > > > permitted IOCTLs can be configured through the ruleset in limited ways
> > > > now.  (See documentation for details.)
> > > > 
> > > > Specifically, when LANDLOCK_ACCESS_FS_IOCTL is handled, granting this
> > > > right on a file or directory will *not* permit to do all IOCTL
> > > > commands, but only influence the IOCTL commands which are not already
> > > > handled through other access rights.  The intent is to keep the groups
> > > > of IOCTL commands more fine-grained.
> > > > 
> > > > 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 need to invoke 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                |  58 +++++-
> > > >  security/landlock/fs.c                       | 176 ++++++++++++++++++-
> > > >  security/landlock/fs.h                       |   2 +
> > > >  security/landlock/limits.h                   |  11 +-
> > > >  security/landlock/ruleset.h                  |   2 +-
> > > >  security/landlock/syscalls.c                 |  19 +-
> > > >  tools/testing/selftests/landlock/base_test.c |   2 +-
> > > >  tools/testing/selftests/landlock/fs_test.c   |   5 +-
> > > >  8 files changed, 253 insertions(+), 22 deletions(-)
> > > > 
> > > 
> > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > > index 9ba989ef46a5..81ce41e9e6db 100644
> > > > --- a/security/landlock/fs.c
> > > > +++ b/security/landlock/fs.c
> > > > @@ -7,12 +7,14 @@
> > > >   * Copyright © 2021-2022 Microsoft Corporation
> > > >   */
> > > >  
> > > > +#include <asm/ioctls.h>
> > > >  #include <linux/atomic.h>
> > > >  #include <linux/bitops.h>
> > > >  #include <linux/bits.h>
> > > >  #include <linux/compiler_types.h>
> > > >  #include <linux/dcache.h>
> > > >  #include <linux/err.h>
> > > > +#include <linux/falloc.h>
> > > >  #include <linux/fs.h>
> > > >  #include <linux/init.h>
> > > >  #include <linux/kernel.h>
> > > > @@ -28,6 +30,7 @@
> > > >  #include <linux/types.h>
> > > >  #include <linux/wait_bit.h>
> > > >  #include <linux/workqueue.h>
> > > > +#include <uapi/linux/fiemap.h>
> > > >  #include <uapi/linux/landlock.h>
> > > >  
> > > >  #include "common.h"
> > > > @@ -83,6 +86,145 @@ static const struct landlock_object_underops landlock_fs_underops = {
> > > >  	.release = release_inode
> > > >  };
> > > >  
> > > > +/* IOCTL helpers */
> > > > +
> > > > +/*
> > > > + * These are synthetic access rights, which are only used within the kernel, but
> > > > + * not exposed to callers in userspace.  The mapping between these access rights
> > > > + * and IOCTL commands is defined in the required_ioctl_access() helper function.
> > > > + */
> > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > > > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4 (LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> > > > +
> > > > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> > > > +/* clang-format off */
> > > > +#define IOCTL_GROUPS (			  \
> > > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP1 | \
> > > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP2 | \
> > > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP3 | \
> > > > +	LANDLOCK_ACCESS_FS_IOCTL_GROUP4)
> > > > +/* clang-format on */
> > > > +
> > > > +static_assert((IOCTL_GROUPS & LANDLOCK_MASK_ACCESS_FS) == IOCTL_GROUPS);
> > > > +
> > > > +/**
> > > > + * required_ioctl_access(): Determine required IOCTL access rights.
> > > > + *
> > > > + * @cmd: The IOCTL command that is supposed to be run.
> > > > + *
> > > > + * Returns: The access rights that must be granted on an opened file in order to
> > > > + * use the given @cmd.
> > > > + */
> > > > +static access_mask_t required_ioctl_access(unsigned int cmd)
> > 
> > Please use a verb for functions, something like
> > get_required_ioctl_access().
> > 
> > > 
> > > You can add __attribute_const__ after "static", and also constify cmd.
> > > 
> > > > +{
> > > > +	switch (cmd) {
> > > > +	case FIOCLEX:
> > > > +	case FIONCLEX:
> > > > +	case FIONBIO:
> > > > +	case FIOASYNC:
> > > > +		/*
> > > > +		 * 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),
> > > > +		 * and are unconditionally permitted in Landlock.
> > > > +		 */
> > > > +		return 0;
> 
> Could you please add comments for the following IOCTL commands
> explaining why they make sense for the related file/dir read/write
> mapping? We discussed about that in the ML but it would be much easier
> to put that doc here for future changes, and for reviewers to understand
> the rationale. Some of this doc is already in the cover letter.

Done, I'm adding documentation inline here.

> 
> To make this easier to follow, what about renaming the IOCTL groups to
> something like this:
> * LANDLOCK_ACCESS_FS_IOCTL_GROUP1:
>   LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE
> * LANDLOCK_ACCESS_FS_IOCTL_GROUP2:
>   LANDLOCK_ACCESS_FS_IOCTL_GET_INNER
> * LANDLOCK_ACCESS_FS_IOCTL_GROUP3:
>   LANDLOCK_ACCESS_FS_IOCTL_READ_FILE
> * LANDLOCK_ACCESS_FS_IOCTL_GROUP4:
>   LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE

Agreed that better names are in order here.
I renamed them as you suggested.

In principle, it would have been nice to name them after the access rights which
enable them, but LANDLOCK_ACCESS_FS_IOCTL_READ_DIR_OR_READ_FILE_OR_WRITE_FILE is
a bit too long for my taste. o_O


> > > > +	case FIOQSIZE:
> > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> > > > +	case FS_IOC_FIEMAP:
> > > > +	case FIBMAP:
> > > > +	case FIGETBSZ:
> 
> Does it make sense to not include FIGETBSZ in
> LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously
> explained but I'd like to get confirmation:
> https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net

It seems that the more standardized way to get file system block sizes is to use
POSIX' statvfs(3) interface, whose functionality is provided through the
statfs(2) syscall.  These functions have the usual path-based and fd-based
variants.  Landlock does not currently restrict statfs(2) at all, but there is
an existing LSM security hook for it.

We should probably introduce an access right to restrict statfs(2) in the
future, because this otherwise lets callers probe for the existence of files.  I
filed https://github.com/landlock-lsm/linux/issues/18 for it.

I am not sure how to group this best.  It seems like a very harmless thing to
allow.  (What is to be learned from the filesystem blocksize anyway?)  If we are
unsure about it, we could do the following though:

 - disallow FIGETBSZ unless LANDLOCK_ACCESS_FS_IOCTL ("misc") is granted
 - allow FIGETBSZ together with a future access right which controls statfs(2)

In that case, the use of FIGETBSZ would be nicely separable from regular read
access for files, and it would be associated with the same right.

(We could also potentially group FS_IOC_FIEMAP and FIBMAP in the same way.
These ones give information about file extents and a file's block numbers.  (You
can check whether your file is stored in a continuous area on disk.))

This would simplify the story somewhat for the IOCTLs that we need to
immediately give access to.

What do you think?


> > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> > > > +	case FIONREAD:
> > > > +	case FIDEDUPERANGE:
> > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> > > > +	case FICLONE:
> > > > +	case FICLONERANGE:
> 
> The FICLONE* commands seems to already check read/write permissions with
> generic_file_rw_checks(). Always allowing them should then be OK (and
> the current tests should still pass), but we can still keep them here to
> make the required access right explicit and test with and without
> Landlock restrictions to make sure this is consistent with the VFS
> access checks. See
> https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
> If this is correct, a new test should check that Landlock restrictions
> are the same as the VFS checks and then don't impact such IOCTLs.

Noted.  I'll look into it.

(My understanding of FICLONE, FIDEDUPRANGE and FICLONERANGE is that they let
files share the same underlying storage, on a per-range basis ("reflink").  The
IOCTL man pages for these do not explain that as explicitly, but the key point
is that the two resulting files still behave like a regular copy, because this
feature exists on COW file systems only.  So that reinforces the approach of
using READ_FILE and WRITE_FILE access rights for these IOCTL commands (because
it behaves just as if we had called read() on one file and written the results
to the other file with write()).)


> > > > +	case FS_IOC_RESVSP:
> > > > +	case FS_IOC_RESVSP64:
> > > > +	case FS_IOC_UNRESVSP:
> > > > +	case FS_IOC_UNRESVSP64:
> > > > +	case FS_IOC_ZERO_RANGE:
> > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> > > > +	default:
> > > > +		/*
> > > > +		 * Other commands are guarded by the catch-all access right.
> > > > +		 */
> > > > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > > > +	}
> > > > +}

> We previously talked about allowing all IOCTLs on unix sockets and named
> pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com

Thanks for the reminder, I missed that.  Putting it on the TODO list.


> I think the remaining issue with this grouping is that if the VFS
> implementation returns -ENOIOCTLCMD, then the IOCTL command can be
> forwarded to the device driver (for character or block devices).
> For instance, FIONREAD on a character device could translate to unknown
> action (on this device), which should then be considered dangerous and
> denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not
> any IOCTL_GROUP*).
>
> For instance, FIONREAD on /dev/null should return -ENOTTY, which should
> then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if
> LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for
> file_ioctl()'s commands.
> 
> One solution to implement this logic would be to add an additional check
> in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe
> exceptions) and IOCTL commands.

In my view this seems OK, because we are primarily protecting access to
resources (files), and only secondarily reducing the exposed kernel attack
surface.

I agree there is a certain risk associated with calling ioctl(fd, FIONREAD, ...)
on a buggy device driver.  But then again, that risk is comparable to the risk
of calling read(fd, &buf, buflen) on the same buggy device driver.  So the
LANDLOCK_ACCESS_FS_READ_FILE right grants access to both.  Users who are
concerned about the security of specific device drivers can enforce a policy
where only the necessary device files can be opened.

Does that make sense?

(Otherwise, if it makes you feel better, we can also change it so that these
IOCTL commands require LANDLOCK_ACCESS_FS_IOCTL if they are used on non-S_ISREG
files.  But it would complicate the IOCTL logic a bit, which we are exposing to
users.)


> Christian, is it correct to say that device drivers are not "required"
> to follow the same semantic as the VFS's IOCTLs and that (for whatever
> reason) collisions may occur? I guess this is not the case for
> filesystems, which should implement similar semantic for the same
> IOCTLs.

Christian, friendly ping! :)  Do you have opinions on this?

If the Landlock LSM makes decisions based on the IOCTL command numbers, do we
have to assume that underlying device drivers might expose different
functionality under the same IOCTL command numbers?

Thanks,
—Günther

Re: Re: [PATCH v8 4/9] landlock: Add IOCTL access right

[Mickaël Salaün]

On Tue, Jan 30, 2024 at 07:13:25PM +0100, Günther Noack wrote:
> Hello!
> 
> On Thu, Dec 14, 2023 at 03:28:10PM +0100, Mickaël Salaün wrote:
> > Christian, what do you think about the following IOCTL groups?
> > 
> > On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote:
> > > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote:
> > > > On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:

> > > > > +	switch (cmd) {
> > > > > +	case FIOCLEX:
> > > > > +	case FIONCLEX:
> > > > > +	case FIONBIO:
> > > > > +	case FIOASYNC:
> > > > > +		/*
> > > > > +		 * 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),
> > > > > +		 * and are unconditionally permitted in Landlock.
> > > > > +		 */
> > > > > +		return 0;
> > 
> > Could you please add comments for the following IOCTL commands
> > explaining why they make sense for the related file/dir read/write
> > mapping? We discussed about that in the ML but it would be much easier
> > to put that doc here for future changes, and for reviewers to understand
> > the rationale. Some of this doc is already in the cover letter.
> 
> Done, I'm adding documentation inline here.
> 
> > 
> > To make this easier to follow, what about renaming the IOCTL groups to
> > something like this:
> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP1:
> >   LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE

Well, this looks better:
LANDLOCK_ACCESS_FS_IOCTL_RW

We could think that it includes LANDLOCK_ACCESS_FS_MAKE_* though (which
is not the case), but it looks like the least worst...  These synthetic
access rights are not public anyway.

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP2:
> >   LANDLOCK_ACCESS_FS_IOCTL_GET_INNER

LANDLOCK_ACCESS_FS_IOCTL_RW_FILE

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP3:
> >   LANDLOCK_ACCESS_FS_IOCTL_READ_FILE

LANDLOCK_ACCESS_FS_IOCTL_R_FILE

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP4:
> >   LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE

LANDLOCK_ACCESS_FS_IOCTL_W_FILE

> 
> Agreed that better names are in order here.
> I renamed them as you suggested.
> 
> In principle, it would have been nice to name them after the access rights which
> enable them, but LANDLOCK_ACCESS_FS_IOCTL_READ_DIR_OR_READ_FILE_OR_WRITE_FILE is
> a bit too long for my taste. o_O
> 
> 
> > > > > +	case FIOQSIZE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> > > > > +	case FS_IOC_FIEMAP:
> > > > > +	case FIBMAP:
> > > > > +	case FIGETBSZ:
> > 
> > Does it make sense to not include FIGETBSZ in
> > LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously

I guess I meant "Does it make sense *to include* FIGETBSZ in
LANDLOCK_ACCESS_FS_IOCTL_GROUP1?" Which means to allow it for the
write_file, read_file and read_dir access rights:
LANDLOCK_ACCESS_FS_IOCTL_RW.

> > explained but I'd like to get confirmation:
> > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
> 
> It seems that the more standardized way to get file system block sizes is to use
> POSIX' statvfs(3) interface, whose functionality is provided through the
> statfs(2) syscall.  These functions have the usual path-based and fd-based
> variants.  Landlock does not currently restrict statfs(2) at all, but there is
> an existing LSM security hook for it.
> 
> We should probably introduce an access right to restrict statfs(2) in the
> future, because this otherwise lets callers probe for the existence of files.  I
> filed https://github.com/landlock-lsm/linux/issues/18 for it.

According to the struct statfs fields, most of them seems to be useful
for file writes (e.g. f_bsize) and file creations (e.g. f_ffree) but
potentially for file read too (e.g. f_bsize). I'm not sure how statfs is
used in practice though.

> 
> I am not sure how to group this best.  It seems like a very harmless thing to
> allow.  (What is to be learned from the filesystem blocksize anyway?)  If we are
> unsure about it, we could do the following though:

I agree that it seems to be harmless.

When we'll be able to control statfs(2), following the same logic,
LANDLOCK_ACCESS_FS_IOCTL_RW should allows to use it.  To said it another
way, implementing the statfs LSM hook doesn't seem to be useful once we
get the ability to restrict path walks:
https://github.com/landlock-lsm/linux/issues/9
Until then, there are other ways to probe for file existence anyway.

> 
>  - disallow FIGETBSZ unless LANDLOCK_ACCESS_FS_IOCTL ("misc") is granted
>  - allow FIGETBSZ together with a future access right which controls statfs(2)
> 
> In that case, the use of FIGETBSZ would be nicely separable from regular read
> access for files, and it would be associated with the same right.

If this FIGETBSZ is legitimately used by applications to optimize their
FS interactions, then we should not mask it under FS_IOCTL because this
would result of applications allowing FS_IOCTL everywhere, which is not
what we want.

> 
> (We could also potentially group FS_IOC_FIEMAP and FIBMAP in the same way.
> These ones give information about file extents and a file's block numbers.  (You
> can check whether your file is stored in a continuous area on disk.))
> 
> This would simplify the story somewhat for the IOCTLs that we need to
> immediately give access to.
> 
> What do you think?

It would help to trace a lot of generic applications and see which IOCTL
command are used in practice, we might discover new ones.

> 
> 
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> > > > > +	case FIONREAD:
> > > > > +	case FIDEDUPERANGE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> > > > > +	case FICLONE:
> > > > > +	case FICLONERANGE:
> > 
> > The FICLONE* commands seems to already check read/write permissions with
> > generic_file_rw_checks(). Always allowing them should then be OK (and
> > the current tests should still pass), but we can still keep them here to
> > make the required access right explicit and test with and without
> > Landlock restrictions to make sure this is consistent with the VFS
> > access checks. See
> > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
> > If this is correct, a new test should check that Landlock restrictions
> > are the same as the VFS checks and then don't impact such IOCTLs.
> 
> Noted.  I'll look into it.
> 
> (My understanding of FICLONE, FIDEDUPRANGE and FICLONERANGE is that they let
> files share the same underlying storage, on a per-range basis ("reflink").  The
> IOCTL man pages for these do not explain that as explicitly, but the key point
> is that the two resulting files still behave like a regular copy, because this
> feature exists on COW file systems only.  So that reinforces the approach of
> using READ_FILE and WRITE_FILE access rights for these IOCTL commands (because
> it behaves just as if we had called read() on one file and written the results
> to the other file with write()).)
> 
> 
> > > > > +	case FS_IOC_RESVSP:
> > > > > +	case FS_IOC_RESVSP64:
> > > > > +	case FS_IOC_UNRESVSP:
> > > > > +	case FS_IOC_UNRESVSP64:
> > > > > +	case FS_IOC_ZERO_RANGE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> > > > > +	default:
> > > > > +		/*
> > > > > +		 * Other commands are guarded by the catch-all access right.
> > > > > +		 */
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > > > > +	}
> > > > > +}
> 
> > We previously talked about allowing all IOCTLs on unix sockets and named
> > pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com
> 
> Thanks for the reminder, I missed that.  Putting it on the TODO list.
> 
> 
> > I think the remaining issue with this grouping is that if the VFS
> > implementation returns -ENOIOCTLCMD, then the IOCTL command can be
> > forwarded to the device driver (for character or block devices).
> > For instance, FIONREAD on a character device could translate to unknown
> > action (on this device), which should then be considered dangerous and
> > denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not
> > any IOCTL_GROUP*).
> >
> > For instance, FIONREAD on /dev/null should return -ENOTTY, which should
> > then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if
> > LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for
> > file_ioctl()'s commands.
> > 
> > One solution to implement this logic would be to add an additional check
> > in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe
> > exceptions) and IOCTL commands.
> 
> In my view this seems OK, because we are primarily protecting access to
> resources (files), and only secondarily reducing the exposed kernel attack
> surface.

Correct, but of course block and character devices need to be handled.
seccomp-bpf is the main tool to protect the kernel in this case.

> 
> I agree there is a certain risk associated with calling ioctl(fd, FIONREAD, ...)
> on a buggy device driver.  But then again, that risk is comparable to the risk
> of calling read(fd, &buf, buflen) on the same buggy device driver.  So the
> LANDLOCK_ACCESS_FS_READ_FILE right grants access to both.  Users who are
> concerned about the security of specific device drivers can enforce a policy
> where only the necessary device files can be opened.

I'm thinking about the case where the FIONREAD value is used by a device
with a completely different semantic. This should be a bad practice but
this could happen, right Christian?

> 
> Does that make sense?
> 
> (Otherwise, if it makes you feel better, we can also change it so that these
> IOCTL commands require LANDLOCK_ACCESS_FS_IOCTL if they are used on non-S_ISREG
> files.  But it would complicate the IOCTL logic a bit, which we are exposing to
> users.)

I think I'd prefer this approach. I'm not sure how special files (but
still S_ISREG) are handled though, nor if this could be an issue.

> 
> 
> > Christian, is it correct to say that device drivers are not "required"
> > to follow the same semantic as the VFS's IOCTLs and that (for whatever
> > reason) collisions may occur? I guess this is not the case for
> > filesystems, which should implement similar semantic for the same
> > IOCTLs.
> 
> Christian, friendly ping! :)  Do you have opinions on this?
> 
> If the Landlock LSM makes decisions based on the IOCTL command numbers, do we
> have to assume that underlying device drivers might expose different
> functionality under the same IOCTL command numbers?
> 
> Thanks,
> —Günther
>