On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote: > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > > Hello! > > > > These patches add simple ioctl(2) support to Landlock. > > > > Objective > > ~~~~~~~~~ > > > > Make ioctl(2) requests restrictable with Landlock, > > in a way that is useful for real-world applications. > > > > Proposed approach > > ~~~~~~~~~~~~~~~~~ > > > > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use > > of ioctl(2) on file descriptors. > > > > We attach IOCTL access rights to opened file descriptors, as we > > already do for LANDLOCK_ACCESS_FS_TRUNCATE. > > > > If LANDLOCK_ACCESS_FS_IOCTL is handled (restricted in the ruleset), > > the LANDLOCK_ACCESS_FS_IOCTL access right governs the use of all IOCTL > > commands. > > > > We make an exception for the common and known-harmless IOCTL commands > > FIOCLEX, FIONCLEX, FIONBIO and FIONREAD. These IOCTL commands are > > always permitted. Their functionality is already available through > > fcntl(2). > > > > If additionally(!), the access rights LANDLOCK_ACCESS_FS_READ_FILE, > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are > > handled, these access rights also unlock some IOCTL commands which are > > considered safe for use with files opened in these ways. > > > > As soon as these access rights are handled, the affected IOCTL > > commands can not be permitted through LANDLOCK_ACCESS_FS_IOCTL any > > more, but only be permitted through the respective more specific > > access rights. A full list of these access rights is listed below in > > this cover letter and in the documentation. > > > > I believe that this approach works for the majority of use cases, and > > offers a good trade-off between Landlock API and implementation > > complexity and flexibility when the feature is used. > > > > Current limitations > > ~~~~~~~~~~~~~~~~~~~ > > > > With this patch set, ioctl(2) requests can *not* be filtered based on > > file type, device number (dev_t) or on the ioctl(2) request number. > > > > On the initial RFC patch set [1], we have reached consensus to start > > with this simpler coarse-grained approach, and build additional IOCTL > > restriction capabilities on top in subsequent steps. > > > > [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@xxxxxxxxxxx/ > > > > Notable implications of this approach > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > * Existing inherited file descriptors stay unaffected > > when a program enables Landlock. > > > > This means in particular that in common scenarios, > > the terminal's IOCTLs (ioctl_tty(2)) continue to work. > > > > * ioctl(2) continues to be available for file descriptors acquired > > through means other than open(2). Example: Network sockets, > > memfd_create(2), file descriptors that are already open before the > > Landlock ruleset is enabled. > > > > Examples > > ~~~~~~~~ > > > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > > > > LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash > > > > The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights > > here, so we expect that newly opened files outside of $HOME don't work > > with most IOCTL commands. > > > > * "stty" works: It probes terminal properties > > > > * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is > > denied. > > > > * "eject" fails: ioctls to use CD-ROM drive are denied. > > > > * "ls /dev" works: It uses ioctl to get the terminal size for > > columnar layout > > > > * The text editors "vim" and "mg" work. (GNU Emacs fails because it > > attempts to reopen /dev/tty.) > > > > IOCTL groups > > ~~~~~~~~~~~~ > > > > To decide which IOCTL commands should be blanket-permitted we went > > through the list of IOCTL commands mentioned in fs/ioctl.c and looked > > at them individually to understand what they are about. The following > > list is for reference. > > > > We should always allow the following IOCTL commands, which are also > > available through fcntl(2) with the F_SETFD and F_SETFL commands: > > > > * FIOCLEX, FIONCLEX - these work on the file descriptor and > > manipulate the close-on-exec flag > > * FIONBIO, FIOASYNC - these work on the struct file and enable > > nonblocking-IO and async flags > > > > The following command is guarded and enabled by either of > > LANDLOCK_ACCESS_FS_WRITE_FILE, LANDLOCK_ACCESS_FS_READ_FILE or > > LANDLOCK_ACCESS_FS_READ_DIR (G2), once one of them is handled > > (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > > > * FIOQSIZE - get the size of the opened file > > > > The following commands are guarded and enabled by either of > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_FILE (G2), > > once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > > > These are commands that read file system internals: > > > > * FS_IOC_FIEMAP - get information about file extent mapping > > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > > * FIBMAP - get a file's file system block number > > * FIGETBSZ - get file system blocksize > > > > The following commands are guarded and enabled by > > LANDLOCK_ACCESS_FS_READ_FILE (G3), if it is handled (otherwise by > > LANDLOCK_ACCESS_FS_IOCTL): > > > > * FIONREAD - get the number of bytes available for reading (the > > implementation is defined per file type) > > * FIDEDUPRANGE - manipulating shared physical storage between files. > > > > The following commands are guarded and enabled by > > LANDLOCK_ACCESS_FS_WRITE_FILE (G4), if it is handled (otherwise by > > LANDLOCK_ACCESS_FS_IOCTL): > > > > * FICLONE, FICLONERANGE - making files share physical storage between > > multiple files. These only work on some file systems, by design. > > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS > > preallocation syscalls which predate fallocate(2). > > > > The following commands are also mentioned in fs/ioctl.c, but are not > > handled specially and are managed by LANDLOCK_ACCESS_FS_IOCTL together > > with all other remaining IOCTL commands: > > > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > > system. Requires CAP_SYS_ADMIN. > > * Accessing file attributes: > > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes > > This looks great! > > It would be nice to copy these IOCTL descriptions to the user > documentation too. That would help explain the rationale and let users > know that they should not be worried about the related IOCTLs. Added. > > Open questions > > ~~~~~~~~~~~~~~ > > > > This is unlikely to be the last iteration, but we are getting closer. > > > > Some notable open questions are: > > > > * Code style > > > > * Should we move the IOCTL access right expansion logic into the > > outer layers in syscall.c? Where it currently lives in > > ruleset.h, this logic feels too FS-specific, and it introduces > > the additional complication that we now have to track which > > access_mask_t-s are already expanded and which are not. It might > > be simpler to do the expansion earlier. > > What about creating a new helper in fs.c that expands the FS access > rights, something like this: > > int landlock_expand_fs_access(access_mask_t *access_mask) > { > if (!*access_mask) > return -ENOMSG; > > *access_mask = expand_all_ioctl(*access_mask, *access_mask); > return 0; > } > > > And in syscalls.c: > > err = > landlock_expand_fs_access(&ruleset_attr.handled_access_fs); > if (err) > return err; > > /* Checks arguments and transforms to kernel struct. */ > ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, > ruleset_attr.handled_access_net); Done, this looks good. I called the landlock_expand_fs_access function slightly differently and made it return the resulting access_mask_t (because it does not make a performance difference, and then there is no potential for calling it with a null pointer, and the function does not need to return an error). As a consequence of doing it like this, I also moved the expansion functions into fs.c, away from ruleset.h where they did not fit in. :) > And patch the landlock_create_ruleset() helper with that: > > - if (!fs_access_mask && !net_access_mask) > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) > return ERR_PTR(-ENOMSG); Why would you want to warn on the case where fs_access_mask is zero? Is it not a legitimate use case to use Landlock for the network aspect only? (If a user is not handling any of the LANDLOCK_ACCESS_FS* rights, the expansion step is not going to add any.) > > * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names. > > Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that > these are in fact (synthetic) access rights? > > I'm not sure we can find better than GROUP because even the content of > these groups might change in the future with new access rights. Makes sense, renamed as suggested. TBH, IOCTL_CMD_G1...4 was more of a placeholder anyway because I was so lazy with my typing. ;) > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > > should this grant the permission to use *any* IOCTL? (Right now, > > it is any IOCTL except for the ones covered by the IOCTL groups, > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > > becomes smaller when other access rights are also handled. > > Are you suggesting to handle differently this right if it is applied to > a directory? No - this applies to files as well. I am suggesting that granting LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access to *all* ioctls, both the ones in the synthetic groups and the remaining ones. Let me spell out the scenario: Steps to reproduce: - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE - permit: LANDLOCK_ACCESS_FS_IOCTL on file f - open file f (for write-only) - attempt to use ioctl(fd, FIOQSIZE, ...) With this patch set: - ioctl(fd, FIOQSIZE, ...) fails, because FIOQSIZE is part of IOCTL_CMD_G1 and because LANDLOCK_ACCESS_FS_READ_FILE is handled, IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE Alternative proposal: - ioctl(fd, FIOQSIZE, ...) should maybe work, because LANDLOCK_ACCESS_FS_IOCTL is permitted on f Implementation-wise, this would mean to add expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups) to expand_all_ioctl(). I feel that this alternative might be less surprising, because granting the IOCTL right would grant all the things that were restricted when handling the IOCTL right, and it would be more "symmetric". What do you think? > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > be OK. But maybe we should rename this right to something like > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > IOCTLs that are not handled by other access rights? Hmm, I'm not convinced this is a good name. It makes sense in the context of allowing "all the other ioctls" for a file or file hierarchy, but when setting LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, so "default" doesn't seem appropriate to me. > > * Backwards compatibility for user-space libraries. > > > > This is not documented yet, because it is currently not necessary > > yet. But as soon as we have a hypothetical Landlock ABI v6 with a > > new IOCTL-enabled "GFX" access right, the "best effort" downgrade > > from v6 to v5 becomes more involved: If the caller handles > > GFX+IOCTL and permits GFX on a file, the correct downgrade to make > > this work on a Landlock v5 kernel is to handle IOCTL only, and > > permit IOCTL(!). > > I don't see any issue to this approach. If there is no way to handle GFX > in v5, then there is nothing more we can do than allowing GFX (on the > same file). Another way to say it is that in v5 we allow any IOCTL > (including GFX ones) on the GFX files, an in v6 we *need* replace this > IOCTL right with the newly available GFX right, *if it is handled* by > the ruleset. > > If GFX would not be tied to a file, I think it would not be a good > design for this access right. Currently all access rights are tied to > objects/data, or relative to the sandbox (e.g. ptrace). Yes, makes sense - we are aligned then. —Günther
