| Message ID | 20231103155717.78042-1-gnoack@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > Hello! > > These patches add simple ioctl(2) support to Landlock. > > Objective > ~~~~~~~~~ > > Make ioctl(2) requests restrictable with Landlock, > in a way that is useful for real-world applications. > > Proposed approach > ~~~~~~~~~~~~~~~~~ > > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use > of ioctl(2) on file descriptors. > > We attach IOCTL access rights to opened file descriptors, as we > already do for LANDLOCK_ACCESS_FS_TRUNCATE. > > If LANDLOCK_ACCESS_FS_IOCTL is handled (restricted in the ruleset), > the LANDLOCK_ACCESS_FS_IOCTL access right governs the use of all IOCTL > commands. > > We make an exception for the common and known-harmless IOCTL commands > FIOCLEX, FIONCLEX, FIONBIO and FIONREAD. These IOCTL commands are > always permitted. Their functionality is already available through > fcntl(2). > > If additionally(!), the access rights LANDLOCK_ACCESS_FS_READ_FILE, > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are > handled, these access rights also unlock some IOCTL commands which are > considered safe for use with files opened in these ways. > > As soon as these access rights are handled, the affected IOCTL > commands can not be permitted through LANDLOCK_ACCESS_FS_IOCTL any > more, but only be permitted through the respective more specific > access rights. A full list of these access rights is listed below in > this cover letter and in the documentation. > > I believe that this approach works for the majority of use cases, and > offers a good trade-off between Landlock API and implementation > complexity and flexibility when the feature is used. > > Current limitations > ~~~~~~~~~~~~~~~~~~~ > > With this patch set, ioctl(2) requests can *not* be filtered based on > file type, device number (dev_t) or on the ioctl(2) request number. > > On the initial RFC patch set [1], we have reached consensus to start > with this simpler coarse-grained approach, and build additional IOCTL > restriction capabilities on top in subsequent steps. > > [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ > > Notable implications of this approach > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > * Existing inherited file descriptors stay unaffected > when a program enables Landlock. > > This means in particular that in common scenarios, > the terminal's IOCTLs (ioctl_tty(2)) continue to work. > > * ioctl(2) continues to be available for file descriptors acquired > through means other than open(2). Example: Network sockets, > memfd_create(2), file descriptors that are already open before the > Landlock ruleset is enabled. > > Examples > ~~~~~~~~ > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > > LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash > > The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights > here, so we expect that newly opened files outside of $HOME don't work > with most IOCTL commands. > > * "stty" works: It probes terminal properties > > * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is > denied. > > * "eject" fails: ioctls to use CD-ROM drive are denied. > > * "ls /dev" works: It uses ioctl to get the terminal size for > columnar layout > > * The text editors "vim" and "mg" work. (GNU Emacs fails because it > attempts to reopen /dev/tty.) > > IOCTL groups > ~~~~~~~~~~~~ > > To decide which IOCTL commands should be blanket-permitted we went > through the list of IOCTL commands mentioned in fs/ioctl.c and looked > at them individually to understand what they are about. The following > list is for reference. > > We should always allow the following IOCTL commands, which are also > available through fcntl(2) with the F_SETFD and F_SETFL commands: > > * FIOCLEX, FIONCLEX - these work on the file descriptor and > manipulate the close-on-exec flag > * FIONBIO, FIOASYNC - these work on the struct file and enable > nonblocking-IO and async flags > > The following command is guarded and enabled by either of > LANDLOCK_ACCESS_FS_WRITE_FILE, LANDLOCK_ACCESS_FS_READ_FILE or > LANDLOCK_ACCESS_FS_READ_DIR (G2), once one of them is handled > (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > * FIOQSIZE - get the size of the opened file > > The following commands are guarded and enabled by either of > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_FILE (G2), > once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > These are commands that read file system internals: > > * FS_IOC_FIEMAP - get information about file extent mapping > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > * FIBMAP - get a file's file system block number > * FIGETBSZ - get file system blocksize > > The following commands are guarded and enabled by > LANDLOCK_ACCESS_FS_READ_FILE (G3), if it is handled (otherwise by > LANDLOCK_ACCESS_FS_IOCTL): > > * FIONREAD - get the number of bytes available for reading (the > implementation is defined per file type) > * FIDEDUPRANGE - manipulating shared physical storage between files. > > The following commands are guarded and enabled by > LANDLOCK_ACCESS_FS_WRITE_FILE (G4), if it is handled (otherwise by > LANDLOCK_ACCESS_FS_IOCTL): > > * FICLONE, FICLONERANGE - making files share physical storage between > multiple files. These only work on some file systems, by design. > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS > preallocation syscalls which predate fallocate(2). > > The following commands are also mentioned in fs/ioctl.c, but are not > handled specially and are managed by LANDLOCK_ACCESS_FS_IOCTL together > with all other remaining IOCTL commands: > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > system. Requires CAP_SYS_ADMIN. > * Accessing file attributes: > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes This looks great! It would be nice to copy these IOCTL descriptions to the user documentation too. That would help explain the rationale and let users know that they should not be worried about the related IOCTLs. > > Open questions > ~~~~~~~~~~~~~~ > > This is unlikely to be the last iteration, but we are getting closer. > > Some notable open questions are: > > * Code style > > * Should we move the IOCTL access right expansion logic into the > outer layers in syscall.c? Where it currently lives in > ruleset.h, this logic feels too FS-specific, and it introduces > the additional complication that we now have to track which > access_mask_t-s are already expanded and which are not. It might > be simpler to do the expansion earlier. What about creating a new helper in fs.c that expands the FS access rights, something like this: int landlock_expand_fs_access(access_mask_t *access_mask) { if (!*access_mask) return -ENOMSG; *access_mask = expand_all_ioctl(*access_mask, *access_mask); return 0; } And in syscalls.c: err = landlock_expand_fs_access(&ruleset_attr.handled_access_fs); if (err) return err; /* Checks arguments and transforms to kernel struct. */ ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, ruleset_attr.handled_access_net); And patch the landlock_create_ruleset() helper with that: - if (!fs_access_mask && !net_access_mask) + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) return ERR_PTR(-ENOMSG); > > * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names. Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that these are in fact (synthetic) access rights? I'm not sure we can find better than GROUP because even the content of these groups might change in the future with new access rights. > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > should this grant the permission to use *any* IOCTL? (Right now, > it is any IOCTL except for the ones covered by the IOCTL groups, > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > becomes smaller when other access rights are also handled. Are you suggesting to handle differently this right if it is applied to a directory? If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should be OK. But maybe we should rename this right to something like LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles IOCTLs that are not handled by other access rights? > > * Backwards compatibility for user-space libraries. > > This is not documented yet, because it is currently not necessary > yet. But as soon as we have a hypothetical Landlock ABI v6 with a > new IOCTL-enabled "GFX" access right, the "best effort" downgrade > from v6 to v5 becomes more involved: If the caller handles > GFX+IOCTL and permits GFX on a file, the correct downgrade to make > this work on a Landlock v5 kernel is to handle IOCTL only, and > permit IOCTL(!). I don't see any issue to this approach. If there is no way to handle GFX in v5, then there is nothing more we can do than allowing GFX (on the same file). Another way to say it is that in v5 we allow any IOCTL (including GFX ones) on the GFX files, an in v6 we *need* replace this IOCTL right with the newly available GFX right, *if it is handled* by the ruleset. If GFX would not be tied to a file, I think it would not be a good design for this access right. Currently all access rights are tied to objects/data, or relative to the sandbox (e.g. ptrace).
On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote: > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > > Hello! > > > > These patches add simple ioctl(2) support to Landlock. > > > > Objective > > ~~~~~~~~~ > > > > Make ioctl(2) requests restrictable with Landlock, > > in a way that is useful for real-world applications. > > > > Proposed approach > > ~~~~~~~~~~~~~~~~~ > > > > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use > > of ioctl(2) on file descriptors. > > > > We attach IOCTL access rights to opened file descriptors, as we > > already do for LANDLOCK_ACCESS_FS_TRUNCATE. > > > > If LANDLOCK_ACCESS_FS_IOCTL is handled (restricted in the ruleset), > > the LANDLOCK_ACCESS_FS_IOCTL access right governs the use of all IOCTL > > commands. > > > > We make an exception for the common and known-harmless IOCTL commands > > FIOCLEX, FIONCLEX, FIONBIO and FIONREAD. These IOCTL commands are > > always permitted. Their functionality is already available through > > fcntl(2). > > > > If additionally(!), the access rights LANDLOCK_ACCESS_FS_READ_FILE, > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are > > handled, these access rights also unlock some IOCTL commands which are > > considered safe for use with files opened in these ways. > > > > As soon as these access rights are handled, the affected IOCTL > > commands can not be permitted through LANDLOCK_ACCESS_FS_IOCTL any > > more, but only be permitted through the respective more specific > > access rights. A full list of these access rights is listed below in > > this cover letter and in the documentation. > > > > I believe that this approach works for the majority of use cases, and > > offers a good trade-off between Landlock API and implementation > > complexity and flexibility when the feature is used. > > > > Current limitations > > ~~~~~~~~~~~~~~~~~~~ > > > > With this patch set, ioctl(2) requests can *not* be filtered based on > > file type, device number (dev_t) or on the ioctl(2) request number. > > > > On the initial RFC patch set [1], we have reached consensus to start > > with this simpler coarse-grained approach, and build additional IOCTL > > restriction capabilities on top in subsequent steps. > > > > [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ > > > > Notable implications of this approach > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > * Existing inherited file descriptors stay unaffected > > when a program enables Landlock. > > > > This means in particular that in common scenarios, > > the terminal's IOCTLs (ioctl_tty(2)) continue to work. > > > > * ioctl(2) continues to be available for file descriptors acquired > > through means other than open(2). Example: Network sockets, > > memfd_create(2), file descriptors that are already open before the > > Landlock ruleset is enabled. > > > > Examples > > ~~~~~~~~ > > > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > > > > LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash > > > > The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights > > here, so we expect that newly opened files outside of $HOME don't work > > with most IOCTL commands. > > > > * "stty" works: It probes terminal properties > > > > * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is > > denied. > > > > * "eject" fails: ioctls to use CD-ROM drive are denied. > > > > * "ls /dev" works: It uses ioctl to get the terminal size for > > columnar layout > > > > * The text editors "vim" and "mg" work. (GNU Emacs fails because it > > attempts to reopen /dev/tty.) > > > > IOCTL groups > > ~~~~~~~~~~~~ > > > > To decide which IOCTL commands should be blanket-permitted we went > > through the list of IOCTL commands mentioned in fs/ioctl.c and looked > > at them individually to understand what they are about. The following > > list is for reference. > > > > We should always allow the following IOCTL commands, which are also > > available through fcntl(2) with the F_SETFD and F_SETFL commands: > > > > * FIOCLEX, FIONCLEX - these work on the file descriptor and > > manipulate the close-on-exec flag > > * FIONBIO, FIOASYNC - these work on the struct file and enable > > nonblocking-IO and async flags > > > > The following command is guarded and enabled by either of > > LANDLOCK_ACCESS_FS_WRITE_FILE, LANDLOCK_ACCESS_FS_READ_FILE or > > LANDLOCK_ACCESS_FS_READ_DIR (G2), once one of them is handled > > (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > > > * FIOQSIZE - get the size of the opened file > > > > The following commands are guarded and enabled by either of > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_FILE (G2), > > once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): > > > > These are commands that read file system internals: > > > > * FS_IOC_FIEMAP - get information about file extent mapping > > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > > * FIBMAP - get a file's file system block number > > * FIGETBSZ - get file system blocksize > > > > The following commands are guarded and enabled by > > LANDLOCK_ACCESS_FS_READ_FILE (G3), if it is handled (otherwise by > > LANDLOCK_ACCESS_FS_IOCTL): > > > > * FIONREAD - get the number of bytes available for reading (the > > implementation is defined per file type) > > * FIDEDUPRANGE - manipulating shared physical storage between files. > > > > The following commands are guarded and enabled by > > LANDLOCK_ACCESS_FS_WRITE_FILE (G4), if it is handled (otherwise by > > LANDLOCK_ACCESS_FS_IOCTL): > > > > * FICLONE, FICLONERANGE - making files share physical storage between > > multiple files. These only work on some file systems, by design. > > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS > > preallocation syscalls which predate fallocate(2). > > > > The following commands are also mentioned in fs/ioctl.c, but are not > > handled specially and are managed by LANDLOCK_ACCESS_FS_IOCTL together > > with all other remaining IOCTL commands: > > > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > > system. Requires CAP_SYS_ADMIN. > > * Accessing file attributes: > > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes > > This looks great! > > It would be nice to copy these IOCTL descriptions to the user > documentation too. That would help explain the rationale and let users > know that they should not be worried about the related IOCTLs. Added. > > Open questions > > ~~~~~~~~~~~~~~ > > > > This is unlikely to be the last iteration, but we are getting closer. > > > > Some notable open questions are: > > > > * Code style > > > > * Should we move the IOCTL access right expansion logic into the > > outer layers in syscall.c? Where it currently lives in > > ruleset.h, this logic feels too FS-specific, and it introduces > > the additional complication that we now have to track which > > access_mask_t-s are already expanded and which are not. It might > > be simpler to do the expansion earlier. > > What about creating a new helper in fs.c that expands the FS access > rights, something like this: > > int landlock_expand_fs_access(access_mask_t *access_mask) > { > if (!*access_mask) > return -ENOMSG; > > *access_mask = expand_all_ioctl(*access_mask, *access_mask); > return 0; > } > > > And in syscalls.c: > > err = > landlock_expand_fs_access(&ruleset_attr.handled_access_fs); > if (err) > return err; > > /* Checks arguments and transforms to kernel struct. */ > ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, > ruleset_attr.handled_access_net); Done, this looks good. I called the landlock_expand_fs_access function slightly differently and made it return the resulting access_mask_t (because it does not make a performance difference, and then there is no potential for calling it with a null pointer, and the function does not need to return an error). As a consequence of doing it like this, I also moved the expansion functions into fs.c, away from ruleset.h where they did not fit in. :) > And patch the landlock_create_ruleset() helper with that: > > - if (!fs_access_mask && !net_access_mask) > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) > return ERR_PTR(-ENOMSG); Why would you want to warn on the case where fs_access_mask is zero? Is it not a legitimate use case to use Landlock for the network aspect only? (If a user is not handling any of the LANDLOCK_ACCESS_FS* rights, the expansion step is not going to add any.) > > * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names. > > Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that > these are in fact (synthetic) access rights? > > I'm not sure we can find better than GROUP because even the content of > these groups might change in the future with new access rights. Makes sense, renamed as suggested. TBH, IOCTL_CMD_G1...4 was more of a placeholder anyway because I was so lazy with my typing. ;) > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > > should this grant the permission to use *any* IOCTL? (Right now, > > it is any IOCTL except for the ones covered by the IOCTL groups, > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > > becomes smaller when other access rights are also handled. > > Are you suggesting to handle differently this right if it is applied to > a directory? No - this applies to files as well. I am suggesting that granting LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access to *all* ioctls, both the ones in the synthetic groups and the remaining ones. Let me spell out the scenario: Steps to reproduce: - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE - permit: LANDLOCK_ACCESS_FS_IOCTL on file f - open file f (for write-only) - attempt to use ioctl(fd, FIOQSIZE, ...) With this patch set: - ioctl(fd, FIOQSIZE, ...) fails, because FIOQSIZE is part of IOCTL_CMD_G1 and because LANDLOCK_ACCESS_FS_READ_FILE is handled, IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE Alternative proposal: - ioctl(fd, FIOQSIZE, ...) should maybe work, because LANDLOCK_ACCESS_FS_IOCTL is permitted on f Implementation-wise, this would mean to add expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups) to expand_all_ioctl(). I feel that this alternative might be less surprising, because granting the IOCTL right would grant all the things that were restricted when handling the IOCTL right, and it would be more "symmetric". What do you think? > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > be OK. But maybe we should rename this right to something like > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > IOCTLs that are not handled by other access rights? Hmm, I'm not convinced this is a good name. It makes sense in the context of allowing "all the other ioctls" for a file or file hierarchy, but when setting LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, so "default" doesn't seem appropriate to me. > > * Backwards compatibility for user-space libraries. > > > > This is not documented yet, because it is currently not necessary > > yet. But as soon as we have a hypothetical Landlock ABI v6 with a > > new IOCTL-enabled "GFX" access right, the "best effort" downgrade > > from v6 to v5 becomes more involved: If the caller handles > > GFX+IOCTL and permits GFX on a file, the correct downgrade to make > > this work on a Landlock v5 kernel is to handle IOCTL only, and > > permit IOCTL(!). > > I don't see any issue to this approach. If there is no way to handle GFX > in v5, then there is nothing more we can do than allowing GFX (on the > same file). Another way to say it is that in v5 we allow any IOCTL > (including GFX ones) on the GFX files, an in v6 we *need* replace this > IOCTL right with the newly available GFX right, *if it is handled* by > the ruleset. > > If GFX would not be tied to a file, I think it would not be a good > design for this access right. Currently all access rights are tied to > objects/data, or relative to the sandbox (e.g. ptrace). Yes, makes sense - we are aligned then. —Günther
On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote: > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote: > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > > > Open questions > > > ~~~~~~~~~~~~~~ > > > > > > This is unlikely to be the last iteration, but we are getting closer. > > > > > > Some notable open questions are: > > > > > > * Code style > > > > > > * Should we move the IOCTL access right expansion logic into the > > > outer layers in syscall.c? Where it currently lives in > > > ruleset.h, this logic feels too FS-specific, and it introduces > > > the additional complication that we now have to track which > > > access_mask_t-s are already expanded and which are not. It might > > > be simpler to do the expansion earlier. > > > > What about creating a new helper in fs.c that expands the FS access > > rights, something like this: > > > > int landlock_expand_fs_access(access_mask_t *access_mask) > > { > > if (!*access_mask) > > return -ENOMSG; > > > > *access_mask = expand_all_ioctl(*access_mask, *access_mask); > > return 0; > > } > > > > > > And in syscalls.c: > > > > err = > > landlock_expand_fs_access(&ruleset_attr.handled_access_fs); > > if (err) > > return err; > > > > /* Checks arguments and transforms to kernel struct. */ > > ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, > > ruleset_attr.handled_access_net); > > Done, this looks good. > > I called the landlock_expand_fs_access function slightly differently and made it > return the resulting access_mask_t (because it does not make a performance > difference, and then there is no potential for calling it with a null pointer, > and the function does not need to return an error). > > As a consequence of doing it like this, I also moved the expansion functions > into fs.c, away from ruleset.h where they did not fit in. :) > > > > And patch the landlock_create_ruleset() helper with that: > > > > - if (!fs_access_mask && !net_access_mask) > > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) > > return ERR_PTR(-ENOMSG); > > Why would you want to warn on the case where fs_access_mask is zero? Because in my suggestion the real check is moved/copied to landlock_expand_fs_access(), which is called before, and it should then not be possible to have this case here. > > Is it not a legitimate use case to use Landlock for the network aspect only? > > (If a user is not handling any of the LANDLOCK_ACCESS_FS* rights, the expansion > step is not going to add any.) Correct > > > > > * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names. > > > > Why not something like LANDLOCK_ACCESS_FS_IOCTL_GROUP* to highlight that > > these are in fact (synthetic) access rights? > > > > I'm not sure we can find better than GROUP because even the content of > > these groups might change in the future with new access rights. > > Makes sense, renamed as suggested. TBH, IOCTL_CMD_G1...4 was more of a > placeholder anyway because I was so lazy with my typing. ;) > > > > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > > > should this grant the permission to use *any* IOCTL? (Right now, > > > it is any IOCTL except for the ones covered by the IOCTL groups, > > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > > > becomes smaller when other access rights are also handled. > > > > Are you suggesting to handle differently this right if it is applied to > > a directory? > > No - this applies to files as well. I am suggesting that granting > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access > to *all* ioctls, both the ones in the synthetic groups and the remaining ones. > > Let me spell out the scenario: > > Steps to reproduce: > - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE > - permit: LANDLOCK_ACCESS_FS_IOCTL > on file f > - open file f (for write-only) > - attempt to use ioctl(fd, FIOQSIZE, ...) > > With this patch set: > - ioctl(fd, FIOQSIZE, ...) fails, > because FIOQSIZE is part of IOCTL_CMD_G1 > and because LANDLOCK_ACCESS_FS_READ_FILE is handled, > IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE Correct, and it looks consistent to me. > > Alternative proposal: > - ioctl(fd, FIOQSIZE, ...) should maybe work, > because LANDLOCK_ACCESS_FS_IOCTL is permitted on f > > Implementation-wise, this would mean to add > > expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups) > > to expand_all_ioctl(). > > I feel that this alternative might be less surprising, because granting the > IOCTL right would grant all the things that were restricted when handling the > IOCTL right, and it would be more "symmetric". > > What do you think? I though that we discussed about that and we agree that it was the way to go. Cf. the table of handled/allowed/not-allowed. Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case of a directory but not a file? These would be two different semantics. > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > > be OK. But maybe we should rename this right to something like > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > > IOCTLs that are not handled by other access rights? > > Hmm, I'm not convinced this is a good name. It makes sense in the context of > allowing "all the other ioctls" for a file or file hierarchy, but when setting > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, > so "default" doesn't seem appropriate to me. It should turn off all IOCTLs that are not handled by another access right. The handled access rights should be expanded the same way as the allowed access rights. > > > > > * Backwards compatibility for user-space libraries. > > > > > > This is not documented yet, because it is currently not necessary > > > yet. But as soon as we have a hypothetical Landlock ABI v6 with a > > > new IOCTL-enabled "GFX" access right, the "best effort" downgrade > > > from v6 to v5 becomes more involved: If the caller handles > > > GFX+IOCTL and permits GFX on a file, the correct downgrade to make > > > this work on a Landlock v5 kernel is to handle IOCTL only, and > > > permit IOCTL(!). > > > > I don't see any issue to this approach. If there is no way to handle GFX > > in v5, then there is nothing more we can do than allowing GFX (on the > > same file). Another way to say it is that in v5 we allow any IOCTL > > (including GFX ones) on the GFX files, an in v6 we *need* replace this > > IOCTL right with the newly available GFX right, *if it is handled* by > > the ruleset. > > > > If GFX would not be tied to a file, I think it would not be a good > > design for this access right. Currently all access rights are tied to > > objects/data, or relative to the sandbox (e.g. ptrace). > > Yes, makes sense - we are aligned then. > > —Günther
On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote: > On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote: > > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote: > > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > > > And patch the landlock_create_ruleset() helper with that: > > > > > > - if (!fs_access_mask && !net_access_mask) > > > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) > > > return ERR_PTR(-ENOMSG); > > > > Why would you want to warn on the case where fs_access_mask is zero? > > Because in my suggestion the real check is moved/copied to > landlock_expand_fs_access(), which is called before, and it should then > not be possible to have this case here. Oh, I see, I misread that code. I guess it does not apply to the version that we ended up with. > > > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > > > > should this grant the permission to use *any* IOCTL? (Right now, > > > > it is any IOCTL except for the ones covered by the IOCTL groups, > > > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > > > > becomes smaller when other access rights are also handled. > > > > > > Are you suggesting to handle differently this right if it is applied to > > > a directory? > > > > No - this applies to files as well. I am suggesting that granting > > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access > > to *all* ioctls, both the ones in the synthetic groups and the remaining ones. > > > > Let me spell out the scenario: > > > > Steps to reproduce: > > - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE > > - permit: LANDLOCK_ACCESS_FS_IOCTL > > on file f > > - open file f (for write-only) > > - attempt to use ioctl(fd, FIOQSIZE, ...) > > > > With this patch set: > > - ioctl(fd, FIOQSIZE, ...) fails, > > because FIOQSIZE is part of IOCTL_CMD_G1 > > and because LANDLOCK_ACCESS_FS_READ_FILE is handled, > > IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE > > Correct, and it looks consistent to me. > > > > > Alternative proposal: > > - ioctl(fd, FIOQSIZE, ...) should maybe work, > > because LANDLOCK_ACCESS_FS_IOCTL is permitted on f > > > > Implementation-wise, this would mean to add > > > > expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups) > > > > to expand_all_ioctl(). > > > > I feel that this alternative might be less surprising, because granting the > > IOCTL right would grant all the things that were restricted when handling the > > IOCTL right, and it would be more "symmetric". > > > > What do you think? > > I though that we discussed about that and we agree that it was the way > to go. Cf. the table of handled/allowed/not-allowed. We can go with the current implementation as well, I don't feel very strongly about it. > Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case > of a directory but not a file? These would be two different semantics. If the ruleset were enforced in that proposal, as in the example above, it would not distinguish whether the affected filesystem paths are files or directories. If LANDLOCK_ACCESS_FS_IOCTL is handled, the semantics would be: * If you permit LANDLOCK_ACCESS_FS_READ_FILE on a directory or file, it would become possible to use these ioctl commands on the affected files which are relevant and harmless for reading files. (As before) * If you permit LANDLOCK_ACCESS_FS_IOCTL on a directory or file, it would become possible to use *all* ioctl commands on the affected files. (That is the difference. In the current implementation, this only affects the ioctl commands which are *not* in the synthetic groups. In the alternative proposal, it would affect *all* ioctl commands. I think this might be simpler to reason about, because the set of ioctl commands which are affected by permitting(!) LANDLOCK_ACCESS_FS_IOCTL would always be the same (namely, all ioctl commands), and it would not be dependent on whether other access rights are handled.) I don't think it is at odds with the backwards-compatibility concerns which we previously discussed. The synthetic groups still exist, it's just the "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a different set of IOCTL commands. > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > > > be OK. But maybe we should rename this right to something like > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > > > IOCTLs that are not handled by other access rights? > > > > Hmm, I'm not convinced this is a good name. It makes sense in the context of > > allowing "all the other ioctls" for a file or file hierarchy, but when setting > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, > > so "default" doesn't seem appropriate to me. > > It should turn off all IOCTLs that are not handled by another access > right. The handled access rights should be expanded the same way as the > allowed access rights. If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files or directories, all IOCTL commands will be forbidden, independent of what else is handled. The opposite is not true: If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work. So if you see it through that lens, you could say that it is only the LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL commands at all. I hope this makes sense. It's not my intent to open this backwards-compatibility can of worms from scratch... :) —Günther
On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote: > On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote: > > On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote: > > > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote: > > > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote: > > > > And patch the landlock_create_ruleset() helper with that: > > > > > > > > - if (!fs_access_mask && !net_access_mask) > > > > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask) > > > > return ERR_PTR(-ENOMSG); > > > > > > Why would you want to warn on the case where fs_access_mask is zero? > > > > Because in my suggestion the real check is moved/copied to > > landlock_expand_fs_access(), which is called before, and it should then > > not be possible to have this case here. > > Oh, I see, I misread that code. I guess it does not apply to the version that > we ended up with. > > > > > > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, > > > > > should this grant the permission to use *any* IOCTL? (Right now, > > > > > it is any IOCTL except for the ones covered by the IOCTL groups, > > > > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL > > > > > becomes smaller when other access rights are also handled. > > > > > > > > Are you suggesting to handle differently this right if it is applied to > > > > a directory? > > > > > > No - this applies to files as well. I am suggesting that granting > > > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access > > > to *all* ioctls, both the ones in the synthetic groups and the remaining ones. > > > > > > Let me spell out the scenario: > > > > > > Steps to reproduce: > > > - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE > > > - permit: LANDLOCK_ACCESS_FS_IOCTL > > > on file f > > > - open file f (for write-only) > > > - attempt to use ioctl(fd, FIOQSIZE, ...) > > > > > > With this patch set: > > > - ioctl(fd, FIOQSIZE, ...) fails, > > > because FIOQSIZE is part of IOCTL_CMD_G1 > > > and because LANDLOCK_ACCESS_FS_READ_FILE is handled, > > > IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE > > > > Correct, and it looks consistent to me. > > > > > > > > Alternative proposal: > > > - ioctl(fd, FIOQSIZE, ...) should maybe work, > > > because LANDLOCK_ACCESS_FS_IOCTL is permitted on f > > > > > > Implementation-wise, this would mean to add > > > > > > expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups) > > > > > > to expand_all_ioctl(). > > > > > > I feel that this alternative might be less surprising, because granting the > > > IOCTL right would grant all the things that were restricted when handling the > > > IOCTL right, and it would be more "symmetric". > > > > > > What do you think? > > > > I though that we discussed about that and we agree that it was the way > > to go. Cf. the table of handled/allowed/not-allowed. > > We can go with the current implementation as well, I don't feel very strongly > about it. > > > > Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case > > of a directory but not a file? These would be two different semantics. > > If the ruleset were enforced in that proposal, as in the example above, it would > not distinguish whether the affected filesystem paths are files or directories. > > If LANDLOCK_ACCESS_FS_IOCTL is handled, the semantics would be: > > * If you permit LANDLOCK_ACCESS_FS_READ_FILE on a directory or file, > it would become possible to use these ioctl commands on the affected files > which are relevant and harmless for reading files. (As before) > > * If you permit LANDLOCK_ACCESS_FS_IOCTL on a directory or file, > it would become possible to use *all* ioctl commands on the affected files. > > (That is the difference. In the current implementation, this only affects > the ioctl commands which are *not* in the synthetic groups. In the > alternative proposal, it would affect *all* ioctl commands. > > I think this might be simpler to reason about, because the set of ioctl > commands which are affected by permitting(!) LANDLOCK_ACCESS_FS_IOCTL would > always be the same (namely, all ioctl commands), and it would not be > dependent on whether other access rights are handled.) > > > I don't think it is at odds with the backwards-compatibility concerns which we > previously discussed. The synthetic groups still exist, it's just the > "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a > different set of IOCTL commands. It would not be a backward-compatibility issue, but it would make LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with new handled access rights should better inform end encourage developers to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed. It would be useful to explain this rationale in the commit message. See https://lore.kernel.org/all/20231020.moefooYeV9ei@digikod.net/ > > > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > > > > be OK. But maybe we should rename this right to something like > > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > > > > IOCTLs that are not handled by other access rights? > > > > > > Hmm, I'm not convinced this is a good name. It makes sense in the context of > > > allowing "all the other ioctls" for a file or file hierarchy, but when setting > > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, > > > so "default" doesn't seem appropriate to me. > > > > It should turn off all IOCTLs that are not handled by another access > > right. The handled access rights should be expanded the same way as the > > allowed access rights. > > If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files > or directories, all IOCTL commands will be forbidden, independent of what else > is handled. > > The opposite is not true: > > If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle > LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work. > > So if you see it through that lens, you could say that it is only the > LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL > commands at all. Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by default. However, to allow IOCTLs, developers may need to allow LANDLOCK_ACCESS_FS_IOCTL or another access right according to the underlying semantic. It would be useful to add an example with your table in the documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file or not...). > > > I hope this makes sense. It's not my intent to open this > backwards-compatibility can of worms from scratch... :) > > —Günther >
On Thu, Nov 30, 2023 at 10:26:47AM +0100, Mickaël Salaün wrote: > On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote: > > On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote: > > I don't think it is at odds with the backwards-compatibility concerns which we > > previously discussed. The synthetic groups still exist, it's just the > > "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a > > different set of IOCTL commands. > > It would not be a backward-compatibility issue, but it would make > LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access > rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with > new handled access rights should better inform end encourage developers > to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed. > It would be useful to explain this rationale in the commit message. > See https://lore.kernel.org/all/20231020.moefooYeV9ei@digikod.net/ Done; I added a section to the commit message. > > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > > > > > be OK. But maybe we should rename this right to something like > > > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > > > > > IOCTLs that are not handled by other access rights? > > > > > > > > Hmm, I'm not convinced this is a good name. It makes sense in the context of > > > > allowing "all the other ioctls" for a file or file hierarchy, but when setting > > > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, > > > > so "default" doesn't seem appropriate to me. > > > > > > It should turn off all IOCTLs that are not handled by another access > > > right. The handled access rights should be expanded the same way as the > > > allowed access rights. > > > > If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files > > or directories, all IOCTL commands will be forbidden, independent of what else > > is handled. > > > > The opposite is not true: > > > > If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle > > LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work. > > > > So if you see it through that lens, you could say that it is only the > > LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL > > commands at all. > > Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by > default. However, to allow IOCTLs, developers may need to allow > LANDLOCK_ACCESS_FS_IOCTL or another access right according to the > underlying semantic. > > It would be useful to add an example with your table in the > documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled > or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file > or not...). Added an example to the documentation. —Günther
Hello! These patches add simple ioctl(2) support to Landlock. Objective ~~~~~~~~~ Make ioctl(2) requests restrictable with Landlock, in a way that is useful for real-world applications. Proposed approach ~~~~~~~~~~~~~~~~~ Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use of ioctl(2) on file descriptors. We attach IOCTL access rights to opened file descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE. If LANDLOCK_ACCESS_FS_IOCTL is handled (restricted in the ruleset), the LANDLOCK_ACCESS_FS_IOCTL access right governs the use of all IOCTL commands. We make an exception for the common and known-harmless IOCTL commands FIOCLEX, FIONCLEX, FIONBIO and FIONREAD. These IOCTL commands are always permitted. Their functionality is already available through fcntl(2). If additionally(!), the access rights LANDLOCK_ACCESS_FS_READ_FILE, LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled, these access rights also unlock some IOCTL commands which are considered safe for use with files opened in these ways. As soon as these access rights are handled, the affected IOCTL commands can not be permitted through LANDLOCK_ACCESS_FS_IOCTL any more, but only be permitted through the respective more specific access rights. A full list of these access rights is listed below in this cover letter and in the documentation. I believe that this approach works for the majority of use cases, and offers a good trade-off between Landlock API and implementation complexity and flexibility when the feature is used. Current limitations ~~~~~~~~~~~~~~~~~~~ With this patch set, ioctl(2) requests can *not* be filtered based on file type, device number (dev_t) or on the ioctl(2) request number. On the initial RFC patch set [1], we have reached consensus to start with this simpler coarse-grained approach, and build additional IOCTL restriction capabilities on top in subsequent steps. [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ Notable implications of this approach ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * Existing inherited file descriptors stay unaffected when a program enables Landlock. This means in particular that in common scenarios, the terminal's IOCTLs (ioctl_tty(2)) continue to work. * ioctl(2) continues to be available for file descriptors acquired through means other than open(2). Example: Network sockets, memfd_create(2), file descriptors that are already open before the Landlock ruleset is enabled. Examples ~~~~~~~~ Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights here, so we expect that newly opened files outside of $HOME don't work with most IOCTL commands. * "stty" works: It probes terminal properties * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is denied. * "eject" fails: ioctls to use CD-ROM drive are denied. * "ls /dev" works: It uses ioctl to get the terminal size for columnar layout * The text editors "vim" and "mg" work. (GNU Emacs fails because it attempts to reopen /dev/tty.) IOCTL groups ~~~~~~~~~~~~ To decide which IOCTL commands should be blanket-permitted we went through the list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually to understand what they are about. The following list is for reference. We should always allow the following IOCTL commands, which are also available through fcntl(2) with the F_SETFD and F_SETFL commands: * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the close-on-exec flag * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO and async flags The following command is guarded and enabled by either of LANDLOCK_ACCESS_FS_WRITE_FILE, LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_READ_DIR (G2), once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): * FIOQSIZE - get the size of the opened file The following commands are guarded and enabled by either of LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_FILE (G2), once one of them is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): These are commands that read file system internals: * FS_IOC_FIEMAP - get information about file extent mapping (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) * FIBMAP - get a file's file system block number * FIGETBSZ - get file system blocksize The following commands are guarded and enabled by LANDLOCK_ACCESS_FS_READ_FILE (G3), if it is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): * FIONREAD - get the number of bytes available for reading (the implementation is defined per file type) * FIDEDUPRANGE - manipulating shared physical storage between files. The following commands are guarded and enabled by LANDLOCK_ACCESS_FS_WRITE_FILE (G4), if it is handled (otherwise by LANDLOCK_ACCESS_FS_IOCTL): * FICLONE, FICLONERANGE - making files share physical storage between multiple files. These only work on some file systems, by design. * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation syscalls which predate fallocate(2). The following commands are also mentioned in fs/ioctl.c, but are not handled specially and are managed by LANDLOCK_ACCESS_FS_IOCTL together with all other remaining IOCTL commands: * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file system. Requires CAP_SYS_ADMIN. * Accessing file attributes: * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes Open questions ~~~~~~~~~~~~~~ This is unlikely to be the last iteration, but we are getting closer. Some notable open questions are: * Code style * Should we move the IOCTL access right expansion logic into the outer layers in syscall.c? Where it currently lives in ruleset.h, this logic feels too FS-specific, and it introduces the additional complication that we now have to track which access_mask_t-s are already expanded and which are not. It might be simpler to do the expansion earlier. * Rename IOCTL_CMD_G1, ..., IOCTL_CMD_G4 and give them better names. * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy, should this grant the permission to use *any* IOCTL? (Right now, it is any IOCTL except for the ones covered by the IOCTL groups, and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL becomes smaller when other access rights are also handled. * Backwards compatibility for user-space libraries. This is not documented yet, because it is currently not necessary yet. But as soon as we have a hypothetical Landlock ABI v6 with a new IOCTL-enabled "GFX" access right, the "best effort" downgrade from v6 to v5 becomes more involved: If the caller handles GFX+IOCTL and permits GFX on a file, the correct downgrade to make this work on a Landlock v5 kernel is to handle IOCTL only, and permit IOCTL(!). Related Work ~~~~~~~~~~~~ OpenBSD's pledge(2) [2] restricts ioctl(2) independent of the file descriptor which is used. The implementers maintain multiple allow-lists of predefined ioctl(2) operations required for different application domains such as "audio", "bpf", "tty" and "inet". OpenBSD does not guarantee ABI backwards compatibility to the same extent as Linux does, so it's easier for them to update these lists in later versions. It might not be a feasible approach for Linux though. [2] https://man.openbsd.org/OpenBSD-7.3/pledge.2 Changes ~~~~~~~ V4: * use "synthetic" IOCTL access rights, as previously discussed * testing changes * use a large fixture-based test, for more exhaustive coverage, and replace some of the earlier tests with it * rebased on mic-next V3: * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL * increment ABI version in the same commit where the feature is introduced * testing changes * use FIOQSIZE instead of TTY IOCTL commands (FIOQSIZE works with regular files, directories and memfds) * run the memfd test with both Landlock enabled and disabled * add a test for the always-permitted IOCTL commands V2: * rebased on mic-next * added documentation * exercise ioctl(2) in the memfd test * test: Use layout0 for the test --- V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/ V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/ V3: https://lore.kernel.org/linux-security-module/20230814172816.3907299-1-gnoack@google.com/ Günther Noack (7): landlock: Optimize the number of calls to get_access_mask slightly landlock: Add IOCTL access right selftests/landlock: Test IOCTL support selftests/landlock: Test IOCTL with memfds selftests/landlock: Test ioctl(2) and ftruncate(2) with open(O_PATH) samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL landlock: Document IOCTL support Documentation/userspace-api/landlock.rst | 74 ++- include/uapi/linux/landlock.h | 51 +- samples/landlock/sandboxer.c | 10 +- security/landlock/fs.c | 74 ++- security/landlock/limits.h | 10 +- security/landlock/ruleset.c | 5 +- security/landlock/ruleset.h | 53 +- security/landlock/syscalls.c | 6 +- tools/testing/selftests/landlock/base_test.c | 2 +- tools/testing/selftests/landlock/fs_test.c | 498 ++++++++++++++++++- 10 files changed, 735 insertions(+), 48 deletions(-) base-commit: f12f8f84509a084399444c4422661345a15cc713