lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 24 Aug 2020 12:48:48 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Eric Biggers <ebiggers@...nel.org>, linux-fscrypt@...r.kernel.org
Cc:     linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, ceph-devel@...r.kernel.org
Subject: Re: [RFC PATCH 1/8] fscrypt: add fscrypt_prepare_new_inode() and
 fscrypt_set_context()

On Sun, 2020-08-23 at 23:17 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> fscrypt_get_encryption_info() (setting up an inode's encryption key) is
> intended to be GFP_NOFS safe.  But actually it isn't, since it uses
> functions like crypto_alloc_skcipher() which aren't GFP_NOFS safe, even
> when called with memalloc_nofs_save().  Therefore it can deadlock when
> called from a context that needs GFP_NOFS, e.g. during an ext4
> filesystem transaction or between f2fs_lock_op() and f2fs_unlock_op().
> Currently, this happens when creating a new encrypted file.
> 
> We can't fix this just by not setting up the key for new inodes right
> away, since new symlinks need their key to encrypt the symlink target.
> 
> So we need to set up the new inode's key before starting the
> transaction.  But fscrypt_get_encryption_info() can't do this currently,
> since it assumes the encryption xattr is already set, and the encryption
> xattr can't be set until the transaction.
> 
> The recently proposed fscrypt support for the ceph filesystem
> (https://lkml.kernel.org/linux-fscrypt/20200821182813.52570-1-jlayton@kernel.org/T/#u)
> will have this same ordering problem too, since when ceph creates a new
> symlink, it will need to encrypt it before setting its encryption xattr.
> 
> To solve this problem, add new helper functions:
> 
> - fscrypt_prepare_new_inode() sets up a new inode's encryption key
>   (fscrypt_info), using the parent directory's encryption policy and a
>   new random nonce.  It neither reads nor writes the xattr.
> 
> - fscrypt_set_context() sets the encryption xattr from the fscrypt_info
>   already in memory.  This replaces fscrypt_inherit_context().
> 
> Keep fscrypt_inherit_context() around temporarily until all filesystems
> are converted to use fscrypt_set_context().
> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
>  fs/crypto/fscrypt_private.h |   3 +
>  fs/crypto/keysetup.c        | 188 ++++++++++++++++++++++++++++--------
>  fs/crypto/policy.c          |  61 ++++++++++--
>  include/linux/fscrypt.h     |  17 ++++
>  4 files changed, 220 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 8117a61b6f558..355f6d9377517 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -572,6 +572,9 @@ int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key);
>  int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
>  			       const struct fscrypt_master_key *mk);
>  
> +void fscrypt_hash_inode_number(struct fscrypt_info *ci,
> +			       const struct fscrypt_master_key *mk);
> +
>  /* keysetup_v1.c */
>  
>  void fscrypt_put_direct_key(struct fscrypt_direct_key *dk);
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index fea6226afc2b0..6ac816d3e8478 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -10,6 +10,7 @@
>  
>  #include <crypto/skcipher.h>
>  #include <linux/key.h>
> +#include <linux/random.h>
>  
>  #include "fscrypt_private.h"
>  
> @@ -222,6 +223,16 @@ int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
>  	return 0;
>  }
>  
> +void fscrypt_hash_inode_number(struct fscrypt_info *ci,
> +			       const struct fscrypt_master_key *mk)
> +{
> +	WARN_ON(ci->ci_inode->i_ino == 0);
> +	WARN_ON(!mk->mk_ino_hash_key_initialized);
> +
> +	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
> +					      &mk->mk_ino_hash_key);

i_ino is an unsigned long. Will this produce a consistent results on
arches with 32 and 64 bit long values? I think it'd be nice to ensure
that we can access an encrypted directory created on a 32-bit host from
(e.g.) a 64-bit host.

It may be better to base this on something besides i_ino or provide an
alternate way of fetching a full 64-bit inode number here when i_ino is
32-bits.


> +}
> +
>  static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
>  					    struct fscrypt_master_key *mk)
>  {
> @@ -254,8 +265,10 @@ static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
>  			return err;
>  	}
>  
> -	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
> -					      &mk->mk_ino_hash_key);
> +	if (ci->ci_inode->i_ino == 0)
> +		WARN_ON(!(ci->ci_inode->i_state & I_NEW));
> +	else
> +		fscrypt_hash_inode_number(ci, mk);
>  	return 0;
>  }
>  
> @@ -454,57 +467,23 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  	kmem_cache_free(fscrypt_info_cachep, ci);
>  }
>  
> -int fscrypt_get_encryption_info(struct inode *inode)
> +static int
> +fscrypt_setup_encryption_info(struct inode *inode,
> +			      const union fscrypt_policy *policy,
> +			      const u8 nonce[FSCRYPT_FILE_NONCE_SIZE])
>  {
>  	struct fscrypt_info *crypt_info;
> -	union fscrypt_context ctx;
>  	struct fscrypt_mode *mode;
>  	struct key *master_key = NULL;
>  	int res;
>  
> -	if (fscrypt_has_encryption_key(inode))
> -		return 0;
> -
> -	res = fscrypt_initialize(inode->i_sb->s_cop->flags);
> -	if (res)
> -		return res;
> -
> -	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
> -	if (res < 0) {
> -		const union fscrypt_context *dummy_ctx =
> -			fscrypt_get_dummy_context(inode->i_sb);
> -
> -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> -			fscrypt_warn(inode,
> -				     "Error %d getting encryption context",
> -				     res);
> -			return res;
> -		}
> -		/* Fake up a context for an unencrypted directory */
> -		res = fscrypt_context_size(dummy_ctx);
> -		memcpy(&ctx, dummy_ctx, res);
> -	}
> -
>  	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_NOFS);
>  	if (!crypt_info)
>  		return -ENOMEM;
>  
>  	crypt_info->ci_inode = inode;
> -
> -	res = fscrypt_policy_from_context(&crypt_info->ci_policy, &ctx, res);
> -	if (res) {
> -		fscrypt_warn(inode,
> -			     "Unrecognized or corrupt encryption context");
> -		goto out;
> -	}
> -
> -	memcpy(crypt_info->ci_nonce, fscrypt_context_nonce(&ctx),
> -	       FSCRYPT_FILE_NONCE_SIZE);
> -
> -	if (!fscrypt_supported_policy(&crypt_info->ci_policy, inode)) {
> -		res = -EINVAL;
> -		goto out;
> -	}
> +	crypt_info->ci_policy = *policy;
> +	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
>  
>  	mode = select_encryption_mode(&crypt_info->ci_policy, inode);
>  	if (IS_ERR(mode)) {
> @@ -555,8 +534,133 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  	put_crypt_info(crypt_info);
>  	return res;
>  }
> +
> +/**
> + * fscrypt_get_encryption_info() - set up an inode's encryption key
> + * @inode: the inode to set up the key for.  Must either be encrypted, or be an
> + *	   unencrypted directory with '-o test_dummy_encryption'.
> + *
> + * Create ->i_crypt_info, if it's not already set.
> + *
> + * Note: unless ->i_crypt_info is already set, this isn't %GFP_NOFS-safe.  So
> + * generally this shouldn't be called from within a filesystem transaction.
> + *
> + * Return: 0 if ->i_crypt_info was set or was already set, *or* if the
> + *	   encryption key is unavailable.  (Use fscrypt_has_encryption_key() to
> + *	   distinguish these cases.)  Also can return another -errno code.
> + */
> +int fscrypt_get_encryption_info(struct inode *inode)
> +{
> +	int res;
> +	union fscrypt_context ctx;
> +	union fscrypt_policy policy;
> +
> +	if (fscrypt_has_encryption_key(inode))
> +		return 0;
> +
> +	res = fscrypt_initialize(inode->i_sb->s_cop->flags);
> +	if (res)
> +		return res;
> +
> +	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
> +	if (res < 0) {
> +		const union fscrypt_context *dummy_ctx =
> +			fscrypt_get_dummy_context(inode->i_sb);
> +
> +		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> +			fscrypt_warn(inode,
> +				     "Error %d getting encryption context",
> +				     res);
> +			return res;
> +		}
> +		/* Fake up a context for an unencrypted directory */
> +		res = fscrypt_context_size(dummy_ctx);
> +		memcpy(&ctx, dummy_ctx, res);
> +	}
> +
> +	res = fscrypt_policy_from_context(&policy, &ctx, res);
> +	if (res) {
> +		fscrypt_warn(inode,
> +			     "Unrecognized or corrupt encryption context");
> +		return res;
> +	}
> +
> +	if (!fscrypt_supported_policy(&policy, inode))
> +		return -EINVAL;
> +
> +	return fscrypt_setup_encryption_info(inode, &policy,
> +					     fscrypt_context_nonce(&ctx));
> +}
>  EXPORT_SYMBOL(fscrypt_get_encryption_info);
>  
> +/**
> + * fscrypt_prepare_new_inode() - prepare to create a new inode in a directory
> + * @dir: a possibly-encrypted directory
> + * @inode: the inode that is being created.  ->i_mode must be set already.
> + *	   ->i_ino does *not* need to be set yet.
> + * @encrypt_ret: (output) set to true if the new inode will be encrypted.
> + *
> + * Prepares to create a new inode in a directory.  If either the inode or its
> + * filename will be encrypted, this sets up the directory's
> + * ->i_crypt_info.  Additionally, if the inode will be encrypted, this sets up
> + * its ->i_crypt_info and sets *encrypt_ret to true.
> + *
> + * Note that the new inode's ->i_crypt_info *usually* isn't actually needed
> + * right away.  However, symlinks do need it.
> + *
> + * This isn't %GFP_NOFS-safe, and therefore it should be called before starting
> + * any filesystem transaction to create the inode.  For this reason, ->i_ino
> + * isn't required to be set yet, as the filesystem may not have set it yet.
> + *
> + * This doesn't actually store the new inode's encryption context to disk.
> + * That still needs to be done later by calling fscrypt_set_context().
> + *
> + * Return: 0 on success, -ENOKEY if the directory's encryption key is missing,
> + *	   or another -errno code
> + */
> +int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
> +			      bool *encrypt_ret)
> +{
> +	int err;
> +	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
> +
> +	/*
> +	 * If the filesystem is mounted with '-o test_dummy_encryption', files
> +	 * created in unencrypted directories are automatically encrypted.  For
> +	 * that case, we just need to treat the directory as encrypted here.
> +	 */
> +
> +	if (!IS_ENCRYPTED(dir) && fscrypt_get_dummy_context(dir->i_sb) == NULL)
> +		return 0;
> +
> +	err = fscrypt_get_encryption_info(dir);
> +	if (err)
> +		return err;
> +	if (!fscrypt_has_encryption_key(dir))
> +		return -ENOKEY;
> +
> +	if (WARN_ON_ONCE(inode->i_mode == 0))
> +		return -EINVAL;
> +
> +	/*
> +	 * Only regular files, directories, and symlinks are encrypted.
> +	 * Special files like device nodes and named pipes aren't.
> +	 */
> +	if (!S_ISREG(inode->i_mode) &&
> +	    !S_ISDIR(inode->i_mode) &&
> +	    !S_ISLNK(inode->i_mode))
> +		return 0;
> +
> +	*encrypt_ret = true;
> +
> +	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
> +
> +	return fscrypt_setup_encryption_info(inode,
> +					     &dir->i_crypt_info->ci_policy,
> +					     nonce);
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
> +
>  /**
>   * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
>   * @inode: an inode being evicted
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 2d73fd39ad96f..fbe4933206469 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -235,14 +235,17 @@ bool fscrypt_supported_policy(const union fscrypt_policy *policy_u,
>   *				       an fscrypt_policy
>   * @ctx_u: output context
>   * @policy_u: input policy
> + * @nonce: nonce to use
>   *
>   * Create an fscrypt_context for an inode that is being assigned the given
> - * encryption policy.  A new nonce is randomly generated.
> + * encryption policy.  @nonce must be a new random nonce.
>   *
>   * Return: the size of the new context in bytes.
>   */
> -static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
> -					   const union fscrypt_policy *policy_u)
> +static int
> +fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
> +				const union fscrypt_policy *policy_u,
> +				const u8 nonce[FSCRYPT_FILE_NONCE_SIZE])
>  {
>  	memset(ctx_u, 0, sizeof(*ctx_u));
>  
> @@ -260,7 +263,7 @@ static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
>  		memcpy(ctx->master_key_descriptor,
>  		       policy->master_key_descriptor,
>  		       sizeof(ctx->master_key_descriptor));
> -		get_random_bytes(ctx->nonce, sizeof(ctx->nonce));
> +		memcpy(ctx->nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
>  		return sizeof(*ctx);
>  	}
>  	case FSCRYPT_POLICY_V2: {
> @@ -276,7 +279,7 @@ static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
>  		memcpy(ctx->master_key_identifier,
>  		       policy->master_key_identifier,
>  		       sizeof(ctx->master_key_identifier));
> -		get_random_bytes(ctx->nonce, sizeof(ctx->nonce));
> +		memcpy(ctx->nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
>  		return sizeof(*ctx);
>  	}
>  	}
> @@ -372,6 +375,7 @@ static int fscrypt_get_policy(struct inode *inode, union fscrypt_policy *policy)
>  static int set_encryption_policy(struct inode *inode,
>  				 const union fscrypt_policy *policy)
>  {
> +	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
>  	union fscrypt_context ctx;
>  	int ctxsize;
>  	int err;
> @@ -409,7 +413,8 @@ static int set_encryption_policy(struct inode *inode,
>  		return -EINVAL;
>  	}
>  
> -	ctxsize = fscrypt_new_context_from_policy(&ctx, policy);
> +	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
> +	ctxsize = fscrypt_new_context_from_policy(&ctx, policy, nonce);
>  
>  	return inode->i_sb->s_cop->set_context(inode, &ctx, ctxsize, NULL);
>  }
> @@ -632,6 +637,7 @@ EXPORT_SYMBOL(fscrypt_has_permitted_context);
>  int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  						void *fs_data, bool preload)
>  {
> +	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
>  	union fscrypt_context ctx;
>  	int ctxsize;
>  	struct fscrypt_info *ci;
> @@ -645,7 +651,8 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  	if (ci == NULL)
>  		return -ENOKEY;
>  
> -	ctxsize = fscrypt_new_context_from_policy(&ctx, &ci->ci_policy);
> +	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
> +	ctxsize = fscrypt_new_context_from_policy(&ctx, &ci->ci_policy, nonce);
>  
>  	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
>  	res = parent->i_sb->s_cop->set_context(child, &ctx, ctxsize, fs_data);
> @@ -655,6 +662,46 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  }
>  EXPORT_SYMBOL(fscrypt_inherit_context);
>  
> +/**
> + * fscrypt_set_context() - Set the fscrypt context of a new inode
> + * @inode: A new inode
> + * @fs_data: private data given by FS and passed to ->set_context()
> + *
> + * This should be called after fscrypt_prepare_new_inode(), generally during a
> + * filesystem transaction.  Everything here must be %GFP_NOFS-safe.
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +int fscrypt_set_context(struct inode *inode, void *fs_data)
> +{
> +	struct fscrypt_info *ci = inode->i_crypt_info;
> +	union fscrypt_context ctx;
> +	int ctxsize;
> +
> +	/* fscrypt_prepare_new_inode() should have set up the key already. */
> +	if (WARN_ON_ONCE(!ci))
> +		return -ENOKEY;
> +
> +	/*
> +	 * This may be the first time the inode number is available, so do any
> +	 * delayed key setup that requires the inode number.
> +	 */
> +	if (ci->ci_policy.version == FSCRYPT_POLICY_V2 &&
> +	    (ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) {
> +		const struct fscrypt_master_key *mk =
> +			ci->ci_master_key->payload.data[0];
> +
> +		fscrypt_hash_inode_number(ci, mk);
> +	}
> +
> +	ctxsize = fscrypt_new_context_from_policy(&ctx, &ci->ci_policy,
> +						  ci->ci_nonce);
> +
> +	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
> +	return inode->i_sb->s_cop->set_context(inode, &ctx, ctxsize, fs_data);
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_set_context);
> +
>  /**
>   * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
>   * @sb: the filesystem on which test_dummy_encryption is being specified
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 991ff8575d0e7..726131dfa0a9b 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -158,6 +158,7 @@ int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg);
>  int fscrypt_has_permitted_context(struct inode *parent, struct inode *child);
>  int fscrypt_inherit_context(struct inode *parent, struct inode *child,
>  			    void *fs_data, bool preload);
> +int fscrypt_set_context(struct inode *inode, void *fs_data);
>  
>  struct fscrypt_dummy_context {
>  	const union fscrypt_context *ctx;
> @@ -184,6 +185,8 @@ int fscrypt_ioctl_get_key_status(struct file *filp, void __user *arg);
>  
>  /* keysetup.c */
>  int fscrypt_get_encryption_info(struct inode *inode);
> +int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
> +			      bool *encrypt_ret);
>  void fscrypt_put_encryption_info(struct inode *inode);
>  void fscrypt_free_inode(struct inode *inode);
>  int fscrypt_drop_inode(struct inode *inode);
> @@ -347,6 +350,11 @@ static inline int fscrypt_inherit_context(struct inode *parent,
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  struct fscrypt_dummy_context {
>  };
>  
> @@ -394,6 +402,15 @@ static inline int fscrypt_get_encryption_info(struct inode *inode)
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline int fscrypt_prepare_new_inode(struct inode *dir,
> +					    struct inode *inode,
> +					    bool *encrypt_ret)
> +{
> +	if (IS_ENCRYPTED(dir))
> +		return -EOPNOTSUPP;
> +	return 0;
> +}
> +
>  static inline void fscrypt_put_encryption_info(struct inode *inode)
>  {
>  	return;

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists