Message-ID: <YriL/2ixE6fsdkpP@magnolia>
Date:   Sun, 26 Jun 2022 09:40:31 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Jeremy Bongio <bongiojp@...il.com>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] Add ioctls to get/set the ext4 superblock uuid.

On Sat, Jun 25, 2022 at 01:22:25AM -0700, Jeremy Bongio wrote:
> This fixes a race between changing the ext4 superblock uuid and operations
> like mounting, resizing, changing features, etc.
> 
> Reviewed-by: Theodore Ts'o <tytso@....edu>
> Signed-off-by: Jeremy Bongio <bongiojp@...il.com>

This is a userspace abi change; it really should cc linux-fsdevel and
linux-api.

> ---
>  fs/ext4/ext4.h  | 10 ++++++
>  fs/ext4/ioctl.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 75b8d81b2469..00747532cc4a 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -724,6 +724,8 @@ enum {
>  #define EXT4_IOC_GETSTATE		_IOW('f', 41, __u32)
>  #define EXT4_IOC_GET_ES_CACHE		_IOWR('f', 42, struct fiemap)
>  #define EXT4_IOC_CHECKPOINT		_IOW('f', 43, __u32)
> +#define EXT4_IOC_GETFSUUID		_IOR('f', 44, struct fsuuid)
> +#define EXT4_IOC_SETFSUUID		_IOW('f', 45, struct fsuuid)

One thing I've noticed people rarely do with a get/set ioctl pair -- the
_IOR and _IOW macros encode the direction (R/W) in the ioctl number,
which means that you don't need to use both 44 and 45 here.  We get 8
bits of namespace ('f') and 8 bits of call number (44), and while
they're generally not in /that/ short supply, a u16 will eventually fill
up.

If you want to be paranoid, you could also encode a BUILD_BUG_ON to
check that they're not the same.

>  
>  #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
>  
> @@ -753,6 +755,14 @@ enum {
>  						EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \
>  						EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN)
>  
> +/*
> + * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
> + */
> +struct fsuuid {
> +	size_t len;

size_t... is a mess for userspace ABI.  If the kernel is running in a
multiarch environment (e.g. i386 program running on x64 kernel) then
you'll have to make sure that the field length is what you think it is.
Given the current typedef hell w.r.t. size_t, I suggest making life
easier on the reviewers and making @len explicitly sized.

Also, please put in a @flags argument just in case someone someday needs
to add some.

> +	__u8 __user *b;

Putting a pointer in an ioctl struct argument is a /very/ bad idea,
because doing so (a) makes it harder for things like seccomp to inspect
arguments, and (b) usually means you have to implement a bunch of fugly
compat ioctl thunking code for multiarch systems (e.g. i386 program
running on x64 kernel) to extract the pointer and convert it to a native
pointer.

You /could/ avoid both of these problems by requiring that the uuid data
be stored in an unsized VLA at the end of the struct:

	struct fsuuid {
		__u32	fu_len;
		__u32	fu_flags;

		__u8	fu_uuid[];
	};

Then your set uuid validation code looks like this:

	int ret = 0;
	__u8 uuid[UUID_SIZE];
	struct fsuuid fsuuid;

	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
		return -EFAULT;

	if (fsuuid.fu_flags || fsuuid.fu_len != UUID_SIZE)
		return -EINVAL;

	if (copy_from_user(uuid, &fsuuid.fu_uuid[0], UUID_SIZE))
		return -EFAULT;

	/* actually set uuid... */

--D

> +};
> +
>  #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
>  /*
>   * ioctl commands in 32 bit emulation
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index cb01c1da0f9d..a47e24fc8c67 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -20,6 +20,7 @@
>  #include <linux/delay.h>
>  #include <linux/iversion.h>
>  #include <linux/fileattr.h>
> +#include <linux/uuid.h>
>  #include "ext4_jbd2.h"
>  #include "ext4.h"
>  #include <linux/fsmap.h>
> @@ -41,6 +42,15 @@ static void ext4_sb_setlabel(struct ext4_super_block *es, const void *arg)
>  	memcpy(es->s_volume_name, (char *)arg, EXT4_LABEL_MAX);
>  }
>  
> +/*
> + * Superblock modification callback function for changing file system
> + * UUID.
> + */
> +static void ext4_sb_setuuid(struct ext4_super_block *es, const void *arg)
> +{
> +	memcpy(es->s_uuid, (__u8 *)arg, UUID_SIZE);
> +}
> +
>  static
>  int ext4_update_primary_sb(struct super_block *sb, handle_t *handle,
>  			   ext4_update_sb_callback func,
> @@ -1131,6 +1141,74 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label
>  	return 0;
>  }
>  
> +static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
> +			struct fsuuid __user *ufsuuid)
> +{
> +	int ret = 0;
> +	__u8 uuid[UUID_SIZE];
> +	struct fsuuid fsuuid;
> +
> +	/* read uuid from userspace*/
> +	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +		return -EFAULT;
> +
> +	/* If invalid, return EINVAL */
> +	if (fsuuid.len != UUID_SIZE)
> +		return -EINVAL;
> +
> +
> +	lock_buffer(sbi->s_sbh);
> +	memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE);
> +	unlock_buffer(sbi->s_sbh);
> +
> +	if (copy_to_user(fsuuid.b, uuid, UUID_SIZE))
> +		ret = -EFAULT;
> +
> +	return ret;
> +}
> +
> +static int ext4_ioctl_setuuid(struct file *filp,
> +			const struct fsuuid __user *ufsuuid)
> +{
> +	int ret = 0;
> +	struct super_block *sb = file_inode(filp)->i_sb;
> +	struct fsuuid fsuuid;
> +	__u8 uuid[UUID_SIZE];
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	/*
> +	 * If any checksums (group descriptors or metadata) are being used
> +	 * then the checksum seed feature is required to change the UUID.
> +	 */
> +	if (((ext4_has_feature_gdt_csum(sb) || ext4_has_metadata_csum(sb))
> +			&& !ext4_has_feature_csum_seed(sb))
> +		|| ext4_has_feature_stable_inodes(sb))
> +		return -EOPNOTSUPP;
> +
> +	/* Read the length uuid and userspace pointer to uuid from userspace. */
> +	if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +		return -EFAULT;
> +
> +	/* If invalid, return EINVAL */
> +	if (fsuuid.len != UUID_SIZE)
> +		return -EINVAL;
> +
> +	/* Read uuid from userspace*/
> +	if (copy_from_user(uuid, fsuuid.b, UUID_SIZE))
> +		return -EFAULT;
> +
> +	ret = mnt_want_write_file(filp);
> +	if (ret)
> +		return ret;
> +
> +	ret = ext4_update_superblocks_fn(sb, ext4_sb_setuuid, &uuid);
> +	mnt_drop_write_file(filp);
> +
> +	return ret;
> +}
> +
>  static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
> @@ -1509,6 +1587,10 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		return ext4_ioctl_setlabel(filp,
>  					   (const void __user *)arg);
>  
> +	case EXT4_IOC_GETFSUUID:
> +		return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg);
> +	case EXT4_IOC_SETFSUUID:
> +		return ext4_ioctl_setuuid(filp, (const void __user *)arg);
>  	default:
>  		return -ENOTTY;
>  	}
> @@ -1586,6 +1668,8 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case EXT4_IOC_CHECKPOINT:
>  	case FS_IOC_GETFSLABEL:
>  	case FS_IOC_SETFSLABEL:
> +	case EXT4_IOC_GETFSUUID:
> +	case EXT4_IOC_SETFSUUID:
>  		break;
>  	default:
>  		return -ENOIOCTLCMD;
> -- 
> 2.37.0.rc0.161.g10f37bed90-goog
> 

Powered by blists - more mailing lists