[RFC PATCH 1/8] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()
Jeff Layton
jlayton at kernel.org
Mon Aug 24 12:48:48 EDT 2020
On Sun, 2020-08-23 at 23:17 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers at google.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 at google.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 at kernel.org>
More information about the linux-mtd
mailing list