[PATCH v14 00/12] Landlock: IOCTL support

[PATCH v14 00/12] Landlock: IOCTL support

[Günther Noack]

Hello!

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

Objective
~~~~~~~~~

Make ioctl(2) requests for device files 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 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 that in common scenarios, where the terminal file
  descriptor is inherited from the parent process, 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),
  regular files and directories.

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 device 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 unconditionally permitted by Landlock, because
they are really operating on the file system's superblock, rather than on the
file itself (the same funcionality is also available from any other file on the
same file system):

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

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

Detailed reasoning about each IOCTL command from fs/ioctl.c is in
get_required_ioctl_dev_access() in security/landlock/fs.c.


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


Implementation Rationale
~~~~~~~~~~~~~~~~~~~~~~~~

A main constraint of this implementation is that the blanket-permitted
IOCTL commands for device files should never dispatch to the
device-specific implementations in f_ops->unlocked_ioctl() and
f_ops->compat_ioctl().

There are many implementations of these f_ops operations and they are
too scattered across the kernel to give strong guarantees about them.
Additionally, some existing implementations do work before even
checking whether they support the cmd number which was passed to them.


In this implementation, we are listing the blanket-permitted IOCTL
commands in the Landlock implementation, mirroring a subset of the
IOCTL commands which are directly implemented in do_vfs_ioctl() in
fs/ioctl.c.  The trade-off is that the Landlock LSM needs to track
future developments in fs/ioctl.c to keep up to date with that, in
particular when new IOCTL commands are introduced there, or when they
are moved there from the f_ops implementations.

We mitigate this risk in this patch set by adding fs/ioctl.c to the
paths that are relevant to Landlock in the MAINTAINERS file.

The trade-off is discussed in more detail in [3].


Previous versions of this patch set have used different implementation
approaches to guarantee the main constraint above, which we have
dismissed due to the following reasons:

* V10: Introduced a new LSM hook file_vfs_ioctl, which gets invoked
  just before the call to f_ops->unlocked_ioctl().

  Not done, because it would have created an avoidable overlap between
  the file_ioctl and file_vfs_ioctl LSM hooks [4].

* V11: Introduced an indirection layer in fs/ioctl.c, so that Landlock
  could figure out the list of IOCTL commands which are handled by
  do_vfs_ioctl().

  Not done due to additional indirection and possible performance
  impact in fs/ioctl.c [5]

* V12: Introduced a special error code to be returned from the
  file_ioctl hook, and matching logic that would disallow the call to
  f_ops->unlocked_ioctl() in case that this error code is returned.

  Not done due because this approach would conflict with Landlock's
  planned audit logging [6] and because LSM hooks with special error
  codes are generally discouraged and have lead to problems in the
  past [7].

Thanks to Arnd Bergmann, Christian Brauner, Kent Overstreet, Mickaël Salaün and
Paul Moore for guiding this implementation on the right track!

[3] https://lore.kernel.org/all/ZgLJG0aN0psur5Z7@google.com/
[4] https://lore.kernel.org/all/CAHC9VhRojXNSU9zi2BrP8z6JmOmT3DAqGNtinvvz=tL1XhVdyg@mail.gmail.com/
[5] https://lore.kernel.org/all/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@app.fastmail.com
[6] https://lore.kernel.org/all/20240326.ahyaaPa0ohs6@digikod.net
[7] https://lore.kernel.org/all/CAHC9VhQJFWYeheR-EqqdfCq0YpvcQX5Scjfgcz1q+jrWg8YsdA@mail.gmail.com/


Changes
~~~~~~~

V14:
 * Revise which IOCTLs are permitted.
   It is almost the same as the vfs_masked_device_ioctl() hooks from
   https://lore.kernel.org/all/20240219183539.2926165-1-mic@digikod.net/,
   with the following differences:
   * Added cases for FS_IOC_GETFSUUID and FS_IOC_GETFSSYSFSPATH
   * Do not blanket-permit FS_IOC_{GET,SET}{FLAGS,XATTR}.
     They fall back to the device implementation.
 * fs/ioctl:
   * Small prerequisite change so that FS_IOC_GETFSUUID and
     FS_IOC_GETFSSYSFSPATH do not fall back to the device implementation.
   * Slightly rephrase wording in the warning above do_vfs_ioctl().
 * Implement compat handler
 * Improve UAPI header documentation
 * Code structure
   * Change helper function style to return a boolean
   * Reorder structure of the IOCTL hooks (much cleaner now -- thanks for the
     hint, Mickaël!)
   * Extract is_device() helper

V13:
 * Using the existing file_ioctl hook and a hardcoded list of IOCTL commands.
   (See the section on implementation rationale above.)
 * Add support for FS_IOC_GETFSUUID, FS_IOC_GETFSSYSFSPATH.
   
V12:
 * Rebased on Arnd's proposal:
   https://lore.kernel.org/all/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@app.fastmail.com/
   This means that:
   * the IOCTL security hooks can return a special value ENOFILEOPS,
     which is treated specially in fs/ioctl.c to permit the IOCTL,
     but only as long as it does not call f_ops->unlocked_ioctl or
     f_ops->compat_ioctl.
 * The only change compared to V11 is commit 1, as well as a small
   adaptation in the commit 2 (The Landlock implementation needs to
   return the new special value).  The tests and documentation commits
   are exactly the same as before.

V11:
 * Rebased on Mickaël's proposal to refactor fs/ioctl.c:
   https://lore.kernel.org/all/20240315145848.1844554-1-mic@digikod.net/
   This means that:
   * we do not add the file_vfs_ioctl() hook as in V10
   * we add vfs_get_ioctl_handler() instead, so that Landlock
     can query which of the IOCTL commands in handled in do_vfs_ioctl()

   That proposal is used here unmodified (except for minor typos in the commit
   description).
 * Use the hook_ioctl_compat LSM hook as well.

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/all/20230502171755.9788-1-gnoack3000@gmail.com/
V2: https://lore.kernel.org/all/20230623144329.136541-1-gnoack@google.com/
V3: https://lore.kernel.org/all/20230814172816.3907299-1-gnoack@google.com/
V4: https://lore.kernel.org/all/20231103155717.78042-1-gnoack@google.com/
V5: https://lore.kernel.org/all/20231117154920.1706371-1-gnoack@google.com/
V6: https://lore.kernel.org/all/20231124173026.3257122-1-gnoack@google.com/
V7: https://lore.kernel.org/all/20231201143042.3276833-1-gnoack@google.com/
V8: https://lore.kernel.org/all/20231208155121.1943775-1-gnoack@google.com/
V9: https://lore.kernel.org/all/20240209170612.1638517-1-gnoack@google.com/
V10: https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/
V11: https://lore.kernel.org/all/20240322151002.3653639-1-gnoack@google.com/
V12: https://lore.kernel.org/all/20240325134004.4074874-1-gnoack@google.com/
V13: https://lore.kernel.org/all/20240327131040.158777-1-gnoack@google.com/

Günther Noack (12):
  fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH
    fail
  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
  selftests/landlock: Exhaustive test for the IOCTL allow-list
  samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL_DEV
  landlock: Document IOCTL support
  MAINTAINERS: Notify Landlock maintainers about changes to fs/ioctl.c
  fs/ioctl: Add a comment to keep the logic in sync with LSM policies

 Documentation/userspace-api/landlock.rst     |  76 ++-
 MAINTAINERS                                  |   1 +
 fs/ioctl.c                                   |   7 +-
 include/uapi/linux/landlock.h                |  38 +-
 samples/landlock/sandboxer.c                 |  13 +-
 security/landlock/fs.c                       | 221 ++++++++-
 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   | 491 ++++++++++++++++++-
 10 files changed, 813 insertions(+), 46 deletions(-)


base-commit: e9df9344b6f3e5e1c745a71f125ff4b5c6ddc96b
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

[Günther Noack]

These IOCTL commands should be implemented by setting attributes on the
superblock, rather than in the IOCTL hooks in struct file_operations.

By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
logic to return -ENOTTY immediately, rather than attempting to call
f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.

Why this is safe:

Before this change, fs/ioctl.c would unsuccessfully attempt calling the
IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
return the same error code immediately, but save ourselves the fallback
attempt.

Motivation:

This simplifies the logic for these IOCTL commands and lets us reason about
the side effects of these IOCTLs more easily.  It will be possible to
permit these IOCTLs under LSM IOCTL policies, without having to worry about
them getting dispatched to problematic device drivers (which sometimes do
work before looking at the IOCTL command number).

Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 fs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d5abfdf0f22..fb0628e680c4 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -769,7 +769,7 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
 
 	if (!sb->s_uuid_len)
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
 
@@ -781,7 +781,7 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
 	struct super_block *sb = file_inode(file)->i_sb;
 
 	if (!strlen(sb->s_sysfs_name))
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	struct fs_sysfs_path u = {};
 
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 02/12] 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                |  38 +++-
 security/landlock/fs.c                       | 221 ++++++++++++++++++-
 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, 259 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677539..68625e728f43 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,33 @@ 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:
+ *
+ *   * IOCTL commands targeting file descriptors (``FIOCLEX``, ``FIONCLEX``),
+ *   * IOCTL commands targeting file descriptions (``FIONBIO``, ``FIOASYNC``),
+ *   * IOCTL commands targeting file systems (``FIFREEZE``, ``FITHAW``,
+ *     ``FIGETBSZ``, ``FS_IOC_GETFSUUID``, ``FS_IOC_GETFSSYSFSPATH``)
+ *   * Some IOCTL commands which do not make sense when used with devices, but
+ *     whose implementations are safe and return the right error codes
+ *     (``FS_IOC_FIEMAP``, ``FICLONE``, ``FICLONERANGE``, ``FIDEDUPERANGE``)
+ *
+ *   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 +244,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 c15559432d3d..b0857541d5e0 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -7,6 +7,7 @@
  * Copyright © 2021-2022 Microsoft Corporation
  */
 
+#include <asm/ioctls.h>
 #include <kunit/test.h>
 #include <linux/atomic.h>
 #include <linux/bitops.h>
@@ -14,6 +15,7 @@
 #include <linux/compiler_types.h>
 #include <linux/dcache.h>
 #include <linux/err.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -29,6 +31,7 @@
 #include <linux/types.h>
 #include <linux/wait_bit.h>
 #include <linux/workqueue.h>
+#include <uapi/linux/fiemap.h>
 #include <uapi/linux/landlock.h>
 
 #include "common.h"
@@ -84,6 +87,158 @@ static const struct landlock_object_underops landlock_fs_underops = {
 	.release = release_inode
 };
 
+/* IOCTL helpers */
+
+/**
+ * is_masked_device_ioctl(): Determine whether an IOCTL command is always
+ * permitted with Landlock for device files.  These commands can not be
+ * restricted on device files by enforcing a Landlock policy.
+ *
+ * @cmd: The IOCTL command that is supposed to be run.
+ *
+ * By default, any IOCTL on a device file requires the
+ * LANDLOCK_ACCESS_FS_IOCTL_DEV right.  However, we blanket-permit some
+ * commands, if:
+ *
+ * 1. The command is implemented in fs/ioctl.c's do_vfs_ioctl(),
+ *    not in f_ops->unlocked_ioctl() or f_ops->compat_ioctl().
+ *
+ * 2. The command is harmless when invoked on devices.
+ *
+ * We also permit commands that do not make sense for devices, but where the
+ * do_vfs_ioctl() implementation returns a more conventional error code.
+ *
+ * Any new IOCTL commands that are implemented in fs/ioctl.c's do_vfs_ioctl()
+ * should be considered for inclusion here.
+ *
+ * Returns: true if the IOCTL @cmd can not be restricted with Landlock for
+ * device files.
+ */
+static __attribute_const__ bool is_masked_device_ioctl(const unsigned int cmd)
+{
+	switch (cmd) {
+	/*
+	 * FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's
+	 * close-on-exec and the file's buffered-IO and async flags.  These
+	 * operations are also available through fcntl(2), and are
+	 * unconditionally permitted in Landlock.
+	 */
+	case FIOCLEX:
+	case FIONCLEX:
+	case FIONBIO:
+	case FIOASYNC:
+	/*
+	 * FIOQSIZE queries the size of a regular file, directory, or link.
+	 *
+	 * We still permit it, because it always returns -ENOTTY for
+	 * other file types.
+	 */
+	case FIOQSIZE:
+	/*
+	 * FIFREEZE and FITHAW freeze and thaw the file system which the
+	 * given file belongs to.  Requires CAP_SYS_ADMIN.
+	 *
+	 * These commands operate on the file system's superblock rather
+	 * than on the file itself.  The same operations can also be
+	 * done through any other file or directory on the same file
+	 * system, so it is safe to permit these.
+	 */
+	case FIFREEZE:
+	case FITHAW:
+	/*
+	 * FS_IOC_FIEMAP queries information about the allocation of
+	 * blocks within a file.
+	 *
+	 * This IOCTL command only makes sense for regular files and is
+	 * not implemented by devices. It is harmless to permit.
+	 */
+	case FS_IOC_FIEMAP:
+	/*
+	 * FIGETBSZ queries the file system's block size for a file or
+	 * directory.
+	 *
+	 * This command operates on the file system's superblock rather
+	 * than on the file itself.  The same operation can also be done
+	 * through any other file or directory on the same file system,
+	 * so it is safe to permit it.
+	 */
+	case FIGETBSZ:
+	/*
+	 * FICLONE, FICLONERANGE and FIDEDUPERANGE make files share
+	 * their underlying storage ("reflink") between source and
+	 * destination FDs, on file systems which support that.
+	 *
+	 * These IOCTL commands only apply to regular files
+	 * and are harmless to permit for device files.
+	 */
+	case FICLONE:
+	case FICLONERANGE:
+	case FIDEDUPERANGE:
+	/*
+	 * FIONREAD, FS_IOC_GETFLAGS, FS_IOC_SETFLAGS, FS_IOC_FSGETXATTR and
+	 * FS_IOC_FSSETXATTR are forwarded to device implementations.
+	 */
+
+	/*
+	 * FS_IOC_GETFSUUID and FS_IOC_GETFSSYSFSPATH both operate on
+	 * the file system superblock, not on the specific file, so
+	 * these operations are available through any other file on the
+	 * same file system as well.
+	 */
+	case FS_IOC_GETFSUUID:
+	case FS_IOC_GETFSSYSFSPATH:
+		return true;
+
+	/*
+	 * file_ioctl() commands (FIBMAP, FS_IOC_RESVSP, FS_IOC_RESVSP64,
+	 * FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64 and FS_IOC_ZERO_RANGE) are
+	 * forwarded to device implementations, so not permitted.
+	 */
+
+	/* Other commands are guarded by the access right. */
+	default:
+		return false;
+	}
+}
+
+/*
+ * is_masked_device_ioctl_compat - same as the helper above, but checking the
+ * "compat" IOCTL commands.
+ *
+ * The IOCTL commands with special handling in compat-mode should behave the
+ * same as their non-compat counterparts.
+ */
+static __attribute_const__ bool
+is_masked_device_ioctl_compat(const unsigned int cmd)
+{
+	switch (cmd) {
+	/* FICLONE is permitted, same as in the non-compat variant. */
+	case FICLONE:
+		return true;
+#if defined(CONFIG_X86_64)
+	/*
+	 * FS_IOC_RESVSP_32, FS_IOC_RESVSP64_32, FS_IOC_UNRESVSP_32,
+	 * FS_IOC_UNRESVSP64_32, FS_IOC_ZERO_RANGE_32: not blanket-permitted,
+	 * for consistency with their non-compat variants.
+	 */
+	case FS_IOC_RESVSP_32:
+	case FS_IOC_RESVSP64_32:
+	case FS_IOC_UNRESVSP_32:
+	case FS_IOC_UNRESVSP64_32:
+	case FS_IOC_ZERO_RANGE_32:
+#endif
+	/*
+	 * FS_IOC32_GETFLAGS, FS_IOC32_SETFLAGS are forwarded to their device
+	 * implementations.
+	 */
+	case FS_IOC32_GETFLAGS:
+	case FS_IOC32_SETFLAGS:
+		return false;
+	default:
+		return is_masked_device_ioctl(cmd);
+	}
+}
+
 /* Ruleset management */
 
 static struct landlock_object *get_inode_object(struct inode *const inode)
@@ -148,7 +303,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,11 +1488,18 @@ static int hook_file_alloc_security(struct file *const file)
 	return 0;
 }
 
+static bool is_device(const struct file *const file)
+{
+	const struct inode *inode = file_inode(file);
+
+	return S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
+}
+
 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 landlock_ruleset *const dom =
 		get_fs_domain(landlock_cred(file->f_cred)->domain);
 
@@ -1354,6 +1517,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(file))
+		optional_access |= LANDLOCK_ACCESS_FS_IOCTL_DEV;
+
 	full_access_request = open_access_request | optional_access;
 
 	if (is_access_to_paths_allowed(
@@ -1410,6 +1577,52 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+static int hook_file_ioctl(struct file *file, unsigned int cmd,
+			   unsigned long arg)
+{
+	access_mask_t allowed_access = landlock_file(file)->allowed_access;
+
+	/*
+	 * It is the access rights at the time of opening the file which
+	 * determine whether IOCTL can be used on the opened file later.
+	 *
+	 * The access right is attached to the opened file in hook_file_open().
+	 */
+	if (allowed_access & LANDLOCK_ACCESS_FS_IOCTL_DEV)
+		return 0;
+
+	if (!is_device(file))
+		return 0;
+
+	if (is_masked_device_ioctl(cmd))
+		return 0;
+
+	return -EACCES;
+}
+
+static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
+				  unsigned long arg)
+{
+	access_mask_t allowed_access = landlock_file(file)->allowed_access;
+
+	/*
+	 * It is the access rights at the time of opening the file which
+	 * determine whether IOCTL can be used on the opened file later.
+	 *
+	 * The access right is attached to the opened file in hook_file_open().
+	 */
+	if (allowed_access & LANDLOCK_ACCESS_FS_IOCTL_DEV)
+		return 0;
+
+	if (!is_device(file))
+		return 0;
+
+	if (is_masked_device_ioctl_compat(cmd))
+		return 0;
+
+	return -EACCES;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
 
@@ -1432,6 +1645,8 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
 	LSM_HOOK_INIT(file_open, hook_file_open),
 	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
+	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
+	LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
 };
 
 __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 a6f89aaea77d..3c1e9f35b531 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 9a6036fbf289..418ad745a5dd 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -529,9 +529,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.478.gd926399ef9-goog

[PATCH v14 03/12] 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 | 227 ++++++++++++++++++++-
 1 file changed, 224 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 418ad745a5dd..8a72e26d4977 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
@@ -737,6 +745,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);
 	}
@@ -3445,7 +3456,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);
@@ -3528,7 +3539,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);
@@ -3754,7 +3765,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);
@@ -3831,6 +3842,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;
@@ -3847,6 +3868,206 @@ TEST(memfd_ftruncate)
 	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;
+}
+
+/* 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_tcgets_ioctl(int fd)
+{
+	struct termios info;
+
+	if (ioctl(fd, TCGETS, &info) < 0)
+		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(0, ioctl(file_fd, FIGETBSZ, &flag));
+
+	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, ioctl(dir_fd, FIGETBSZ, &flag));
+
+	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(0, ioctl(file_fd, FIGETBSZ, &flag));
+
+	ASSERT_EQ(0, close(file_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 04/12] 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 8a72e26d4977..70a05651619d 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3852,20 +3852,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));
+	}
 }
 
 static int test_fionread_ioctl(int fd)
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 05/12] 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 70a05651619d..84e5477d2e36 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3895,6 +3895,46 @@ static int test_fionread_ioctl(int fd)
 	return 0;
 }
 
+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.478.gd926399ef9-goog

[PATCH v14 06/12] 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 | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 84e5477d2e36..215f0e8bcd69 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3935,6 +3935,49 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
 	ASSERT_EQ(0, close(fd));
 }
 
+/*
+ * 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) {};
 
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 07/12] 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 | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 215f0e8bcd69..10b29a288e9c 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>
 
@@ -3978,6 +3980,55 @@ 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;
+
+	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.478.gd926399ef9-goog

[PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

[Günther Noack]

This test checks all IOCTL commands implemented in do_vfs_ioctl().

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 | 95 ++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 10b29a288e9c..e4ba149cf6fd 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -10,6 +10,7 @@
 #define _GNU_SOURCE
 #include <asm/termbits.h>
 #include <fcntl.h>
+#include <linux/fiemap.h>
 #include <linux/landlock.h>
 #include <linux/magic.h>
 #include <sched.h>
@@ -3937,6 +3938,100 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
 	ASSERT_EQ(0, close(fd));
 }
 
+/*
+ * ioctl_error - generically call the given ioctl with a pointer to a
+ * sufficiently large memory region
+ *
+ * Returns the IOCTLs error, or 0.
+ */
+static int ioctl_error(int fd, unsigned int cmd)
+{
+	char buf[1024]; /* sufficiently large */
+	int res = ioctl(fd, cmd, &buf);
+
+	if (res < 0)
+		return errno;
+
+	return 0;
+}
+
+/* Define some linux/falloc.h IOCTL commands which are not available in uapi headers. */
+struct space_resv {
+	__s16 l_type;
+	__s16 l_whence;
+	__s64 l_start;
+	__s64 l_len; /* len == 0 means until end of file */
+	__s32 l_sysid;
+	__u32 l_pid;
+	__s32 l_pad[4]; /* reserved area */
+};
+
+#define FS_IOC_RESVSP _IOW('X', 40, struct space_resv)
+#define FS_IOC_UNRESVSP _IOW('X', 41, struct space_resv)
+#define FS_IOC_RESVSP64 _IOW('X', 42, struct space_resv)
+#define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv)
+#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv)
+
+/*
+ * Tests a series of blanket-permitted and denied IOCTLs.
+ */
+TEST_F_FORK(layout1, blanket_permitted_ioctls)
+{
+	const struct landlock_ruleset_attr attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
+	};
+	int ruleset_fd, 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));
+
+	fd = open("/dev/null", O_RDWR | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+
+	/*
+	 * Checks permitted commands.
+	 * These ones may return errors, but should not be blocked by Landlock.
+	 */
+	EXPECT_NE(EACCES, ioctl_error(fd, FIOCLEX));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIONCLEX));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIONBIO));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIOASYNC));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIOQSIZE));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIFREEZE));
+	EXPECT_NE(EACCES, ioctl_error(fd, FITHAW));
+	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_FIEMAP));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIGETBSZ));
+	EXPECT_NE(EACCES, ioctl_error(fd, FICLONE));
+	EXPECT_NE(EACCES, ioctl_error(fd, FICLONERANGE));
+	EXPECT_NE(EACCES, ioctl_error(fd, FIDEDUPERANGE));
+	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSUUID));
+	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));
+
+	/*
+	 * Checks blocked commands.
+	 * A call to a blocked IOCTL command always returns EACCES.
+	 */
+	EXPECT_EQ(EACCES, ioctl_error(fd, FIONREAD));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_GETFLAGS));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_SETFLAGS));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSGETXATTR));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSSETXATTR));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FIBMAP));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP64));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP64));
+	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_ZERO_RANGE));
+
+	/* Default case is also blocked. */
+	EXPECT_EQ(EACCES, ioctl_error(fd, 0xc00ffeee));
+
+	ASSERT_EQ(0, close(fd));
+}
+
 /*
  * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
  * because they are not character or block devices.
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 09/12] 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 8b8ecd65c28c..e8223c3e781a 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 */
 
@@ -202,11 +203,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)
 {
@@ -320,6 +322,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.478.gd926399ef9-goog

[PATCH v14 10/12] 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 9dd636aaa829..145bd869c684 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.478.gd926399ef9-goog

[PATCH v14 11/12] MAINTAINERS: Notify Landlock maintainers about changes to fs/ioctl.c

[Günther Noack]

Landlock needs to track changes to do_vfs_ioctl() when new IOCTL
implementations are added to it.

Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..c95dabf4ecc9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12222,6 +12222,7 @@ W:	https://landlock.io
 T:	git https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
 F:	Documentation/security/landlock.rst
 F:	Documentation/userspace-api/landlock.rst
+F:	fs/ioctl.c
 F:	include/uapi/linux/landlock.h
 F:	samples/landlock/
 F:	security/landlock/
-- 
2.44.0.478.gd926399ef9-goog

[PATCH v14 12/12] fs/ioctl: Add a comment to keep the logic in sync with LSM policies

[Günther Noack]

Landlock's IOCTL support needs to partially replicate the list of
IOCTLs from do_vfs_ioctl().  The list of commands implemented in
do_vfs_ioctl() should be kept in sync with Landlock's IOCTL policies.

Suggested-by: Paul Moore <paul@paul-moore.com>
Suggested-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Günther Noack <gnoack@google.com>
---
 fs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index fb0628e680c4..64776891120c 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -796,6 +796,9 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
  *
  * When you add any new common ioctls to the switches above and below,
  * please ensure they have compatible arguments in compat mode.
+ *
+ * The LSM mailing list should also be notified of any command additions or
+ * changes, as specific LSMs may be affected.
  */
 static int do_vfs_ioctl(struct file *filp, unsigned int fd,
 			unsigned int cmd, unsigned long arg)
-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH v14 01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

[Kent Overstreet]

On Fri, Apr 05, 2024 at 09:40:29PM +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> Why this is safe:
> 
> Before this change, fs/ioctl.c would unsuccessfully attempt calling the
> IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
> return the same error code immediately, but save ourselves the fallback
> attempt.
> 
> Motivation:
> 
> This simplifies the logic for these IOCTL commands and lets us reason about
> the side effects of these IOCTLs more easily.  It will be possible to
> permit these IOCTLs under LSM IOCTL policies, without having to worry about
> them getting dispatched to problematic device drivers (which sometimes do
> work before looking at the IOCTL command number).
> 
> Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Günther Noack <gnoack@google.com>

Acked-by: Kent Overstreet <kent.overstreet@linux.dev>

> ---
>  fs/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d5abfdf0f22..fb0628e680c4 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -769,7 +769,7 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  	struct fsuuid2 u = { .len = sb->s_uuid_len, };
>  
>  	if (!sb->s_uuid_len)
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
>  
> @@ -781,7 +781,7 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
>  	struct super_block *sb = file_inode(file)->i_sb;
>  
>  	if (!strlen(sb->s_sysfs_name))
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	struct fs_sysfs_path u = {};
>  
> -- 
> 2.44.0.478.gd926399ef9-goog
> 

Re: (subset) [PATCH v14 01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

[Christian Brauner]

On Fri, 05 Apr 2024 21:40:29 +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> [...]

Taking this as a bugfix for this cycle.

---

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail
        https://git.kernel.org/vfs/vfs/c/abe6acfa7d7b

Re: (subset) [PATCH v14 01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

[Mickaël Salaün]

On Tue, Apr 09, 2024 at 12:08:23PM +0200, Christian Brauner wrote:
> On Fri, 05 Apr 2024 21:40:29 +0000, Günther Noack wrote:
> > These IOCTL commands should be implemented by setting attributes on the
> > superblock, rather than in the IOCTL hooks in struct file_operations.
> > 
> > By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> > logic to return -ENOTTY immediately, rather than attempting to call
> > f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> > 
> > [...]
> 
> Taking this as a bugfix for this cycle.

Looks good.

FYI, I added the following tags and re-formated the commit message in my
next branch, so it is already in linux-next (but I'll remove it when
yours will be merged):
https://git.kernel.org/mic/c/5b5c05340e67d1127a3839d1ccb7d7acbb7b9a82

Fixes: 41bcbe59c3b3 ("fs: FS_IOC_GETUUID")
Fixes: ae8c51175730 ("fs: add FS_IOC_GETFSSYSFSPATH")

You can also add:
Acked-by: Mickaël Salaün <mic@digikod.net>

> 
> ---
> 
> Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> Patches in the vfs.fixes branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.fixes
> 
> [01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail
>         https://git.kernel.org/vfs/vfs/c/abe6acfa7d7b
> 

Re: [PATCH v14 02/12] landlock: Add IOCTL access right for character and block devices

[Mickaël Salaün]

I like this patch very much! This patch series is in linux-next and I
don't expect it to change much. Just a few comments below and for test
patches.

The only remaining question is: should we allow non-device files to
receive the LANDLOCK_ACCESS_FS_IOCTL_DEV right?

On Fri, Apr 05, 2024 at 09:40:30PM +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.
> 
> 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                |  38 +++-
>  security/landlock/fs.c                       | 221 ++++++++++++++++++-

You contributed a lot and you may want to add a copyright in this file.

>  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, 259 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 25c8d7677539..68625e728f43 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,33 @@ 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:
> + *
> + *   * IOCTL commands targeting file descriptors (``FIOCLEX``, ``FIONCLEX``),
> + *   * IOCTL commands targeting file descriptions (``FIONBIO``, ``FIOASYNC``),
> + *   * IOCTL commands targeting file systems (``FIFREEZE``, ``FITHAW``,
> + *     ``FIGETBSZ``, ``FS_IOC_GETFSUUID``, ``FS_IOC_GETFSSYSFSPATH``)
> + *   * Some IOCTL commands which do not make sense when used with devices, but
> + *     whose implementations are safe and return the right error codes
> + *     (``FS_IOC_FIEMAP``, ``FICLONE``, ``FICLONERANGE``, ``FIDEDUPERANGE``)
> + *
> + *   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 +244,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 c15559432d3d..b0857541d5e0 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -7,6 +7,7 @@
>   * Copyright © 2021-2022 Microsoft Corporation
>   */
>  
> +#include <asm/ioctls.h>
>  #include <kunit/test.h>
>  #include <linux/atomic.h>
>  #include <linux/bitops.h>
> @@ -14,6 +15,7 @@
>  #include <linux/compiler_types.h>
>  #include <linux/dcache.h>
>  #include <linux/err.h>
> +#include <linux/falloc.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -29,6 +31,7 @@
>  #include <linux/types.h>
>  #include <linux/wait_bit.h>
>  #include <linux/workqueue.h>
> +#include <uapi/linux/fiemap.h>
>  #include <uapi/linux/landlock.h>
>  
>  #include "common.h"
> @@ -84,6 +87,158 @@ static const struct landlock_object_underops landlock_fs_underops = {
>  	.release = release_inode
>  };
>  
> +/* IOCTL helpers */
> +
> +/**
> + * is_masked_device_ioctl(): Determine whether an IOCTL command is always
> + * permitted with Landlock for device files.  These commands can not be
> + * restricted on device files by enforcing a Landlock policy.
> + *
> + * @cmd: The IOCTL command that is supposed to be run.
> + *
> + * By default, any IOCTL on a device file requires the
> + * LANDLOCK_ACCESS_FS_IOCTL_DEV right.  However, we blanket-permit some
> + * commands, if:
> + *
> + * 1. The command is implemented in fs/ioctl.c's do_vfs_ioctl(),
> + *    not in f_ops->unlocked_ioctl() or f_ops->compat_ioctl().
> + *
> + * 2. The command is harmless when invoked on devices.
> + *
> + * We also permit commands that do not make sense for devices, but where the
> + * do_vfs_ioctl() implementation returns a more conventional error code.
> + *
> + * Any new IOCTL commands that are implemented in fs/ioctl.c's do_vfs_ioctl()
> + * should be considered for inclusion here.
> + *
> + * Returns: true if the IOCTL @cmd can not be restricted with Landlock for
> + * device files.
> + */

Great documentation!

> +static __attribute_const__ bool is_masked_device_ioctl(const unsigned int cmd)
> +{
> +	switch (cmd) {
> +	/*
> +	 * FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's
> +	 * close-on-exec and the file's buffered-IO and async flags.  These
> +	 * operations are also available through fcntl(2), and are
> +	 * unconditionally permitted in Landlock.
> +	 */
> +	case FIOCLEX:
> +	case FIONCLEX:
> +	case FIONBIO:
> +	case FIOASYNC:
> +	/*
> +	 * FIOQSIZE queries the size of a regular file, directory, or link.
> +	 *
> +	 * We still permit it, because it always returns -ENOTTY for
> +	 * other file types.
> +	 */
> +	case FIOQSIZE:
> +	/*
> +	 * FIFREEZE and FITHAW freeze and thaw the file system which the
> +	 * given file belongs to.  Requires CAP_SYS_ADMIN.
> +	 *
> +	 * These commands operate on the file system's superblock rather
> +	 * than on the file itself.  The same operations can also be
> +	 * done through any other file or directory on the same file
> +	 * system, so it is safe to permit these.
> +	 */
> +	case FIFREEZE:
> +	case FITHAW:
> +	/*
> +	 * FS_IOC_FIEMAP queries information about the allocation of
> +	 * blocks within a file.
> +	 *
> +	 * This IOCTL command only makes sense for regular files and is
> +	 * not implemented by devices. It is harmless to permit.
> +	 */
> +	case FS_IOC_FIEMAP:
> +	/*
> +	 * FIGETBSZ queries the file system's block size for a file or
> +	 * directory.
> +	 *
> +	 * This command operates on the file system's superblock rather
> +	 * than on the file itself.  The same operation can also be done
> +	 * through any other file or directory on the same file system,
> +	 * so it is safe to permit it.
> +	 */
> +	case FIGETBSZ:
> +	/*
> +	 * FICLONE, FICLONERANGE and FIDEDUPERANGE make files share
> +	 * their underlying storage ("reflink") between source and
> +	 * destination FDs, on file systems which support that.
> +	 *
> +	 * These IOCTL commands only apply to regular files
> +	 * and are harmless to permit for device files.
> +	 */
> +	case FICLONE:
> +	case FICLONERANGE:
> +	case FIDEDUPERANGE:

> +	/*
> +	 * FIONREAD, FS_IOC_GETFLAGS, FS_IOC_SETFLAGS, FS_IOC_FSGETXATTR and
> +	 * FS_IOC_FSSETXATTR are forwarded to device implementations.
> +	 */

The above comment should be better near the file_ioctl() one.

> +
> +	/*
> +	 * FS_IOC_GETFSUUID and FS_IOC_GETFSSYSFSPATH both operate on
> +	 * the file system superblock, not on the specific file, so
> +	 * these operations are available through any other file on the
> +	 * same file system as well.
> +	 */
> +	case FS_IOC_GETFSUUID:
> +	case FS_IOC_GETFSSYSFSPATH:
> +		return true;
> +
> +	/*
> +	 * file_ioctl() commands (FIBMAP, FS_IOC_RESVSP, FS_IOC_RESVSP64,
> +	 * FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64 and FS_IOC_ZERO_RANGE) are
> +	 * forwarded to device implementations, so not permitted.
> +	 */
> +
> +	/* Other commands are guarded by the access right. */
> +	default:
> +		return false;
> +	}
> +}
> +
> +/*
> + * is_masked_device_ioctl_compat - same as the helper above, but checking the
> + * "compat" IOCTL commands.
> + *
> + * The IOCTL commands with special handling in compat-mode should behave the
> + * same as their non-compat counterparts.
> + */
> +static __attribute_const__ bool
> +is_masked_device_ioctl_compat(const unsigned int cmd)
> +{
> +	switch (cmd) {
> +	/* FICLONE is permitted, same as in the non-compat variant. */
> +	case FICLONE:
> +		return true;

A new line before and after if/endif would be good.

> +#if defined(CONFIG_X86_64)
> +	/*
> +	 * FS_IOC_RESVSP_32, FS_IOC_RESVSP64_32, FS_IOC_UNRESVSP_32,
> +	 * FS_IOC_UNRESVSP64_32, FS_IOC_ZERO_RANGE_32: not blanket-permitted,
> +	 * for consistency with their non-compat variants.
> +	 */
> +	case FS_IOC_RESVSP_32:
> +	case FS_IOC_RESVSP64_32:
> +	case FS_IOC_UNRESVSP_32:
> +	case FS_IOC_UNRESVSP64_32:
> +	case FS_IOC_ZERO_RANGE_32:
> +#endif
> +	/*
> +	 * FS_IOC32_GETFLAGS, FS_IOC32_SETFLAGS are forwarded to their device
> +	 * implementations.
> +	 */
> +	case FS_IOC32_GETFLAGS:
> +	case FS_IOC32_SETFLAGS:
> +		return false;
> +	default:
> +		return is_masked_device_ioctl(cmd);
> +	}
> +}
> +
>  /* Ruleset management */
>  
>  static struct landlock_object *get_inode_object(struct inode *const inode)

Re: [PATCH v14 01/12] fs: Return ENOTTY directly if FS_IOC_GETUUID or FS_IOC_GETFSSYSFSPATH fail

[Mickaël Salaün]

Could we have a test that failed if this patch is not applied?  I'm not
sure this is possible because it would require a device file to handle
FS_IOC_GETUUID, which wouldn't be worth it implementing for this test.


On Fri, Apr 05, 2024 at 09:40:29PM +0000, Günther Noack wrote:
> These IOCTL commands should be implemented by setting attributes on the
> superblock, rather than in the IOCTL hooks in struct file_operations.
> 
> By returning -ENOTTY instead of -ENOIOCTLCMD, we instruct the fs/ioctl.c
> logic to return -ENOTTY immediately, rather than attempting to call
> f_op->unlocked_ioctl() or f_op->compat_ioctl() as a fallback.
> 
> Why this is safe:
> 
> Before this change, fs/ioctl.c would unsuccessfully attempt calling the
> IOCTL hooks, and then return -ENOTTY.  By returning -ENOTTY directly, we
> return the same error code immediately, but save ourselves the fallback
> attempt.
> 
> Motivation:
> 
> This simplifies the logic for these IOCTL commands and lets us reason about
> the side effects of these IOCTLs more easily.  It will be possible to
> permit these IOCTLs under LSM IOCTL policies, without having to worry about
> them getting dispatched to problematic device drivers (which sometimes do
> work before looking at the IOCTL command number).
> 
> Link: https://lore.kernel.org/all/cnwpkeovzbumhprco7q2c2y6zxzmxfpwpwe3tyy6c3gg2szgqd@vfzjaw5v5imr/
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  fs/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d5abfdf0f22..fb0628e680c4 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -769,7 +769,7 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  	struct fsuuid2 u = { .len = sb->s_uuid_len, };
>  
>  	if (!sb->s_uuid_len)
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
>  
> @@ -781,7 +781,7 @@ static int ioctl_get_fs_sysfs_path(struct file *file, void __user *argp)
>  	struct super_block *sb = file_inode(file)->i_sb;
>  
>  	if (!strlen(sb->s_sysfs_name))
> -		return -ENOIOCTLCMD;
> +		return -ENOTTY;
>  
>  	struct fs_sysfs_path u = {};
>  
> -- 
> 2.44.0.478.gd926399ef9-goog
> 
> 

Re: [PATCH v14 03/12] selftests/landlock: Test IOCTL support

[Mickaël Salaün]

On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> 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 | 227 ++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 418ad745a5dd..8a72e26d4977 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c

> +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);

Why /dev/tty? Could we use /dev/null or something less tied to the
current context and less sensitive?

> +	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(0, ioctl(file_fd, FIGETBSZ, &flag));
> +
> +	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, ioctl(dir_fd, FIGETBSZ, &flag));
> +
> +	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",

Same here (and elsewhere), /dev/null or a similar harmless device file
would be better.

> +			.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");

Can we avoid using SKIP() in this case?  Tests should only be skipped
when not supported, in other words, we should be able to run all tests
with the right combination of architecture and kernel options.

> +	}
> +
> +	/* 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(0, ioctl(file_fd, FIGETBSZ, &flag));
> +
> +	ASSERT_EQ(0, close(file_fd));
> +}
> +
>  /* clang-format off */
>  FIXTURE(layout1_bind) {};
>  /* clang-format on */
> -- 
> 2.44.0.478.gd926399ef9-goog
> 
> 

Re: [PATCH v14 07/12] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets

[Mickaël Salaün]

On Fri, Apr 05, 2024 at 09:40:35PM +0000, Günther Noack wrote:

Please add a small patch description.  You can list the name of the
test.

> 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 | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 215f0e8bcd69..10b29a288e9c 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>
>  
> @@ -3978,6 +3980,55 @@ 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)));

I'd prefer not to have this kind of assignment and check at the same
time.

> +
> +	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;
> +
> +	ASSERT_LE(0, (cli_fd = socket(AF_UNIX, SOCK_STREAM, 0)));

Same here.

> +
> +	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.478.gd926399ef9-goog
> 
> 

Re: [PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

[Mickaël Salaün]

On Fri, Apr 05, 2024 at 09:40:36PM +0000, Günther Noack wrote:
> This test checks all IOCTL commands implemented in do_vfs_ioctl().
> 
> 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 | 95 ++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 10b29a288e9c..e4ba149cf6fd 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -10,6 +10,7 @@
>  #define _GNU_SOURCE
>  #include <asm/termbits.h>
>  #include <fcntl.h>
> +#include <linux/fiemap.h>
>  #include <linux/landlock.h>
>  #include <linux/magic.h>
>  #include <sched.h>
> @@ -3937,6 +3938,100 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
>  	ASSERT_EQ(0, close(fd));
>  }
>  
> +/*
> + * ioctl_error - generically call the given ioctl with a pointer to a
> + * sufficiently large memory region
> + *
> + * Returns the IOCTLs error, or 0.
> + */
> +static int ioctl_error(int fd, unsigned int cmd)
> +{
> +	char buf[1024]; /* sufficiently large */

Could we shrink a bit this buffer?

> +	int res = ioctl(fd, cmd, &buf);
> +
> +	if (res < 0)
> +		return errno;
> +
> +	return 0;
> +}
> +
> +/* Define some linux/falloc.h IOCTL commands which are not available in uapi headers. */
> +struct space_resv {
> +	__s16 l_type;
> +	__s16 l_whence;
> +	__s64 l_start;
> +	__s64 l_len; /* len == 0 means until end of file */
> +	__s32 l_sysid;
> +	__u32 l_pid;
> +	__s32 l_pad[4]; /* reserved area */
> +};
> +
> +#define FS_IOC_RESVSP _IOW('X', 40, struct space_resv)
> +#define FS_IOC_UNRESVSP _IOW('X', 41, struct space_resv)
> +#define FS_IOC_RESVSP64 _IOW('X', 42, struct space_resv)
> +#define FS_IOC_UNRESVSP64 _IOW('X', 43, struct space_resv)
> +#define FS_IOC_ZERO_RANGE _IOW('X', 57, struct space_resv)
> +
> +/*
> + * Tests a series of blanket-permitted and denied IOCTLs.
> + */
> +TEST_F_FORK(layout1, blanket_permitted_ioctls)
> +{
> +	const struct landlock_ruleset_attr attr = {
> +		.handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> +	};
> +	int ruleset_fd, 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));
> +
> +	fd = open("/dev/null", O_RDWR | O_CLOEXEC);
> +	ASSERT_LE(0, fd);
> +
> +	/*
> +	 * Checks permitted commands.
> +	 * These ones may return errors, but should not be blocked by Landlock.
> +	 */
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIOCLEX));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIONCLEX));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIONBIO));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIOASYNC));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIOQSIZE));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIFREEZE));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FITHAW));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_FIEMAP));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIGETBSZ));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONE));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONERANGE));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FIDEDUPERANGE));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSUUID));
> +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));

Could we check for ENOTTY instead of !EACCES? /dev/null should be pretty
stable.

> +
> +	/*
> +	 * Checks blocked commands.
> +	 * A call to a blocked IOCTL command always returns EACCES.
> +	 */
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FIONREAD));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_GETFLAGS));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_SETFLAGS));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSGETXATTR));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_FSSETXATTR));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FIBMAP));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_RESVSP64));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_UNRESVSP64));
> +	EXPECT_EQ(EACCES, ioctl_error(fd, FS_IOC_ZERO_RANGE));

Good!

> +
> +	/* Default case is also blocked. */
> +	EXPECT_EQ(EACCES, ioctl_error(fd, 0xc00ffeee));
> +
> +	ASSERT_EQ(0, close(fd));
> +}
> +
>  /*
>   * Named pipes are not governed by the LANDLOCK_ACCESS_FS_IOCTL_DEV right,
>   * because they are not character or block devices.
> -- 
> 2.44.0.478.gd926399ef9-goog
> 

Re: [PATCH v14 02/12] landlock: Add IOCTL access right for character and block devices

[Günther Noack]

On Fri, Apr 12, 2024 at 05:16:59PM +0200, Mickaël Salaün wrote:
> I like this patch very much! This patch series is in linux-next and I
> don't expect it to change much. Just a few comments below and for test
> patches.

Thanks!

> The only remaining question is: should we allow non-device files to
> receive the LANDLOCK_ACCESS_FS_IOCTL_DEV right?

I think that yes, non-device files should be able to receive the
LANDLOCK_ACCESS_FS_IOCTL_DEV right.  I played through some examples to
ponder this:

It should be possible to grant this access right on a file hierarchy,
for example on /dev/bus/usb to permit IOCTLs on all USB devices.

But such directories can also be empty (e.g. no USB devices plugged
in)!  Asking user space Landlock users to traverse /dev/bus/usb to
look for device files before using Landlock would needlessly
complicate the API, and it would be a race condition anyway, because
devices files can disappear at any time later as well (by unplugging
your mouse and keyboard).

So when applies to a directory, the LANDLOCK_ACCESS_FS_IOCTL_DEV right
already inherently needs to deal with cases where there is not a
single device file within the directory.  (But there can technically
be other files.)

So if the access right can be granted on any directory (with or
without device files), it seems inconsistent that the requirements for
using it on a file within that hierarchy should be stricter than that.

Another data point:

This would also be consistent with the LANDLOCK_ACCESS_FS_EXECUTE
right: This access right only has an effect on files that are marked
executable in the first place, yet the access right can be granted on
non-executable files as well.

To sum up:

* It seems harmless to permit, and the name of the access rights
  already spells out that it has no effect on non-device files.

* It frees the user space libraries from doing up-front file type
  checks.

* It would be consistent with how the access right can be granted on a
  directory (where it really needs to be more flexible, as discussed
  above).

* The LANDLOCK_ACCESS_FS_EXECUTE right has not been a point of
  confusion so far, even though is has similar semantics.

So yes, I think it should be possible to use this access right on
non-device files as well.


> On Fri, Apr 05, 2024 at 09:40:30PM +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.
> > 
> > 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                |  38 +++-
> >  security/landlock/fs.c                       | 221 ++++++++++++++++++-
> 
> You contributed a lot and you may want to add a copyright in this file.

Thanks, good point.

I'll add the Google copyright and will also retroactively add the
copyright for the truncate contributions going back to 2022.


> > +static __attribute_const__ bool is_masked_device_ioctl(const unsigned int cmd)
> > +{
> > +   [...]
> > +	case FICLONE:
> > +	case FICLONERANGE:
> > +	case FIDEDUPERANGE:
> 
> > +	/*
> > +	 * FIONREAD, FS_IOC_GETFLAGS, FS_IOC_SETFLAGS, FS_IOC_FSGETXATTR and
> > +	 * FS_IOC_FSSETXATTR are forwarded to device implementations.
> > +	 */
> 
> The above comment should be better near the file_ioctl() one.

Done.


> > +static __attribute_const__ bool
> > +is_masked_device_ioctl_compat(const unsigned int cmd)
> > +{
> > +	switch (cmd) {
> > +	/* FICLONE is permitted, same as in the non-compat variant. */
> > +	case FICLONE:
> > +		return true;
> 
> A new line before and after if/endif would be good.

Done.

> 
> > +#if defined(CONFIG_X86_64)
> > +	/*

Re: [PATCH v14 03/12] selftests/landlock: Test IOCTL support

[Günther Noack]

On Fri, Apr 12, 2024 at 05:17:44PM +0200, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> > 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 | 227 ++++++++++++++++++++-
> >  1 file changed, 224 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 418ad745a5dd..8a72e26d4977 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> 
> > +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);
> 
> Why /dev/tty? Could we use /dev/null or something less tied to the
> current context and less sensitive?

Absolutely, good point -- I'm switching to /dev/zero.

I am dropping the TCGETS test and only keeping FIONREAD,
but these two IOCTL commands work the same way as far as Landlock is concerned.


> > +TEST_F_FORK(ioctl, handle_file_access_file)
> > +{
> > +	const int flag = 0;
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = "/dev/tty0",
> 
> Same here (and elsewhere), /dev/null or a similar harmless device file
> would be better.

Ditto, /dev/zero it is.


> > +	if (variant->allowed & LANDLOCK_ACCESS_FS_READ_DIR) {
> > +		SKIP(return, "LANDLOCK_ACCESS_FS_READ_DIR "
> > +			     "can not be granted on files");
> 
> Can we avoid using SKIP() in this case?  Tests should only be skipped
> when not supported, in other words, we should be able to run all tests
> with the right combination of architecture and kernel options.

Good point.  After double checking, I realized that this was left over from an
earlier test where we still had more fixture variants.  I'm removing that check.


Thanks for the review!
—Günther

Re: [PATCH v14 07/12] selftests/landlock: Check IOCTL restrictions for named UNIX domain sockets

[Günther Noack]

On Fri, Apr 12, 2024 at 05:17:54PM +0200, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:35PM +0000, Günther Noack wrote:
> 
> Please add a small patch description.  You can list the name of the
> test.

Done - I explained what the test checks for (that the access right should have
no effect on named UNIX domain sockets).

> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index 215f0e8bcd69..10b29a288e9c 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c

> > +	ASSERT_LE(0, (srv_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
> 
> I'd prefer not to have this kind of assignment and check at the same
> time.

Done.


> > +	ASSERT_LE(0, (cli_fd = socket(AF_UNIX, SOCK_STREAM, 0)));
> 
> Same here.

Done.

—Günther

Re: [PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

[Günther Noack]

On Fri, Apr 12, 2024 at 05:18:06PM +0200, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:36PM +0000, Günther Noack wrote:
> > +static int ioctl_error(int fd, unsigned int cmd)
> > +{
> > +	char buf[1024]; /* sufficiently large */
> 
> Could we shrink a bit this buffer?

Shrunk to 128.

I'm also zeroing the buffer now, which was missing before,
to make the behaviour deterministic.


> > +	int res = ioctl(fd, cmd, &buf);
> > +
> > +	if (res < 0)
> > +		return errno;
> > +
> > +	return 0;
> > +}


> > +TEST_F_FORK(layout1, blanket_permitted_ioctls)
> > +{
> > +   [...]
> > +	/*
> > +	 * Checks permitted commands.
> > +	 * These ones may return errors, but should not be blocked by Landlock.
> > +	 */
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOCLEX));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONCLEX));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONBIO));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOASYNC));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOQSIZE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIFREEZE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FITHAW));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_FIEMAP));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIGETBSZ));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONERANGE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FIDEDUPERANGE));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSUUID));
> > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));
> 
> Could we check for ENOTTY instead of !EACCES? /dev/null should be pretty
> stable.

The expected results are all over the place, unfortunately.
When I tried it, I got this:

        EXPECT_EQ(0, ioctl_error(fd, FIOCLEX));
        EXPECT_EQ(0, ioctl_error(fd, FIONCLEX));
        EXPECT_EQ(0, ioctl_error(fd, FIONBIO));
        EXPECT_EQ(0, ioctl_error(fd, FIOASYNC));
        EXPECT_EQ(ENOTTY, ioctl_error(fd, FIOQSIZE));
        EXPECT_EQ(EPERM, ioctl_error(fd, FIFREEZE));
        EXPECT_EQ(EPERM, ioctl_error(fd, FITHAW));
        EXPECT_EQ(EOPNOTSUPP, ioctl_error(fd, FS_IOC_FIEMAP));
        EXPECT_EQ(0, ioctl_error(fd, FIGETBSZ));
        EXPECT_EQ(EBADF, ioctl_error(fd, FICLONE));
        EXPECT_EQ(EXDEV, ioctl_error(fd, FICLONERANGE));  // <----
        EXPECT_EQ(EINVAL, ioctl_error(fd, FIDEDUPERANGE));
        EXPECT_EQ(0, ioctl_error(fd, FS_IOC_GETFSUUID));
        EXPECT_EQ(ENOTTY, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));

I find this difficult to read and it distracts from the main point, which
is that we got past the Landlock check which would have returned an EACCES.

I spotted an additional problem with FICLONERANGE -- when we pass a
zero-initialized buffer to that IOCTL, it'll interpret some of these zeros
to refer to file descriptor 0 (stdin)... and what that means is not
controlled by the test - the error code can change depending on what that
FD is.  (I don't want to start filling in all these structs individually.)

The only thing that really matters to us is that the result is not EACCES
(==> we have gotten past the Landlock policy check).  Testing the exact
behaviour of all of these IOCTLs is maybe stepping too much on the turf of
these IOCTL implementations and making the test more brittle towards
cahnges unrelated to Landlock than they need to be [1].

So, if you are OK with that, I would prefer to keep these checks using
EXPECT_NE(EACCES, ...).

—Günther

[1] https://abseil.io/resources/swe-book/html/ch12.html has a discussion on
    why to avoid brittle tests (written about Google, but applicable here
    as well, IMHO)

Re: [PATCH v14 02/12] landlock: Add IOCTL access right for character and block devices

[Mickaël Salaün]

On Thu, Apr 18, 2024 at 11:28:00AM +0200, Günther Noack wrote:
> On Fri, Apr 12, 2024 at 05:16:59PM +0200, Mickaël Salaün wrote:
> > I like this patch very much! This patch series is in linux-next and I
> > don't expect it to change much. Just a few comments below and for test
> > patches.
> 
> Thanks!
> 
> > The only remaining question is: should we allow non-device files to
> > receive the LANDLOCK_ACCESS_FS_IOCTL_DEV right?
> 
> I think that yes, non-device files should be able to receive the
> LANDLOCK_ACCESS_FS_IOCTL_DEV right.  I played through some examples to
> ponder this:
> 
> It should be possible to grant this access right on a file hierarchy,
> for example on /dev/bus/usb to permit IOCTLs on all USB devices.
> 
> But such directories can also be empty (e.g. no USB devices plugged
> in)!  Asking user space Landlock users to traverse /dev/bus/usb to
> look for device files before using Landlock would needlessly
> complicate the API, and it would be a race condition anyway, because
> devices files can disappear at any time later as well (by unplugging
> your mouse and keyboard).
> 
> So when applies to a directory, the LANDLOCK_ACCESS_FS_IOCTL_DEV right
> already inherently needs to deal with cases where there is not a
> single device file within the directory.  (But there can technically
> be other files.)
> 
> So if the access right can be granted on any directory (with or
> without device files), it seems inconsistent that the requirements for
> using it on a file within that hierarchy should be stricter than that.

Yes, directories should be able to receive all access rights because of
file hierarchies. I was thinking about char/block devices vs.
regular/fifo/socket/symlink files.

> 
> Another data point:
> 
> This would also be consistent with the LANDLOCK_ACCESS_FS_EXECUTE
> right: This access right only has an effect on files that are marked
> executable in the first place, yet the access right can be granted on
> non-executable files as well.

I would say that LANDLOCK_ACCESS_FS_EXECUTE can be granted on fifo, but
I get your point.

> 
> To sum up:
> 
> * It seems harmless to permit, and the name of the access rights
>   already spells out that it has no effect on non-device files.
> 
> * It frees the user space libraries from doing up-front file type
>   checks.
> 
> * It would be consistent with how the access right can be granted on a
>   directory (where it really needs to be more flexible, as discussed
>   above).
> 
> * The LANDLOCK_ACCESS_FS_EXECUTE right has not been a point of
>   confusion so far, even though is has similar semantics.
> 
> So yes, I think it should be possible to use this access right on
> non-device files as well.

OK

> 
> 
> > On Fri, Apr 05, 2024 at 09:40:30PM +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.
> > > 
> > > 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                |  38 +++-
> > >  security/landlock/fs.c                       | 221 ++++++++++++++++++-
> > 
> > You contributed a lot and you may want to add a copyright in this file.
> 
> Thanks, good point.
> 
> I'll add the Google copyright and will also retroactively add the
> copyright for the truncate contributions going back to 2022.

Good

> 
> 
> > > +static __attribute_const__ bool is_masked_device_ioctl(const unsigned int cmd)
> > > +{
> > > +   [...]
> > > +	case FICLONE:
> > > +	case FICLONERANGE:
> > > +	case FIDEDUPERANGE:
> > 
> > > +	/*
> > > +	 * FIONREAD, FS_IOC_GETFLAGS, FS_IOC_SETFLAGS, FS_IOC_FSGETXATTR and
> > > +	 * FS_IOC_FSSETXATTR are forwarded to device implementations.
> > > +	 */
> > 
> > The above comment should be better near the file_ioctl() one.
> 
> Done.
> 
> 
> > > +static __attribute_const__ bool
> > > +is_masked_device_ioctl_compat(const unsigned int cmd)
> > > +{
> > > +	switch (cmd) {
> > > +	/* FICLONE is permitted, same as in the non-compat variant. */
> > > +	case FICLONE:
> > > +		return true;
> > 
> > A new line before and after if/endif would be good.
> 
> Done.
> 
> > 
> > > +#if defined(CONFIG_X86_64)
> > > +	/*
> 

Re: [PATCH v14 03/12] selftests/landlock: Test IOCTL support

[Mickaël Salaün]

On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> 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 | 227 ++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 418ad745a5dd..8a72e26d4977 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c

> @@ -3831,6 +3842,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_fs_ioc_getflags_ioctl() should be moved to the next patch where it
is used.

Re: [PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

[Mickaël Salaün]

On Thu, Apr 18, 2024 at 02:21:49PM +0200, Günther Noack wrote:
> On Fri, Apr 12, 2024 at 05:18:06PM +0200, Mickaël Salaün wrote:
> > On Fri, Apr 05, 2024 at 09:40:36PM +0000, Günther Noack wrote:
> > > +static int ioctl_error(int fd, unsigned int cmd)
> > > +{
> > > +	char buf[1024]; /* sufficiently large */
> > 
> > Could we shrink a bit this buffer?
> 
> Shrunk to 128.
> 
> I'm also zeroing the buffer now, which was missing before,
> to make the behaviour deterministic.
> 
> 
> > > +	int res = ioctl(fd, cmd, &buf);
> > > +
> > > +	if (res < 0)
> > > +		return errno;
> > > +
> > > +	return 0;
> > > +}
> 
> 
> > > +TEST_F_FORK(layout1, blanket_permitted_ioctls)
> > > +{
> > > +   [...]
> > > +	/*
> > > +	 * Checks permitted commands.
> > > +	 * These ones may return errors, but should not be blocked by Landlock.
> > > +	 */
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOCLEX));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONCLEX));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIONBIO));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOASYNC));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIOQSIZE));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIFREEZE));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FITHAW));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_FIEMAP));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIGETBSZ));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONE));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FICLONERANGE));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FIDEDUPERANGE));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSUUID));
> > > +	EXPECT_NE(EACCES, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));
> > 
> > Could we check for ENOTTY instead of !EACCES? /dev/null should be pretty
> > stable.
> 
> The expected results are all over the place, unfortunately.
> When I tried it, I got this:
> 
>         EXPECT_EQ(0, ioctl_error(fd, FIOCLEX));
>         EXPECT_EQ(0, ioctl_error(fd, FIONCLEX));
>         EXPECT_EQ(0, ioctl_error(fd, FIONBIO));
>         EXPECT_EQ(0, ioctl_error(fd, FIOASYNC));
>         EXPECT_EQ(ENOTTY, ioctl_error(fd, FIOQSIZE));
>         EXPECT_EQ(EPERM, ioctl_error(fd, FIFREEZE));
>         EXPECT_EQ(EPERM, ioctl_error(fd, FITHAW));
>         EXPECT_EQ(EOPNOTSUPP, ioctl_error(fd, FS_IOC_FIEMAP));
>         EXPECT_EQ(0, ioctl_error(fd, FIGETBSZ));
>         EXPECT_EQ(EBADF, ioctl_error(fd, FICLONE));
>         EXPECT_EQ(EXDEV, ioctl_error(fd, FICLONERANGE));  // <----
>         EXPECT_EQ(EINVAL, ioctl_error(fd, FIDEDUPERANGE));
>         EXPECT_EQ(0, ioctl_error(fd, FS_IOC_GETFSUUID));
>         EXPECT_EQ(ENOTTY, ioctl_error(fd, FS_IOC_GETFSSYSFSPATH));
> 
> I find this difficult to read and it distracts from the main point, which
> is that we got past the Landlock check which would have returned an EACCES.

OK

> 
> I spotted an additional problem with FICLONERANGE -- when we pass a
> zero-initialized buffer to that IOCTL, it'll interpret some of these zeros
> to refer to file descriptor 0 (stdin)... and what that means is not
> controlled by the test - the error code can change depending on what that
> FD is.  (I don't want to start filling in all these structs individually.)

I'm OK with your approach as long as the tests are deterministic,
whatever FD 0 is (or not), and as long at they don't have an impact on
FD 0.  To make it more generic and to avoid side effects, I think we
should (always) close FD 0 in ioctl_error() (and explain the reason).

> 
> The only thing that really matters to us is that the result is not EACCES
> (==> we have gotten past the Landlock policy check).  Testing the exact
> behaviour of all of these IOCTLs is maybe stepping too much on the turf of
> these IOCTL implementations and making the test more brittle towards
> cahnges unrelated to Landlock than they need to be [1].
> 
> So, if you are OK with that, I would prefer to keep these checks using
> EXPECT_NE(EACCES, ...).

Yes, it looks good.

> 
> —Günther
> 
> [1] https://abseil.io/resources/swe-book/html/ch12.html has a discussion on
>     why to avoid brittle tests (written about Google, but applicable here
>     as well, IMHO)
> 

Re: [PATCH v14 03/12] selftests/landlock: Test IOCTL support

[Günther Noack]

On Thu, Apr 18, 2024 at 10:44:00PM -0700, Mickaël Salaün wrote:
> On Fri, Apr 05, 2024 at 09:40:31PM +0000, Günther Noack wrote:
> > +/* 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_fs_ioc_getflags_ioctl() should be moved to the next patch where it
> is used.

Thanks, done.

Re: [PATCH v14 08/12] selftests/landlock: Exhaustive test for the IOCTL allow-list

[Günther Noack]

On Thu, Apr 18, 2024 at 10:44:43PM -0700, Mickaël Salaün wrote:
> On Thu, Apr 18, 2024 at 02:21:49PM +0200, Günther Noack wrote:
> > I spotted an additional problem with FICLONERANGE -- when we pass a
> > zero-initialized buffer to that IOCTL, it'll interpret some of these zeros
> > to refer to file descriptor 0 (stdin)... and what that means is not
> > controlled by the test - the error code can change depending on what that
> > FD is.  (I don't want to start filling in all these structs individually.)
> 
> I'm OK with your approach as long as the tests are deterministic,
> whatever FD 0 is (or not), and as long at they don't have an impact on
> FD 0.  To make it more generic and to avoid side effects, I think we
> should (always) close FD 0 in ioctl_error() (and explain the reason).

Done, good idea.

—Günther