[PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

[Günther Noack]

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 at paul-moore.com>
Cc: Arnd Bergmann <arnd at arndb.de>
Cc: Christian Brauner <brauner at kernel.org>
Cc: Dave Chinner <david at fromorbit.com>
Cc: Mickaël Salaün <mic at digikod.net>
Signed-off-by: Günther Noack <gnoack at 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(-)

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)
 {
 	/*
-- 
2.44.0.278.ge034bb2e1d-goog