| Message ID | 20240405214040.101396-3-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
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)
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) > > + /*
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) > > > + /* >
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 | \
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(-)