[PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks

[Günther Noack]

If security_file_ioctl or security_file_ioctl_compat return
ENOFILEOPS, the IOCTL logic in fs/ioctl.c will permit the given IOCTL
command, but only as long as the IOCTL command is implemented directly
in fs/ioctl.c and does not use the f_ops->unhandled_ioctl or
f_ops->compat_ioctl operations, which are defined by the given file.

The possible return values for security_file_ioctl and
security_file_ioctl_compat are now:

 * 0 - to permit the IOCTL
 * ENOFILEOPS - to permit the IOCTL, but forbid it if it needs to fall
   back to the file implementation.
 * any other error - to forbid the IOCTL and return that error

This is an alternative to the previously discussed approaches [1] and [2],
and implements the proposal from [3].

Cc: Christian Brauner <brauner at kernel.org>
Cc: Paul Moore <paul at paul-moore.com>
Cc: Mickaël Salaün <mic at digikod.net>
Cc: linux-fsdevel at vger.kernel.org
Link: https://lore.kernel.org/r/20240309075320.160128-2-gnoack@google.com [1]
Link: https://lore.kernel.org/r/20240322151002.3653639-2-gnoack@google.com/ [2]
Link: https://lore.kernel.org/r/32b1164e-9d5f-40c0-9a4e-001b2c9b822f@app.fastmail.com/ [3]
Suggested-by: Arnd Bergmann <arnd at arndb.de>
Signed-off-by: Günther Noack <gnoack at google.com>
---
 fs/ioctl.c               | 25 ++++++++++++++++++++-----
 include/linux/security.h |  6 ++++++
 security/security.c      | 10 ++++++++--
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..8244354ad04d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -828,7 +828,7 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
 
 	case FIONREAD:
 		if (!S_ISREG(inode->i_mode))
-			return vfs_ioctl(filp, cmd, arg);
+			return -ENOIOCTLCMD;
 
 		return put_user(i_size_read(inode) - filp->f_pos,
 				(int __user *)argp);
@@ -858,17 +858,24 @@ SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, unsigned long, arg)
 {
 	struct fd f = fdget(fd);
 	int error;
+	bool use_file_ops = true;
 
 	if (!f.file)
 		return -EBADF;
 
 	error = security_file_ioctl(f.file, cmd, arg);
-	if (error)
+	if (error == -ENOFILEOPS)
+		use_file_ops = false;
+	else if (error)
 		goto out;
 
 	error = do_vfs_ioctl(f.file, fd, cmd, arg);
-	if (error == -ENOIOCTLCMD)
-		error = vfs_ioctl(f.file, cmd, arg);
+	if (error == -ENOIOCTLCMD) {
+		if (use_file_ops)
+			error = vfs_ioctl(f.file, cmd, arg);
+		else
+			error = -EACCES;
+	}
 
 out:
 	fdput(f);
@@ -916,12 +923,15 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 {
 	struct fd f = fdget(fd);
 	int error;
+	bool use_file_ops = true;
 
 	if (!f.file)
 		return -EBADF;
 
 	error = security_file_ioctl_compat(f.file, cmd, arg);
-	if (error)
+	if (error == -ENOFILEOPS)
+		use_file_ops = false;
+	else if (error)
 		goto out;
 
 	switch (cmd) {
@@ -967,6 +977,11 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 		if (error != -ENOIOCTLCMD)
 			break;
 
+		if (!use_file_ops) {
+			error = -EACCES;
+			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/security.h b/include/linux/security.h
index d0eb20f90b26..b769dc888d07 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -248,6 +248,12 @@ static const char * const kernel_load_data_str[] = {
 	__kernel_read_file_id(__data_id_stringify)
 };
 
+/*
+ * Returned by security_file_ioctl and security_file_ioctl_compat to indicate
+ * that the IOCTL request may not be dispatched to the file's f_ops IOCTL impl.
+ */
+#define ENOFILEOPS 532
+
 static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
 {
 	if ((unsigned)id >= LOADING_MAX_ID)
diff --git a/security/security.c b/security/security.c
index 7035ee35a393..000c54a1e541 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2719,7 +2719,10 @@ void security_file_free(struct file *file)
  * value.  When @arg represents a user space pointer, it should never be used
  * by the security module.
  *
- * Return: Returns 0 if permission is granted.
+ * Return: Returns 0 if permission is granted.  Returns -ENOFILEOPS if
+ *         permission is granted for IOCTL commands that do not get handled by
+ *         f_ops->unlocked_ioctl().  Returns another negative error code is
+ *         permission is denied.
  */
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
@@ -2736,7 +2739,10 @@ EXPORT_SYMBOL_GPL(security_file_ioctl);
  * Compat version of security_file_ioctl() that correctly handles 32-bit
  * processes running on 64-bit kernels.
  *
- * Return: Returns 0 if permission is granted.
+ * Return: Returns 0 if permission is granted. Returns -ENOFILEOPS if permission
+ *         is granted for IOCTL commands that do not get handled by
+ *         f_ops->compat_ioctl().  Returns another negative error code is
+ *         permission is denied.
  */
 int security_file_ioctl_compat(struct file *file, unsigned int cmd,
 			       unsigned long arg)
-- 
2.44.0.396.g6e790dbe36-goog