| Message ID | 20240309075320.160128-2-gnoack@google.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@google.com> wrote: > > This LSM hook gets called just before the fs/ioctl.c logic delegates > the requested IOCTL command to the file-specific implementation as > implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat). > > It is impractical for LSMs to make security guarantees about these > f_op operations without having intimate knowledge of how they are > implemented. > > Therefore, depending on the enabled Landlock policy, Landlock aims to > block the calls to filp->f_op->unlocked_ioctl(), but permit the calls > to the IOCTL commands which are already implemented in fs/ioctl.c. > > The current call graph is: > > * ioctl syscall > * security_file_ioctl() LSM hook > * do_vfs_ioctl() - standard operations > * file_ioctl() - standard file operations > * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op) > * filp->f_op->unlocked_ioctl() > > Why not use the existing security_file_ioctl() hook? > > With the existing security_file_ioctl() hook, it is technically > feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it > would be difficult to maintain: security_file_ioctl() gets called > further up the call stack, so an implementation of it would need to > predict whether the logic further below will decide to call > f_op->unlocked_ioctl(). That can only be done by mirroring the logic > in do_vfs_ioctl() to some extent, and keeping this in sync. Once again, I don't see this as an impossible task, and I would think that you would want to inspect each new ioctl command/op added in do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted behavior from a Landlock sandbox perspective. Looking at the git log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are added very frequently, meaning that keeping Landlock sync'd with fs/ioctl.c shouldn't be a terrible task. I'm also not excited about the overlap between the existing security_file_ioctl() hook and the proposed security_file_vfs_ioctl() hook. There are some cases where we have no choice and we have to tolerate the overlap, but this doesn't look like one of those cases to me. I'm sorry, but I don't agree with this new hook.
On Thu, Mar 14, 2024 at 01:56:25PM -0400, Paul Moore wrote: > On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@google.com> wrote: > > > > This LSM hook gets called just before the fs/ioctl.c logic delegates > > the requested IOCTL command to the file-specific implementation as > > implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat). > > > > It is impractical for LSMs to make security guarantees about these > > f_op operations without having intimate knowledge of how they are > > implemented. > > > > Therefore, depending on the enabled Landlock policy, Landlock aims to > > block the calls to filp->f_op->unlocked_ioctl(), but permit the calls > > to the IOCTL commands which are already implemented in fs/ioctl.c. > > > > The current call graph is: > > > > * ioctl syscall > > * security_file_ioctl() LSM hook > > * do_vfs_ioctl() - standard operations > > * file_ioctl() - standard file operations > > * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op) > > * filp->f_op->unlocked_ioctl() > > > > Why not use the existing security_file_ioctl() hook? > > > > With the existing security_file_ioctl() hook, it is technically > > feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it > > would be difficult to maintain: security_file_ioctl() gets called > > further up the call stack, so an implementation of it would need to > > predict whether the logic further below will decide to call > > f_op->unlocked_ioctl(). That can only be done by mirroring the logic > > in do_vfs_ioctl() to some extent, and keeping this in sync. > > Once again, I don't see this as an impossible task, and I would think > that you would want to inspect each new ioctl command/op added in > do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted > behavior from a Landlock sandbox perspective. About the LANDLOCK_ACCESS_FS_IOCTL_DEV semantic, we only care about the IOCTLs that are actually delivered to device drivers, so any new IOCTL directly handled by fs/ioctl.c should be treated the same way for this access right. > Looking at the git > log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are > added very frequently, meaning that keeping Landlock sync'd with > fs/ioctl.c shouldn't be a terrible task. do_vfs_ioctl() is indeed not changed often, but this doesn't mean we should not provide strong guarantees, avoid future security bugs, lower the maintenance cost, and improve code readability. > > I'm also not excited about the overlap between the existing > security_file_ioctl() hook and the proposed security_file_vfs_ioctl() > hook. There are some cases where we have no choice and we have to > tolerate the overlap, but this doesn't look like one of those cases to > me. > > I'm sorry, but I don't agree with this new hook. OK, I sent a new RFC (in reply to your email) as an alternative approach. Instead of adding a new LSM hook, this patch adds the vfs_get_ioctl_handler() helper and some code refactoring that should be both interesting for the VFS subsystem and for Landlock. I guess this could be interesting for other security mechanisms as well (e.g. BPF LSM). What do you think? Arnd, Christian, would this sound good to you?
diff --git a/fs/ioctl.c b/fs/ioctl.c index 76cf22ac97d7..e0a8ce300dcd 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -43,10 +43,16 @@ */ long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { - int error = -ENOTTY; + int error; + + error = security_file_vfs_ioctl(filp, cmd, arg); + if (error) + goto out; - if (!filp->f_op->unlocked_ioctl) + if (!filp->f_op->unlocked_ioctl) { + error = -ENOTTY; goto out; + } error = filp->f_op->unlocked_ioctl(filp, cmd, arg); if (error == -ENOIOCTLCMD) @@ -967,6 +973,10 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, if (error != -ENOIOCTLCMD) break; + error = security_file_vfs_ioctl(f.file, cmd, arg); + if (error != 0) + break; + if (f.file->f_op->compat_ioctl) error = f.file->f_op->compat_ioctl(f.file, cmd, arg); if (error == -ENOIOCTLCMD) diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 76458b6d53da..d8a7c49b7eef 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -173,6 +173,8 @@ LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd, unsigned long arg) LSM_HOOK(int, 0, file_ioctl_compat, struct file *file, unsigned int cmd, unsigned long arg) +LSM_HOOK(int, 0, file_vfs_ioctl, struct file *file, unsigned int cmd, + unsigned long arg) LSM_HOOK(int, 0, mmap_addr, unsigned long addr) LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot, unsigned long prot, unsigned long flags) diff --git a/include/linux/security.h b/include/linux/security.h index d0eb20f90b26..05a2e1852f66 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -396,6 +396,8 @@ void security_file_free(struct file *file); int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg); int security_file_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg); +int security_file_vfs_ioctl(struct file *file, unsigned int cmd, + unsigned long arg); int security_mmap_file(struct file *file, unsigned long prot, unsigned long flags); int security_mmap_addr(unsigned long addr); @@ -1011,6 +1013,12 @@ static inline int security_file_ioctl_compat(struct file *file, return 0; } +static inline int security_file_vfs_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + return 0; +} + static inline int security_mmap_file(struct file *file, unsigned long prot, unsigned long flags) { diff --git a/security/security.c b/security/security.c index 7035ee35a393..15c635cd8366 100644 --- a/security/security.c +++ b/security/security.c @@ -2745,6 +2745,28 @@ int security_file_ioctl_compat(struct file *file, unsigned int cmd, } EXPORT_SYMBOL_GPL(security_file_ioctl_compat); +/** + * security_file_vfs_ioctl() - Check if a ioctl implemented by the file is allowed + * @file: associated file + * @cmd: ioctl cmd + * @arg: ioctl arguments + * + * Check permission for an ioctl operation on @file. Note that @arg sometimes + * represents a user space pointer; in other cases, it may be a simple integer + * value. When @arg represents a user space pointer, it should never be used + * by the security module. + * + * This hook is checked just before calling f_op->unlocked_ioctl() or + * f_op->compat_ioctl(). + * + * Return: Returns 0 if permission is granted. + */ +int security_file_vfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + return call_int_hook(file_vfs_ioctl, 0, file, cmd, arg); +} +EXPORT_SYMBOL_GPL(security_file_vfs_ioctl); + static inline unsigned long mmap_prot(struct file *file, unsigned long prot) { /*
This LSM hook gets called just before the fs/ioctl.c logic delegates the requested IOCTL command to the file-specific implementation as implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat). It is impractical for LSMs to make security guarantees about these f_op operations without having intimate knowledge of how they are implemented. Therefore, depending on the enabled Landlock policy, Landlock aims to block the calls to filp->f_op->unlocked_ioctl(), but permit the calls to the IOCTL commands which are already implemented in fs/ioctl.c. The current call graph is: * ioctl syscall * security_file_ioctl() LSM hook * do_vfs_ioctl() - standard operations * file_ioctl() - standard file operations * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op) * filp->f_op->unlocked_ioctl() Why not use the existing security_file_ioctl() hook? With the existing security_file_ioctl() hook, it is technically feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it would be difficult to maintain: security_file_ioctl() gets called further up the call stack, so an implementation of it would need to predict whether the logic further below will decide to call f_op->unlocked_ioctl(). That can only be done by mirroring the logic in do_vfs_ioctl() to some extent, and keeping this in sync. We therefore think that it is cleaner to add a new LSM hook, which gets called closer to the security boundary which we actually want to block, filp->f_op->unlocked_ioctl(). Why is it difficult to reason about f_op->unlocked_ioctl()? The unlocked_ioctl() and ioctl_compat() operations generally do the following things: 1. Execute code depending on the IOCTL command number, to implement the IOCTL command. 2. Execute code independent(!) of the IOCTL command number, usually to implement common locking and resource allocation behavior. Notably, this often happens before(!) the implementation looks at the command number. Due to the number of device drivers in Linux, it is infeasible for LSM maintainers to keep track of what these implementations do in detail. Due to 2., even permitting selected IOCTL command numbers to be implemented by devices would probably be a mistake, because even when a device does not implement a given IOCTL command, it might still execute code when you try to call it. Cc: Paul Moore <paul@paul-moore.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Christian Brauner <brauner@kernel.org> Cc: Dave Chinner <david@fromorbit.com> Cc: Mickaël Salaün <mic@digikod.net> Signed-off-by: Günther Noack <gnoack@google.com> --- fs/ioctl.c | 14 ++++++++++++-- include/linux/lsm_hook_defs.h | 2 ++ include/linux/security.h | 8 ++++++++ security/security.c | 22 ++++++++++++++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-)