[PATCH v4 0/7] Landlock: IOCTL support

[PATCH v4 0/7] 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

Open questions
~~~~~~~~~~~~~~

This is unlikely to be the last iteration, but we are getting closer.

Some notable open questions are:

 * Code style
 
   * Should we move the IOCTL access right expansion logic into the
     outer layers in syscall.c?  Where it currently lives in
     ruleset.h, this logic feels too FS-specific, and it introduces
     the additional complication that we now have to track which
     access_mask_t-s are already expanded and which are not.  It might
     be simpler to do the expansion earlier.

   * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names.

 * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
   should this grant the permission to use *any* IOCTL?  (Right now,
   it is any IOCTL except for the ones covered by the IOCTL groups,
   and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
   becomes smaller when other access rights are also handled.

 * Backwards compatibility for user-space libraries.

   This is not documented yet, because it is currently not necessary
   yet.  But as soon as we have a hypothetical Landlock ABI v6 with a
   new IOCTL-enabled "GFX" access right, the "best effort" downgrade
   from v6 to v5 becomes more involved: If the caller handles
   GFX+IOCTL and permits GFX on a file, the correct downgrade to make
   this work on a Landlock v5 kernel is to handle IOCTL only, and
   permit IOCTL(!).

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

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/

Günther Noack (7):
  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     |  74 ++-
 include/uapi/linux/landlock.h                |  51 +-
 samples/landlock/sandboxer.c                 |  10 +-
 security/landlock/fs.c                       |  74 ++-
 security/landlock/limits.h                   |  10 +-
 security/landlock/ruleset.c                  |   5 +-
 security/landlock/ruleset.h                  |  53 +-
 security/landlock/syscalls.c                 |   6 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 498 ++++++++++++++++++-
 10 files changed, 735 insertions(+), 48 deletions(-)


base-commit: f12f8f84509a084399444c4422661345a15cc713
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 1/7] 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 ffedc99f2b68..fd348633281c 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -724,10 +724,11 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
 	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
 		const unsigned long access_req = access_request;
 		unsigned long access_bit;
+		access_mask_t access_mask =
+			get_access_mask(domain, layer_level);
 
 		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.42.0.869.gea05f2083d-goog

[PATCH v4 2/7] 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.)

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                | 31 +++++---
 security/landlock/fs.c                       | 74 ++++++++++++++++++--
 security/landlock/limits.h                   | 10 ++-
 security/landlock/ruleset.h                  | 53 +++++++++++++-
 security/landlock/syscalls.c                 |  6 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  5 +-
 7 files changed, 161 insertions(+), 20 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677539..6d41c059e910 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,26 @@ 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``, ``FIOASYNC`` and ``FIONREAD``.
+ *   These commands continue to be invokable independent of the
+ *   %LANDLOCK_ACCESS_FS_IOCTL access right.
+ *
+ *   This access right is available since the fourth version of the Landlock
+ *   ABI.
+ *
  * .. warning::
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
  *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
  *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -223,6 +237,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 bc7c126deea2..aa54970c235f 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"
@@ -147,7 +150,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 +161,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 +174,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 |= expand_all_ioctl(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 +1130,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_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 | IOCTL_CMD_G4;
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 
 	if (!dom)
@@ -1196,6 +1205,62 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+static access_mask_t required_ioctl_access(unsigned int cmd)
+{
+	switch (cmd) {
+	case FIOQSIZE:
+		return IOCTL_CMD_G1;
+	case FS_IOC_FIEMAP:
+	case FIBMAP:
+	case FIGETBSZ:
+		return IOCTL_CMD_G2;
+	case FIONREAD:
+	case FIDEDUPERANGE:
+		return IOCTL_CMD_G3;
+	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 IOCTL_CMD_G4;
+	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;
+	default:
+		/*
+		 * Other commands are guarded by the catch-all access right.
+		 */
+		return LANDLOCK_ACCESS_FS_IOCTL;
+	}
+}
+
+static int hook_file_ioctl(struct file *file, unsigned int cmd,
+			   unsigned long arg)
+{
+	access_mask_t required_access = required_ioctl_access(cmd);
+	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 +1283,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
 	LSM_HOOK_INIT(file_open, hook_file_open),
 	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
+	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
 };
 
 __init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 93c9c6f91556..d0a95169ba3f 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,15 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_PUBLIC_ACCESS_FS	LANDLOCK_ACCESS_FS_IOCTL
+#define LANDLOCK_MASK_PUBLIC_ACCESS_FS	((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1)
+
+#define IOCTL_CMD_G1			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
+#define IOCTL_CMD_G2			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
+#define IOCTL_CMD_G3			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
+#define IOCTL_CMD_G4			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
+
+#define LANDLOCK_LAST_ACCESS_FS		IOCTL_CMD_G4
 #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..58d96aff3980 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. */
@@ -256,6 +256,54 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
 		refcount_inc(&ruleset->usage);
 }
 
+/**
+ * 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 inline access_mask_t expand_ioctl(access_mask_t handled,
+					 access_mask_t access,
+					 access_mask_t src, access_mask_t dst)
+{
+	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
+		return 0;
+
+	access_mask_t copy_from = (handled & src) ? src :
+						    LANDLOCK_ACCESS_FS_IOCTL;
+	if (access & copy_from)
+		return dst;
+	return 0;
+}
+
+/**
+ * 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 inline access_mask_t expand_all_ioctl(access_mask_t handled,
+					     access_mask_t access)
+{
+	return access |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_WRITE_FILE,
+			    IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4) |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_FILE,
+			    IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3) |
+	       expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_READ_DIR,
+			    IOCTL_CMD_G1);
+}
+
 static inline void
 landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
 			    const access_mask_t fs_access_mask,
@@ -265,6 +313,9 @@ landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
 
 	/* Should already be checked in sys_landlock_create_ruleset(). */
 	WARN_ON_ONCE(fs_access_mask != fs_mask);
+
+	fs_mask = expand_all_ioctl(fs_mask, fs_mask);
+
 	ruleset->access_masks[layer_level] |=
 		(fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
 }
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 898358f57fa0..67121cf7165d 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). */
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 18e1f86a6234..256cd9a96eb7 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.42.0.869.gea05f2083d-goog

[PATCH v4 3/7] 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 | 423 ++++++++++++++++++++-
 1 file changed, 420 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 256cd9a96eb7..564e73087e08 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>
@@ -3380,7 +3381,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);
@@ -3463,7 +3464,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);
@@ -3690,7 +3691,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);
@@ -3767,6 +3768,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;
@@ -3783,6 +3794,412 @@ 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 permitted;
+	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, ioctl_handled_i_permitted_none) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = LANDLOCK_ACCESS_FS_EXECUTE,
+	.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, ioctl_handled_i_permitted_i) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_unhandled) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_EXECUTE,
+	.permitted = 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, ioctl_handled_rwd_permitted_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_READ_DIR,
+	.permitted = 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, ioctl_handled_rwd_permitted_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_READ_DIR,
+	.permitted = 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, ioctl_handled_ri_permitted_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_wi_permitted_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_di_permitted_d) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_rwi_permitted_rw) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_rwi_permitted_r) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_rwi_permitted_ri) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_rwi_permitted_w) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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, ioctl_handled_rwi_permitted_wi) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_READ_FILE |
+		   LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_IOCTL,
+	.permitted = 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->permitted,
+		},
+		{},
+	};
+	int 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));
+
+	fd = open(file1_s1d1, variant->open_mode);
+	ASSERT_LE(0, fd);
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result, test_fioqsize_ioctl(fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(fd));
+	EXPECT_EQ(variant->expected_fionread_result, test_fionread_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(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->permitted,
+		},
+		{},
+	};
+	int 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.
+	 */
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return;
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result, test_fioqsize_ioctl(fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(fd));
+	EXPECT_EQ(variant->expected_fionread_result, test_fionread_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(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->permitted,
+		},
+		{},
+	};
+	int fd, ruleset_fd;
+
+	if (variant->permitted & LANDLOCK_ACCESS_FS_READ_DIR) {
+		/* This access right can not be granted on files. */
+		return;
+	}
+
+	/* 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));
+
+	fd = open(path, variant->open_mode);
+	ASSERT_LE(0, fd);
+
+	/*
+	 * Checks that IOCTL commands in each IOCTL group return the expected
+	 * errors.
+	 */
+	EXPECT_EQ(variant->expected_fioqsize_result, test_fioqsize_ioctl(fd));
+	EXPECT_EQ(variant->expected_fibmap_result, test_fibmap_ioctl(fd));
+	EXPECT_EQ(variant->expected_fionread_result, test_fionread_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_zero_range_result,
+		  test_fs_ioc_zero_range_ioctl(fd));
+	EXPECT_EQ(variant->expected_fs_ioc_getflags_result,
+		  test_fs_ioc_getflags_ioctl(fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(fd, FIOASYNC, &flag));
+
+	ASSERT_EQ(0, close(fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.42.0.869.gea05f2083d-goog

[PATCH v4 4/7] 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 564e73087e08..8a244c9cd030 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3778,20 +3778,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.42.0.869.gea05f2083d-goog

[PATCH v4 5/7] 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.

Signed-off-by: Günther Noack <gnoack@google.com>
Suggested-by: Mickaël Salaün <mic@digikod.net>
---
 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 8a244c9cd030..06c47c816c51 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3812,6 +3812,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.42.0.869.gea05f2083d-goog

[PATCH v4 6/7] 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 08596c0ef070..a4b2bebaf203 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,7 +200,8 @@ 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 */
 
@@ -317,6 +319,10 @@ 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);
+	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.42.0.869.gea05f2083d-goog

[PATCH v4 7/7] landlock: Document IOCTL support

[Günther Noack]

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

Signed-off-by: Günther Noack <gnoack@google.com>
---
 Documentation/userspace-api/landlock.rst | 74 +++++++++++++++++++-----
 include/uapi/linux/landlock.h            | 28 +++++++--
 2 files changed, 83 insertions(+), 19 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 2e3822677061..c64f315d5a2e 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,24 @@ It should also be noted that truncating files does not require the
 system call, this can also be done through :manpage:`open(2)` with the flags
 ``O_RDONLY | O_TRUNC``.
 
-When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE``
-right is associated with the newly created file descriptor and will be used for
-subsequent truncation attempts using :manpage:`ftruncate(2)`.  The behavior is
-similar to opening a file for reading or writing, where permissions are checked
-during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
+The truncate right is associated with the opened file (see below).
+
+Rights associated with file descriptors
+---------------------------------------
+
+When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` and
+``LANDLOCK_ACCESS_FS_IOCTL`` rights is associated with the newly created file
+descriptor and will be used for subsequent truncation and ioctl attempts using
+:manpage:`ftruncate(2)` and :manpage:`ioctl(2)`.  The behavior is similar to
+opening a file for reading or writing, where permissions are checked during
+:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
 :manpage:`write(2)` calls.
 
-As a consequence, it is possible to have multiple open file descriptors for the
-same file, where one grants the right to truncate the file and the other does
-not.  It is also possible to pass such file descriptors between processes,
-keeping their Landlock properties, even when these processes do not have an
-enforced Landlock ruleset.
+As a consequence, it is possible to have multiple open file descriptors
+referring to the same file, where one grants the truncate or ioctl right and the
+other does not.  It is also possible to pass such file descriptors between
+processes, keeping their Landlock properties, even when these processes do not
+have an enforced Landlock ruleset.
 
 Compatibility
 =============
@@ -457,6 +469,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 +528,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 fourth Landlock ABI, so
+:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an
+earlier ABI.
+
+Starting with the Landlock ABI version 4, it is possible to restrict the use of
+:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL`` access right.
+
 .. _kernel_support:
 
 Kernel support
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 6d41c059e910..3af0b1590f1b 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -205,11 +205,31 @@ struct landlock_net_port_attr {
  *   file or directory.
  *
  *   This access right applies to all :manpage:`ioctl(2)` commands, except of
- *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC`` and ``FIONREAD``.
- *   These commands continue to be invokable independent of the
- *   %LANDLOCK_ACCESS_FS_IOCTL access right.
+ *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO`` and ``FIOASYNC``.  These commands
+ *   continue to be invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL
+ *   access right.
  *
- *   This access right is available since the fourth version of the Landlock
+ *   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.
+ *
+ *   This access right is available since the fifth version of the Landlock
  *   ABI.
  *
  * .. warning::
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH v4 6/7] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

[kernel test robot]

Hi Günther,

kernel test robot noticed the following build errors:

[auto build test ERROR on f12f8f84509a084399444c4422661345a15cc713]

url:    https://github.com/intel-lab-lkp/linux/commits/G-nther-Noack/landlock-Optimize-the-number-of-calls-to-get_access_mask-slightly/20231104-000659
base:   f12f8f84509a084399444c4422661345a15cc713
patch link:    https://lore.kernel.org/r/20231103155717.78042-7-gnoack%40google.com
patch subject: [PATCH v4 6/7] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL
config: x86_64-randconfig-011-20231104 (https://download.01.org/0day-ci/archive/20231104/202311040923.tlGduM5r-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311040923.tlGduM5r-lkp@intel.com/reproduce)

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

All error/warnings (new ones prefixed by >>):

   samples/landlock/sandboxer.c: In function 'main':
>> samples/landlock/sandboxer.c:332:2: error: duplicate case value
     332 |  case LANDLOCK_ABI_LAST:
         |  ^~~~
   samples/landlock/sandboxer.c:322:2: note: previously used here
     322 |  case 4:
         |  ^~~~
>> samples/landlock/sandboxer.c:331:3: warning: attribute 'fallthrough' not preceding a case label or default label
     331 |   __attribute__((fallthrough));
         |   ^~~~~~~~~~~~~


vim +332 samples/landlock/sandboxer.c

903cfe8a7aa889 Mickaël Salaün       2022-09-23  209  
ba84b0bf5a164f Mickaël Salaün       2021-04-22  210  int main(const int argc, char *const argv[], char *const *const envp)
ba84b0bf5a164f Mickaël Salaün       2021-04-22  211  {
ba84b0bf5a164f Mickaël Salaün       2021-04-22  212  	const char *cmd_path;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  213  	char *const *cmd_argv;
76b902f874ff4d Mickaël Salaün       2022-05-06  214  	int ruleset_fd, abi;
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  215  	char *env_port_name;
76b902f874ff4d Mickaël Salaün       2022-05-06  216  	__u64 access_fs_ro = ACCESS_FS_ROUGHLY_READ,
76b902f874ff4d Mickaël Salaün       2022-05-06  217  	      access_fs_rw = ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE;
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  218  
ba84b0bf5a164f Mickaël Salaün       2021-04-22  219  	struct landlock_ruleset_attr ruleset_attr = {
76b902f874ff4d Mickaël Salaün       2022-05-06  220  		.handled_access_fs = access_fs_rw,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  221  		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  222  				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
ba84b0bf5a164f Mickaël Salaün       2021-04-22  223  	};
ba84b0bf5a164f Mickaël Salaün       2021-04-22  224  
ba84b0bf5a164f Mickaël Salaün       2021-04-22  225  	if (argc < 2) {
81709f3dccacf4 Mickaël Salaün       2022-05-06  226  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  227  			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  228  			"<cmd> [args]...\n\n",
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  229  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  230  			ENV_TCP_CONNECT_NAME, argv[0]);
81709f3dccacf4 Mickaël Salaün       2022-05-06  231  		fprintf(stderr,
81709f3dccacf4 Mickaël Salaün       2022-05-06  232  			"Launch a command in a restricted environment.\n\n");
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  233  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  234  			"Environment variables containing paths and ports "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  235  			"each separated by a colon:\n");
81709f3dccacf4 Mickaël Salaün       2022-05-06  236  		fprintf(stderr,
81709f3dccacf4 Mickaël Salaün       2022-05-06  237  			"* %s: list of paths allowed to be used in a read-only way.\n",
ba84b0bf5a164f Mickaël Salaün       2021-04-22  238  			ENV_FS_RO_NAME);
81709f3dccacf4 Mickaël Salaün       2022-05-06  239  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  240  			"* %s: list of paths allowed to be used in a read-write way.\n\n",
ba84b0bf5a164f Mickaël Salaün       2021-04-22  241  			ENV_FS_RW_NAME);
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  242  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  243  			"Environment variables containing ports are optional "
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  244  			"and could be skipped.\n");
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  245  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  246  			"* %s: list of ports allowed to bind (server).\n",
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  247  			ENV_TCP_BIND_NAME);
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  248  		fprintf(stderr,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  249  			"* %s: list of ports allowed to connect (client).\n",
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  250  			ENV_TCP_CONNECT_NAME);
81709f3dccacf4 Mickaël Salaün       2022-05-06  251  		fprintf(stderr,
81709f3dccacf4 Mickaël Salaün       2022-05-06  252  			"\nexample:\n"
ba84b0bf5a164f Mickaël Salaün       2021-04-22  253  			"%s=\"/bin:/lib:/usr:/proc:/etc:/dev/urandom\" "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  254  			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  255  			"%s=\"9418\" "
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  256  			"%s=\"80:443\" "
903cfe8a7aa889 Mickaël Salaün       2022-09-23  257  			"%s bash -i\n\n",
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  258  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  259  			ENV_TCP_CONNECT_NAME, argv[0]);
903cfe8a7aa889 Mickaël Salaün       2022-09-23  260  		fprintf(stderr,
903cfe8a7aa889 Mickaël Salaün       2022-09-23  261  			"This sandboxer can use Landlock features "
903cfe8a7aa889 Mickaël Salaün       2022-09-23  262  			"up to ABI version %d.\n",
903cfe8a7aa889 Mickaël Salaün       2022-09-23  263  			LANDLOCK_ABI_LAST);
ba84b0bf5a164f Mickaël Salaün       2021-04-22  264  		return 1;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  265  	}
ba84b0bf5a164f Mickaël Salaün       2021-04-22  266  
76b902f874ff4d Mickaël Salaün       2022-05-06  267  	abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
76b902f874ff4d Mickaël Salaün       2022-05-06  268  	if (abi < 0) {
ba84b0bf5a164f Mickaël Salaün       2021-04-22  269  		const int err = errno;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  270  
76b902f874ff4d Mickaël Salaün       2022-05-06  271  		perror("Failed to check Landlock compatibility");
ba84b0bf5a164f Mickaël Salaün       2021-04-22  272  		switch (err) {
ba84b0bf5a164f Mickaël Salaün       2021-04-22  273  		case ENOSYS:
81709f3dccacf4 Mickaël Salaün       2022-05-06  274  			fprintf(stderr,
81709f3dccacf4 Mickaël Salaün       2022-05-06  275  				"Hint: Landlock is not supported by the current kernel. "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  276  				"To support it, build the kernel with "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  277  				"CONFIG_SECURITY_LANDLOCK=y and prepend "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  278  				"\"landlock,\" to the content of CONFIG_LSM.\n");
ba84b0bf5a164f Mickaël Salaün       2021-04-22  279  			break;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  280  		case EOPNOTSUPP:
81709f3dccacf4 Mickaël Salaün       2022-05-06  281  			fprintf(stderr,
81709f3dccacf4 Mickaël Salaün       2022-05-06  282  				"Hint: Landlock is currently disabled. "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  283  				"It can be enabled in the kernel configuration by "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  284  				"prepending \"landlock,\" to the content of CONFIG_LSM, "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  285  				"or at boot time by setting the same content to the "
ba84b0bf5a164f Mickaël Salaün       2021-04-22  286  				"\"lsm\" kernel parameter.\n");
ba84b0bf5a164f Mickaël Salaün       2021-04-22  287  			break;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  288  		}
ba84b0bf5a164f Mickaël Salaün       2021-04-22  289  		return 1;
ba84b0bf5a164f Mickaël Salaün       2021-04-22  290  	}
903cfe8a7aa889 Mickaël Salaün       2022-09-23  291  
76b902f874ff4d Mickaël Salaün       2022-05-06  292  	/* Best-effort security. */
903cfe8a7aa889 Mickaël Salaün       2022-09-23  293  	switch (abi) {
903cfe8a7aa889 Mickaël Salaün       2022-09-23  294  	case 1:
f6e53fb2d7bd70 Günther Noack        2022-11-07  295  		/*
f6e53fb2d7bd70 Günther Noack        2022-11-07  296  		 * Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2
f6e53fb2d7bd70 Günther Noack        2022-11-07  297  		 *
f6e53fb2d7bd70 Günther Noack        2022-11-07  298  		 * Note: The "refer" operations (file renaming and linking
f6e53fb2d7bd70 Günther Noack        2022-11-07  299  		 * across different directories) are always forbidden when using
f6e53fb2d7bd70 Günther Noack        2022-11-07  300  		 * Landlock with ABI 1.
f6e53fb2d7bd70 Günther Noack        2022-11-07  301  		 *
f6e53fb2d7bd70 Günther Noack        2022-11-07  302  		 * If only ABI 1 is available, this sandboxer knowingly forbids
f6e53fb2d7bd70 Günther Noack        2022-11-07  303  		 * refer operations.
f6e53fb2d7bd70 Günther Noack        2022-11-07  304  		 *
f6e53fb2d7bd70 Günther Noack        2022-11-07  305  		 * If a program *needs* to do refer operations after enabling
f6e53fb2d7bd70 Günther Noack        2022-11-07  306  		 * Landlock, it can not use Landlock at ABI level 1.  To be
f6e53fb2d7bd70 Günther Noack        2022-11-07  307  		 * compatible with different kernel versions, such programs
f6e53fb2d7bd70 Günther Noack        2022-11-07  308  		 * should then fall back to not restrict themselves at all if
f6e53fb2d7bd70 Günther Noack        2022-11-07  309  		 * the running kernel only supports ABI 1.
f6e53fb2d7bd70 Günther Noack        2022-11-07  310  		 */
903cfe8a7aa889 Mickaël Salaün       2022-09-23  311  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
faeb9197669c23 Günther Noack        2022-10-18  312  		__attribute__((fallthrough));
faeb9197669c23 Günther Noack        2022-10-18  313  	case 2:
faeb9197669c23 Günther Noack        2022-10-18  314  		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
faeb9197669c23 Günther Noack        2022-10-18  315  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  316  		__attribute__((fallthrough));
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  317  	case 3:
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  318  		/* Removes network support for ABI < 4 */
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  319  		ruleset_attr.handled_access_net &=
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  320  			~(LANDLOCK_ACCESS_NET_BIND_TCP |
5e990dcef12eeb Konstantin Meskhidze 2023-10-26  321  			  LANDLOCK_ACCESS_NET_CONNECT_TCP);
c5aa323f1f3126 Günther Noack        2023-11-03  322  	case 4:
c5aa323f1f3126 Günther Noack        2023-11-03  323  		/* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 5 */
c5aa323f1f3126 Günther Noack        2023-11-03  324  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
c5aa323f1f3126 Günther Noack        2023-11-03  325  
903cfe8a7aa889 Mickaël Salaün       2022-09-23  326  		fprintf(stderr,
903cfe8a7aa889 Mickaël Salaün       2022-09-23  327  			"Hint: You should update the running kernel "
903cfe8a7aa889 Mickaël Salaün       2022-09-23  328  			"to leverage Landlock features "
903cfe8a7aa889 Mickaël Salaün       2022-09-23  329  			"provided by ABI version %d (instead of %d).\n",
903cfe8a7aa889 Mickaël Salaün       2022-09-23  330  			LANDLOCK_ABI_LAST, abi);
903cfe8a7aa889 Mickaël Salaün       2022-09-23 @331  		__attribute__((fallthrough));
903cfe8a7aa889 Mickaël Salaün       2022-09-23 @332  	case LANDLOCK_ABI_LAST:

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

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:
> Hello!
> 
> These patches add simple ioctl(2) support to Landlock.
> 
> Objective
> ~~~~~~~~~
> 
> Make ioctl(2) requests restrictable with Landlock,
> in a way that is useful for real-world applications.
> 
> Proposed approach
> ~~~~~~~~~~~~~~~~~
> 
> Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
> of ioctl(2) on file descriptors.
> 
> We attach 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

This looks great!

It would be nice to copy these IOCTL descriptions to the user
documentation too. That would help explain the rationale and let users
know that they should not be worried about the related IOCTLs.

> 
> Open questions
> ~~~~~~~~~~~~~~
> 
> This is unlikely to be the last iteration, but we are getting closer.
> 
> Some notable open questions are:
> 
>  * Code style
>  
>    * Should we move the IOCTL access right expansion logic into the
>      outer layers in syscall.c?  Where it currently lives in
>      ruleset.h, this logic feels too FS-specific, and it introduces
>      the additional complication that we now have to track which
>      access_mask_t-s are already expanded and which are not.  It might
>      be simpler to do the expansion earlier.

What about creating a new helper in fs.c that expands the FS access
rights, something like this:

int landlock_expand_fs_access(access_mask_t *access_mask)
{
	if (!*access_mask)
		return -ENOMSG;

	*access_mask = expand_all_ioctl(*access_mask, *access_mask);
	return 0;
}


And in syscalls.c:

	err =
		landlock_expand_fs_access(&ruleset_attr.handled_access_fs);
	if (err)
		return err;

	/* Checks arguments and transforms to kernel struct. */
	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
					  ruleset_attr.handled_access_net);


And patch the landlock_create_ruleset() helper with that:

-	if (!fs_access_mask && !net_access_mask)
+	if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
		return ERR_PTR(-ENOMSG);

> 
>    * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names.

Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that
these are in fact (synthetic) access rights?

I'm not sure we can find better than GROUP because even the content of
these groups might change in the future with new access rights.

> 
>  * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
>    should this grant the permission to use *any* IOCTL?  (Right now,
>    it is any IOCTL except for the ones covered by the IOCTL groups,
>    and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
>    becomes smaller when other access rights are also handled.

Are you suggesting to handle differently this right if it is applied to
a directory?

If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
be OK. But maybe we should rename this right to something like
LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
IOCTLs that are not handled by other access rights?


> 
>  * Backwards compatibility for user-space libraries.
> 
>    This is not documented yet, because it is currently not necessary
>    yet.  But as soon as we have a hypothetical Landlock ABI v6 with a
>    new IOCTL-enabled "GFX" access right, the "best effort" downgrade
>    from v6 to v5 becomes more involved: If the caller handles
>    GFX+IOCTL and permits GFX on a file, the correct downgrade to make
>    this work on a Landlock v5 kernel is to handle IOCTL only, and
>    permit IOCTL(!).

I don't see any issue to this approach. If there is no way to handle GFX
in v5, then there is nothing more we can do than allowing GFX (on the
same file). Another way to say it is that in v5 we allow any IOCTL
(including GFX ones) on the GFX files, an in v6 we *need* replace this
IOCTL right with the newly available GFX right, *if it is handled* by
the ruleset.

If GFX would not be tied to a file, I think it would not be a good
design for this access right. Currently all access rights are tied to
objects/data, or relative to the sandbox (e.g. ptrace).

Re: [PATCH v4 1/7] landlock: Optimize the number of calls to get_access_mask slightly

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 04:57:11PM +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 ffedc99f2b68..fd348633281c 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -724,10 +724,11 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
>  	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
>  		const unsigned long access_req = access_request;
>  		unsigned long access_bit;
> +		access_mask_t access_mask =

You can make it const and move it below the other const.

> +			get_access_mask(domain, layer_level);
>  
>  		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.42.0.869.gea05f2083d-goog
> 

Re: [PATCH v4 6/7] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 04:57:16PM +0100, Günther Noack wrote:
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index 08596c0ef070..a4b2bebaf203 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,7 +200,8 @@ 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 5

> @@ -317,6 +319,10 @@ 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.42.0.869.gea05f2083d-goog
> 

Re: [PATCH v4 2/7] landlock: Add IOCTL access right

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 04:57:12PM +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.)
> 
> 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                | 31 +++++---
>  security/landlock/fs.c                       | 74 ++++++++++++++++++--
>  security/landlock/limits.h                   | 10 ++-
>  security/landlock/ruleset.h                  | 53 +++++++++++++-
>  security/landlock/syscalls.c                 |  6 +-
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  tools/testing/selftests/landlock/fs_test.c   |  5 +-
>  7 files changed, 161 insertions(+), 20 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 25c8d7677539..6d41c059e910 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,26 @@ 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``, ``FIOASYNC`` and ``FIONREAD``.
> + *   These commands continue to be invokable independent of the
> + *   %LANDLOCK_ACCESS_FS_IOCTL access right.
> + *
> + *   This access right is available since the fourth version of the Landlock

It is now the fifth version. Same for the documentation.

> + *   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 +237,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 bc7c126deea2..aa54970c235f 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"
> @@ -147,7 +150,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 +161,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 +174,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 |= expand_all_ioctl(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 +1130,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_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 | IOCTL_CMD_G4;

I think it would be more future-proof to declare a new global const and
use it here for optional_access:

static const access_mask_t ioctl_groups =
	IOCTL_CMD_G1 |
	IOCTL_CMD_G2 |
	IOCTL_CMD_G3 |
	IOCTL_CMD_G4;

>  	const struct landlock_ruleset *const dom = get_current_fs_domain();
>  
>  	if (!dom)
> @@ -1196,6 +1205,62 @@ static int hook_file_truncate(struct file *const file)
>  	return -EACCES;
>  }
>  
> +static access_mask_t required_ioctl_access(unsigned int cmd)
> +{
> +	switch (cmd) {
> +	case FIOQSIZE:
> +		return IOCTL_CMD_G1;
> +	case FS_IOC_FIEMAP:
> +	case FIBMAP:
> +	case FIGETBSZ:
> +		return IOCTL_CMD_G2;
> +	case FIONREAD:
> +	case FIDEDUPERANGE:
> +		return IOCTL_CMD_G3;
> +	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 IOCTL_CMD_G4;
> +	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;

You can move this block at the top.

> +	default:
> +		/*
> +		 * Other commands are guarded by the catch-all access right.
> +		 */
> +		return LANDLOCK_ACCESS_FS_IOCTL;
> +	}
> +}
> +
> +static int hook_file_ioctl(struct file *file, unsigned int cmd,
> +			   unsigned long arg)
> +{
> +	access_mask_t required_access = required_ioctl_access(cmd);
> +	access_mask_t allowed_access = landlock_file(file)->allowed_access;

You can use const for these variables.

> +
> +	/*
> +	 * 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;

Please add a new line.

> +	return -EACCES;
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
>  
> @@ -1218,6 +1283,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
>  	LSM_HOOK_INIT(file_open, hook_file_open),
>  	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> +	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
>  };
>  
>  __init void landlock_add_fs_hooks(void)
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 93c9c6f91556..d0a95169ba3f 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,15 @@
>  #define LANDLOCK_MAX_NUM_LAYERS		16
>  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>  
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_PUBLIC_ACCESS_FS	LANDLOCK_ACCESS_FS_IOCTL
> +#define LANDLOCK_MASK_PUBLIC_ACCESS_FS	((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1)
> +

Please add a comment to explain that the semantic of these synthetic
access rights is defined by the required_ioctl_access() helper.

> +#define IOCTL_CMD_G1			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> +#define IOCTL_CMD_G2			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> +#define IOCTL_CMD_G3			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> +#define IOCTL_CMD_G4			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> +
> +#define LANDLOCK_LAST_ACCESS_FS		IOCTL_CMD_G4
>  #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..58d96aff3980 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. */
> @@ -256,6 +256,54 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>  		refcount_inc(&ruleset->usage);
>  }
>  
> +/**
> + * expand_ioctl - return the dst flags from either the src flag or the

Return...

> + * 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

Please don't add extra spaces, that would avoid to shift all the lines
if we get another name which is longer.

> + *
> + * Returns:
> + * @dst, or 0
> + */
> +static inline access_mask_t expand_ioctl(access_mask_t handled,
> +					 access_mask_t access,
> +					 access_mask_t src, access_mask_t dst)
> +{
> +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> +		return 0;
> +
> +	access_mask_t copy_from = (handled & src) ? src :
> +						    LANDLOCK_ACCESS_FS_IOCTL;
> +	if (access & copy_from)
> +		return dst;

Please add a newline after return.

> +	return 0;
> +}
> +
> +/**
> + * Returns @access with the synthetic IOCTL group flags enabled if necessary.

I think htmldocs will print a warning with this format.

> + *
> + * @handled: Handled FS access rights.
> + * @access:  FS access rights to expand.
> + *
> + * Returns:
> + * @access expanded by the necessary flags for the synthetic IOCTL access rights.
> + */

Re: [PATCH v4 2/7] landlock: Add IOCTL access right

[Günther Noack]

Hi!

On Thu, Nov 16, 2023 at 04:50:56PM -0500, Mickaël Salaün wrote:
> On Fri, Nov 03, 2023 at 04:57:12PM +0100, Günther Noack wrote:
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 25c8d7677539..6d41c059e910 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -198,13 +199,26 @@ 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``, ``FIOASYNC`` and ``FIONREAD``.
> > + *   These commands continue to be invokable independent of the
> > + *   %LANDLOCK_ACCESS_FS_IOCTL access right.
> > + *
> > + *   This access right is available since the fourth version of the Landlock
> 
> It is now the fifth version. Same for the documentation.

Thanks!

I've accidentally put the last update to this docstring into the "Documentation"
commit.  I'll rebase that so that it becomes part of the commit that touches the
implementation and the header.  The version is fixed there.


> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index bc7c126deea2..aa54970c235f 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -1123,7 +1130,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_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 | IOCTL_CMD_G4;
> 
> I think it would be more future-proof to declare a new global const and
> use it here for optional_access:
> 
> static const access_mask_t ioctl_groups =
> 	IOCTL_CMD_G1 |
> 	IOCTL_CMD_G2 |
> 	IOCTL_CMD_G3 |
> 	IOCTL_CMD_G4;

Done.


> > +static access_mask_t required_ioctl_access(unsigned int cmd)
> > +{
> > +	switch (cmd) {
> > +	case FIOQSIZE:
> > +		return IOCTL_CMD_G1;
> > +	case FS_IOC_FIEMAP:
> > +	case FIBMAP:
> > +	case FIGETBSZ:
> > +		return IOCTL_CMD_G2;
> > +	case FIONREAD:
> > +	case FIDEDUPERANGE:
> > +		return IOCTL_CMD_G3;
> > +	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 IOCTL_CMD_G4;
> > +	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;
> 
> You can move this block at the top.

Done.


> > +	default:
> > +		/*
> > +		 * Other commands are guarded by the catch-all access right.
> > +		 */
> > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > +	}
> > +}
> > +
> > +static int hook_file_ioctl(struct file *file, unsigned int cmd,
> > +			   unsigned long arg)
> > +{
> > +	access_mask_t required_access = required_ioctl_access(cmd);
> > +	access_mask_t allowed_access = landlock_file(file)->allowed_access;
> 
> You can use const for these variables.

Done.


> > +
> > +	/*
> > +	 * 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;
> 
> Please add a new line.

Done.


> > +	return -EACCES;
> > +}
> > +
> >  static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >  	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
> >  
> > @@ -1218,6 +1283,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >  	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
> >  	LSM_HOOK_INIT(file_open, hook_file_open),
> >  	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> > +	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
> >  };
> >  
> >  __init void landlock_add_fs_hooks(void)
> > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > index 93c9c6f91556..d0a95169ba3f 100644
> > --- a/security/landlock/limits.h
> > +++ b/security/landlock/limits.h
> > @@ -18,7 +18,15 @@
> >  #define LANDLOCK_MAX_NUM_LAYERS		16
> >  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
> >  
> > -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> > +#define LANDLOCK_LAST_PUBLIC_ACCESS_FS	LANDLOCK_ACCESS_FS_IOCTL
> > +#define LANDLOCK_MASK_PUBLIC_ACCESS_FS	((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1)
> > +
> 
> Please add a comment to explain that the semantic of these synthetic
> access rights is defined by the required_ioctl_access() helper.

Done.


> > +#define IOCTL_CMD_G1			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > +#define IOCTL_CMD_G2			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > +#define IOCTL_CMD_G3			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > +#define IOCTL_CMD_G4			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> > +
> > +#define LANDLOCK_LAST_ACCESS_FS		IOCTL_CMD_G4
> >  #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..58d96aff3980 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. */
> > @@ -256,6 +256,54 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
> >  		refcount_inc(&ruleset->usage);
> >  }
> >  
> > +/**
> > + * expand_ioctl - return the dst flags from either the src flag or the
> 
> Return...

Done.


> > + * 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
> 
> Please don't add extra spaces, that would avoid to shift all the lines
> if we get another name which is longer.

Done.


> > + *
> > + * Returns:
> > + * @dst, or 0
> > + */
> > +static inline access_mask_t expand_ioctl(access_mask_t handled,
> > +					 access_mask_t access,
> > +					 access_mask_t src, access_mask_t dst)
> > +{
> > +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> > +		return 0;
> > +
> > +	access_mask_t copy_from = (handled & src) ? src :
> > +						    LANDLOCK_ACCESS_FS_IOCTL;
> > +	if (access & copy_from)
> > +		return dst;
> 
> Please add a newline after return.

Done.


> > +	return 0;
> > +}
> > +
> > +/**
> > + * Returns @access with the synthetic IOCTL group flags enabled if necessary.
> 
> I think htmldocs will print a warning with this format.

You are absolutely right, I got a warning about that. %-)  Fixed.


—Günther

Re: [PATCH v4 6/7] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

[Günther Noack]

Thanks!  (I see you fixed these two on mic-next already.)

On Thu, Nov 16, 2023 at 04:50:03PM -0500, Micka�l Sala�n wrote:
> On Fri, Nov 03, 2023 at 04:57:16PM +0100, G�nther Noack wrote:
> > 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 | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > index 08596c0ef070..a4b2bebaf203 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,7 +200,8 @@ 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 5
> 
> > @@ -317,6 +319,10 @@ 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.42.0.869.gea05f2083d-goog
> > 

Re: [PATCH v4 1/7] landlock: Optimize the number of calls to get_access_mask slightly

[Günther Noack]

On Thu, Nov 16, 2023 at 04:49:46PM -0500, Micka�l Sala�n wrote:
> On Fri, Nov 03, 2023 at 04:57:11PM +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 ffedc99f2b68..fd348633281c 100644
> > --- a/security/landlock/ruleset.c
> > +++ b/security/landlock/ruleset.c
> > @@ -724,10 +724,11 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
> >  	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
> >  		const unsigned long access_req = access_request;
> >  		unsigned long access_bit;
> > +		access_mask_t access_mask =
> 
> You can make it const and move it below the other const.

Done.

> > +			get_access_mask(domain, layer_level);
> >  
> >  		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.42.0.869.gea05f2083d-goog
> > 

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Günther Noack]

On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote:
> On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:
> > Hello!
> > 
> > These patches add simple ioctl(2) support to Landlock.
> > 
> > Objective
> > ~~~~~~~~~
> > 
> > Make ioctl(2) requests restrictable with Landlock,
> > in a way that is useful for real-world applications.
> > 
> > Proposed approach
> > ~~~~~~~~~~~~~~~~~
> > 
> > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
> > of ioctl(2) on file descriptors.
> > 
> > We attach 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
> 
> This looks great!
> 
> It would be nice to copy these IOCTL descriptions to the user
> documentation too. That would help explain the rationale and let users
> know that they should not be worried about the related IOCTLs.

Added.


> > Open questions
> > ~~~~~~~~~~~~~~
> > 
> > This is unlikely to be the last iteration, but we are getting closer.
> > 
> > Some notable open questions are:
> > 
> >  * Code style
> >  
> >    * Should we move the IOCTL access right expansion logic into the
> >      outer layers in syscall.c?  Where it currently lives in
> >      ruleset.h, this logic feels too FS-specific, and it introduces
> >      the additional complication that we now have to track which
> >      access_mask_t-s are already expanded and which are not.  It might
> >      be simpler to do the expansion earlier.
> 
> What about creating a new helper in fs.c that expands the FS access
> rights, something like this:
> 
> int landlock_expand_fs_access(access_mask_t *access_mask)
> {
> 	if (!*access_mask)
> 		return -ENOMSG;
> 
> 	*access_mask = expand_all_ioctl(*access_mask, *access_mask);
> 	return 0;
> }
> 
> 
> And in syscalls.c:
> 
> 	err =
> 		landlock_expand_fs_access(&ruleset_attr.handled_access_fs);
> 	if (err)
> 		return err;
> 
> 	/* Checks arguments and transforms to kernel struct. */
> 	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> 					  ruleset_attr.handled_access_net);

Done, this looks good.

I called the landlock_expand_fs_access function slightly differently and made it
return the resulting access_mask_t (because it does not make a performance
difference, and then there is no potential for calling it with a null pointer,
and the function does not need to return an error).

As a consequence of doing it like this, I also moved the expansion functions
into fs.c, away from ruleset.h where they did not fit in. :)


> And patch the landlock_create_ruleset() helper with that:
> 
> -	if (!fs_access_mask && !net_access_mask)
> +	if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
> 		return ERR_PTR(-ENOMSG);

Why would you want to warn on the case where fs_access_mask is zero?

Is it not a legitimate use case to use Landlock for the network aspect only?

(If a user is not handling any of the LANDLOCK_ACCESS_FS* rights, the expansion
step is not going to add any.)


> >    * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names.
> 
> Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that
> these are in fact (synthetic) access rights?
> 
> I'm not sure we can find better than GROUP because even the content of
> these groups might change in the future with new access rights.

Makes sense, renamed as suggested.  TBH, IOCTL_CMD_G1...4 was more of a
placeholder anyway because I was so lazy with my typing. ;)


> >  * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
> >    should this grant the permission to use *any* IOCTL?  (Right now,
> >    it is any IOCTL except for the ones covered by the IOCTL groups,
> >    and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
> >    becomes smaller when other access rights are also handled.
> 
> Are you suggesting to handle differently this right if it is applied to
> a directory?

No - this applies to files as well.  I am suggesting that granting
LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access
to *all* ioctls, both the ones in the synthetic groups and the remaining ones.

Let me spell out the scenario:

Steps to reproduce:
  - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE
  - permit: LANDLOCK_ACCESS_FS_IOCTL
            on file f
  - open file f (for write-only)
  - attempt to use ioctl(fd, FIOQSIZE, ...)

With this patch set:
  - ioctl(fd, FIOQSIZE, ...) fails,
    because FIOQSIZE is part of IOCTL_CMD_G1
    and because LANDLOCK_ACCESS_FS_READ_FILE is handled,
    IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE

Alternative proposal:
  - ioctl(fd, FIOQSIZE, ...) should maybe work,
    because LANDLOCK_ACCESS_FS_IOCTL is permitted on f

    Implementation-wise, this would mean to add

    expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups)

    to expand_all_ioctl().

I feel that this alternative might be less surprising, because granting the
IOCTL right would grant all the things that were restricted when handling the
IOCTL right, and it would be more "symmetric".

What do you think?


> If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> be OK. But maybe we should rename this right to something like
> LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> IOCTLs that are not handled by other access rights?

Hmm, I'm not convinced this is a good name.  It makes sense in the context of
allowing "all the other ioctls" for a file or file hierarchy, but when setting
LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
so "default" doesn't seem appropriate to me.


> >  * Backwards compatibility for user-space libraries.
> > 
> >    This is not documented yet, because it is currently not necessary
> >    yet.  But as soon as we have a hypothetical Landlock ABI v6 with a
> >    new IOCTL-enabled "GFX" access right, the "best effort" downgrade
> >    from v6 to v5 becomes more involved: If the caller handles
> >    GFX+IOCTL and permits GFX on a file, the correct downgrade to make
> >    this work on a Landlock v5 kernel is to handle IOCTL only, and
> >    permit IOCTL(!).
> 
> I don't see any issue to this approach. If there is no way to handle GFX
> in v5, then there is nothing more we can do than allowing GFX (on the
> same file). Another way to say it is that in v5 we allow any IOCTL
> (including GFX ones) on the GFX files, an in v6 we *need* replace this
> IOCTL right with the newly available GFX right, *if it is handled* by
> the ruleset.
> 
> If GFX would not be tied to a file, I think it would not be a good
> design for this access right. Currently all access rights are tied to
> objects/data, or relative to the sandbox (e.g. ptrace).

Yes, makes sense - we are aligned then.

—Günther

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote:
> On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote:
> > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:

> > > Open questions
> > > ~~~~~~~~~~~~~~
> > > 
> > > This is unlikely to be the last iteration, but we are getting closer.
> > > 
> > > Some notable open questions are:
> > > 
> > >  * Code style
> > >  
> > >    * Should we move the IOCTL access right expansion logic into the
> > >      outer layers in syscall.c?  Where it currently lives in
> > >      ruleset.h, this logic feels too FS-specific, and it introduces
> > >      the additional complication that we now have to track which
> > >      access_mask_t-s are already expanded and which are not.  It might
> > >      be simpler to do the expansion earlier.
> > 
> > What about creating a new helper in fs.c that expands the FS access
> > rights, something like this:
> > 
> > int landlock_expand_fs_access(access_mask_t *access_mask)
> > {
> > 	if (!*access_mask)
> > 		return -ENOMSG;
> > 
> > 	*access_mask = expand_all_ioctl(*access_mask, *access_mask);
> > 	return 0;
> > }
> > 
> > 
> > And in syscalls.c:
> > 
> > 	err =
> > 		landlock_expand_fs_access(&ruleset_attr.handled_access_fs);
> > 	if (err)
> > 		return err;
> > 
> > 	/* Checks arguments and transforms to kernel struct. */
> > 	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> > 					  ruleset_attr.handled_access_net);
> 
> Done, this looks good.
> 
> I called the landlock_expand_fs_access function slightly differently and made it
> return the resulting access_mask_t (because it does not make a performance
> difference, and then there is no potential for calling it with a null pointer,
> and the function does not need to return an error).
> 
> As a consequence of doing it like this, I also moved the expansion functions
> into fs.c, away from ruleset.h where they did not fit in. :)
> 
> 
> > And patch the landlock_create_ruleset() helper with that:
> > 
> > -	if (!fs_access_mask && !net_access_mask)
> > +	if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
> > 		return ERR_PTR(-ENOMSG);
> 
> Why would you want to warn on the case where fs_access_mask is zero?

Because in my suggestion the real check is moved/copied to
landlock_expand_fs_access(), which is called before, and it should then
not be possible to have this case here.

> 
> Is it not a legitimate use case to use Landlock for the network aspect only?
> 
> (If a user is not handling any of the LANDLOCK_ACCESS_FS* rights, the expansion
> step is not going to add any.)

Correct

> 
> 
> > >    * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names.
> > 
> > Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that
> > these are in fact (synthetic) access rights?
> > 
> > I'm not sure we can find better than GROUP because even the content of
> > these groups might change in the future with new access rights.
> 
> Makes sense, renamed as suggested.  TBH, IOCTL_CMD_G1...4 was more of a
> placeholder anyway because I was so lazy with my typing. ;)
> 
> 
> > >  * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
> > >    should this grant the permission to use *any* IOCTL?  (Right now,
> > >    it is any IOCTL except for the ones covered by the IOCTL groups,
> > >    and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
> > >    becomes smaller when other access rights are also handled.
> > 
> > Are you suggesting to handle differently this right if it is applied to
> > a directory?
> 
> No - this applies to files as well.  I am suggesting that granting
> LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access
> to *all* ioctls, both the ones in the synthetic groups and the remaining ones.
> 
> Let me spell out the scenario:
> 
> Steps to reproduce:
>   - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE
>   - permit: LANDLOCK_ACCESS_FS_IOCTL
>             on file f
>   - open file f (for write-only)
>   - attempt to use ioctl(fd, FIOQSIZE, ...)
> 
> With this patch set:
>   - ioctl(fd, FIOQSIZE, ...) fails,
>     because FIOQSIZE is part of IOCTL_CMD_G1
>     and because LANDLOCK_ACCESS_FS_READ_FILE is handled,
>     IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE

Correct, and it looks consistent to me.

> 
> Alternative proposal:
>   - ioctl(fd, FIOQSIZE, ...) should maybe work,
>     because LANDLOCK_ACCESS_FS_IOCTL is permitted on f
> 
>     Implementation-wise, this would mean to add
> 
>     expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups)
> 
>     to expand_all_ioctl().
> 
> I feel that this alternative might be less surprising, because granting the
> IOCTL right would grant all the things that were restricted when handling the
> IOCTL right, and it would be more "symmetric".
> 
> What do you think?

I though that we discussed about that and we agree that it was the way
to go. Cf. the table of handled/allowed/not-allowed.

Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case
of a directory but not a file? These would be two different semantics.

> 
> 
> > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > be OK. But maybe we should rename this right to something like
> > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > IOCTLs that are not handled by other access rights?
> 
> Hmm, I'm not convinced this is a good name.  It makes sense in the context of
> allowing "all the other ioctls" for a file or file hierarchy, but when setting
> LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> so "default" doesn't seem appropriate to me.

It should turn off all IOCTLs that are not handled by another access
right.  The handled access rights should be expanded the same way as the
allowed access rights.

> 
> 
> > >  * Backwards compatibility for user-space libraries.
> > > 
> > >    This is not documented yet, because it is currently not necessary
> > >    yet.  But as soon as we have a hypothetical Landlock ABI v6 with a
> > >    new IOCTL-enabled "GFX" access right, the "best effort" downgrade
> > >    from v6 to v5 becomes more involved: If the caller handles
> > >    GFX+IOCTL and permits GFX on a file, the correct downgrade to make
> > >    this work on a Landlock v5 kernel is to handle IOCTL only, and
> > >    permit IOCTL(!).
> > 
> > I don't see any issue to this approach. If there is no way to handle GFX
> > in v5, then there is nothing more we can do than allowing GFX (on the
> > same file). Another way to say it is that in v5 we allow any IOCTL
> > (including GFX ones) on the GFX files, an in v6 we *need* replace this
> > IOCTL right with the newly available GFX right, *if it is handled* by
> > the ruleset.
> > 
> > If GFX would not be tied to a file, I think it would not be a good
> > design for this access right. Currently all access rights are tied to
> > objects/data, or relative to the sandbox (e.g. ptrace).
> 
> Yes, makes sense - we are aligned then.
> 
> —Günther

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Günther Noack]

On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote:
> On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote:
> > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote:
> > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:
> > > And patch the landlock_create_ruleset() helper with that:
> > > 
> > > -	if (!fs_access_mask && !net_access_mask)
> > > +	if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
> > > 		return ERR_PTR(-ENOMSG);
> > 
> > Why would you want to warn on the case where fs_access_mask is zero?
> 
> Because in my suggestion the real check is moved/copied to
> landlock_expand_fs_access(), which is called before, and it should then
> not be possible to have this case here.

Oh, I see, I misread that code.  I guess it does not apply to the version that
we ended up with.


> > > >  * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
> > > >    should this grant the permission to use *any* IOCTL?  (Right now,
> > > >    it is any IOCTL except for the ones covered by the IOCTL groups,
> > > >    and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
> > > >    becomes smaller when other access rights are also handled.
> > > 
> > > Are you suggesting to handle differently this right if it is applied to
> > > a directory?
> > 
> > No - this applies to files as well.  I am suggesting that granting
> > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access
> > to *all* ioctls, both the ones in the synthetic groups and the remaining ones.
> > 
> > Let me spell out the scenario:
> > 
> > Steps to reproduce:
> >   - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE
> >   - permit: LANDLOCK_ACCESS_FS_IOCTL
> >             on file f
> >   - open file f (for write-only)
> >   - attempt to use ioctl(fd, FIOQSIZE, ...)
> > 
> > With this patch set:
> >   - ioctl(fd, FIOQSIZE, ...) fails,
> >     because FIOQSIZE is part of IOCTL_CMD_G1
> >     and because LANDLOCK_ACCESS_FS_READ_FILE is handled,
> >     IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE
> 
> Correct, and it looks consistent to me.
> 
> > 
> > Alternative proposal:
> >   - ioctl(fd, FIOQSIZE, ...) should maybe work,
> >     because LANDLOCK_ACCESS_FS_IOCTL is permitted on f
> > 
> >     Implementation-wise, this would mean to add
> > 
> >     expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups)
> > 
> >     to expand_all_ioctl().
> > 
> > I feel that this alternative might be less surprising, because granting the
> > IOCTL right would grant all the things that were restricted when handling the
> > IOCTL right, and it would be more "symmetric".
> > 
> > What do you think?
> 
> I though that we discussed about that and we agree that it was the way
> to go. Cf. the table of handled/allowed/not-allowed.

We can go with the current implementation as well, I don't feel very strongly
about it.


> Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case
> of a directory but not a file? These would be two different semantics.

If the ruleset were enforced in that proposal, as in the example above, it would
not distinguish whether the affected filesystem paths are files or directories.

If LANDLOCK_ACCESS_FS_IOCTL is handled, the semantics would be:

  * If you permit LANDLOCK_ACCESS_FS_READ_FILE on a directory or file,
    it would become possible to use these ioctl commands on the affected files
    which are relevant and harmless for reading files.  (As before)

  * If you permit LANDLOCK_ACCESS_FS_IOCTL on a directory or file,
    it would become possible to use *all* ioctl commands on the affected files.

    (That is the difference.  In the current implementation, this only affects
     the ioctl commands which are *not* in the synthetic groups.  In the
     alternative proposal, it would affect *all* ioctl commands.

     I think this might be simpler to reason about, because the set of ioctl
     commands which are affected by permitting(!) LANDLOCK_ACCESS_FS_IOCTL would
     always be the same (namely, all ioctl commands), and it would not be
     dependent on whether other access rights are handled.)


I don't think it is at odds with the backwards-compatibility concerns which we
previously discussed.  The synthetic groups still exist, it's just the
"permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a
different set of IOCTL commands.


> > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > > be OK. But maybe we should rename this right to something like
> > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > > IOCTLs that are not handled by other access rights?
> > 
> > Hmm, I'm not convinced this is a good name.  It makes sense in the context of
> > allowing "all the other ioctls" for a file or file hierarchy, but when setting
> > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> > so "default" doesn't seem appropriate to me.
> 
> It should turn off all IOCTLs that are not handled by another access
> right.  The handled access rights should be expanded the same way as the
> allowed access rights.

If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files
or directories, all IOCTL commands will be forbidden, independent of what else
is handled.

The opposite is not true:

If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle
LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work.

So if you see it through that lens, you could say that it is only the
LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL
commands at all.


I hope this makes sense.  It's not my intent to open this
backwards-compatibility can of worms from scratch... :)

—Günther

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote:
> On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote:
> > On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote:
> > > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote:
> > > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:
> > > > And patch the landlock_create_ruleset() helper with that:
> > > > 
> > > > -	if (!fs_access_mask && !net_access_mask)
> > > > +	if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
> > > > 		return ERR_PTR(-ENOMSG);
> > > 
> > > Why would you want to warn on the case where fs_access_mask is zero?
> > 
> > Because in my suggestion the real check is moved/copied to
> > landlock_expand_fs_access(), which is called before, and it should then
> > not be possible to have this case here.
> 
> Oh, I see, I misread that code.  I guess it does not apply to the version that
> we ended up with.
> 
> 
> > > > >  * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
> > > > >    should this grant the permission to use *any* IOCTL?  (Right now,
> > > > >    it is any IOCTL except for the ones covered by the IOCTL groups,
> > > > >    and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
> > > > >    becomes smaller when other access rights are also handled.
> > > > 
> > > > Are you suggesting to handle differently this right if it is applied to
> > > > a directory?
> > > 
> > > No - this applies to files as well.  I am suggesting that granting
> > > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access
> > > to *all* ioctls, both the ones in the synthetic groups and the remaining ones.
> > > 
> > > Let me spell out the scenario:
> > > 
> > > Steps to reproduce:
> > >   - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE
> > >   - permit: LANDLOCK_ACCESS_FS_IOCTL
> > >             on file f
> > >   - open file f (for write-only)
> > >   - attempt to use ioctl(fd, FIOQSIZE, ...)
> > > 
> > > With this patch set:
> > >   - ioctl(fd, FIOQSIZE, ...) fails,
> > >     because FIOQSIZE is part of IOCTL_CMD_G1
> > >     and because LANDLOCK_ACCESS_FS_READ_FILE is handled,
> > >     IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE
> > 
> > Correct, and it looks consistent to me.
> > 
> > > 
> > > Alternative proposal:
> > >   - ioctl(fd, FIOQSIZE, ...) should maybe work,
> > >     because LANDLOCK_ACCESS_FS_IOCTL is permitted on f
> > > 
> > >     Implementation-wise, this would mean to add
> > > 
> > >     expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups)
> > > 
> > >     to expand_all_ioctl().
> > > 
> > > I feel that this alternative might be less surprising, because granting the
> > > IOCTL right would grant all the things that were restricted when handling the
> > > IOCTL right, and it would be more "symmetric".
> > > 
> > > What do you think?
> > 
> > I though that we discussed about that and we agree that it was the way
> > to go. Cf. the table of handled/allowed/not-allowed.
> 
> We can go with the current implementation as well, I don't feel very strongly
> about it.
> 
> 
> > Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case
> > of a directory but not a file? These would be two different semantics.
> 
> If the ruleset were enforced in that proposal, as in the example above, it would
> not distinguish whether the affected filesystem paths are files or directories.
> 
> If LANDLOCK_ACCESS_FS_IOCTL is handled, the semantics would be:
> 
>   * If you permit LANDLOCK_ACCESS_FS_READ_FILE on a directory or file,
>     it would become possible to use these ioctl commands on the affected files
>     which are relevant and harmless for reading files.  (As before)
> 
>   * If you permit LANDLOCK_ACCESS_FS_IOCTL on a directory or file,
>     it would become possible to use *all* ioctl commands on the affected files.
> 
>     (That is the difference.  In the current implementation, this only affects
>      the ioctl commands which are *not* in the synthetic groups.  In the
>      alternative proposal, it would affect *all* ioctl commands.
> 
>      I think this might be simpler to reason about, because the set of ioctl
>      commands which are affected by permitting(!) LANDLOCK_ACCESS_FS_IOCTL would
>      always be the same (namely, all ioctl commands), and it would not be
>      dependent on whether other access rights are handled.)
> 
> 
> I don't think it is at odds with the backwards-compatibility concerns which we
> previously discussed.  The synthetic groups still exist, it's just the
> "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a
> different set of IOCTL commands.

It would not be a backward-compatibility issue, but it would make
LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access
rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with
new handled access rights should better inform end encourage developers
to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed.
It would be useful to explain this rationale in the commit message.
See https://lore.kernel.org/all/20231020.moefooYeV9ei@digikod.net/

> 
> 
> > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > > > be OK. But maybe we should rename this right to something like
> > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > > > IOCTLs that are not handled by other access rights?
> > > 
> > > Hmm, I'm not convinced this is a good name.  It makes sense in the context of
> > > allowing "all the other ioctls" for a file or file hierarchy, but when setting
> > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> > > so "default" doesn't seem appropriate to me.
> > 
> > It should turn off all IOCTLs that are not handled by another access
> > right.  The handled access rights should be expanded the same way as the
> > allowed access rights.
> 
> If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files
> or directories, all IOCTL commands will be forbidden, independent of what else
> is handled.
> 
> The opposite is not true:
> 
> If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle
> LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work.
> 
> So if you see it through that lens, you could say that it is only the
> LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL
> commands at all.

Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by
default. However, to allow IOCTLs, developers may need to allow
LANDLOCK_ACCESS_FS_IOCTL or another access right according to the
underlying semantic.

It would be useful to add an example with your table in the
documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled
or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file
or not...).

> 
> 
> I hope this makes sense.  It's not my intent to open this
> backwards-compatibility can of worms from scratch... :)
> 
> —Günther
> 

Re: [PATCH v4 0/7] Landlock: IOCTL support

[Günther Noack]

On Thu, Nov 30, 2023 at 10:26:47AM +0100, Mickaël Salaün wrote:
> On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote:
> > On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote:
> > I don't think it is at odds with the backwards-compatibility concerns which we
> > previously discussed.  The synthetic groups still exist, it's just the
> > "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a
> > different set of IOCTL commands.
> 
> It would not be a backward-compatibility issue, but it would make
> LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access
> rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with
> new handled access rights should better inform end encourage developers
> to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed.
> It would be useful to explain this rationale in the commit message.
> See https://lore.kernel.org/all/20231020.moefooYeV9ei@digikod.net/

Done; I added a section to the commit message.


> > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > > > > be OK. But maybe we should rename this right to something like
> > > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > > > > IOCTLs that are not handled by other access rights?
> > > > 
> > > > Hmm, I'm not convinced this is a good name.  It makes sense in the context of
> > > > allowing "all the other ioctls" for a file or file hierarchy, but when setting
> > > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> > > > so "default" doesn't seem appropriate to me.
> > > 
> > > It should turn off all IOCTLs that are not handled by another access
> > > right.  The handled access rights should be expanded the same way as the
> > > allowed access rights.
> > 
> > If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files
> > or directories, all IOCTL commands will be forbidden, independent of what else
> > is handled.
> > 
> > The opposite is not true:
> > 
> > If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle
> > LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work.
> > 
> > So if you see it through that lens, you could say that it is only the
> > LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL
> > commands at all.
> 
> Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by
> default. However, to allow IOCTLs, developers may need to allow
> LANDLOCK_ACCESS_FS_IOCTL or another access right according to the
> underlying semantic.
> 
> It would be useful to add an example with your table in the
> documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled
> or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file
> or not...).

Added an example to the documentation.

—Günther