This patch series has been in -next for some time now. I just added some tiny cosmetic fixes and a missing (on some distros) C header file. I plan to send it for v6.10 but I'll probably rebase it again because of kselftest changes. It is noteworthy that test coverage dropped by 1.5%: from 92.4% to 90.9% . This is due to the tests not covering the IOCTL compat code. It would be good to find a way to cover this case, probably building 32-bit test binary stubs (to avoid depending on 32-bit libraries). Thanks again! Mickaël On Fri, Apr 19, 2024 at 04:11:11PM +0000, Günther Noack wrote: > 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@xxxxxxxxxxx/ > > 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@xxxxxxxxxx/ > [4] https://lore.kernel.org/all/CAHC9VhRojXNSU9zi2BrP8z6JmOmT3DAqGNtinvvz=tL1XhVdyg@xxxxxxxxxxxxxx/ > [5] https://lore.kernel.org/all/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@xxxxxxxxxxxxxxxx > [6] https://lore.kernel.org/all/20240326.ahyaaPa0ohs6@xxxxxxxxxxx > [7] https://lore.kernel.org/all/CAHC9VhQJFWYeheR-EqqdfCq0YpvcQX5Scjfgcz1q+jrWg8YsdA@xxxxxxxxxxxxxx/ > > > Changes > ~~~~~~~ > > V15: > * Drop the commit about FS_IOC_GETFSUUID / FS_IOC_GETFSSYSFSPATH -- > it is already assumed as a prerequisite now. > * security/landlock/fs.c: > * Add copyright notice for my contributions (also for the truncate > patch set) > * Tests: > * In commit "Test IOCTL support": > * Test with /dev/zero instead of /dev/tty > * Check only FIONREAD instead of both FIONREAD and TCGETS > * Remove a now-unused SKIP() > * In test for Named UNIX Domain Sockets: > * Do not inline variable assignments in ASSERT() usages > * In commit "Exhaustive test for the IOCTL allow-list": > * Make IOCTL results deterministic: > * Zero the input buffer > * Close FD 0 for the ioctl() call, to avoid accidentally using it > * Cosmetic changes and cleanups > * Remove a leftover mention of "synthetic" access rights > * Fix docstring format for is_masked_device_ioctl() > * Newline and comment ordering cleanups as discussed in v14 review > > 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@xxxxxxxxxxx/, > 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@xxxxxxxxxxxxxxxx/ > 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@xxxxxxxxxxx/ > 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+@xxxxxxxxxx > * 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@xxxxxxx>, who pointed this out in > https://lore.kernel.org/all/f25be6663bcc4608adf630509f045a76@xxxxxxx/ > 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@xxxxxxxxx/ > V2: https://lore.kernel.org/all/20230623144329.136541-1-gnoack@xxxxxxxxxx/ > V3: https://lore.kernel.org/all/20230814172816.3907299-1-gnoack@xxxxxxxxxx/ > V4: https://lore.kernel.org/all/20231103155717.78042-1-gnoack@xxxxxxxxxx/ > V5: https://lore.kernel.org/all/20231117154920.1706371-1-gnoack@xxxxxxxxxx/ > V6: https://lore.kernel.org/all/20231124173026.3257122-1-gnoack@xxxxxxxxxx/ > V7: https://lore.kernel.org/all/20231201143042.3276833-1-gnoack@xxxxxxxxxx/ > V8: https://lore.kernel.org/all/20231208155121.1943775-1-gnoack@xxxxxxxxxx/ > V9: https://lore.kernel.org/all/20240209170612.1638517-1-gnoack@xxxxxxxxxx/ > V10: https://lore.kernel.org/all/20240309075320.160128-1-gnoack@xxxxxxxxxx/ > V11: https://lore.kernel.org/all/20240322151002.3653639-1-gnoack@xxxxxxxxxx/ > V12: https://lore.kernel.org/all/20240325134004.4074874-1-gnoack@xxxxxxxxxx/ > V13: https://lore.kernel.org/all/20240327131040.158777-1-gnoack@xxxxxxxxxx/ > V14: https://lore.kernel.org/all/20240405214040.101396-1-gnoack@xxxxxxxxxx/ > > Günther Noack (11): > 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 | 3 + > include/uapi/linux/landlock.h | 38 +- > samples/landlock/sandboxer.c | 13 +- > security/landlock/fs.c | 225 ++++++++- > security/landlock/limits.h | 2 +- > security/landlock/syscalls.c | 2 +- > tools/testing/selftests/landlock/base_test.c | 2 +- > tools/testing/selftests/landlock/fs_test.c | 486 ++++++++++++++++++- > 10 files changed, 805 insertions(+), 43 deletions(-) > > > base-commit: fe611b72031cc211a96cf0b3b58838953950cb13 > -- > 2.44.0.769.g3c40516874-goog > >
