[PATCH v10 0/9] Landlock: IOCTL support

[PATCH v10 0/9] Landlock: IOCTL support

[Günther Noack]

Hello!

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

Objective
~~~~~~~~~

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

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

Introduce the LANDLOCK_ACCESS_FS_IOCTL_DEV right, which restricts the
use of ioctl(2) on block and character devices.

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

If LANDLOCK_ACCESS_FS_IOCTL_DEV is handled (restricted in the
ruleset), the LANDLOCK_ACCESS_FS_IOCTL_DEV right governs the use of
all device-specific IOCTL commands.  We make exceptions for common and
known-harmless IOCTL commands such as FIOCLEX, FIONCLEX, FIONBIO and
FIOASYNC, as well as other IOCTL commands for regular files, which are
implemented in fs/ioctl.c.  A full list of these IOCTL commands is
listed in the documentation.

I believe that this approach works for the majority of use cases, and
offers a good trade-off between complexity of the Landlock API and
implementation 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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* A processes' existing open file descriptors stay unaffected
  when a process 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 for
  non-device files.  Example: Network sockets, memfd_create(2).

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_DEV 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.)

Unaffected IOCTL commands
~~~~~~~~~~~~~~~~~~~~~~~~~

To decide which IOCTL commands should be blanket-permitted, we went
through the list of IOCTL commands which are handled directly in
fs/ioctl.c and looked at them individually to understand what they are
about.

The following commands are permitted by Landlock unconditionally:

 * FIOCLEX, FIONCLEX - these work on the file descriptor and
   manipulate the close-on-exec flag (also available through
   fcntl(2) with F_SETFD)
 * FIONBIO, FIOASYNC - these work on the struct file and enable
   nonblocking-IO and async flags (also available through
   fcntl(2) with F_SETFL)

The following commands are also technically permitted by Landlock
unconditionally, but are not supported by device files.  By permitting
them in Landlock on device files, we naturally return the normal error
code.

 * FIOQSIZE - get the size of the opened file or directory
 * FIBMAP - get the file system block numbers underlying a file
 * 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 technically permitted by Landlock, but
they are really operating on the file system's superblock, rather than
on the file itself:

 * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file
   system. Requires CAP_SYS_ADMIN.
 * FIGETBSZ - get file system blocksize

The following commands are technically permitted by Landlock:

 * FS_IOC_FIEMAP - get information about file extent mapping
   (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt)
 * FIDEDUPERANGE, FICLONE, FICLONERANGE - manipulating shared physical storage
   between multiple files.  These only work on some COW file systems, by design.
 * Accessing file attributes:
   * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2))
   * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes

Notably, the command FIONREAD is *not* blanket-permitted,
because it would be a device-specific implementation.


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 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.4/pledge.2


Open Questions
~~~~~~~~~~~~~~

 * Is it OK to introduce the LSM file_vfs_ioctl hook?

 * We may need to revise the tests that we added in V9 and before,
   so of them probably don't make sense any more.


Changes
~~~~~~~

V10:
 * Major change: only restrict IOCTL invocations on device files
   * Rename access right to LANDLOCK_ACCESS_FS_IOCTL_DEV
   * Remove the notion of synthetic access rights and IOCTL right groups
 * Introduce a new LSM hook file_vfs_ioctl, which gets invoked just
   before the call to f_ops->unlocked_ioctl()
 * Documentation
   * Various complications were removed or simplified:
     * Suggestion to mount file systems as nodev is not needed any more,
       as Landlock already lets users distinguish device files.
     * Remarks about fscrypt were removed.  The fscrypt-related IOCTLs only
       applied to regular files and directories, so this patch does not affect
       them any more.
     * Various documentation of the IOCTL grouping approach was removed,
       as it's not needed any more.

V9:
 * in “landlock: Add IOCTL access right”:
   * Change IOCTL group names and grouping as discussed with Mickaël.
     This makes the grouping coarser, and we occasionally rely on the
     underlying implementation to perform the appropriate read/write
     checks.
     * Group IOCTL_RW (one of READ_FILE, WRITE_FILE or READ_DIR):
       FIONREAD, FIOQSIZE, FIGETBSZ
     * Group IOCTL_RWF (one of READ_FILE or WRITE_FILE):
       FS_IOC_FIEMAP, FIBMAP, FIDEDUPERANGE, FICLONE, FICLONERANGE,
       FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64,
       FS_IOC_ZERO_RANGE
   * Excempt pipe file descriptors from IOCTL restrictions,
     even for named pipes which are opened from the file system.
     This is to be consistent with anonymous pipes created with pipe(2).
     As discussed in https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com
   * Document rationale for the IOCTL grouping in the code
   * Use __attribute_const__
   * Rename required_ioctl_access() to get_required_ioctl_access()
 * Selftests
   * Simplify IOCTL test fixtures as a result of simpler grouping.
   * Test that IOCTLs are permitted on named pipe FDs.
   * Test that IOCTLs are permitted on named Unix Domain Socket FDs.
   * Work around compilation issue with old GCC / glibc.
     https://sourceware.org/glibc/wiki/Synchronizing_Headers
     Thanks to Huyadi <hu.yadi@h3c.com>, who pointed this out in
     https://lore.kernel.org/all/f25be6663bcc4608adf630509f045a76@h3c.com/
     and Mickaël, who fixed it through #include reordering.
 * Documentation changes
   * Reword "IOCTL commands" section a bit
   * s/permit/allow/
   * s/access right/right/, if preceded by LANDLOCK_ACCESS_FS_*
   * s/IOCTL/FS_IOCTL/ in ASCII table
   * Update IOCTL grouping documentation in header file
 * Removed a few of the earlier commits in this patch set,
   which have already been merged.

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

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

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

V5:
 * Implementation:
   * move IOCTL group expansion logic into fs.c (implementation suggested by
     mic)
   * rename IOCTL_CMD_G* constants to LANDLOCK_ACCESS_FS_IOCTL_GROUP*
   * fs.c: create ioctl_groups constant
   * add "const" to some variables
 * Formatting and docstring fixes (including wrong kernel-doc format)
 * samples/landlock: fix ABI version and fallback attribute (mic)
 * Documentation
   * move header documentation changes into the implementation commit
   * spell out how FIFREEZE, FITHAW and attribute-manipulation ioctls from
     fs/ioctl.c are handled
   * change ABI 4 to ABI 5 in some missing places

V4:
 * use "synthetic" IOCTL access rights, as previously discussed
 * testing changes
   * use a large fixture-based test, for more exhaustive coverage,
     and replace some of the earlier tests with it
 * rebased on mic-next

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

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

---

V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/
V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/
V3: https://lore.kernel.org/linux-security-module/20230814172816.3907299-1-gnoack@google.com/
V4: https://lore.kernel.org/linux-security-module/20231103155717.78042-1-gnoack@google.com/
V5: https://lore.kernel.org/linux-security-module/20231117154920.1706371-1-gnoack@google.com/
V6: https://lore.kernel.org/linux-security-module/20231124173026.3257122-1-gnoack@google.com/
V7: https://lore.kernel.org/linux-security-module/20231201143042.3276833-1-gnoack@google.com/
V8: https://lore.kernel.org/linux-security-module/20231208155121.1943775-1-gnoack@google.com/
V9: https://lore.kernel.org/linux-security-module/20240209170612.1638517-1-gnoack@google.com/

Günther Noack (9):
  security: Create security_file_vfs_ioctl hook
  landlock: Add IOCTL access right for character and block devices
  selftests/landlock: Test IOCTL support
  selftests/landlock: Test IOCTL with memfds
  selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH)
  selftests/landlock: Test IOCTLs on named pipes
  selftests/landlock: Check IOCTL restrictions for named UNIX domain
    sockets
  samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV
  landlock: Document IOCTL support

 Documentation/userspace-api/landlock.rst     |  76 +++-
 fs/ioctl.c                                   |  14 +-
 include/linux/lsm_hook_defs.h                |   2 +
 include/linux/security.h                     |   8 +
 include/uapi/linux/landlock.h                |  35 +-
 samples/landlock/sandboxer.c                 |  13 +-
 security/landlock/fs.c                       |  38 +-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   8 +-
 security/security.c                          |  22 +
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   | 407 ++++++++++++++++++-
 12 files changed, 581 insertions(+), 46 deletions(-)


base-commit: d8482176c8c319b11d683913f780d63b44257d0f
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

[Günther Noack]

This LSM hook gets called just before the fs/ioctl.c logic delegates
the requested IOCTL command to the file-specific implementation as
implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat).

It is impractical for LSMs to make security guarantees about these
f_op operations without having intimate knowledge of how they are
implemented.

Therefore, depending on the enabled Landlock policy, Landlock aims to
block the calls to filp->f_op->unlocked_ioctl(), but permit the calls
to the IOCTL commands which are already implemented in fs/ioctl.c.

The current call graph is:

  * ioctl syscall
    * security_file_ioctl() LSM hook
    * do_vfs_ioctl() - standard operations
      * file_ioctl() - standard file operations
    * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op)
      * filp->f_op->unlocked_ioctl()

Why not use the existing security_file_ioctl() hook?

With the existing security_file_ioctl() hook, it is technically
feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it
would be difficult to maintain: security_file_ioctl() gets called
further up the call stack, so an implementation of it would need to
predict whether the logic further below will decide to call
f_op->unlocked_ioctl().  That can only be done by mirroring the logic
in do_vfs_ioctl() to some extent, and keeping this in sync.

We therefore think that it is cleaner to add a new LSM hook, which
gets called closer to the security boundary which we actually want to
block, filp->f_op->unlocked_ioctl().

Why is it difficult to reason about f_op->unlocked_ioctl()?

The unlocked_ioctl() and ioctl_compat() operations generally do the
following things:

 1. Execute code depending on the IOCTL command number,
    to implement the IOCTL command.

 2. Execute code independent(!) of the IOCTL command number,
    usually to implement common locking and resource allocation
    behavior.

    Notably, this often happens before(!) the implementation looks
    at the command number.

Due to the number of device drivers in Linux, it is infeasible for LSM
maintainers to keep track of what these implementations do in detail.

Due to 2., even permitting selected IOCTL command numbers to be
implemented by devices would probably be a mistake, because even when
a device does not implement a given IOCTL command, it might still
execute code when you try to call it.

Cc: Paul Moore <paul@paul-moore.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 fs/ioctl.c                    | 14 ++++++++++++--
 include/linux/lsm_hook_defs.h |  2 ++
 include/linux/security.h      |  8 ++++++++
 security/security.c           | 22 ++++++++++++++++++++++
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..e0a8ce300dcd 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -43,10 +43,16 @@
  */
 long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	int error = -ENOTTY;
+	int error;
+
+	error = security_file_vfs_ioctl(filp, cmd, arg);
+	if (error)
+		goto out;
 
-	if (!filp->f_op->unlocked_ioctl)
+	if (!filp->f_op->unlocked_ioctl) {
+		error = -ENOTTY;
 		goto out;
+	}
 
 	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
 	if (error == -ENOIOCTLCMD)
@@ -967,6 +973,10 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 		if (error != -ENOIOCTLCMD)
 			break;
 
+		error = security_file_vfs_ioctl(f.file, cmd, arg);
+		if (error != 0)
+			break;
+
 		if (f.file->f_op->compat_ioctl)
 			error = f.file->f_op->compat_ioctl(f.file, cmd, arg);
 		if (error == -ENOIOCTLCMD)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..d8a7c49b7eef 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -173,6 +173,8 @@ LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
 	 unsigned long arg)
 LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd,
 	 unsigned long arg)
+LSM_HOOK(int, 0, file_vfs_ioctl, struct file *file, unsigned int cmd,
+	 unsigned long arg)
 LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
 LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
 	 unsigned long prot, unsigned long flags)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..05a2e1852f66 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -396,6 +396,8 @@ void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int security_file_ioctl_compat(struct file *file, unsigned int cmd,
 			       unsigned long arg);
+int security_file_vfs_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg);
 int security_mmap_file(struct file *file, unsigned long prot,
 			unsigned long flags);
 int security_mmap_addr(unsigned long addr);
@@ -1011,6 +1013,12 @@ static inline int security_file_ioctl_compat(struct file *file,
 	return 0;
 }
 
+static inline int security_file_vfs_ioctl(struct file *file, unsigned int cmd,
+					  unsigned long arg)
+{
+	return 0;
+}
+
 static inline int security_mmap_file(struct file *file, unsigned long prot,
 				     unsigned long flags)
 {
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..15c635cd8366 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2745,6 +2745,28 @@ int security_file_ioctl_compat(struct file *file, unsigned int cmd,
 }
 EXPORT_SYMBOL_GPL(security_file_ioctl_compat);
 
+/**
+ * security_file_vfs_ioctl() - Check if a ioctl implemented by the file is allowed
+ * @file: associated file
+ * @cmd: ioctl cmd
+ * @arg: ioctl arguments
+ *
+ * Check permission for an ioctl operation on @file.  Note that @arg sometimes
+ * represents a user space pointer; in other cases, it may be a simple integer
+ * value.  When @arg represents a user space pointer, it should never be used
+ * by the security module.
+ *
+ * This hook is checked just before calling f_op->unlocked_ioctl() or
+ * f_op->compat_ioctl().
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_file_vfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return call_int_hook(file_vfs_ioctl, 0, file, cmd, arg);
+}
+EXPORT_SYMBOL_GPL(security_file_vfs_ioctl);
+
 static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
 {
 	/*
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 2/9] landlock: Add IOCTL access right for character and block devices

[Günther Noack]

Introduces the LANDLOCK_ACCESS_FS_IOCTL_DEV right
and increments the Landlock ABI version to 5.

This access right applies to device-custom IOCTL commands
when they are invoked on block or character device files.

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

Therefore, a newly enabled Landlock policy does not apply to file
descriptors which are already open.

If the LANDLOCK_ACCESS_FS_IOCTL_DEV right is handled, only a small
number of safe IOCTL commands will be permitted on newly opened device
files.  These include FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC, as well
as other IOCTL commands for regular files which are implemented in
fs/ioctl.c.

Noteworthy scenarios which require special attention:

TTY devices are often passed into a process from the parent process,
and so a newly enabled Landlock policy does not retroactively apply to
them automatically.  In the past, TTY devices have often supported
IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
letting callers control the TTY input buffer (and simulate
keypresses).  This should be restricted to CAP_SYS_ADMIN programs on
modern kernels though.

Known limitations:

The LANDLOCK_ACCESS_FS_IOCTL_DEV access right is a coarse-grained
control over IOCTL commands.

Landlock users may use path-based restrictions in combination with
their knowledge about the file system layout to control what IOCTLs
can be done.

Cc: Paul Moore <paul@paul-moore.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 include/uapi/linux/landlock.h                | 35 +++++++++++++-----
 security/landlock/fs.c                       | 38 ++++++++++++++++++--
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  8 +++--
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   |  5 +--
 6 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677539..193733d833b1 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,30 @@ 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_DEV: Invoke :manpage:`ioctl(2)` commands on an opened
+ *   character or block device.
+ *
+ *   This access right applies to all `ioctl(2)` commands implemented by device
+ *   drivers.  However, the following common IOCTL commands continue to be
+ *   invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL_DEV right:
+ *
+ *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC``, ``FIOQSIZE``,
+ *   ``FIFREEZE``, ``FITHAW``, ``FS_IOC_FIEMAP``, ``FIGETBSZ``, ``FICLONE``,
+ *   ``FICLONERANGE``, ``FIDEDUPERANGE``, ``FS_IOC_GETFLAGS``,
+ *   ``FS_IOC_SETFLAGS``, ``FS_IOC_FSGETXATTR``, ``FS_IOC_FSSETXATTR``
+ *
+ *   This access right is available since the fifth version of the Landlock
+ *   ABI.
+ *
  * .. warning::
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
  *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
  *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -223,6 +241,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_DEV			(1ULL << 15)
 /* clang-format on */
 
 /**
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 6f0bf1434a2c..bfa69ea94cf8 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -148,7 +148,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 /* clang-format on */
 
 /*
@@ -1332,8 +1333,10 @@ static int hook_file_alloc_security(struct file *const file)
 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;
+	access_mask_t open_access_request, full_access_request, allowed_access,
+		optional_access;
+	const struct inode *inode = file_inode(file);
+	const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
 	const struct landlock_ruleset *const dom = get_current_fs_domain();
 
 	if (!dom)
@@ -1350,6 +1353,10 @@ static int hook_file_open(struct file *const file)
 	 * We look up more access than what we immediately need for open(), so
 	 * that we can later authorize operations on opened files.
 	 */
+	optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
+	if (is_device)
+		optional_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
 	full_access_request = open_access_request | optional_access;
 
 	if (is_access_to_paths_allowed(
@@ -1406,6 +1413,30 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+static int hook_file_vfs_ioctl(struct file *file, unsigned int cmd,
+			       unsigned long arg)
+{
+	const struct inode *inode = file_inode(file);
+	const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
+	access_mask_t required_access, allowed_access;
+
+	if (!is_device)
+		return 0;
+
+	/*
+	 * It is the access rights at the time of opening the file which
+	 * determine whether IOCTL can be used on the opened file later.
+	 *
+	 * The access right is attached to the opened file in hook_file_open().
+	 */
+	required_access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
+	allowed_access = landlock_file(file)->allowed_access;
+	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),
 
@@ -1428,6 +1459,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_vfs_ioctl, hook_file_vfs_ioctl),
 };
 
 __init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 93c9c6f91556..20fdb5ff3514 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
 #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/syscalls.c b/security/landlock/syscalls.c
index 6788e73b6681..9ae3dfa47443 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -149,7 +149,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
@@ -321,7 +321,11 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
 	if (!path_beneath_attr.allowed_access)
 		return -ENOMSG;
 
-	/* Checks that allowed_access matches the @ruleset constraints. */
+	/*
+	 * Checks that allowed_access matches the @ruleset constraints and only
+	 * consists of publicly visible access rights (as opposed to synthetic
+	 * ones).
+	 */
 	mask = landlock_get_raw_fs_access_mask(ruleset, 0);
 	if ((path_beneath_attr.allowed_access | mask) != mask)
 		return -EINVAL;
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 646f778dfb1e..d292b419ccba 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 2d6d9b43d958..0bcbbf594fd7 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -527,9 +527,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_DEV)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 3/9] selftests/landlock: Test IOCTL support

[Günther Noack]

Exercises Landlock's IOCTL feature in different combinations of
handling and permitting the LANDLOCK_ACCESS_FS_IOCTL_DEV right, 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 | 236 ++++++++++++++++++++-
 1 file changed, 233 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 0bcbbf594fd7..91567e5db0fd 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -8,6 +8,7 @@
  */
 
 #define _GNU_SOURCE
+#include <asm/termbits.h>
 #include <fcntl.h>
 #include <linux/landlock.h>
 #include <linux/magic.h>
@@ -15,6 +16,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <sys/capability.h>
+#include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/sendfile.h>
@@ -23,6 +25,12 @@
 #include <sys/vfs.h>
 #include <unistd.h>
 
+/*
+ * Intentionally included last to work around header conflict.
+ * See https://sourceware.org/glibc/wiki/Synchronizing_Headers.
+ */
+#include <linux/fs.h>
+
 #include "common.h"
 
 #ifndef renameat2
@@ -735,6 +743,9 @@ static int create_ruleset(struct __test_metadata *const _metadata,
 	}
 
 	for (i = 0; rules[i].path; i++) {
+		if (!rules[i].access)
+			continue;
+
 		add_path_beneath(_metadata, ruleset_fd, rules[i].access,
 				 rules[i].path);
 	}
@@ -3443,7 +3454,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);
@@ -3526,7 +3537,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);
@@ -3752,7 +3763,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);
@@ -3829,6 +3840,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;
@@ -3845,6 +3866,215 @@ TEST(memfd_ftruncate)
 	ASSERT_EQ(0, close(fd));
 }
 
+/* clang-format off */
+FIXTURE(ioctl) {};
+
+FIXTURE_SETUP(ioctl) {};
+
+FIXTURE_TEARDOWN(ioctl) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(ioctl)
+{
+	const __u64 handled;
+	const __u64 allowed;
+	const mode_t open_mode;
+	/*
+	 * TCGETS is used as a characteristic device-specific IOCTL command.
+	 * The logic is the same for other IOCTL commands as well.
+	 */
+	const int expected_tcgets_result; /* terminal device IOCTL */
+	/*
+	 * FIONREAD is implemented in fs/ioctl.c for regular files,
+	 * but we do not blanket-permit it for devices.
+	 */
+	const int expected_fionread_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_none) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.allowed = 0,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = EACCES,
+	.expected_fionread_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, handled_i_allowed_i) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.allowed = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = 0,
+	.expected_fionread_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(ioctl, unhandled) {
+	/* clang-format on */
+	.handled = LANDLOCK_ACCESS_FS_EXECUTE,
+	.allowed = LANDLOCK_ACCESS_FS_EXECUTE,
+	.open_mode = O_RDWR,
+	.expected_tcgets_result = 0,
+	.expected_fionread_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_tcgets_ioctl(int fd)
+{
+	struct termios info;
+
+	if (ioctl(fd, TCGETS, &info) < 0)
+		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;
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_file)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open("/dev/tty", variant->open_mode);
+	ASSERT_LE(0, file_fd);
+
+	/* Checks that IOCTL commands return the expected errors. */
+	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+	EXPECT_EQ(ENOTTY, test_fioqsize_ioctl(file_fd));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
+TEST_F_FORK(ioctl, handle_dir_access_dir)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int dir_fd, ruleset_fd;
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/*
+	 * Ignore variant->open_mode for this test, as we intend to open a
+	 * directory.  If the directory can not be opened, the variant is
+	 * infeasible to test with an opened directory.
+	 */
+	dir_fd = open("/dev", O_RDONLY);
+	if (dir_fd < 0)
+		return;
+
+	/*
+	 * Checks that IOCTL commands return the expected errors.
+	 * We do not use the expected values from the fixture here.
+	 *
+	 * When using IOCTL on a directory, no Landlock restrictions apply.
+	 * TCGETS will fail anyway because it is not invoked on a TTY device.
+	 */
+	EXPECT_EQ(ENOTTY, test_tcgets_ioctl(dir_fd));
+	EXPECT_EQ(0, test_fionread_ioctl(dir_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(dir_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(dir_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(dir_fd, FIOASYNC, &flag));
+	EXPECT_EQ(0, test_fioqsize_ioctl(dir_fd));
+
+	ASSERT_EQ(0, close(dir_fd));
+}
+
+TEST_F_FORK(ioctl, handle_file_access_file)
+{
+	const int flag = 0;
+	const struct rule rules[] = {
+		{
+			.path = "/dev/tty0",
+			.access = variant->allowed,
+		},
+		{},
+	};
+	int file_fd, ruleset_fd;
+
+	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
+		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
+			     "can not be granted on files");
+	}
+
+	/* Enables Landlock. */
+	ruleset_fd = create_ruleset(_metadata, variant->handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	file_fd = open("/dev/tty0", variant->open_mode);
+	ASSERT_LE(0, file_fd)
+	{
+		TH_LOG("Failed to open /dev/tty0: %s", strerror(errno));
+	}
+
+	/* Checks that IOCTL commands return the expected errors. */
+	EXPECT_EQ(variant->expected_tcgets_result, test_tcgets_ioctl(file_fd));
+	EXPECT_EQ(variant->expected_fionread_result,
+		  test_fionread_ioctl(file_fd));
+
+	/* Checks that unrestrictable commands are unrestricted. */
+	EXPECT_EQ(0, ioctl(file_fd, FIOCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONCLEX));
+	EXPECT_EQ(0, ioctl(file_fd, FIONBIO, &flag));
+	EXPECT_EQ(0, ioctl(file_fd, FIOASYNC, &flag));
+	EXPECT_EQ(ENOTTY, test_fioqsize_ioctl(file_fd));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 4/9] selftests/landlock: Test IOCTL with memfds

[Günther Noack]

Because the LANDLOCK_ACCESS_FS_IOCTL_DEV 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 91567e5db0fd..f0e545c428eb 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3850,20 +3850,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.44.0.278.ge034bb2e1d-goog

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

[Günther Noack]

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

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index f0e545c428eb..5c47231a722e 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3884,6 +3884,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) {};
 
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 6/9] selftests/landlock: Test IOCTLs on named pipes

[Günther Noack]

Named pipes should behave like pipes created with pipe(2),
so we don't want to restrict IOCTLs on them.

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 5c47231a722e..d991f44875bc 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
 	ASSERT_EQ(0, close(fd));
 }
 
+static int test_fionread_ioctl(int fd)
+{
+	size_t sz = 0;
+
+	if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
+		return errno;
+	return 0;
+}
+
+/*
+ * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
+ * because they are not character or block devices.
+ */
+TEST_F_FORK(layout1, named_pipe_ioctl)
+{
+	pid_t child_pid;
+	int fd, ruleset_fd;
+	const char *const path = file1_s1d1;
+	const struct landlock_ruleset_attr attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	};
+
+	ASSERT_EQ(0, unlink(path));
+	ASSERT_EQ(0, mkfifo(path, 0600));
+
+	/* 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));
+
+	/* The child process opens the pipe for writing. */
+	child_pid = fork();
+	ASSERT_NE(-1, child_pid);
+	if (child_pid == 0) {
+		fd = open(path, O_WRONLY);
+		close(fd);
+		exit(0);
+	}
+
+	fd = open(path, O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	/* FIONREAD is implemented by pipefifo_fops. */
+	EXPECT_EQ(0, test_fionread_ioctl(fd));
+
+	ASSERT_EQ(0, close(fd));
+	ASSERT_EQ(0, unlink(path));
+
+	ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
+}
+
 /* clang-format off */
 FIXTURE(ioctl) {};
 
@@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd)
 	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;
-}
-
 TEST_F_FORK(ioctl, handle_dir_access_file)
 {
 	const int flag = 0;
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 7/9] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets

[Günther Noack]

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d991f44875bc..941e6f9702b7 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -20,8 +20,10 @@
 #include <sys/mount.h>
 #include <sys/prctl.h>
 #include <sys/sendfile.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
+#include <sys/un.h>
 #include <sys/vfs.h>
 #include <unistd.h>
 
@@ -3976,6 +3978,57 @@ TEST_F_FORK(layout1, named_pipe_ioctl)
 	ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
 }
 
+/* For named UNIX domain sockets, no IOCTL restrictions apply. */
+TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
+{
+	const char *const path = file1_s1d1;
+	int srv_fd, cli_fd, ruleset_fd;
+	socklen_t size;
+	struct sockaddr_un srv_un, cli_un;
+	const struct landlock_ruleset_attr attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	};
+
+	/* Sets up a server */
+	srv_un.sun_family = AF_UNIX;
+	strncpy(srv_un.sun_path, path, sizeof(srv_un.sun_path));
+
+	ASSERT_EQ(0, unlink(path));
+	ASSERT_LE(0, (srv_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
+
+	size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
+	ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
+	ASSERT_EQ(0, listen(srv_fd, 10 /* qlen */));
+
+	/* 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));
+
+	/* Sets up a client connection to it */
+	cli_un.sun_family = AF_UNIX;
+	snprintf(cli_un.sun_path, sizeof(cli_un.sun_path), "%s%ld", path,
+		 (long)getpid());
+
+	ASSERT_LE(0, (cli_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
+
+	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
+	ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
+
+	bzero(&cli_un, sizeof(cli_un));
+	cli_un.sun_family = AF_UNIX;
+	strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
+	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
+
+	ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
+
+	/* FIONREAD and other IOCTLs should not be forbidden. */
+	EXPECT_EQ(0, test_fionread_ioctl(cli_fd));
+
+	ASSERT_EQ(0, close(cli_fd));
+}
+
 /* clang-format off */
 FIXTURE(ioctl) {};
 
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 8/9] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV

[Günther Noack]

Add IOCTL support to the Landlock sample tool.

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

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

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 08596c0ef070..c5228e8c4817 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_DEV)
 
 /* clang-format on */
 
@@ -199,11 +200,12 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
 	LANDLOCK_ACCESS_FS_REFER | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 4
+#define LANDLOCK_ABI_LAST 5
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -317,6 +319,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		ruleset_attr.handled_access_net &=
 			~(LANDLOCK_ACCESS_NET_BIND_TCP |
 			  LANDLOCK_ACCESS_NET_CONNECT_TCP);
+		__attribute__((fallthrough));
+	case 4:
+		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
-- 
2.44.0.278.ge034bb2e1d-goog

[PATCH v10 9/9] landlock: Document IOCTL support

[Günther Noack]

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

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

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 838cc27db232..32391247f19a 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -76,7 +76,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_DEV,
         .handled_access_net =
             LANDLOCK_ACCESS_NET_BIND_TCP |
             LANDLOCK_ACCESS_NET_CONNECT_TCP,
@@ -85,10 +86,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
 
@@ -114,6 +115,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_DEV for ABI < 5 */
+        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -225,6 +230,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,
@@ -318,18 +324,26 @@ 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_DEV`` rights is associated with the newly created
+file descriptor and will be used for subsequent truncation and ioctl attempts
+using :manpage:`ftruncate(2)` and :manpage:`ioctl(2)`.  The behavior is similar
+to opening a file for reading or writing, where permissions are checked during
+:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and
 :manpage:`write(2)` calls.
 
-As a consequence, it is possible to have multiple open file descriptors for the
-same file, where one grants the right to truncate the file and the other does
-not.  It is also possible to pass such file descriptors between processes,
-keeping their Landlock properties, even when these processes do not have an
-enforced Landlock ruleset.
+As a consequence, it is possible that a process has multiple open file
+descriptors referring to the same file, but Landlock enforces different things
+when operating with these file descriptors.  This can happen when a Landlock
+ruleset gets enforced and the process keeps file descriptors which were opened
+both before and after the enforcement.  It is also possible to pass such file
+descriptors between processes, keeping their Landlock properties, even when some
+of the involved processes do not have an enforced Landlock ruleset.
 
 Compatibility
 =============
@@ -458,6 +472,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_DEV`` right restricts the use of
+:manpage:`ioctl(2)`, but it only applies to *newly opened* device 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.  Both of these require ``CAP_SYS_ADMIN`` on modern Linux systems, but
+the behavior is configurable for ``TIOCSTI``.
+
+On older systems, it is therefore recommended to close inherited TTY file
+descriptors, or to reopen them from ``/proc/self/fd/*`` without the
+``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right, if possible.
+
+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 allowing the
+``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right on files where it is really required.
+
 Previous limitations
 ====================
 
@@ -495,6 +531,16 @@ bind and connect actions to only a set of allowed ports thanks to the new
 ``LANDLOCK_ACCESS_NET_BIND_TCP`` and ``LANDLOCK_ACCESS_NET_CONNECT_TCP``
 access rights.
 
+IOCTL (ABI < 5)
+---------------
+
+IOCTL operations could not be denied before the fifth Landlock ABI, so
+:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an
+earlier ABI.
+
+Starting with the Landlock ABI version 5, it is possible to restrict the use of
+:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` access right.
+
 .. _kernel_support:
 
 Kernel support
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH v10 2/9] landlock: Add IOCTL access right for character and block devices

[Mickaël Salaün]

Adding Alex, Jon and the API ML for interface-related question: file
type check or not?


On Sat, Mar 09, 2024 at 07:53:13AM +0000, Günther Noack wrote:
> Introduces the LANDLOCK_ACCESS_FS_IOCTL_DEV right
> and increments the Landlock ABI version to 5.
> 
> This access right applies to device-custom IOCTL commands
> when they are invoked on block or character device files.
> 
> Like the truncate right, this right is associated with a file
> descriptor at the time of open(2), and gets respected even when the
> file descriptor is used outside of the thread which it was originally
> opened in.
> 
> Therefore, a newly enabled Landlock policy does not apply to file
> descriptors which are already open.
> 
> If the LANDLOCK_ACCESS_FS_IOCTL_DEV right is handled, only a small
> number of safe IOCTL commands will be permitted on newly opened device
> files.  These include FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC, as well
> as other IOCTL commands for regular files which are implemented in
> fs/ioctl.c.
> 
> Noteworthy scenarios which require special attention:
> 
> TTY devices are often passed into a process from the parent process,
> and so a newly enabled Landlock policy does not retroactively apply to
> them automatically.  In the past, TTY devices have often supported
> IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
> letting callers control the TTY input buffer (and simulate
> keypresses).  This should be restricted to CAP_SYS_ADMIN programs on
> modern kernels though.
> 
> Known limitations:
> 
> The LANDLOCK_ACCESS_FS_IOCTL_DEV access right is a coarse-grained
> control over IOCTL commands.
> 
> Landlock users may use path-based restrictions in combination with
> their knowledge about the file system layout to control what IOCTLs
> can be done.

A few minor (or not) nitpicks, but overall I really like this series.

> 
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  include/uapi/linux/landlock.h                | 35 +++++++++++++-----
>  security/landlock/fs.c                       | 38 ++++++++++++++++++--
>  security/landlock/limits.h                   |  2 +-
>  security/landlock/syscalls.c                 |  8 +++--
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  tools/testing/selftests/landlock/fs_test.c   |  5 +--
>  6 files changed, 73 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 25c8d7677539..193733d833b1 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,30 @@ 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_DEV: Invoke :manpage:`ioctl(2)` commands on an opened
> + *   character or block device.
> + *
> + *   This access right applies to all `ioctl(2)` commands implemented by device
> + *   drivers.  However, the following common IOCTL commands continue to be
> + *   invokable independent of the %LANDLOCK_ACCESS_FS_IOCTL_DEV right:
> + *
> + *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC``, ``FIOQSIZE``,
> + *   ``FIFREEZE``, ``FITHAW``, ``FS_IOC_FIEMAP``, ``FIGETBSZ``, ``FICLONE``,
> + *   ``FICLONERANGE``, ``FIDEDUPERANGE``, ``FS_IOC_GETFLAGS``,
> + *   ``FS_IOC_SETFLAGS``, ``FS_IOC_FSGETXATTR``, ``FS_IOC_FSSETXATTR``
> + *
> + *   This access right is available since the fifth version of the Landlock
> + *   ABI.
> + *
>   * .. warning::
>   *
>   *   It is currently not possible to restrict some file-related actions
>   *   accessible through these syscall families: :manpage:`chdir(2)`,
>   *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>   *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> + *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
>   *   Future Landlock evolutions will enable to restrict them.
>   */
>  /* clang-format off */
> @@ -223,6 +241,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_DEV			(1ULL << 15)
>  /* clang-format on */
>  
>  /**
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6f0bf1434a2c..bfa69ea94cf8 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -148,7 +148,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_EXECUTE | \
>  	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>  	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_IOCTL_DEV)

We may want to check the file type to make sure we set the
LANDLOCK_ACCESS_FS_IOCTL_DEV right on char/block devices only, the same
way we already check with d_is_dir() [1].  From user space point of
view, it should not change much because a call to statfs(2) may already
be in place.  From kernel space point of view it would only be a matter
of checking the related inode in landlock_append_fs_rule().

Checking for the file type is not strictly necessarily, but I
implemented the d_is_dir() call and get_path_from_fd() checks to
encourage/force user space to check the file/directory on which it wants
to give access to (e.g. and not erroneously grant access to a whole file
hierarchy rather than a file thanks to statfs(2) information, not the
Landlock syscall itself).  Applications sandboxing themselves should not
be surprise that a file descriptor refers to a directory or a file, and
they should not require additional call to statfs(2).  Another
motivation was that I think this kind of conservative check would have
been difficult to implement later (with an option) because of the
potential user space architectural changes.  Finally, this kind of type
checking can be silently ignored with help from user space libraries
when needed.

About the char/block device check, it might also be a good idea for user
space to check the major/minor numbers to make sure they match
expectations (i.e. related IOCTL commands).

I'm convinced the get_path_from_fd() checks are good because special
files are not restricted (and can then be silently ignored without
impact), whereas a non-special file could still get a valid (super)set
of access rights (and maybe better follow the principle of least
astonishment?).  I'm wondering if checking dir/file was the best
decision, if this is enough, or if we should extend that to char/block
devices.  Any opinion an that?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/security/landlock/fs.c?h=v6.8#n166

>  /* clang-format on */
>  
>  /*
> @@ -1332,8 +1333,10 @@ static int hook_file_alloc_security(struct file *const file)
>  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;
> +	access_mask_t open_access_request, full_access_request, allowed_access,
> +		optional_access;
> +	const struct inode *inode = file_inode(file);
> +	const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
>  	const struct landlock_ruleset *const dom = get_current_fs_domain();
>  
>  	if (!dom)
> @@ -1350,6 +1353,10 @@ static int hook_file_open(struct file *const file)
>  	 * We look up more access than what we immediately need for open(), so
>  	 * that we can later authorize operations on opened files.
>  	 */
> +	optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> +	if (is_device)
> +		optional_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
> +
>  	full_access_request = open_access_request | optional_access;
>  
>  	if (is_access_to_paths_allowed(
> @@ -1406,6 +1413,30 @@ static int hook_file_truncate(struct file *const file)
>  	return -EACCES;
>  }
>  
> +static int hook_file_vfs_ioctl(struct file *file, unsigned int cmd,
> +			       unsigned long arg)
> +{
> +	const struct inode *inode = file_inode(file);
> +	const bool is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
> +	access_mask_t required_access, allowed_access;
> +
> +	if (!is_device)
> +		return 0;

We should first check landlock_file(file)->allowed_access as in
hook_file_truncate() to return as soon as possible for non-sandboxed
tasks.  Any other computation should be done after that (e.g. with an
is_device() helper).

> +
> +	/*
> +	 * 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().
> +	 */
> +	required_access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> +	allowed_access = landlock_file(file)->allowed_access;
> +	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),
>  
> @@ -1428,6 +1459,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_vfs_ioctl, hook_file_vfs_ioctl),
>  };
>  
>  __init void landlock_add_fs_hooks(void)
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 93c9c6f91556..20fdb5ff3514 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>  #define LANDLOCK_MAX_NUM_LAYERS		16
>  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>  
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
>  #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/syscalls.c b/security/landlock/syscalls.c
> index 6788e73b6681..9ae3dfa47443 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -149,7 +149,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
> @@ -321,7 +321,11 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>  	if (!path_beneath_attr.allowed_access)
>  		return -ENOMSG;
>  
> -	/* Checks that allowed_access matches the @ruleset constraints. */
> +	/*
> +	 * Checks that allowed_access matches the @ruleset constraints and only
> +	 * consists of publicly visible access rights (as opposed to synthetic
> +	 * ones).
> +	 */

This change is not needed anymore.

>  	mask = landlock_get_raw_fs_access_mask(ruleset, 0);
>  	if ((path_beneath_attr.allowed_access | mask) != mask)
>  		return -EINVAL;
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 646f778dfb1e..d292b419ccba 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>  	const struct landlock_ruleset_attr ruleset_attr = {
>  		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>  	};
> -	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
>  					     LANDLOCK_CREATE_RULESET_VERSION));
>  
>  	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 2d6d9b43d958..0bcbbf594fd7 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -527,9 +527,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_DEV)
>  
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
>  
>  #define ACCESS_ALL ( \
>  	ACCESS_FILE | \
> -- 
> 2.44.0.278.ge034bb2e1d-goog
> 

Re: [PATCH v10 2/9] landlock: Add IOCTL access right for character and block devices

[Alejandro Colomar]

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

On Mon, Mar 11, 2024 at 03:46:36PM +0100, Mickaël Salaün wrote:
> Adding Alex, Jon and the API ML for interface-related question: file
> type check or not?

Hi Mickaël!

I'm sorry, I don't know.  :)

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

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

Re: [PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

[Paul Moore]

On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@google.com> wrote:
>
> This LSM hook gets called just before the fs/ioctl.c logic delegates
> the requested IOCTL command to the file-specific implementation as
> implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat).
>
> It is impractical for LSMs to make security guarantees about these
> f_op operations without having intimate knowledge of how they are
> implemented.
>
> Therefore, depending on the enabled Landlock policy, Landlock aims to
> block the calls to filp->f_op->unlocked_ioctl(), but permit the calls
> to the IOCTL commands which are already implemented in fs/ioctl.c.
>
> The current call graph is:
>
>   * ioctl syscall
>     * security_file_ioctl() LSM hook
>     * do_vfs_ioctl() - standard operations
>       * file_ioctl() - standard file operations
>     * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op)
>       * filp->f_op->unlocked_ioctl()
>
> Why not use the existing security_file_ioctl() hook?
>
> With the existing security_file_ioctl() hook, it is technically
> feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it
> would be difficult to maintain: security_file_ioctl() gets called
> further up the call stack, so an implementation of it would need to
> predict whether the logic further below will decide to call
> f_op->unlocked_ioctl().  That can only be done by mirroring the logic
> in do_vfs_ioctl() to some extent, and keeping this in sync.

Once again, I don't see this as an impossible task, and I would think
that you would want to inspect each new ioctl command/op added in
do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted
behavior from a Landlock sandbox perspective.  Looking at the git
log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are
added very frequently, meaning that keeping Landlock sync'd with
fs/ioctl.c shouldn't be a terrible task.

I'm also not excited about the overlap between the existing
security_file_ioctl() hook and the proposed security_file_vfs_ioctl()
hook.  There are some cases where we have no choice and we have to
tolerate the overlap, but this doesn't look like one of those cases to
me.

I'm sorry, but I don't agree with this new hook.

-- 
paul-moore.com

[RFC PATCH] fs: Add an use vfs_get_ioctl_handler()

[Mickaël Salaün]

Add a new vfs_get_ioctl_handler() helper to identify if an IOCTL command
is handled by the first IOCTL layer.  Each IOCTL command is now handled
by a dedicated function, and all of them use the same signature.

Apart from the VFS, this helper is also intended to be used by Landlock
to cleanly categorize VFS IOCTLs and create appropriate security
policies.

This is an alternative to a first RFC [1] and a proposal for a new LSM
hook [2].

By dereferencing some pointers only when required, this should also
slightly improve do_vfs_ioctl().

Remove (double) pointer castings on put_user() calls.

Remove potential double vfs_ioctl() call for FIONREAD.

Fix ioctl_file_clone_range() return type from long to int.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Link: https://lore.kernel.org/r/20240219183539.2926165-1-mic@digikod.net [1]
Link: https://lore.kernel.org/r/20240309075320.160128-2-gnoack@google.com [2]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 fs/ioctl.c         | 213 +++++++++++++++++++++++++++++++--------------
 include/linux/fs.h |   6 ++
 2 files changed, 155 insertions(+), 64 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..d2b6691ded16 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -56,8 +56,9 @@ long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 }
 EXPORT_SYMBOL(vfs_ioctl);
 
-static int ioctl_fibmap(struct file *filp, int __user *p)
+static int ioctl_fibmap(struct file *filp, unsigned int fd, unsigned long arg)
 {
+	int __user *p = (void __user *)arg;
 	struct inode *inode = file_inode(filp);
 	struct super_block *sb = inode->i_sb;
 	int error, ur_block;
@@ -197,11 +198,12 @@ int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
 }
 EXPORT_SYMBOL(fiemap_prep);
 
-static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
+static int ioctl_fiemap(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct fiemap fiemap;
 	struct fiemap_extent_info fieinfo = { 0, };
 	struct inode *inode = file_inode(filp);
+	struct fiemap __user *ufiemap = (void __user *)arg;
 	int error;
 
 	if (!inode->i_op->fiemap)
@@ -228,6 +230,18 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
 	return error;
 }
 
+static int ioctl_figetbsz(struct file *file, unsigned int fd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	int __user *argp = (void __user *)arg;
+
+	/* anon_bdev filesystems may not have a block size */
+	if (!inode->i_sb->s_blocksize)
+		return -EINVAL;
+
+	return put_user(inode->i_sb->s_blocksize, argp);
+}
+
 static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 			     u64 off, u64 olen, u64 destoff)
 {
@@ -249,9 +263,15 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	return ret;
 }
 
-static long ioctl_file_clone_range(struct file *file,
-				   struct file_clone_range __user *argp)
+static int ioctl_ficlone(struct file *file, unsigned int fd, unsigned long arg)
+{
+	return ioctl_file_clone(file, arg, 0, 0, 0);
+}
+
+static int ioctl_file_clone_range(struct file *file, unsigned int fd,
+				  unsigned long arg)
 {
+	struct file_clone_range __user *argp = (void __user *)arg;
 	struct file_clone_range args;
 
 	if (copy_from_user(&args, argp, sizeof(args)))
@@ -292,6 +312,27 @@ static int ioctl_preallocate(struct file *filp, int mode, void __user *argp)
 			sr.l_len);
 }
 
+static int ioctl_resvsp(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, 0, p);
+}
+
+static int ioctl_unresvsp(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
+}
+
+static int ioctl_zero_range(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
+}
+
 /* on ia32 l_start is on a 32-bit boundary */
 #if defined CONFIG_COMPAT && defined(CONFIG_X86_64)
 /* just account for different alignment */
@@ -321,28 +362,41 @@ static int compat_ioctl_preallocate(struct file *file, int mode,
 }
 #endif
 
-static int file_ioctl(struct file *filp, unsigned int cmd, int __user *p)
+static ioctl_handler_t file_ioctl(unsigned int cmd)
 {
 	switch (cmd) {
 	case FIBMAP:
-		return ioctl_fibmap(filp, p);
+		return ioctl_fibmap;
 	case FS_IOC_RESVSP:
 	case FS_IOC_RESVSP64:
-		return ioctl_preallocate(filp, 0, p);
+		return ioctl_resvsp;
 	case FS_IOC_UNRESVSP:
 	case FS_IOC_UNRESVSP64:
-		return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
+		return ioctl_unresvsp;
 	case FS_IOC_ZERO_RANGE:
-		return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
+		return ioctl_zero_range;
 	}
 
-	return -ENOIOCTLCMD;
+	return NULL;
+}
+
+static int ioctl_fioclex(struct file *file, unsigned int fd, unsigned long arg)
+{
+	set_close_on_exec(fd, 1);
+	return 0;
+}
+
+static int ioctl_fionclex(struct file *file, unsigned int fd, unsigned long arg)
+{
+	set_close_on_exec(fd, 0);
+	return 0;
 }
 
-static int ioctl_fionbio(struct file *filp, int __user *argp)
+static int ioctl_fionbio(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	unsigned int flag;
 	int on, error;
+	int __user *argp = (void __user *)arg;
 
 	error = get_user(on, argp);
 	if (error)
@@ -362,11 +416,11 @@ static int ioctl_fionbio(struct file *filp, int __user *argp)
 	return error;
 }
 
-static int ioctl_fioasync(unsigned int fd, struct file *filp,
-			  int __user *argp)
+static int ioctl_fioasync(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	unsigned int flag;
 	int on, error;
+	int __user *argp = (void __user *)arg;
 
 	error = get_user(on, argp);
 	if (error)
@@ -384,7 +438,22 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
 	return error < 0 ? error : 0;
 }
 
-static int ioctl_fsfreeze(struct file *filp)
+static int ioctl_fioqsize(struct file *file, unsigned int fd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	void __user *argp = (void __user *)arg;
+
+	if (S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) ||
+	    S_ISLNK(inode->i_mode)) {
+		loff_t res = inode_get_bytes(inode);
+
+		return copy_to_user(argp, &res, sizeof(res)) ? -EFAULT : 0;
+	}
+
+	return -ENOTTY;
+}
+
+static int ioctl_fsfreeze(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct super_block *sb = file_inode(filp)->i_sb;
 
@@ -401,7 +470,7 @@ static int ioctl_fsfreeze(struct file *filp)
 	return freeze_super(sb, FREEZE_HOLDER_USERSPACE);
 }
 
-static int ioctl_fsthaw(struct file *filp)
+static int ioctl_fsthaw(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct super_block *sb = file_inode(filp)->i_sb;
 
@@ -414,9 +483,9 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb, FREEZE_HOLDER_USERSPACE);
 }
 
-static int ioctl_file_dedupe_range(struct file *file,
-				   struct file_dedupe_range __user *argp)
+static int ioctl_file_dedupe_range(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct file_dedupe_range __user *argp = (void __user *)arg;
 	struct file_dedupe_range *same = NULL;
 	int ret;
 	unsigned long size;
@@ -454,6 +523,14 @@ static int ioctl_file_dedupe_range(struct file *file,
 	return ret;
 }
 
+static int ioctl_fionread(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *argp = (void __user *)arg;
+	struct inode *inode = file_inode(filp);
+
+	return put_user(i_size_read(inode) - filp->f_pos, argp);
+}
+
 /**
  * fileattr_fill_xflags - initialize fileattr with xflags
  * @fa:		fileattr pointer
@@ -702,8 +779,9 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
 }
 EXPORT_SYMBOL(vfs_fileattr_set);
 
-static int ioctl_getflags(struct file *file, unsigned int __user *argp)
+static int ioctl_getflags(struct file *file, unsigned int fd, unsigned long arg)
 {
+	unsigned int __user *argp = (void __user *)arg;
 	struct fileattr fa = { .flags_valid = true }; /* hint only */
 	int err;
 
@@ -713,8 +791,9 @@ static int ioctl_getflags(struct file *file, unsigned int __user *argp)
 	return err;
 }
 
-static int ioctl_setflags(struct file *file, unsigned int __user *argp)
+static int ioctl_setflags(struct file *file, unsigned int fd, unsigned long arg)
 {
+	unsigned int __user *argp = (void __user *)arg;
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct fileattr fa;
@@ -733,8 +812,9 @@ static int ioctl_setflags(struct file *file, unsigned int __user *argp)
 	return err;
 }
 
-static int ioctl_fsgetxattr(struct file *file, void __user *argp)
+static int ioctl_fsgetxattr(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct fsxattr __user *argp = (void __user *)arg;
 	struct fileattr fa = { .fsx_valid = true }; /* hint only */
 	int err;
 
@@ -745,8 +825,9 @@ static int ioctl_fsgetxattr(struct file *file, void __user *argp)
 	return err;
 }
 
-static int ioctl_fssetxattr(struct file *file, void __user *argp)
+static int ioctl_fssetxattr(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct fsxattr __user *argp = (void __user *)arg;
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct fileattr fa;
@@ -764,94 +845,98 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
 }
 
 /*
- * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
- * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
+ * Return NULL when no handler exists for @cmd, or the appropriate function
+ * otherwise.  This means that these handlers should never return -ENOIOCTLCMD.
  *
  * When you add any new common ioctls to the switches above and below,
  * please ensure they have compatible arguments in compat mode.
  */
-static int do_vfs_ioctl(struct file *filp, unsigned int fd,
-			unsigned int cmd, unsigned long arg)
+ioctl_handler_t vfs_get_ioctl_handler(struct file *filp, unsigned int cmd)
 {
-	void __user *argp = (void __user *)arg;
-	struct inode *inode = file_inode(filp);
-
 	switch (cmd) {
 	case FIOCLEX:
-		set_close_on_exec(fd, 1);
-		return 0;
+		return ioctl_fioclex;
 
 	case FIONCLEX:
-		set_close_on_exec(fd, 0);
-		return 0;
+		return ioctl_fionclex;
 
 	case FIONBIO:
-		return ioctl_fionbio(filp, argp);
+		return ioctl_fionbio;
 
 	case FIOASYNC:
-		return ioctl_fioasync(fd, filp, argp);
+		return ioctl_fioasync;
 
 	case FIOQSIZE:
-		if (S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) ||
-		    S_ISLNK(inode->i_mode)) {
-			loff_t res = inode_get_bytes(inode);
-			return copy_to_user(argp, &res, sizeof(res)) ?
-					    -EFAULT : 0;
-		}
-
-		return -ENOTTY;
+		return ioctl_fioqsize;
 
 	case FIFREEZE:
-		return ioctl_fsfreeze(filp);
+		return ioctl_fsfreeze;
 
 	case FITHAW:
-		return ioctl_fsthaw(filp);
+		return ioctl_fsthaw;
 
 	case FS_IOC_FIEMAP:
-		return ioctl_fiemap(filp, argp);
+		return ioctl_fiemap;
 
 	case FIGETBSZ:
-		/* anon_bdev filesystems may not have a block size */
-		if (!inode->i_sb->s_blocksize)
-			return -EINVAL;
-
-		return put_user(inode->i_sb->s_blocksize, (int __user *)argp);
+		return ioctl_figetbsz;
 
 	case FICLONE:
-		return ioctl_file_clone(filp, arg, 0, 0, 0);
+		return ioctl_ficlone;
 
 	case FICLONERANGE:
-		return ioctl_file_clone_range(filp, argp);
+		return ioctl_file_clone_range;
 
 	case FIDEDUPERANGE:
-		return ioctl_file_dedupe_range(filp, argp);
+		return ioctl_file_dedupe_range;
 
 	case FIONREAD:
-		if (!S_ISREG(inode->i_mode))
-			return vfs_ioctl(filp, cmd, arg);
+		if (!S_ISREG(file_inode(filp)->i_mode))
+			break;
 
-		return put_user(i_size_read(inode) - filp->f_pos,
-				(int __user *)argp);
+		return ioctl_fionread;
 
 	case FS_IOC_GETFLAGS:
-		return ioctl_getflags(filp, argp);
+		return ioctl_getflags;
 
 	case FS_IOC_SETFLAGS:
-		return ioctl_setflags(filp, argp);
+		return ioctl_setflags;
 
 	case FS_IOC_FSGETXATTR:
-		return ioctl_fsgetxattr(filp, argp);
+		return ioctl_fsgetxattr;
 
 	case FS_IOC_FSSETXATTR:
-		return ioctl_fssetxattr(filp, argp);
+		return ioctl_fssetxattr;
 
 	default:
-		if (S_ISREG(inode->i_mode))
-			return file_ioctl(filp, cmd, argp);
+		if (S_ISREG(file_inode(filp)->i_mode))
+			return file_ioctl(cmd);
 		break;
 	}
 
-	return -ENOIOCTLCMD;
+	/* Forwards call to vfs_ioctl(filp, cmd, arg) */
+	return NULL;
+}
+
+/*
+ * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
+ * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
+ */
+static int do_vfs_ioctl(struct file *filp, unsigned int fd,
+			unsigned int cmd, unsigned long arg)
+{
+	ioctl_handler_t handler = vfs_get_ioctl_handler(filp, cmd);
+	int ret;
+
+	if (!handler)
+		return -ENOIOCTLCMD;
+
+	ret = (*handler)(filp, fd, arg);
+	/* Makes sure handle() really handles this command. */
+	if (WARN_ON_ONCE(ret == -ENOIOCTLCMD))
+		return -ENOTTY;
+
+	return ret;
 }
 
 SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, unsigned long, arg)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1fbc72c5f112..92bf421aae83 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1904,6 +1904,12 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 #define compat_ptr_ioctl NULL
 #endif
 
+typedef int (*ioctl_handler_t)(struct file *file, unsigned int fd,
+			       unsigned long arg);
+
+extern ioctl_handler_t vfs_get_ioctl_handler(struct file *filp,
+					     unsigned int cmd);
+
 /*
  * VFS file helper functions.
  */
-- 
2.44.0

Re: [PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

[Mickaël Salaün]

On Thu, Mar 14, 2024 at 01:56:25PM -0400, Paul Moore wrote:
> On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@google.com> wrote:
> >
> > This LSM hook gets called just before the fs/ioctl.c logic delegates
> > the requested IOCTL command to the file-specific implementation as
> > implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat).
> >
> > It is impractical for LSMs to make security guarantees about these
> > f_op operations without having intimate knowledge of how they are
> > implemented.
> >
> > Therefore, depending on the enabled Landlock policy, Landlock aims to
> > block the calls to filp->f_op->unlocked_ioctl(), but permit the calls
> > to the IOCTL commands which are already implemented in fs/ioctl.c.
> >
> > The current call graph is:
> >
> >   * ioctl syscall
> >     * security_file_ioctl() LSM hook
> >     * do_vfs_ioctl() - standard operations
> >       * file_ioctl() - standard file operations
> >     * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op)
> >       * filp->f_op->unlocked_ioctl()
> >
> > Why not use the existing security_file_ioctl() hook?
> >
> > With the existing security_file_ioctl() hook, it is technically
> > feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it
> > would be difficult to maintain: security_file_ioctl() gets called
> > further up the call stack, so an implementation of it would need to
> > predict whether the logic further below will decide to call
> > f_op->unlocked_ioctl().  That can only be done by mirroring the logic
> > in do_vfs_ioctl() to some extent, and keeping this in sync.
> 
> Once again, I don't see this as an impossible task, and I would think
> that you would want to inspect each new ioctl command/op added in
> do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted
> behavior from a Landlock sandbox perspective.

About the LANDLOCK_ACCESS_FS_IOCTL_DEV semantic, we only care about the
IOCTLs that are actually delivered to device drivers, so any new IOCTL
directly handled by fs/ioctl.c should be treated the same way for this
access right.

> Looking at the git
> log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are
> added very frequently, meaning that keeping Landlock sync'd with
> fs/ioctl.c shouldn't be a terrible task.

do_vfs_ioctl() is indeed not changed often, but this doesn't mean we
should not provide strong guarantees, avoid future security bugs, lower
the maintenance cost, and improve code readability.

> 
> I'm also not excited about the overlap between the existing
> security_file_ioctl() hook and the proposed security_file_vfs_ioctl()
> hook.  There are some cases where we have no choice and we have to
> tolerate the overlap, but this doesn't look like one of those cases to
> me.
> 
> I'm sorry, but I don't agree with this new hook.

OK, I sent a new RFC (in reply to your email) as an alternative
approach.  Instead of adding a new LSM hook, this patch adds the
vfs_get_ioctl_handler() helper and some code refactoring that should be
both interesting for the VFS subsystem and for Landlock.  I guess this
could be interesting for other security mechanisms as well (e.g. BPF
LSM).  What do you think?

Arnd, Christian, would this sound good to you?

Re: [PATCH v10 6/9] selftests/landlock: Test IOCTLs on named pipes

[Mickaël Salaün]

It might be interesting to create a layout with one file of each type
and use that for the IOCTL tests.

On Sat, Mar 09, 2024 at 07:53:17AM +0000, Günther Noack wrote:
> Named pipes should behave like pipes created with pipe(2),
> so we don't want to restrict IOCTLs on them.
> 
> Suggested-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 61 ++++++++++++++++++----
>  1 file changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 5c47231a722e..d991f44875bc 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
>  	ASSERT_EQ(0, close(fd));
>  }
>  
> +static int test_fionread_ioctl(int fd)
> +{
> +	size_t sz = 0;
> +
> +	if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
> + * because they are not character or block devices.
> + */
> +TEST_F_FORK(layout1, named_pipe_ioctl)
> +{
> +	pid_t child_pid;
> +	int fd, ruleset_fd;
> +	const char *const path = file1_s1d1;
> +	const struct landlock_ruleset_attr attr = {
> +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> +	};
> +
> +	ASSERT_EQ(0, unlink(path));
> +	ASSERT_EQ(0, mkfifo(path, 0600));
> +
> +	/* 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));
> +
> +	/* The child process opens the pipe for writing. */
> +	child_pid = fork();
> +	ASSERT_NE(-1, child_pid);
> +	if (child_pid == 0) {

What is the purpose of this child's code?

> +		fd = open(path, O_WRONLY);
> +		close(fd);
> +		exit(0);
> +	}
> +
> +	fd = open(path, O_RDONLY);
> +	ASSERT_LE(0, fd);
> +
> +	/* FIONREAD is implemented by pipefifo_fops. */
> +	EXPECT_EQ(0, test_fionread_ioctl(fd));
> +
> +	ASSERT_EQ(0, close(fd));
> +	ASSERT_EQ(0, unlink(path));
> +
> +	ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
> +}
> +
>  /* clang-format off */
>  FIXTURE(ioctl) {};
>  
> @@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd)
>  	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;
> -}
> -

You should add test_fionread_ioctl() at the right place from the start.

>  TEST_F_FORK(ioctl, handle_dir_access_file)
>  {
>  	const int flag = 0;
> -- 
> 2.44.0.278.ge034bb2e1d-goog
> 
> 

Re: [PATCH v10 7/9] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets

[Mickaël Salaün]

On Sat, Mar 09, 2024 at 07:53:18AM +0000, Günther Noack wrote:
> Suggested-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index d991f44875bc..941e6f9702b7 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -20,8 +20,10 @@
>  #include <sys/mount.h>
>  #include <sys/prctl.h>
>  #include <sys/sendfile.h>
> +#include <sys/socket.h>
>  #include <sys/stat.h>
>  #include <sys/sysmacros.h>
> +#include <sys/un.h>
>  #include <sys/vfs.h>
>  #include <unistd.h>
>  
> @@ -3976,6 +3978,57 @@ TEST_F_FORK(layout1, named_pipe_ioctl)
>  	ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
>  }
>  
> +/* For named UNIX domain sockets, no IOCTL restrictions apply. */
> +TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
> +{
> +	const char *const path = file1_s1d1;
> +	int srv_fd, cli_fd, ruleset_fd;
> +	socklen_t size;
> +	struct sockaddr_un srv_un, cli_un;
> +	const struct landlock_ruleset_attr attr = {
> +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> +	};
> +
> +	/* Sets up a server */
> +	srv_un.sun_family = AF_UNIX;
> +	strncpy(srv_un.sun_path, path, sizeof(srv_un.sun_path));
> +
> +	ASSERT_EQ(0, unlink(path));
> +	ASSERT_LE(0, (srv_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
> +
> +	size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
> +	ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
> +	ASSERT_EQ(0, listen(srv_fd, 10 /* qlen */));
> +
> +	/* 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));
> +
> +	/* Sets up a client connection to it */
> +	cli_un.sun_family = AF_UNIX;
> +	snprintf(cli_un.sun_path, sizeof(cli_un.sun_path), "%s%ld", path,
> +		 (long)getpid());

I don't think it is useful to have a unique sun_path for a named unix
socket, that's the purpose of naming it right?

> +
> +	ASSERT_LE(0, (cli_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
> +
> +	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
> +	ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
> +
> +	bzero(&cli_un, sizeof(cli_un));
> +	cli_un.sun_family = AF_UNIX;
> +	strncpy(cli_un.sun_path, path, sizeof(cli_un.sun_path));
> +	size = offsetof(struct sockaddr_un, sun_path) + strlen(cli_un.sun_path);
> +
> +	ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
> +
> +	/* FIONREAD and other IOCTLs should not be forbidden. */
> +	EXPECT_EQ(0, test_fionread_ioctl(cli_fd));
> +
> +	ASSERT_EQ(0, close(cli_fd));
> +}
> +
>  /* clang-format off */
>  FIXTURE(ioctl) {};
>  
> -- 
> 2.44.0.278.ge034bb2e1d-goog
> 
> 

Re: [PATCH v10 6/9] selftests/landlock: Test IOCTLs on named pipes

[Mickaël Salaün]

On Fri, Mar 22, 2024 at 08:48:30AM +0100, Mickaël Salaün wrote:
> It might be interesting to create a layout with one file of each type
> and use that for the IOCTL tests.

To make sure we only restrict the first layer of IOCTL (handled by the
VFS) we should check that an IOCTL command that should be handled by a
specific filesystem is indeed passed through this filesystem and not
blocked by Landlock.  Because Landlock would return EACCES, I guess it
should be enough to check that we get a ENOTTY for non-block/char
devices.  We should find an IOCTL command number that has little chance
to be taken to avoid updating this test too often.

Re: [PATCH v10 6/9] selftests/landlock: Test IOCTLs on named pipes

[Günther Noack]

On Fri, Mar 22, 2024 at 08:48:29AM +0100, Mickaël Salaün wrote:
> It might be interesting to create a layout with one file of each type
> and use that for the IOCTL tests.

We have already written these tests and we can keep them, but I think that we
only gain little additional confidence from testing non-device files.  The
implementation is saying pretty directly that IOCTLs are permitted if the file
is not a character or block device, at the top of the file_ioctl hook.  I don't
see much value in testing this even more exhaustively and would like to keep it
as it is for now.


> On Sat, Mar 09, 2024 at 07:53:17AM +0000, Günther Noack wrote:
> > Named pipes should behave like pipes created with pipe(2),
> > so we don't want to restrict IOCTLs on them.
> > 
> > Suggested-by: Mickaël Salaün <mic@digikod.net>
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> >  tools/testing/selftests/landlock/fs_test.c | 61 ++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 5c47231a722e..d991f44875bc 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -3924,6 +3924,58 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
> >  	ASSERT_EQ(0, close(fd));
> >  }
> >  
> > +static int test_fionread_ioctl(int fd)
> > +{
> > +	size_t sz = 0;
> > +
> > +	if (ioctl(fd, FIONREAD, &sz) < 0 && errno == EACCES)
> > +		return errno;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
> > + * because they are not character or block devices.
> > + */
> > +TEST_F_FORK(layout1, named_pipe_ioctl)
> > +{
> > +	pid_t child_pid;
> > +	int fd, ruleset_fd;
> > +	const char *const path = file1_s1d1;
> > +	const struct landlock_ruleset_attr attr = {
> > +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> > +	};
> > +
> > +	ASSERT_EQ(0, unlink(path));
> > +	ASSERT_EQ(0, mkfifo(path, 0600));
> > +
> > +	/* 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));
> > +
> > +	/* The child process opens the pipe for writing. */
> > +	child_pid = fork();
> > +	ASSERT_NE(-1, child_pid);
> > +	if (child_pid == 0) {
> 
> What is the purpose of this child's code?

From fifo(7):

  Opening the FIFO blocks until the other end is opened also.

So the child and parent process both wait for the other open to happen.

I suspect I could technically also use O_RDWR here, but that is undefined
behaviour in POSIX and less conventional code.  (This is described further down,
also in fifo(7).)

> 
> > +		fd = open(path, O_WRONLY);
> > +		close(fd);
> > +		exit(0);
> > +	}
> > +
> > +	fd = open(path, O_RDONLY);
> > +	ASSERT_LE(0, fd);
> > +
> > +	/* FIONREAD is implemented by pipefifo_fops. */
> > +	EXPECT_EQ(0, test_fionread_ioctl(fd));
> > +
> > +	ASSERT_EQ(0, close(fd));
> > +	ASSERT_EQ(0, unlink(path));
> > +
> > +	ASSERT_EQ(child_pid, waitpid(child_pid, NULL, 0));
> > +}
> > +
> >  /* clang-format off */
> >  FIXTURE(ioctl) {};
> >  
> > @@ -3997,15 +4049,6 @@ static int test_tcgets_ioctl(int fd)
> >  	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;
> > -}
> > -
> 
> You should add test_fionread_ioctl() at the right place from the start.

Fair enough, done.

> >  TEST_F_FORK(ioctl, handle_dir_access_file)
> >  {
> >  	const int flag = 0;
> > -- 
> > 2.44.0.278.ge034bb2e1d-goog
> > 
> > 

—Günther

Re: [PATCH v10 7/9] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets

[Günther Noack]

On Fri, Mar 22, 2024 at 08:57:18AM +0100, Mickaël Salaün wrote:
> On Sat, Mar 09, 2024 at 07:53:18AM +0000, Günther Noack wrote:
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index d991f44875bc..941e6f9702b7 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c

[...]

> > +/* For named UNIX domain sockets, no IOCTL restrictions apply. */
> > +TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
> > +{

[...]

> > +	/* Sets up a client connection to it */
> > +	cli_un.sun_family = AF_UNIX;
> > +	snprintf(cli_un.sun_path, sizeof(cli_un.sun_path), "%s%ld", path,
> > +		 (long)getpid());
> 
> I don't think it is useful to have a unique sun_path for a named unix
> socket, that's the purpose of naming it right?

Removed, well spotted!  I did not realize that I could omit that.

—Günther

Re: [PATCH v10 6/9] selftests/landlock: Test IOCTLs on named pipes

[Mickaël Salaün]

On Fri, Mar 22, 2024 at 03:39:55PM +0100, Günther Noack wrote:
> On Fri, Mar 22, 2024 at 08:48:29AM +0100, Mickaël Salaün wrote:
> > It might be interesting to create a layout with one file of each type
> > and use that for the IOCTL tests.
> 
> We have already written these tests and we can keep them, but I think that we
> only gain little additional confidence from testing non-device files.  The
> implementation is saying pretty directly that IOCTLs are permitted if the file
> is not a character or block device, at the top of the file_ioctl hook.  I don't
> see much value in testing this even more exhaustively and would like to keep it
> as it is for now.

OK, let's keep them for now, it's easy to remove a patch/commit.