Re: [PATCH v5 8/8] fscrypt: make prepared keys record their type

["Luís Henriques" <lhenriques@suse.de>]

Sweet Tea Dorminy <sweettea-kernel@dorminy.me> writes:

> Right now fscrypt_infos have two fields dedicated solely to recording
> what type of prepared key the info has: whether it solely owns the
> prepared key, or has borrowed it from a master key, or from a direct
> key.
>
> The ci_direct_key field is only used for v1 direct key policies,
> recording the direct key that needs to have its refcount reduced when
> the crypt_info is freed. However, now that crypt_info->ci_enc_key is a
> pointer to the authoritative prepared key -- embedded in the direct key,
> in this case, we no longer need to keep a full pointer to the direct key
> -- we can use container_of() to go from the prepared key to its
> surrounding direct key.
>
> The key ownership information doesn't change during the lifetime of a
> prepared key.  Since at worst there's a prepared key per info, and at
> best many infos share a single prepared key, it can be slightly more
> efficient to store this ownership info in the prepared key instead of in
> the fscrypt_info, especially since we can squash both fields down into
> a single enum.
>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/fscrypt_private.h | 31 +++++++++++++++++++++++--------
>  fs/crypto/keysetup.c        | 21 +++++++++++++--------
>  fs/crypto/keysetup_v1.c     |  7 +++++--
>  3 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 5011737b60b3..e726a1fb9f7e 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -174,18 +174,39 @@ struct fscrypt_symlink_data {
>  	char encrypted_path[1];
>  } __packed;
>  
> +/**
> + * enum fscrypt_prepared_key_type - records a prepared key's ownership
> + *
> + * @FSCRYPT_KEY_PER_INFO: this prepared key is allocated for a specific info
> + *		          and is never shared.
> + * @FSCRYPT_KEY_DIRECT_V1: this prepared key is embedded in a fscrypt_direct_key
> + *		           used in v1 direct key policies.
> + * @FSCRYPT_KEY_MASTER_KEY: this prepared key is a per-mode and policy key,
> + *			    part of a fscrypt_master_key, shared between all
> + *			    users of this master key having this mode and
> + *			    policy.
> + */
> +enum fscrypt_prepared_key_type {
> +	FSCRYPT_KEY_PER_INFO = 1,
> +	FSCRYPT_KEY_DIRECT_V1,
> +	FSCRYPT_KEY_MASTER_KEY,
> +} __packed;
> +
>  /**
>   * struct fscrypt_prepared_key - a key prepared for actual encryption/decryption
>   * @tfm: crypto API transform object
>   * @blk_key: key for blk-crypto
> + * @type: records the ownership type of the prepared key
>   *
> - * Normally only one of the fields will be non-NULL.
> + * Normally only one of @tfm and @blk_key will be non-NULL, although it is
> + * possible if @type is FSCRYPT_KEY_MASTER_KEY.
>   */
>  struct fscrypt_prepared_key {
>  	struct crypto_skcipher *tfm;
>  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
>  	struct blk_crypto_key *blk_key;
>  #endif
> +	enum fscrypt_prepared_key_type type;
>  };
>  
>  /*
> @@ -233,12 +254,6 @@ struct fscrypt_info {
>  	 */
>  	struct list_head ci_master_key_link;
>  
> -	/*
> -	 * If non-NULL, then encryption is done using the master key directly
> -	 * and ci_enc_key will equal ci_direct_key->dk_key.
> -	 */
> -	struct fscrypt_direct_key *ci_direct_key;
> -
>  	/*
>  	 * This inode's hash key for filenames.  This is a 128-bit SipHash-2-4
>  	 * key.  This is only set for directories that use a keyed dirhash over
> @@ -641,7 +656,7 @@ static inline int fscrypt_require_key(struct inode *inode)
>  
>  /* keysetup_v1.c */
>  
> -void fscrypt_put_direct_key(struct fscrypt_direct_key *dk);
> +void fscrypt_put_direct_key(struct fscrypt_prepared_key *prep_key);
>  
>  int fscrypt_setup_v1_file_key(struct fscrypt_info *ci,
>  			      const u8 *raw_master_key);
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index 4f04999ecfd1..a19650f954e2 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -191,11 +191,11 @@ void fscrypt_destroy_prepared_key(struct super_block *sb,
>  /* Given a per-file encryption key, set up the file's crypto transform object */
>  int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key)
>  {
> -	ci->ci_owns_key = true;
>  	ci->ci_enc_key = kzalloc(sizeof(*ci->ci_enc_key), GFP_KERNEL);
>  	if (!ci->ci_enc_key)
>  		return -ENOMEM;
>  
> +	ci->ci_enc_key->type = FSCRYPT_KEY_PER_INFO;
>  	return fscrypt_prepare_key(ci->ci_enc_key, raw_key, ci);
>  }
>  
> @@ -290,7 +290,8 @@ static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
>  				  hkdf_context, hkdf_info, hkdf_infolen,
>  				  mode_key, mode->keysize);
>  	if (err)
> -		goto out_unlock;
> +		return err;

Is this change really intended?  I guess it's a mistake, because if we
simply return we'll leave keysetup mutex locked, which is probably not
what we want here.

Cheers,
-- 
Luís

> +	prep_key->type = FSCRYPT_KEY_MASTER_KEY;
>  	err = fscrypt_prepare_key(prep_key, mode_key, ci);
>  	memzero_explicit(mode_key, mode->keysize);
>