[v11,1/9] fs: Add and use vfs_get_ioctl_handler()

Commit Message

Günther Noack March 22, 2024, 3:09 p.m. UTC

From: Mickaël Salaün <[e-mail]>

Add a new vfs_get_ioctl_handler() helper to identify if an IOCTL command
is handled by the first IOCTL layer.  Each IOCTL command is now handled
by a dedicated function, and all of them use the same signature.

Apart from the VFS, this helper is also intended to be used by Landlock
to cleanly categorize VFS IOCTLs and create appropriate security
policies.

This is an alternative to a first RFC [1] and a proposal for a new LSM
hook [2].

By dereferencing some pointers only when required, this should also
slightly improve do_vfs_ioctl().

Remove (double) pointer castings on put_user() calls.

Remove potential double vfs_ioctl() call for FIONREAD.

Fix ioctl_file_clone_range() return type from long to int.

Cc: Arnd Bergmann <[e-mail]>
Cc: Christian Brauner <[e-mail]>
Cc: Günther Noack <[e-mail]>
Cc: Paul Moore <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail] [1]
Link: https://lore.kernel.org/r/[e-mail] [2]
Signed-off-by: Mickaël Salaün <[e-mail]>
---
 fs/ioctl.c         | 213 +++++++++++++++++++++++++++++++--------------
 include/linux/fs.h |   6 ++
 2 files changed, 155 insertions(+), 64 deletions(-)

Comments

Arnd Bergmann March 22, 2024, 3:31 p.m. UTC | #1

On Fri, Mar 22, 2024, at 16:09, Günther Noack wrote:
> From: Mickaël Salaün <[e-mail]>
>
> Add a new vfs_get_ioctl_handler() helper to identify if an IOCTL command
> is handled by the first IOCTL layer.  Each IOCTL command is now handled
> by a dedicated function, and all of them use the same signature.

Sorry I didn't already reply the previous time you sent this.
I don't really like the idea of going through another indirect
pointer for each of the ioctls here, both because of the
complexity at the source level, and the potential cost on
architectures that need heavy barriers around indirect
function calls.
 
> -static int ioctl_fibmap(struct file *filp, int __user *p)
> +static int ioctl_fibmap(struct file *filp, unsigned int fd, unsigned 
> long arg)
>  {
> +	int __user *p = (void __user *)arg;

The new version doesn't seem like an improvement when you
need the extra type casts here. 

As a completely different approach, would it perhaps be
sufficient to define security_file_ioctl_compat() in a
way that it may return a special error code signifying
"don't call into fops->{unlocked,compat}_ioctl"?

This way landlock could trivially allow ioctls on e.g.
normal file systems, sockets and block devices but prevent
them on character devices it does not trust.

      Arnd

Günther Noack March 25, 2024, 1:25 p.m. UTC | #2

On Fri, Mar 22, 2024 at 04:31:58PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 22, 2024, at 16:09, Günther Noack wrote:
> > From: Mickaël Salaün <[e-mail]>
> >
> > Add a new vfs_get_ioctl_handler() helper to identify if an IOCTL command
> > is handled by the first IOCTL layer.  Each IOCTL command is now handled
> > by a dedicated function, and all of them use the same signature.
> 
> Sorry I didn't already reply the previous time you sent this.
> I don't really like the idea of going through another indirect
> pointer for each of the ioctls here, both because of the
> complexity at the source level, and the potential cost on
> architectures that need heavy barriers around indirect
> function calls.
>  
> > -static int ioctl_fibmap(struct file *filp, int __user *p)
> > +static int ioctl_fibmap(struct file *filp, unsigned int fd, unsigned 
> > long arg)
> >  {
> > +	int __user *p = (void __user *)arg;
> 
> The new version doesn't seem like an improvement when you
> need the extra type casts here. 
> 
> As a completely different approach, would it perhaps be
> sufficient to define security_file_ioctl_compat() in a
> way that it may return a special error code signifying
> "don't call into fops->{unlocked,compat}_ioctl"?
> 
> This way landlock could trivially allow ioctls on e.g.
> normal file systems, sockets and block devices but prevent
> them on character devices it does not trust.

Thank you for the review, Arnd!  I gave your suggestion a shot and it seems
cleaner - I'll send an updated patch shortly.

—Günther

Patch

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..d2b6691ded16 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -56,8 +56,9 @@  long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 }
 EXPORT_SYMBOL(vfs_ioctl);
 
-static int ioctl_fibmap(struct file *filp, int __user *p)
+static int ioctl_fibmap(struct file *filp, unsigned int fd, unsigned long arg)
 {
+	int __user *p = (void __user *)arg;
 	struct inode *inode = file_inode(filp);
 	struct super_block *sb = inode->i_sb;
 	int error, ur_block;
@@ -197,11 +198,12 @@  int fiemap_prep(struct inode *inode, struct fiemap_extent_info *fieinfo,
 }
 EXPORT_SYMBOL(fiemap_prep);
 
-static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
+static int ioctl_fiemap(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct fiemap fiemap;
 	struct fiemap_extent_info fieinfo = { 0, };
 	struct inode *inode = file_inode(filp);
+	struct fiemap __user *ufiemap = (void __user *)arg;
 	int error;
 
 	if (!inode->i_op->fiemap)
@@ -228,6 +230,18 @@  static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
 	return error;
 }
 
+static int ioctl_figetbsz(struct file *file, unsigned int fd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	int __user *argp = (void __user *)arg;
+
+	/* anon_bdev filesystems may not have a block size */
+	if (!inode->i_sb->s_blocksize)
+		return -EINVAL;
+
+	return put_user(inode->i_sb->s_blocksize, argp);
+}
+
 static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 			     u64 off, u64 olen, u64 destoff)
 {
@@ -249,9 +263,15 @@  static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	return ret;
 }
 
-static long ioctl_file_clone_range(struct file *file,
-				   struct file_clone_range __user *argp)
+static int ioctl_ficlone(struct file *file, unsigned int fd, unsigned long arg)
+{
+	return ioctl_file_clone(file, arg, 0, 0, 0);
+}
+
+static int ioctl_file_clone_range(struct file *file, unsigned int fd,
+				  unsigned long arg)
 {
+	struct file_clone_range __user *argp = (void __user *)arg;
 	struct file_clone_range args;
 
 	if (copy_from_user(&args, argp, sizeof(args)))
@@ -292,6 +312,27 @@  static int ioctl_preallocate(struct file *filp, int mode, void __user *argp)
 			sr.l_len);
 }
 
+static int ioctl_resvsp(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, 0, p);
+}
+
+static int ioctl_unresvsp(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
+}
+
+static int ioctl_zero_range(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *p = (void __user *)arg;
+
+	return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
+}
+
 /* on ia32 l_start is on a 32-bit boundary */
 #if defined CONFIG_COMPAT && defined(CONFIG_X86_64)
 /* just account for different alignment */
@@ -321,28 +362,41 @@  static int compat_ioctl_preallocate(struct file *file, int mode,
 }
 #endif
 
-static int file_ioctl(struct file *filp, unsigned int cmd, int __user *p)
+static ioctl_handler_t file_ioctl(unsigned int cmd)
 {
 	switch (cmd) {
 	case FIBMAP:
-		return ioctl_fibmap(filp, p);
+		return ioctl_fibmap;
 	case FS_IOC_RESVSP:
 	case FS_IOC_RESVSP64:
-		return ioctl_preallocate(filp, 0, p);
+		return ioctl_resvsp;
 	case FS_IOC_UNRESVSP:
 	case FS_IOC_UNRESVSP64:
-		return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
+		return ioctl_unresvsp;
 	case FS_IOC_ZERO_RANGE:
-		return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
+		return ioctl_zero_range;
 	}
 
-	return -ENOIOCTLCMD;
+	return NULL;
+}
+
+static int ioctl_fioclex(struct file *file, unsigned int fd, unsigned long arg)
+{
+	set_close_on_exec(fd, 1);
+	return 0;
+}
+
+static int ioctl_fionclex(struct file *file, unsigned int fd, unsigned long arg)
+{
+	set_close_on_exec(fd, 0);
+	return 0;
 }
 
-static int ioctl_fionbio(struct file *filp, int __user *argp)
+static int ioctl_fionbio(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	unsigned int flag;
 	int on, error;
+	int __user *argp = (void __user *)arg;
 
 	error = get_user(on, argp);
 	if (error)
@@ -362,11 +416,11 @@  static int ioctl_fionbio(struct file *filp, int __user *argp)
 	return error;
 }
 
-static int ioctl_fioasync(unsigned int fd, struct file *filp,
-			  int __user *argp)
+static int ioctl_fioasync(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	unsigned int flag;
 	int on, error;
+	int __user *argp = (void __user *)arg;
 
 	error = get_user(on, argp);
 	if (error)
@@ -384,7 +438,22 @@  static int ioctl_fioasync(unsigned int fd, struct file *filp,
 	return error < 0 ? error : 0;
 }
 
-static int ioctl_fsfreeze(struct file *filp)
+static int ioctl_fioqsize(struct file *file, unsigned int fd, unsigned long arg)
+{
+	struct inode *inode = file_inode(file);
+	void __user *argp = (void __user *)arg;
+
+	if (S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) ||
+	    S_ISLNK(inode->i_mode)) {
+		loff_t res = inode_get_bytes(inode);
+
+		return copy_to_user(argp, &res, sizeof(res)) ? -EFAULT : 0;
+	}
+
+	return -ENOTTY;
+}
+
+static int ioctl_fsfreeze(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct super_block *sb = file_inode(filp)->i_sb;
 
@@ -401,7 +470,7 @@  static int ioctl_fsfreeze(struct file *filp)
 	return freeze_super(sb, FREEZE_HOLDER_USERSPACE);
 }
 
-static int ioctl_fsthaw(struct file *filp)
+static int ioctl_fsthaw(struct file *filp, unsigned int fd, unsigned long arg)
 {
 	struct super_block *sb = file_inode(filp)->i_sb;
 
@@ -414,9 +483,9 @@  static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb, FREEZE_HOLDER_USERSPACE);
 }
 
-static int ioctl_file_dedupe_range(struct file *file,
-				   struct file_dedupe_range __user *argp)
+static int ioctl_file_dedupe_range(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct file_dedupe_range __user *argp = (void __user *)arg;
 	struct file_dedupe_range *same = NULL;
 	int ret;
 	unsigned long size;
@@ -454,6 +523,14 @@  static int ioctl_file_dedupe_range(struct file *file,
 	return ret;
 }
 
+static int ioctl_fionread(struct file *filp, unsigned int fd, unsigned long arg)
+{
+	int __user *argp = (void __user *)arg;
+	struct inode *inode = file_inode(filp);
+
+	return put_user(i_size_read(inode) - filp->f_pos, argp);
+}
+
 /**
  * fileattr_fill_xflags - initialize fileattr with xflags
  * @fa:		fileattr pointer
@@ -702,8 +779,9 @@  int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
 }
 EXPORT_SYMBOL(vfs_fileattr_set);
 
-static int ioctl_getflags(struct file *file, unsigned int __user *argp)
+static int ioctl_getflags(struct file *file, unsigned int fd, unsigned long arg)
 {
+	unsigned int __user *argp = (void __user *)arg;
 	struct fileattr fa = { .flags_valid = true }; /* hint only */
 	int err;
 
@@ -713,8 +791,9 @@  static int ioctl_getflags(struct file *file, unsigned int __user *argp)
 	return err;
 }
 
-static int ioctl_setflags(struct file *file, unsigned int __user *argp)
+static int ioctl_setflags(struct file *file, unsigned int fd, unsigned long arg)
 {
+	unsigned int __user *argp = (void __user *)arg;
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct fileattr fa;
@@ -733,8 +812,9 @@  static int ioctl_setflags(struct file *file, unsigned int __user *argp)
 	return err;
 }
 
-static int ioctl_fsgetxattr(struct file *file, void __user *argp)
+static int ioctl_fsgetxattr(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct fsxattr __user *argp = (void __user *)arg;
 	struct fileattr fa = { .fsx_valid = true }; /* hint only */
 	int err;
 
@@ -745,8 +825,9 @@  static int ioctl_fsgetxattr(struct file *file, void __user *argp)
 	return err;
 }
 
-static int ioctl_fssetxattr(struct file *file, void __user *argp)
+static int ioctl_fssetxattr(struct file *file, unsigned int fd, unsigned long arg)
 {
+	struct fsxattr __user *argp = (void __user *)arg;
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct fileattr fa;
@@ -764,94 +845,98 @@  static int ioctl_fssetxattr(struct file *file, void __user *argp)
 }
 
 /*
- * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
- * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
+ * Return NULL when no handler exists for @cmd, or the appropriate function
+ * otherwise.  This means that these handlers should never return -ENOIOCTLCMD.
  *
  * When you add any new common ioctls to the switches above and below,
  * please ensure they have compatible arguments in compat mode.
  */
-static int do_vfs_ioctl(struct file *filp, unsigned int fd,
-			unsigned int cmd, unsigned long arg)
+ioctl_handler_t vfs_get_ioctl_handler(struct file *filp, unsigned int cmd)
 {
-	void __user *argp = (void __user *)arg;
-	struct inode *inode = file_inode(filp);
-
 	switch (cmd) {
 	case FIOCLEX:
-		set_close_on_exec(fd, 1);
-		return 0;
+		return ioctl_fioclex;
 
 	case FIONCLEX:
-		set_close_on_exec(fd, 0);
-		return 0;
+		return ioctl_fionclex;
 
 	case FIONBIO:
-		return ioctl_fionbio(filp, argp);
+		return ioctl_fionbio;
 
 	case FIOASYNC:
-		return ioctl_fioasync(fd, filp, argp);
+		return ioctl_fioasync;
 
 	case FIOQSIZE:
-		if (S_ISDIR(inode->i_mode) || S_ISREG(inode->i_mode) ||
-		    S_ISLNK(inode->i_mode)) {
-			loff_t res = inode_get_bytes(inode);
-			return copy_to_user(argp, &res, sizeof(res)) ?
-					    -EFAULT : 0;
-		}
-
-		return -ENOTTY;
+		return ioctl_fioqsize;
 
 	case FIFREEZE:
-		return ioctl_fsfreeze(filp);
+		return ioctl_fsfreeze;
 
 	case FITHAW:
-		return ioctl_fsthaw(filp);
+		return ioctl_fsthaw;
 
 	case FS_IOC_FIEMAP:
-		return ioctl_fiemap(filp, argp);
+		return ioctl_fiemap;
 
 	case FIGETBSZ:
-		/* anon_bdev filesystems may not have a block size */
-		if (!inode->i_sb->s_blocksize)
-			return -EINVAL;
-
-		return put_user(inode->i_sb->s_blocksize, (int __user *)argp);
+		return ioctl_figetbsz;
 
 	case FICLONE:
-		return ioctl_file_clone(filp, arg, 0, 0, 0);
+		return ioctl_ficlone;
 
 	case FICLONERANGE:
-		return ioctl_file_clone_range(filp, argp);
+		return ioctl_file_clone_range;
 
 	case FIDEDUPERANGE:
-		return ioctl_file_dedupe_range(filp, argp);
+		return ioctl_file_dedupe_range;
 
 	case FIONREAD:
-		if (!S_ISREG(inode->i_mode))
-			return vfs_ioctl(filp, cmd, arg);
+		if (!S_ISREG(file_inode(filp)->i_mode))
+			break;
 
-		return put_user(i_size_read(inode) - filp->f_pos,
-				(int __user *)argp);
+		return ioctl_fionread;
 
 	case FS_IOC_GETFLAGS:
-		return ioctl_getflags(filp, argp);
+		return ioctl_getflags;
 
 	case FS_IOC_SETFLAGS:
-		return ioctl_setflags(filp, argp);
+		return ioctl_setflags;
 
 	case FS_IOC_FSGETXATTR:
-		return ioctl_fsgetxattr(filp, argp);
+		return ioctl_fsgetxattr;
 
 	case FS_IOC_FSSETXATTR:
-		return ioctl_fssetxattr(filp, argp);
+		return ioctl_fssetxattr;
 
 	default:
-		if (S_ISREG(inode->i_mode))
-			return file_ioctl(filp, cmd, argp);
+		if (S_ISREG(file_inode(filp)->i_mode))
+			return file_ioctl(cmd);
 		break;
 	}
 
-	return -ENOIOCTLCMD;
+	/* Forwards call to vfs_ioctl(filp, cmd, arg) */
+	return NULL;
+}
+
+/*
+ * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
+ * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
+ */
+static int do_vfs_ioctl(struct file *filp, unsigned int fd,
+			unsigned int cmd, unsigned long arg)
+{
+	ioctl_handler_t handler = vfs_get_ioctl_handler(filp, cmd);
+	int ret;
+
+	if (!handler)
+		return -ENOIOCTLCMD;
+
+	ret = (*handler)(filp, fd, arg);
+	/* Makes sure handle() really handles this command. */
+	if (WARN_ON_ONCE(ret == -ENOIOCTLCMD))
+		return -ENOTTY;
+
+	return ret;
 }
 
 SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, unsigned long, arg)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1fbc72c5f112..92bf421aae83 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1904,6 +1904,12 @@  extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 #define compat_ptr_ioctl NULL
 #endif
 
+typedef int (*ioctl_handler_t)(struct file *file, unsigned int fd,
+			       unsigned long arg);
+
+extern ioctl_handler_t vfs_get_ioctl_handler(struct file *filp,
+					     unsigned int cmd);
+
 /*
  * VFS file helper functions.
  */