Re: [PATCH 1/2] ecryptfs: Factor out mount option validation

[Tyler Hicks <code@tyhicks.com>]

On 2024-10-07 10:27:42, Eric Sandeen wrote:
> Under the new mount API, mount options are parsed one at a time.
> Any validation that examines multiple options must be done after parsing
> is complete, so factor out a ecryptfs_validate_options() which can be
> called separately.
> 
> To facilitate this, temporarily move the local variables that tracked
> whether various options have been set in the parsing function, into the
> ecryptfs_mount_crypt_stat structure so that they can be examined later.
> 
> These will be moved to a more ephemeral struct in the mount api conversion
> patch to follow.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Thanks, Eric!

I was going to push back on storing the *_set bools in the
ecryptfs_mount_crypt_stat struct since those bools only meant to be
short lived and that struct lives for the lifetime of the mount.
However, I see you clean that up in the following patch.

Acked-by: Tyler Hicks <code@tyhicks.com>

Tyler

> ---
>  fs/ecryptfs/ecryptfs_kernel.h |  7 +++++
>  fs/ecryptfs/main.c            | 55 ++++++++++++++++++++---------------
>  2 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index c586c5db18b5..d359ec085a70 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -343,6 +343,13 @@ struct ecryptfs_mount_crypt_stat {
>  	unsigned char global_default_fn_cipher_name[
>  		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
>  	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> +	/* Mount option status trackers */
> +	bool check_ruid;
> +	bool sig_set;
> +	bool cipher_name_set;
> +	bool cipher_key_bytes_set;
> +	bool fn_cipher_name_set;
> +	bool fn_cipher_key_bytes_set;
>  };
>  
>  /* superblock private data. */
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 577c56302314..d03f1c6ccc1c 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -239,18 +239,12 @@ static void ecryptfs_init_mount_crypt_stat(
>   *
>   * Returns zero on success; non-zero on error
>   */
> -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
> -				  uid_t *check_ruid)
> +static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options)
>  {
>  	char *p;
>  	int rc = 0;
> -	int sig_set = 0;
> -	int cipher_name_set = 0;
> -	int fn_cipher_name_set = 0;
>  	int cipher_key_bytes;
> -	int cipher_key_bytes_set = 0;
>  	int fn_cipher_key_bytes;
> -	int fn_cipher_key_bytes_set = 0;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
>  		&sbi->mount_crypt_stat;
>  	substring_t args[MAX_OPT_ARGS];
> @@ -261,9 +255,6 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  	char *fnek_src;
>  	char *cipher_key_bytes_src;
>  	char *fn_cipher_key_bytes_src;
> -	u8 cipher_code;
> -
> -	*check_ruid = 0;
>  
>  	if (!options) {
>  		rc = -EINVAL;
> @@ -285,14 +276,14 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  				       "global sig; rc = [%d]\n", rc);
>  				goto out;
>  			}
> -			sig_set = 1;
> +			mount_crypt_stat->sig_set = 1;
>  			break;
>  		case ecryptfs_opt_cipher:
>  		case ecryptfs_opt_ecryptfs_cipher:
>  			cipher_name_src = args[0].from;
>  			strscpy(mount_crypt_stat->global_default_cipher_name,
>  				cipher_name_src);
> -			cipher_name_set = 1;
> +			mount_crypt_stat->cipher_name_set = 1;
>  			break;
>  		case ecryptfs_opt_ecryptfs_key_bytes:
>  			cipher_key_bytes_src = args[0].from;
> @@ -301,7 +292,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  						   &cipher_key_bytes_src, 0);
>  			mount_crypt_stat->global_default_cipher_key_size =
>  				cipher_key_bytes;
> -			cipher_key_bytes_set = 1;
> +			mount_crypt_stat->cipher_key_bytes_set = 1;
>  			break;
>  		case ecryptfs_opt_passthrough:
>  			mount_crypt_stat->flags |=
> @@ -340,7 +331,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  			fn_cipher_name_src = args[0].from;
>  			strscpy(mount_crypt_stat->global_default_fn_cipher_name,
>  				fn_cipher_name_src);
> -			fn_cipher_name_set = 1;
> +			mount_crypt_stat->fn_cipher_name_set = 1;
>  			break;
>  		case ecryptfs_opt_fn_cipher_key_bytes:
>  			fn_cipher_key_bytes_src = args[0].from;
> @@ -349,7 +340,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  						   &fn_cipher_key_bytes_src, 0);
>  			mount_crypt_stat->global_default_fn_cipher_key_bytes =
>  				fn_cipher_key_bytes;
> -			fn_cipher_key_bytes_set = 1;
> +			mount_crypt_stat->fn_cipher_key_bytes_set = 1;
>  			break;
>  		case ecryptfs_opt_unlink_sigs:
>  			mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
> @@ -359,7 +350,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  				ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
>  			break;
>  		case ecryptfs_opt_check_dev_ruid:
> -			*check_ruid = 1;
> +			mount_crypt_stat->check_ruid = 1;
>  			break;
>  		case ecryptfs_opt_err:
>  		default:
> @@ -368,14 +359,25 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  			       __func__, p);
>  		}
>  	}
> -	if (!sig_set) {
> +
> +out:
> +	return rc;
> +}
> +
> +static int ecryptfs_validate_options(
> +		struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> +{
> +	int rc = 0;
> +	u8 cipher_code;
> +
> +	if (!mount_crypt_stat->sig_set) {
>  		rc = -EINVAL;
>  		ecryptfs_printk(KERN_ERR, "You must supply at least one valid "
>  				"auth tok signature as a mount "
>  				"parameter; see the eCryptfs README\n");
>  		goto out;
>  	}
> -	if (!cipher_name_set) {
> +	if (!mount_crypt_stat->cipher_name_set) {
>  		int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
>  
>  		BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
> @@ -383,13 +385,13 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  		       ECRYPTFS_DEFAULT_CIPHER);
>  	}
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !fn_cipher_name_set)
> +	    && !mount_crypt_stat->fn_cipher_name_set)
>  		strcpy(mount_crypt_stat->global_default_fn_cipher_name,
>  		       mount_crypt_stat->global_default_cipher_name);
> -	if (!cipher_key_bytes_set)
> +	if (!mount_crypt_stat->cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_cipher_key_size = 0;
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !fn_cipher_key_bytes_set)
> +	    && !mount_crypt_stat->fn_cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_fn_cipher_key_bytes =
>  			mount_crypt_stat->global_default_cipher_key_size;
>  
> @@ -469,7 +471,6 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	const char *err = "Getting sb failed";
>  	struct inode *inode;
>  	struct path path;
> -	uid_t check_ruid;
>  	int rc;
>  
>  	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
> @@ -484,12 +485,17 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  		goto out;
>  	}
>  
> -	rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid);
> +	rc = ecryptfs_parse_options(sbi, raw_data);
>  	if (rc) {
>  		err = "Error parsing options";
>  		goto out;
>  	}
>  	mount_crypt_stat = &sbi->mount_crypt_stat;
> +	rc = ecryptfs_validate_options(mount_crypt_stat);
> +	if (rc) {
> +		err = "Error validationg options";
> +		goto out;
> +	}
>  
>  	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
>  	if (IS_ERR(s)) {
> @@ -529,7 +535,8 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  		goto out_free;
>  	}
>  
> -	if (check_ruid && !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
> +	if (mount_crypt_stat->check_ruid &&
> +	    !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
>  		rc = -EPERM;
>  		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
>  		       "requested user (uid: %d)\n",
> -- 
> 2.46.0
>