[PATCH v6 0/8] fscrypt: preliminary rearrangmeents of key setup

[PATCH v6 0/8] fscrypt: preliminary rearrangmeents of key setup

[Sweet Tea Dorminy]

For btrfs extent encryption, prepared keys need to be asynchronously
freed after the fscrypt_info is freed. This set of various
rearrangements of key setup turns the prepared key member of the info
into a pointer so this is possible.

Patchset is built on kdave/misc-next as per base commit and needs a tiny
fixup to apply to fscrypt/for-next. It passes ext4/f2fs tests for me.

[1] https://lore.kernel.org/linux-fscrypt/cover.1681837335.git.sweettea-kernel@dorminy.me/

Changelog:
v6:
 - Reword 'make infos have a pointer to prepared keys' to elaborate
   on why it is a useful change.

Sweet Tea Dorminy (8):
  fscrypt: move inline crypt decision to info setup
  fscrypt: split and rename setup_file_encryption_key()
  fscrypt: split setup_per_mode_enc_key()
  fscrypt: move dirhash key setup away from IO key setup
  fscrypt: reduce special-casing of IV_INO_LBLK_32
  fscrypt: move all the shared mode key setup deeper
  fscrypt: make infos have a pointer to prepared keys
  fscrypt: make prepared keys record their type

 fs/crypto/crypto.c          |   2 +-
 fs/crypto/fname.c           |   4 +-
 fs/crypto/fscrypt_private.h |  33 +++-
 fs/crypto/inline_crypt.c    |   4 +-
 fs/crypto/keysetup.c        | 357 +++++++++++++++++++++++-------------
 fs/crypto/keysetup_v1.c     |   9 +-
 6 files changed, 265 insertions(+), 144 deletions(-)


base-commit: 54d2161835d828a9663f548f61d1d9c3d3482122
-- 
2.41.0

[PATCH v6 1/8] fscrypt: move inline crypt decision to info setup

[Sweet Tea Dorminy]

setup_file_encryption_key() is doing a lot of things at the moment --
setting the crypt_info's inline encryption bit, finding and locking a
master key, and calling the functions to get the appropriate prepared
key for this info. Since setting the inline encryption bit has nothing
to do with finding the master key, it's easy and hopefully clearer to
select the encryption implementation in fscrypt_setup_encryption_info(),
the main fscrypt_info setup function, instead of in
setup_file_encryption_key() which will long-term only deal in setting
up the prepared key for the info.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 361f41ef46c7..b89c32ad19fb 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -443,10 +443,6 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
 	struct fscrypt_master_key *mk;
 	int err;
 
-	err = fscrypt_select_encryption_impl(ci);
-	if (err)
-		return err;
-
 	err = fscrypt_policy_to_key_spec(&ci->ci_policy, &mk_spec);
 	if (err)
 		return err;
@@ -580,6 +576,10 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	WARN_ON_ONCE(mode->ivsize > FSCRYPT_MAX_IV_SIZE);
 	crypt_info->ci_mode = mode;
 
+	res = fscrypt_select_encryption_impl(crypt_info);
+	if (res)
+		goto out;
+
 	res = setup_file_encryption_key(crypt_info, need_dirhash_key, &mk);
 	if (res)
 		goto out;
-- 
2.41.0

Re: [PATCH v6 1/8] fscrypt: move inline crypt decision to info setup

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:01PM -0400, Sweet Tea Dorminy wrote:
> setup_file_encryption_key() is doing a lot of things at the moment --
> setting the crypt_info's inline encryption bit, finding and locking a
> master key, and calling the functions to get the appropriate prepared
> key for this info. Since setting the inline encryption bit has nothing
> to do with finding the master key, it's easy and hopefully clearer to
> select the encryption implementation in fscrypt_setup_encryption_info(),
> the main fscrypt_info setup function, instead of in
> setup_file_encryption_key() which will long-term only deal in setting
> up the prepared key for the info.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH v6 2/8] fscrypt: split and rename setup_file_encryption_key()

[Sweet Tea Dorminy]

At present, setup_file_encryption_key() does several things: it finds
and locks the master key, and then calls into the appropriate functions
to setup the prepared key for the fscrypt_info. The code is clearer to
follow if these functions are divided.

Thus, move calling the appropriate file key setup function into a new
fscrypt_setup_file_key() function. After the file key setup functions
are moved, the remaining function can take a const fscrypt_info, and is
renamed find_and_lock_master_key() to precisely describe its action.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 77 ++++++++++++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index b89c32ad19fb..727d473b6b03 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -386,6 +386,43 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 	return 0;
 }
 
+/*
+ * Find or create the appropriate prepared key for an info.
+ */
+static int fscrypt_setup_file_key(struct fscrypt_info *ci,
+				  struct fscrypt_master_key *mk,
+				  bool need_dirhash_key)
+{
+	int err;
+
+	if (!mk) {
+		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
+			return -ENOKEY;
+
+		/*
+		 * As a legacy fallback for v1 policies, search for the key in
+		 * the current task's subscribed keyrings too.  Don't move this
+		 * to before the search of ->s_master_keys, since users
+		 * shouldn't be able to override filesystem-level keys.
+		 */
+		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
+	}
+
+	switch (ci->ci_policy.version) {
+	case FSCRYPT_POLICY_V1:
+		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
+		break;
+	case FSCRYPT_POLICY_V2:
+		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		err = -EINVAL;
+		break;
+	}
+	return err;
+}
+
 /*
  * Check whether the size of the given master key (@mk) is appropriate for the
  * encryption settings which a particular file will use (@ci).
@@ -426,7 +463,7 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
 }
 
 /*
- * Find the master key, then set up the inode's actual encryption key.
+ * Find and lock the master key.
  *
  * If the master key is found in the filesystem-level keyring, then it is
  * returned in *mk_ret with its semaphore read-locked.  This is needed to ensure
@@ -434,9 +471,8 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
  * multiple tasks may race to create an fscrypt_info for the same inode), and to
  * synchronize the master key being removed with a new inode starting to use it.
  */
-static int setup_file_encryption_key(struct fscrypt_info *ci,
-				     bool need_dirhash_key,
-				     struct fscrypt_master_key **mk_ret)
+static int find_and_lock_master_key(const struct fscrypt_info *ci,
+				    struct fscrypt_master_key **mk_ret)
 {
 	struct super_block *sb = ci->ci_inode->i_sb;
 	struct fscrypt_key_specifier mk_spec;
@@ -466,17 +502,19 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
 			mk = fscrypt_find_master_key(sb, &mk_spec);
 		}
 	}
+
 	if (unlikely(!mk)) {
 		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
 			return -ENOKEY;
 
 		/*
-		 * As a legacy fallback for v1 policies, search for the key in
-		 * the current task's subscribed keyrings too.  Don't move this
-		 * to before the search of ->s_master_keys, since users
-		 * shouldn't be able to override filesystem-level keys.
+		 * This might be the case of a v1 policy using a process
+		 * subscribed keyring to get the key, so there may not be
+		 * a relevant master key.
 		 */
-		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
+
+		*mk_ret = NULL;
+		return 0;
 	}
 	down_read(&mk->mk_sem);
 
@@ -491,21 +529,6 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
 		goto out_release_key;
 	}
 
-	switch (ci->ci_policy.version) {
-	case FSCRYPT_POLICY_V1:
-		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
-		break;
-	case FSCRYPT_POLICY_V2:
-		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
-		break;
-	default:
-		WARN_ON_ONCE(1);
-		err = -EINVAL;
-		break;
-	}
-	if (err)
-		goto out_release_key;
-
 	*mk_ret = mk;
 	return 0;
 
@@ -580,7 +603,11 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	if (res)
 		goto out;
 
-	res = setup_file_encryption_key(crypt_info, need_dirhash_key, &mk);
+	res = find_and_lock_master_key(crypt_info, &mk);
+	if (res)
+		goto out;
+
+	res = fscrypt_setup_file_key(crypt_info, mk, need_dirhash_key);
 	if (res)
 		goto out;
 
-- 
2.41.0

Re: [PATCH v6 2/8] fscrypt: split and rename setup_file_encryption_key()

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:02PM -0400, Sweet Tea Dorminy wrote:
> At present, setup_file_encryption_key() does several things: it finds
> and locks the master key, and then calls into the appropriate functions
> to setup the prepared key for the fscrypt_info. The code is clearer to
> follow if these functions are divided.
> 
> Thus, move calling the appropriate file key setup function into a new
> fscrypt_setup_file_key() function. After the file key setup functions
> are moved, the remaining function can take a const fscrypt_info, and is
> renamed find_and_lock_master_key() to precisely describe its action.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/keysetup.c | 77 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index b89c32ad19fb..727d473b6b03 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -386,6 +386,43 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
>  	return 0;
>  }
>  
> +/*
> + * Find or create the appropriate prepared key for an info.
> + */
> +static int fscrypt_setup_file_key(struct fscrypt_info *ci,
> +				  struct fscrypt_master_key *mk,
> +				  bool need_dirhash_key)
> +{
> +	int err;
> +
> +	if (!mk) {
> +		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
> +			return -ENOKEY;
> +
> +		/*
> +		 * As a legacy fallback for v1 policies, search for the key in
> +		 * the current task's subscribed keyrings too.  Don't move this
> +		 * to before the search of ->s_master_keys, since users
> +		 * shouldn't be able to override filesystem-level keys.
> +		 */
> +		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
> +	}
> +
> +	switch (ci->ci_policy.version) {
> +	case FSCRYPT_POLICY_V1:
> +		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
> +		break;
> +	case FSCRYPT_POLICY_V2:
> +		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		err = -EINVAL;
> +		break;
> +	}
> +	return err;
> +}
> +
>  /*
>   * Check whether the size of the given master key (@mk) is appropriate for the
>   * encryption settings which a particular file will use (@ci).
> @@ -426,7 +463,7 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
>  }
>  
>  /*
> - * Find the master key, then set up the inode's actual encryption key.
> + * Find and lock the master key.
>   *
>   * If the master key is found in the filesystem-level keyring, then it is
>   * returned in *mk_ret with its semaphore read-locked.  This is needed to ensure
> @@ -434,9 +471,8 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
>   * multiple tasks may race to create an fscrypt_info for the same inode), and to
>   * synchronize the master key being removed with a new inode starting to use it.
>   */
> -static int setup_file_encryption_key(struct fscrypt_info *ci,
> -				     bool need_dirhash_key,
> -				     struct fscrypt_master_key **mk_ret)
> +static int find_and_lock_master_key(const struct fscrypt_info *ci,
> +				    struct fscrypt_master_key **mk_ret)
>  {
>  	struct super_block *sb = ci->ci_inode->i_sb;
>  	struct fscrypt_key_specifier mk_spec;
> @@ -466,17 +502,19 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
>  			mk = fscrypt_find_master_key(sb, &mk_spec);
>  		}
>  	}
> +

Random newline, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

once you fix it up.  Thanks,

Josef

Re: [PATCH v6 2/8] fscrypt: split and rename setup_file_encryption_key()

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:02PM -0400, Sweet Tea Dorminy wrote:
> +/*
> + * Find or create the appropriate prepared key for an info.
> + */
> +static int fscrypt_setup_file_key(struct fscrypt_info *ci,
> +				  struct fscrypt_master_key *mk,
> +				  bool need_dirhash_key)
> +{
> +	int err;
> +
> +	if (!mk) {
> +		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
> +			return -ENOKEY;
> +
> +		/*
> +		 * As a legacy fallback for v1 policies, search for the key in
> +		 * the current task's subscribed keyrings too.  Don't move this
> +		 * to before the search of ->s_master_keys, since users
> +		 * shouldn't be able to override filesystem-level keys.
> +		 */
> +		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
> +	}
> +
> +	switch (ci->ci_policy.version) {
> +	case FSCRYPT_POLICY_V1:
> +		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
> +		break;
> +	case FSCRYPT_POLICY_V2:
> +		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		err = -EINVAL;
> +		break;
> +	}
> +	return err;
> +}

'err' is not needed.  The switch statement should look like:

	switch (ci->ci_policy.version) {
	case FSCRYPT_POLICY_V1:
		return fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
	case FSCRYPT_POLICY_V2:
		return fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
	default:
		WARN_ON_ONCE(1);
		return -EINVAL;
	}

>  /*
> - * Find the master key, then set up the inode's actual encryption key.
> + * Find and lock the master key.
>   *
>   * If the master key is found in the filesystem-level keyring, then it is
>   * returned in *mk_ret with its semaphore read-locked.  This is needed to ensure
> @@ -434,9 +471,8 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
>   * multiple tasks may race to create an fscrypt_info for the same inode), and to
>   * synchronize the master key being removed with a new inode starting to use it.
>   */
> -static int setup_file_encryption_key(struct fscrypt_info *ci,
> -				     bool need_dirhash_key,
> -				     struct fscrypt_master_key **mk_ret)
> +static int find_and_lock_master_key(const struct fscrypt_info *ci,
> +				    struct fscrypt_master_key **mk_ret)

I think it would be a bit cleaner if this returned
'struct fscrypt_master_key *'.  Use NULL for not found, ERR_PTR() for errors.

>  {
>  	struct super_block *sb = ci->ci_inode->i_sb;
>  	struct fscrypt_key_specifier mk_spec;
> @@ -466,17 +502,19 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
>  			mk = fscrypt_find_master_key(sb, &mk_spec);
>  		}
>  	}
> +
>  	if (unlikely(!mk)) {
>  		if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
>  			return -ENOKEY;
>  
>  		/*
> -		 * As a legacy fallback for v1 policies, search for the key in
> -		 * the current task's subscribed keyrings too.  Don't move this
> -		 * to before the search of ->s_master_keys, since users
> -		 * shouldn't be able to override filesystem-level keys.
> +		 * This might be the case of a v1 policy using a process
> +		 * subscribed keyring to get the key, so there may not be
> +		 * a relevant master key.
>  		 */
> -		return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
> +
> +		*mk_ret = NULL;
> +		return 0;
>  	}

'ci->ci_policy.version != FSCRYPT_POLICY_V1' is duplicated with
fscrypt_setup_file_key().  The problem is really that this patch makes the
handling of "master key not found" happen in two different places.

I think find_and_lock_master_key() should just return NULL for the master key
when it's not found.  Then fscrypt_setup_file_key() decides what to do about it.

Also, the comment for find_and_lock_master_key() needs to be updated.  The last
sentence in particular is not necessary anymore.  I think your refactoring fixes
the reason why that explanation was needed in the first place.  With my
suggestion to return a pointer, I think a good comment would be:

/*
 * Find the master key for ci_policy in the filesystem-level keyring.  Returns
 * the read-locked key if found, NULL if not found, or an ERR_PTR on error.  The
 * caller is responsible for unlocking and putting the key if found.
 */

- Eric

[PATCH v6 3/8] fscrypt: split setup_per_mode_enc_key()

[Sweet Tea Dorminy]

At present, setup_per_mode_enc_key() tries to find, within an array of
mode keys in the master key, an already prepared key, and if it doesn't
find a pre-prepared key, sets up a new one. This caching is not super
clear, at least to me, and splitting this function makes it clearer.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 61 +++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 727d473b6b03..06deac6f4487 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -184,34 +184,24 @@ int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key)
 	return fscrypt_prepare_key(&ci->ci_enc_key, raw_key, ci);
 }
 
-static int setup_per_mode_enc_key(struct fscrypt_info *ci,
-				  struct fscrypt_master_key *mk,
-				  struct fscrypt_prepared_key *keys,
-				  u8 hkdf_context, bool include_fs_uuid)
+static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
+				       struct fscrypt_prepared_key *prep_key,
+				       const struct fscrypt_info *ci,
+				       u8 hkdf_context, bool include_fs_uuid)
 {
 	const struct inode *inode = ci->ci_inode;
 	const struct super_block *sb = inode->i_sb;
 	struct fscrypt_mode *mode = ci->ci_mode;
 	const u8 mode_num = mode - fscrypt_modes;
-	struct fscrypt_prepared_key *prep_key;
 	u8 mode_key[FSCRYPT_MAX_KEY_SIZE];
 	u8 hkdf_info[sizeof(mode_num) + sizeof(sb->s_uuid)];
 	unsigned int hkdf_infolen = 0;
-	int err;
-
-	if (WARN_ON_ONCE(mode_num > FSCRYPT_MODE_MAX))
-		return -EINVAL;
-
-	prep_key = &keys[mode_num];
-	if (fscrypt_is_key_prepared(prep_key, ci)) {
-		ci->ci_enc_key = *prep_key;
-		return 0;
-	}
+	int err = 0;
 
 	mutex_lock(&fscrypt_mode_key_setup_mutex);
 
 	if (fscrypt_is_key_prepared(prep_key, ci))
-		goto done_unlock;
+		goto out_unlock;
 
 	BUILD_BUG_ON(sizeof(mode_num) != 1);
 	BUILD_BUG_ON(sizeof(sb->s_uuid) != 16);
@@ -229,16 +219,39 @@ static int setup_per_mode_enc_key(struct fscrypt_info *ci,
 		goto out_unlock;
 	err = fscrypt_prepare_key(prep_key, mode_key, ci);
 	memzero_explicit(mode_key, mode->keysize);
-	if (err)
-		goto out_unlock;
-done_unlock:
-	ci->ci_enc_key = *prep_key;
-	err = 0;
+
 out_unlock:
 	mutex_unlock(&fscrypt_mode_key_setup_mutex);
 	return err;
 }
 
+static int find_mode_prepared_key(struct fscrypt_info *ci,
+				  struct fscrypt_master_key *mk,
+				  struct fscrypt_prepared_key *keys,
+				  u8 hkdf_context, bool include_fs_uuid)
+{
+	struct fscrypt_mode *mode = ci->ci_mode;
+	const u8 mode_num = mode - fscrypt_modes;
+	struct fscrypt_prepared_key *prep_key;
+	int err;
+
+	if (WARN_ON_ONCE(mode_num > FSCRYPT_MODE_MAX))
+		return -EINVAL;
+
+	prep_key = &keys[mode_num];
+	if (fscrypt_is_key_prepared(prep_key, ci)) {
+		ci->ci_enc_key = *prep_key;
+		return 0;
+	}
+	err = setup_new_mode_prepared_key(mk, prep_key, ci, hkdf_context,
+					  include_fs_uuid);
+	if (err)
+		return err;
+
+	ci->ci_enc_key = *prep_key;
+	return 0;
+}
+
 /*
  * Derive a SipHash key from the given fscrypt master key and the given
  * application-specific information string.
@@ -294,7 +307,7 @@ static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
 {
 	int err;
 
-	err = setup_per_mode_enc_key(ci, mk, mk->mk_iv_ino_lblk_32_keys,
+	err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys,
 				     HKDF_CONTEXT_IV_INO_LBLK_32_KEY, true);
 	if (err)
 		return err;
@@ -344,7 +357,7 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 		 * encryption key.  This ensures that the master key is
 		 * consistently used only for HKDF, avoiding key reuse issues.
 		 */
-		err = setup_per_mode_enc_key(ci, mk, mk->mk_direct_keys,
+		err = find_mode_prepared_key(ci, mk, mk->mk_direct_keys,
 					     HKDF_CONTEXT_DIRECT_KEY, false);
 	} else if (ci->ci_policy.v2.flags &
 		   FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) {
@@ -354,7 +367,7 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 		 * the IVs.  This format is optimized for use with inline
 		 * encryption hardware compliant with the UFS standard.
 		 */
-		err = setup_per_mode_enc_key(ci, mk, mk->mk_iv_ino_lblk_64_keys,
+		err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_64_keys,
 					     HKDF_CONTEXT_IV_INO_LBLK_64_KEY,
 					     true);
 	} else if (ci->ci_policy.v2.flags &
-- 
2.41.0

Re: [PATCH v6 3/8] fscrypt: split setup_per_mode_enc_key()

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:03PM -0400, Sweet Tea Dorminy wrote:
> At present, setup_per_mode_enc_key() tries to find, within an array of
> mode keys in the master key, an already prepared key, and if it doesn't
> find a pre-prepared key, sets up a new one. This caching is not super
> clear, at least to me, and splitting this function makes it clearer.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v6 3/8] fscrypt: split setup_per_mode_enc_key()

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:03PM -0400, Sweet Tea Dorminy wrote:
> +static int find_mode_prepared_key(struct fscrypt_info *ci,
> +				  struct fscrypt_master_key *mk,
> +				  struct fscrypt_prepared_key *keys,
> +				  u8 hkdf_context, bool include_fs_uuid)
> +{
> +	struct fscrypt_mode *mode = ci->ci_mode;
> +	const u8 mode_num = mode - fscrypt_modes;
> +	struct fscrypt_prepared_key *prep_key;
> +	int err;
> +
> +	if (WARN_ON_ONCE(mode_num > FSCRYPT_MODE_MAX))
> +		return -EINVAL;
> +
> +	prep_key = &keys[mode_num];
> +	if (fscrypt_is_key_prepared(prep_key, ci)) {
> +		ci->ci_enc_key = *prep_key;
> +		return 0;
> +	}
> +	err = setup_new_mode_prepared_key(mk, prep_key, ci, hkdf_context,
> +					  include_fs_uuid);
> +	if (err)
> +		return err;
> +
> +	ci->ci_enc_key = *prep_key;
> +	return 0;
> +}

Any thoughts about the feedback I gave about this on v2
(https://lore.kernel.org/linux-fscrypt/20230411032935.GC47625@sol.localdomain/)?
The new naming with "find" seems wrong.

- Eric

[PATCH v6 4/8] fscrypt: move dirhash key setup away from IO key setup

[Sweet Tea Dorminy]

The function named fscrypt_setup_v2_file_key() has as its main focus the
setting up of the fscrypt_info's ci_enc_key member, the prepared key
with which filenames or file contents are encrypted or decrypted.
However, it currently also sets up the dirhash key, used by some
directories, based on a parameter. There are no dependencies on
setting up the dirhash key beyond having the master key locked, and it's
clearer having fscrypt_setup_file_key() be only about setting up the
prepared key for IO.

Thus, move dirhash key setup to fscrypt_setup_encryption_info(), which
calls out to each function setting up parts of the fscrypt_info, and
stop passing the need_dirhash_key parameter around.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 06deac6f4487..430e2455ea2d 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -343,8 +343,7 @@ static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
 }
 
 static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
-				     struct fscrypt_master_key *mk,
-				     bool need_dirhash_key)
+				     struct fscrypt_master_key *mk)
 {
 	int err;
 
@@ -386,25 +385,15 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 		err = fscrypt_set_per_file_enc_key(ci, derived_key);
 		memzero_explicit(derived_key, ci->ci_mode->keysize);
 	}
-	if (err)
-		return err;
 
-	/* Derive a secret dirhash key for directories that need it. */
-	if (need_dirhash_key) {
-		err = fscrypt_derive_dirhash_key(ci, mk);
-		if (err)
-			return err;
-	}
-
-	return 0;
+	return err;
 }
 
 /*
  * Find or create the appropriate prepared key for an info.
  */
 static int fscrypt_setup_file_key(struct fscrypt_info *ci,
-				  struct fscrypt_master_key *mk,
-				  bool need_dirhash_key)
+				  struct fscrypt_master_key *mk)
 {
 	int err;
 
@@ -426,7 +415,7 @@ static int fscrypt_setup_file_key(struct fscrypt_info *ci,
 		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
 		break;
 	case FSCRYPT_POLICY_V2:
-		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
+		err = fscrypt_setup_v2_file_key(ci, mk);
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -620,10 +609,26 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	if (res)
 		goto out;
 
-	res = fscrypt_setup_file_key(crypt_info, mk, need_dirhash_key);
+	res = fscrypt_setup_file_key(crypt_info, mk);
 	if (res)
 		goto out;
 
+	/*
+	 * Derive a secret dirhash key for directories that need it. It
+	 * should be impossible to set flags such that a v1 policy sets
+	 * need_dirhash_key, but check it anyway.
+	 */
+	if (need_dirhash_key) {
+		if (WARN_ON_ONCE(policy->version == FSCRYPT_POLICY_V1)) {
+			res = -EINVAL;
+			goto out;
+		}
+
+		res = fscrypt_derive_dirhash_key(crypt_info, mk);
+		if (res)
+			goto out;
+	}
+
 	/*
 	 * For existing inodes, multiple tasks may race to set ->i_crypt_info.
 	 * So use cmpxchg_release().  This pairs with the smp_load_acquire() in
-- 
2.41.0

Re: [PATCH v6 4/8] fscrypt: move dirhash key setup away from IO key setup

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:04PM -0400, Sweet Tea Dorminy wrote:
> The function named fscrypt_setup_v2_file_key() has as its main focus the
> setting up of the fscrypt_info's ci_enc_key member, the prepared key
> with which filenames or file contents are encrypted or decrypted.
> However, it currently also sets up the dirhash key, used by some
> directories, based on a parameter. There are no dependencies on
> setting up the dirhash key beyond having the master key locked, and it's
> clearer having fscrypt_setup_file_key() be only about setting up the
> prepared key for IO.
> 
> Thus, move dirhash key setup to fscrypt_setup_encryption_info(), which
> calls out to each function setting up parts of the fscrypt_info, and
> stop passing the need_dirhash_key parameter around.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH v6 5/8] fscrypt: reduce special-casing of IV_INO_LBLK_32

[Sweet Tea Dorminy]

Right now, the IV_INO_LBLK_32 policy is handled by its own function
called in fscrypt_setup_v2_file_key(), different from all other policies
which just call find_mode_prepared_key() with various parameters. The
function additionally sets up the relevant inode hashing key in the
master key, and uses it to hash the inode number if possible. This is
not particularly relevant to setting up a prepared key, so this change
tries to make it clear that every non-default policy uses basically the
same setup mechanism for its prepared key. The other setup is moved to
be called from the top crypt_info setup function.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 430e2455ea2d..8b201b91c036 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -302,44 +302,30 @@ void fscrypt_hash_inode_number(struct fscrypt_info *ci,
 					      &mk->mk_ino_hash_key);
 }
 
-static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
-					    struct fscrypt_master_key *mk)
+static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk)
 {
 	int err;
 
-	err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys,
-				     HKDF_CONTEXT_IV_INO_LBLK_32_KEY, true);
-	if (err)
-		return err;
-
 	/* pairs with smp_store_release() below */
-	if (!smp_load_acquire(&mk->mk_ino_hash_key_initialized)) {
+	if (smp_load_acquire(&mk->mk_ino_hash_key_initialized))
+		return 0;
 
-		mutex_lock(&fscrypt_mode_key_setup_mutex);
+	mutex_lock(&fscrypt_mode_key_setup_mutex);
 
-		if (mk->mk_ino_hash_key_initialized)
-			goto unlock;
+	if (mk->mk_ino_hash_key_initialized)
+		goto unlock;
 
-		err = fscrypt_derive_siphash_key(mk,
-						 HKDF_CONTEXT_INODE_HASH_KEY,
-						 NULL, 0, &mk->mk_ino_hash_key);
-		if (err)
-			goto unlock;
-		/* pairs with smp_load_acquire() above */
-		smp_store_release(&mk->mk_ino_hash_key_initialized, true);
+	err = fscrypt_derive_siphash_key(mk,
+					 HKDF_CONTEXT_INODE_HASH_KEY,
+					 NULL, 0, &mk->mk_ino_hash_key);
+	if (err)
+		goto unlock;
+	/* pairs with smp_load_acquire() above */
+	smp_store_release(&mk->mk_ino_hash_key_initialized, true);
 unlock:
-		mutex_unlock(&fscrypt_mode_key_setup_mutex);
-		if (err)
-			return err;
-	}
+	mutex_unlock(&fscrypt_mode_key_setup_mutex);
 
-	/*
-	 * New inodes may not have an inode number assigned yet.
-	 * Hashing their inode number is delayed until later.
-	 */
-	if (ci->ci_inode->i_ino)
-		fscrypt_hash_inode_number(ci, mk);
-	return 0;
+	return err;
 }
 
 static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
@@ -371,7 +357,9 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 					     true);
 	} else if (ci->ci_policy.v2.flags &
 		   FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) {
-		err = fscrypt_setup_iv_ino_lblk_32_key(ci, mk);
+		err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys,
+					     HKDF_CONTEXT_IV_INO_LBLK_32_KEY,
+					     true);
 	} else {
 		u8 derived_key[FSCRYPT_MAX_KEY_SIZE];
 
@@ -629,6 +617,20 @@ fscrypt_setup_encryption_info(struct inode *inode,
 			goto out;
 	}
 
+	/*
+	 * The IV_INO_LBLK_32 policy needs a hashed inode number, but new
+	 * inodes may not have an inode number assigned yet.
+	 */
+	if (policy->version == FSCRYPT_POLICY_V2 &&
+	    (policy->v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) {
+		res = fscrypt_setup_ino_hash_key(mk);
+		if (res)
+			goto out;
+
+		if (inode->i_ino)
+			fscrypt_hash_inode_number(crypt_info, mk);
+	}
+
 	/*
 	 * For existing inodes, multiple tasks may race to set ->i_crypt_info.
 	 * So use cmpxchg_release().  This pairs with the smp_load_acquire() in
-- 
2.41.0

Re: [PATCH v6 5/8] fscrypt: reduce special-casing of IV_INO_LBLK_32

[kernel test robot]

Hi Sweet,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 54d2161835d828a9663f548f61d1d9c3d3482122]

url:    https://github.com/intel-lab-lkp/linux/commits/Sweet-Tea-Dorminy/fscrypt-move-inline-crypt-decision-to-info-setup/20230809-030251
base:   54d2161835d828a9663f548f61d1d9c3d3482122
patch link:    https://lore.kernel.org/r/542ea134771e2caa3043dfe48c2825d93495c626.1691505830.git.sweettea-kernel%40dorminy.me
patch subject: [PATCH v6 5/8] fscrypt: reduce special-casing of IV_INO_LBLK_32
config: hexagon-randconfig-r041-20230808 (https://download.01.org/0day-ci/archive/20230809/202308091311.R1mgw8Sk-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230809/202308091311.R1mgw8Sk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308091311.R1mgw8Sk-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from fs/crypto/keysetup.c:14:
   In file included from fs/crypto/fscrypt_private.h:17:
   In file included from include/linux/blk-crypto.h:72:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from fs/crypto/keysetup.c:14:
   In file included from fs/crypto/fscrypt_private.h:17:
   In file included from include/linux/blk-crypto.h:72:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from fs/crypto/keysetup.c:14:
   In file included from fs/crypto/fscrypt_private.h:17:
   In file included from include/linux/blk-crypto.h:72:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> fs/crypto/keysetup.c:315:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (mk->mk_ino_hash_key_initialized)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/crypto/keysetup.c:328:9: note: uninitialized use occurs here
           return err;
                  ^~~
   fs/crypto/keysetup.c:315:2: note: remove the 'if' if its condition is always false
           if (mk->mk_ino_hash_key_initialized)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/crypto/keysetup.c:307:9: note: initialize the variable 'err' to silence this warning
           int err;
                  ^
                   = 0
   7 warnings generated.


vim +315 fs/crypto/keysetup.c

a992b20cd4ee36 Eric Biggers      2020-09-16  304  
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08  305  static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk)
e3b1078bedd323 Eric Biggers      2020-05-15  306  {
e3b1078bedd323 Eric Biggers      2020-05-15  307  	int err;
e3b1078bedd323 Eric Biggers      2020-05-15  308  
e3b1078bedd323 Eric Biggers      2020-05-15  309  	/* pairs with smp_store_release() below */
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08  310  	if (smp_load_acquire(&mk->mk_ino_hash_key_initialized))
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08  311  		return 0;
e3b1078bedd323 Eric Biggers      2020-05-15  312  
e3b1078bedd323 Eric Biggers      2020-05-15  313  	mutex_lock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers      2020-05-15  314  
e3b1078bedd323 Eric Biggers      2020-05-15 @315  	if (mk->mk_ino_hash_key_initialized)
e3b1078bedd323 Eric Biggers      2020-05-15  316  		goto unlock;
e3b1078bedd323 Eric Biggers      2020-05-15  317  
2fc2b430f559fd Eric Biggers      2021-06-05  318  	err = fscrypt_derive_siphash_key(mk,
2fc2b430f559fd Eric Biggers      2021-06-05  319  					 HKDF_CONTEXT_INODE_HASH_KEY,
2fc2b430f559fd Eric Biggers      2021-06-05  320  					 NULL, 0, &mk->mk_ino_hash_key);
e3b1078bedd323 Eric Biggers      2020-05-15  321  	if (err)
e3b1078bedd323 Eric Biggers      2020-05-15  322  		goto unlock;
e3b1078bedd323 Eric Biggers      2020-05-15  323  	/* pairs with smp_load_acquire() above */
e3b1078bedd323 Eric Biggers      2020-05-15  324  	smp_store_release(&mk->mk_ino_hash_key_initialized, true);
e3b1078bedd323 Eric Biggers      2020-05-15  325  unlock:
e3b1078bedd323 Eric Biggers      2020-05-15  326  	mutex_unlock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers      2020-05-15  327  
3d3afe7cb13fb9 Sweet Tea Dorminy 2023-08-08  328  	return err;
e3b1078bedd323 Eric Biggers      2020-05-15  329  }
e3b1078bedd323 Eric Biggers      2020-05-15  330  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 5/8] fscrypt: reduce special-casing of IV_INO_LBLK_32

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:05PM -0400, Sweet Tea Dorminy wrote:
> Right now, the IV_INO_LBLK_32 policy is handled by its own function
> called in fscrypt_setup_v2_file_key(), different from all other policies
> which just call find_mode_prepared_key() with various parameters. The
> function additionally sets up the relevant inode hashing key in the
> master key, and uses it to hash the inode number if possible. This is
> not particularly relevant to setting up a prepared key, so this change
> tries to make it clear that every non-default policy uses basically the
> same setup mechanism for its prepared key. The other setup is moved to
> be called from the top crypt_info setup function.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

It seems the goal of this patch is to finish the refactoring started by patches
2 and 4 of making fscrypt_setup_file_key() only set up the I/O key ("prepared
key").  The title and description don't make it very clear, though.  I think a
better title would be the following which is analogous to patch 4:

    fscrypt: move ino hashing setup away from IO key setup

BTW, it seems patch 3 should not be where it is in the series, since 2, 4, and 5
seem to be on one topic.

> +static int fscrypt_setup_ino_hash_key(struct fscrypt_master_key *mk)
>  {
>  	int err;

err needs to be initialized to 0.

> +	/*
> +	 * The IV_INO_LBLK_32 policy needs a hashed inode number, but new
> +	 * inodes may not have an inode number assigned yet.
> +	 */
> +	if (policy->version == FSCRYPT_POLICY_V2 &&
> +	    (policy->v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) {
> +		res = fscrypt_setup_ino_hash_key(mk);
> +		if (res)
> +			goto out;
> +
> +		if (inode->i_ino)
> +			fscrypt_hash_inode_number(crypt_info, mk);
> +	}

This seems to be another case where a comment was copied but it doesn't make as
much sense in the new context.  How about the following:

	/* 
	 * If the file is using an IV_INO_LBLK_32 policy, derive the inode hash
	 * key if it wasn't done already.  Then hash the inode number and cache
         * the resulting hash.  New inodes might not have an inode number
         * assigned yet; hashing their inode number is delayed until later.
	 */

- Eric

[PATCH v6 6/8] fscrypt: move all the shared mode key setup deeper

[Sweet Tea Dorminy]

Currently, fscrypt_setup_v2_file_key() has a set of ifs which encode
various information about how to set up a new mode key if necessary for
a shared-key policy (DIRECT or IV_INO_LBLK_*). This is somewhat awkward
-- this information is only needed at the point that we need to setup a
new key, which is not the common case; the setup details are recorded as
function parameters relatively far from where they're actually used; and
at the point we use the parameters, we can derive the information
equally well.

So this moves mode and policy checking as deep into the callstack as
possible. mk_prepared_key_for_mode_policy() deals with the array lookup
within a master key. And fill_hkdf_info_for mode_key() deals with
filling in the hkdf info as necessary for a particular policy. These
seem a little clearer in broad strokes, emphasizing the similarities
between the policies, but it does spread out the information on how the
key is derived for a particular policy more.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 131 +++++++++++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 43 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 8b201b91c036..7dd12c1821dd 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -13,6 +13,17 @@
 
 #include "fscrypt_private.h"
 
+#define MAX_MODE_KEY_HKDF_INFO_SIZE 17
+
+/*
+ * Constant defining the various policy flags which require a non-default key
+ * policy.
+ */
+#define FSCRYPT_POLICY_FLAGS_KEY_MASK		\
+	(FSCRYPT_POLICY_FLAG_DIRECT_KEY		\
+	 | FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64	\
+	 | FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)
+
 struct fscrypt_mode fscrypt_modes[] = {
 	[FSCRYPT_MODE_AES_256_XTS] = {
 		.friendly_name = "AES-256-XTS",
@@ -184,20 +195,83 @@ int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key)
 	return fscrypt_prepare_key(&ci->ci_enc_key, raw_key, ci);
 }
 
+static struct fscrypt_prepared_key *
+mk_prepared_key_for_mode_policy(struct fscrypt_master_key *mk,
+				union fscrypt_policy *policy,
+				struct fscrypt_mode *mode)
+{
+	const u8 mode_num = mode - fscrypt_modes;
+
+	switch (policy->v2.flags & FSCRYPT_POLICY_FLAGS_KEY_MASK) {
+	case FSCRYPT_POLICY_FLAG_DIRECT_KEY:
+		return &mk->mk_direct_keys[mode_num];
+	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64:
+		return &mk->mk_iv_ino_lblk_64_keys[mode_num];
+	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32:
+		return &mk->mk_iv_ino_lblk_32_keys[mode_num];
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+static size_t
+fill_hkdf_info_for_mode_key(const struct fscrypt_info *ci,
+			    u8 hkdf_info[MAX_MODE_KEY_HKDF_INFO_SIZE])
+{
+	const u8 mode_num = ci->ci_mode - fscrypt_modes;
+	const struct super_block *sb = ci->ci_inode->i_sb;
+	u8 hkdf_infolen = 0;
+
+	hkdf_info[hkdf_infolen++] = mode_num;
+	if (!(ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY)) {
+		memcpy(&hkdf_info[hkdf_infolen], &sb->s_uuid,
+				sizeof(sb->s_uuid));
+		hkdf_infolen += sizeof(sb->s_uuid);
+	}
+	return hkdf_infolen;
+}
+
 static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
 				       struct fscrypt_prepared_key *prep_key,
-				       const struct fscrypt_info *ci,
-				       u8 hkdf_context, bool include_fs_uuid)
+				       const struct fscrypt_info *ci)
 {
 	const struct inode *inode = ci->ci_inode;
 	const struct super_block *sb = inode->i_sb;
+	unsigned int policy_flags = fscrypt_policy_flags(&ci->ci_policy);
 	struct fscrypt_mode *mode = ci->ci_mode;
 	const u8 mode_num = mode - fscrypt_modes;
 	u8 mode_key[FSCRYPT_MAX_KEY_SIZE];
 	u8 hkdf_info[sizeof(mode_num) + sizeof(sb->s_uuid)];
 	unsigned int hkdf_infolen = 0;
+	u8 hkdf_context = 0;
 	int err = 0;
 
+	switch (policy_flags & FSCRYPT_POLICY_FLAGS_KEY_MASK) {
+	case FSCRYPT_POLICY_FLAG_DIRECT_KEY:
+		hkdf_context = HKDF_CONTEXT_DIRECT_KEY;
+		break;
+	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64:
+		hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_64_KEY;
+		break;
+	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32:
+		hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_32_KEY;
+		break;
+	}
+
+	/*
+	 * For DIRECT_KEY policies: instead of deriving per-file encryption
+	 * keys, the per-file nonce will be included in all the IVs.  But
+	 * unlike v1 policies, for v2 policies in this case we don't encrypt
+	 * with the master key directly but rather derive a per-mode encryption
+	 * key.  This ensures that the master key is consistently used only for
+	 * HKDF, avoiding key reuse issues.
+	 *
+	 * For IV_INO_LBLK policies: encryption keys are derived from
+	 * (master_key, mode_num, filesystem_uuid), and inode number is
+	 * included in the IVs.  This format is optimized for use with inline
+	 * encryption hardware compliant with the UFS standard.
+	 */
+
 	mutex_lock(&fscrypt_mode_key_setup_mutex);
 
 	if (fscrypt_is_key_prepared(prep_key, ci))
@@ -205,13 +279,9 @@ static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
 
 	BUILD_BUG_ON(sizeof(mode_num) != 1);
 	BUILD_BUG_ON(sizeof(sb->s_uuid) != 16);
-	BUILD_BUG_ON(sizeof(hkdf_info) != 17);
-	hkdf_info[hkdf_infolen++] = mode_num;
-	if (include_fs_uuid) {
-		memcpy(&hkdf_info[hkdf_infolen], &sb->s_uuid,
-		       sizeof(sb->s_uuid));
-		hkdf_infolen += sizeof(sb->s_uuid);
-	}
+	BUILD_BUG_ON(sizeof(hkdf_info) != MAX_MODE_KEY_HKDF_INFO_SIZE);
+	hkdf_infolen = fill_hkdf_info_for_mode_key(ci, hkdf_info);
+
 	err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
 				  hkdf_context, hkdf_info, hkdf_infolen,
 				  mode_key, mode->keysize);
@@ -225,10 +295,8 @@ static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
 	return err;
 }
 
-static int find_mode_prepared_key(struct fscrypt_info *ci,
-				  struct fscrypt_master_key *mk,
-				  struct fscrypt_prepared_key *keys,
-				  u8 hkdf_context, bool include_fs_uuid)
+static int setup_mode_prepared_key(struct fscrypt_info *ci,
+				  struct fscrypt_master_key *mk)
 {
 	struct fscrypt_mode *mode = ci->ci_mode;
 	const u8 mode_num = mode - fscrypt_modes;
@@ -238,13 +306,15 @@ static int find_mode_prepared_key(struct fscrypt_info *ci,
 	if (WARN_ON_ONCE(mode_num > FSCRYPT_MODE_MAX))
 		return -EINVAL;
 
-	prep_key = &keys[mode_num];
+	prep_key = mk_prepared_key_for_mode_policy(mk, &ci->ci_policy, mode);
+	if (IS_ERR(prep_key))
+		return PTR_ERR(prep_key);
+
 	if (fscrypt_is_key_prepared(prep_key, ci)) {
 		ci->ci_enc_key = *prep_key;
 		return 0;
 	}
-	err = setup_new_mode_prepared_key(mk, prep_key, ci, hkdf_context,
-					  include_fs_uuid);
+	err = setup_new_mode_prepared_key(mk, prep_key, ci);
 	if (err)
 		return err;
 
@@ -333,33 +403,8 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 {
 	int err;
 
-	if (ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAG_DIRECT_KEY) {
-		/*
-		 * DIRECT_KEY: instead of deriving per-file encryption keys, the
-		 * per-file nonce will be included in all the IVs.  But unlike
-		 * v1 policies, for v2 policies in this case we don't encrypt
-		 * with the master key directly but rather derive a per-mode
-		 * encryption key.  This ensures that the master key is
-		 * consistently used only for HKDF, avoiding key reuse issues.
-		 */
-		err = find_mode_prepared_key(ci, mk, mk->mk_direct_keys,
-					     HKDF_CONTEXT_DIRECT_KEY, false);
-	} else if (ci->ci_policy.v2.flags &
-		   FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) {
-		/*
-		 * IV_INO_LBLK_64: encryption keys are derived from (master_key,
-		 * mode_num, filesystem_uuid), and inode number is included in
-		 * the IVs.  This format is optimized for use with inline
-		 * encryption hardware compliant with the UFS standard.
-		 */
-		err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_64_keys,
-					     HKDF_CONTEXT_IV_INO_LBLK_64_KEY,
-					     true);
-	} else if (ci->ci_policy.v2.flags &
-		   FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32) {
-		err = find_mode_prepared_key(ci, mk, mk->mk_iv_ino_lblk_32_keys,
-					     HKDF_CONTEXT_IV_INO_LBLK_32_KEY,
-					     true);
+	if (ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAGS_KEY_MASK) {
+		err = setup_mode_prepared_key(ci, mk);
 	} else {
 		u8 derived_key[FSCRYPT_MAX_KEY_SIZE];
 
-- 
2.41.0

Re: [PATCH v6 6/8] fscrypt: move all the shared mode key setup deeper

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:06PM -0400, Sweet Tea Dorminy wrote:
> Currently, fscrypt_setup_v2_file_key() has a set of ifs which encode
> various information about how to set up a new mode key if necessary for
> a shared-key policy (DIRECT or IV_INO_LBLK_*). This is somewhat awkward
> -- this information is only needed at the point that we need to setup a
> new key, which is not the common case; the setup details are recorded as
> function parameters relatively far from where they're actually used; and
> at the point we use the parameters, we can derive the information
> equally well.
> 
> So this moves mode and policy checking as deep into the callstack as
> possible. mk_prepared_key_for_mode_policy() deals with the array lookup
> within a master key. And fill_hkdf_info_for mode_key() deals with
> filling in the hkdf info as necessary for a particular policy. These
> seem a little clearer in broad strokes, emphasizing the similarities
> between the policies, but it does spread out the information on how the
> key is derived for a particular policy more.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v6 6/8] fscrypt: move all the shared mode key setup deeper

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:06PM -0400, Sweet Tea Dorminy wrote:
> Currently, fscrypt_setup_v2_file_key() has a set of ifs which encode
> various information about how to set up a new mode key if necessary for
> a shared-key policy (DIRECT or IV_INO_LBLK_*). This is somewhat awkward
> -- this information is only needed at the point that we need to setup a
> new key, which is not the common case; the setup details are recorded as
> function parameters relatively far from where they're actually used; and
> at the point we use the parameters, we can derive the information
> equally well.
> 
> So this moves mode and policy checking as deep into the callstack as
> possible. mk_prepared_key_for_mode_policy() deals with the array lookup
> within a master key. And fill_hkdf_info_for mode_key() deals with
> filling in the hkdf info as necessary for a particular policy. These
> seem a little clearer in broad strokes, emphasizing the similarities
> between the policies, but it does spread out the information on how the
> key is derived for a particular policy more.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/keysetup.c | 131 +++++++++++++++++++++++++++++--------------
>  1 file changed, 88 insertions(+), 43 deletions(-)

Looking at this again, I think this patch makes things worse, not better.  The
diffstat is significantly positive, and it splits the handling of each policy
type into several more places, which makes it harder to understand how each one
works.  Also problematic is how the new code makes it look like HKDF context 0
is potentially used, which could confuse people trying to review the crypto.

Do any of your later patches depend on this patch?

- Eric

[PATCH v6 7/8] fscrypt: make infos have a pointer to prepared keys

[Sweet Tea Dorminy]

Adding a layer of indirection between infos and prepared keys makes
everything clearer at the cost of another pointer. Now everyone sharing
a prepared key within a direct key or a master key have the same pointer
to the single prepared key.  Followups move information from the
crypt_info into the prepared key, which ends up reducing memory usage
slightly. Additionally, it makes asynchronous freeing of prepared keys
possible later.

So this change makes crypt_info->ci_enc_key a pointer and updates all
users thereof.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/crypto.c          |  2 +-
 fs/crypto/fname.c           |  4 ++--
 fs/crypto/fscrypt_private.h |  2 +-
 fs/crypto/inline_crypt.c    |  4 ++--
 fs/crypto/keysetup.c        | 16 +++++++++++-----
 fs/crypto/keysetup_v1.c     |  2 +-
 6 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 6a837e4b80dc..9f3bda18c797 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -108,7 +108,7 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist dst, src;
 	struct fscrypt_info *ci = inode->i_crypt_info;
-	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
+	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	int res = 0;
 
 	if (WARN_ON_ONCE(len <= 0))
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6eae3f12ad50..edb78cd1b0e7 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -101,7 +101,7 @@ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	const struct fscrypt_info *ci = inode->i_crypt_info;
-	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
+	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	union fscrypt_iv iv;
 	struct scatterlist sg;
 	int res;
@@ -158,7 +158,7 @@ static int fname_decrypt(const struct inode *inode,
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist src_sg, dst_sg;
 	const struct fscrypt_info *ci = inode->i_crypt_info;
-	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
+	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	union fscrypt_iv iv;
 	int res;
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 2d63da48635a..20b8ea1e3518 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -198,7 +198,7 @@ struct fscrypt_prepared_key {
 struct fscrypt_info {
 
 	/* The key in a form prepared for actual encryption/decryption */
-	struct fscrypt_prepared_key ci_enc_key;
+	struct fscrypt_prepared_key *ci_enc_key;
 
 	/* True if ci_enc_key should be freed when this fscrypt_info is freed */
 	bool ci_owns_key;
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 8bfb3ce86476..2063f7941ce6 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -273,7 +273,7 @@ void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 	ci = inode->i_crypt_info;
 
 	fscrypt_generate_dun(ci, first_lblk, dun);
-	bio_crypt_set_ctx(bio, ci->ci_enc_key.blk_key, dun, gfp_mask);
+	bio_crypt_set_ctx(bio, ci->ci_enc_key->blk_key, dun, gfp_mask);
 }
 EXPORT_SYMBOL_GPL(fscrypt_set_bio_crypt_ctx);
 
@@ -360,7 +360,7 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 	 * uses the same pointer.  I.e., there's currently no need to support
 	 * merging requests where the keys are the same but the pointers differ.
 	 */
-	if (bc->bc_key != inode->i_crypt_info->ci_enc_key.blk_key)
+	if (bc->bc_key != inode->i_crypt_info->ci_enc_key->blk_key)
 		return false;
 
 	fscrypt_generate_dun(inode->i_crypt_info, next_lblk, next_dun);
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 7dd12c1821dd..4f04999ecfd1 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -192,7 +192,11 @@ void fscrypt_destroy_prepared_key(struct super_block *sb,
 int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key)
 {
 	ci->ci_owns_key = true;
-	return fscrypt_prepare_key(&ci->ci_enc_key, raw_key, ci);
+	ci->ci_enc_key = kzalloc(sizeof(*ci->ci_enc_key), GFP_KERNEL);
+	if (!ci->ci_enc_key)
+		return -ENOMEM;
+
+	return fscrypt_prepare_key(ci->ci_enc_key, raw_key, ci);
 }
 
 static struct fscrypt_prepared_key *
@@ -311,14 +315,14 @@ static int setup_mode_prepared_key(struct fscrypt_info *ci,
 		return PTR_ERR(prep_key);
 
 	if (fscrypt_is_key_prepared(prep_key, ci)) {
-		ci->ci_enc_key = *prep_key;
+		ci->ci_enc_key = prep_key;
 		return 0;
 	}
 	err = setup_new_mode_prepared_key(mk, prep_key, ci);
 	if (err)
 		return err;
 
-	ci->ci_enc_key = *prep_key;
+	ci->ci_enc_key = prep_key;
 	return 0;
 }
 
@@ -582,9 +586,11 @@ static void put_crypt_info(struct fscrypt_info *ci)
 
 	if (ci->ci_direct_key)
 		fscrypt_put_direct_key(ci->ci_direct_key);
-	else if (ci->ci_owns_key)
+	else if (ci->ci_owns_key) {
 		fscrypt_destroy_prepared_key(ci->ci_inode->i_sb,
-					     &ci->ci_enc_key);
+					     ci->ci_enc_key);
+		kfree_sensitive(ci->ci_enc_key);
+	}
 
 	mk = ci->ci_master_key;
 	if (mk) {
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 75dabd9b27f9..e1d761e8067f 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -259,7 +259,7 @@ static int setup_v1_file_key_direct(struct fscrypt_info *ci,
 	if (IS_ERR(dk))
 		return PTR_ERR(dk);
 	ci->ci_direct_key = dk;
-	ci->ci_enc_key = dk->dk_key;
+	ci->ci_enc_key = &dk->dk_key;
 	return 0;
 }
 
-- 
2.41.0

Re: [PATCH v6 7/8] fscrypt: make infos have a pointer to prepared keys

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:07PM -0400, Sweet Tea Dorminy wrote:
> Adding a layer of indirection between infos and prepared keys makes
> everything clearer at the cost of another pointer. Now everyone sharing
> a prepared key within a direct key or a master key have the same pointer
> to the single prepared key.  Followups move information from the
> crypt_info into the prepared key, which ends up reducing memory usage
> slightly. Additionally, it makes asynchronous freeing of prepared keys
> possible later.
> 
> So this change makes crypt_info->ci_enc_key a pointer and updates all
> users thereof.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[Sweet Tea Dorminy]

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 20b8ea1e3518..e2acd8894ea7 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -174,18 +174,39 @@ struct fscrypt_symlink_data {
 	char encrypted_path[];
 } __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;
+	prep_key->type = FSCRYPT_KEY_MASTER_KEY;
 	err = fscrypt_prepare_key(prep_key, mode_key, ci);
 	memzero_explicit(mode_key, mode->keysize);
 
@@ -584,12 +585,16 @@ static void put_crypt_info(struct fscrypt_info *ci)
 	if (!ci)
 		return;
 
-	if (ci->ci_direct_key)
-		fscrypt_put_direct_key(ci->ci_direct_key);
-	else if (ci->ci_owns_key) {
-		fscrypt_destroy_prepared_key(ci->ci_inode->i_sb,
-					     ci->ci_enc_key);
-		kfree_sensitive(ci->ci_enc_key);
+	if (ci->ci_enc_key) {
+		enum fscrypt_prepared_key_type type = ci->ci_enc_key->type;
+
+		if (type == FSCRYPT_KEY_DIRECT_V1)
+			fscrypt_put_direct_key(ci->ci_enc_key);
+		if (type == FSCRYPT_KEY_PER_INFO) {
+			fscrypt_destroy_prepared_key(ci->ci_inode->i_sb,
+						     ci->ci_enc_key);
+			kfree_sensitive(ci->ci_enc_key);
+		}
 	}
 
 	mk = ci->ci_master_key;
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index e1d761e8067f..1e785cedead0 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -160,8 +160,11 @@ static void free_direct_key(struct fscrypt_direct_key *dk)
 	}
 }
 
-void fscrypt_put_direct_key(struct fscrypt_direct_key *dk)
+void fscrypt_put_direct_key(struct fscrypt_prepared_key *prep_key)
 {
+	struct fscrypt_direct_key *dk =
+		container_of(prep_key, struct fscrypt_direct_key, dk_key);
+
 	if (!refcount_dec_and_lock(&dk->dk_refcount, &fscrypt_direct_keys_lock))
 		return;
 	hash_del(&dk->dk_node);
@@ -235,6 +238,7 @@ fscrypt_get_direct_key(const struct fscrypt_info *ci, const u8 *raw_key)
 	dk->dk_sb = ci->ci_inode->i_sb;
 	refcount_set(&dk->dk_refcount, 1);
 	dk->dk_mode = ci->ci_mode;
+	dk->dk_key.type = FSCRYPT_KEY_DIRECT_V1;
 	err = fscrypt_prepare_key(&dk->dk_key, raw_key, ci);
 	if (err)
 		goto err_free_dk;
@@ -258,7 +262,6 @@ static int setup_v1_file_key_direct(struct fscrypt_info *ci,
 	dk = fscrypt_get_direct_key(ci, raw_master_key);
 	if (IS_ERR(dk))
 		return PTR_ERR(dk);
-	ci->ci_direct_key = dk;
 	ci->ci_enc_key = &dk->dk_key;
 	return 0;
 }
-- 
2.41.0

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

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:08PM -0400, Sweet Tea Dorminy wrote:
> 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>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

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

[Dan Carpenter]

Hi Sweet,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Sweet-Tea-Dorminy/fscrypt-move-inline-crypt-decision-to-info-setup/20230809-030251
base:   54d2161835d828a9663f548f61d1d9c3d3482122
patch link:    https://lore.kernel.org/r/64c47243cea5a8eca15538b51f88c0a6d53799cf.1691505830.git.sweettea-kernel%40dorminy.me
patch subject: [PATCH v6 8/8] fscrypt: make prepared keys record their type
config: x86_64-randconfig-m001-20230808 (https://download.01.org/0day-ci/archive/20230809/202308092324.d0OCNA1O-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230809/202308092324.d0OCNA1O-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202308092324.d0OCNA1O-lkp@intel.com/

smatch warnings:
fs/crypto/keysetup.c:300 setup_new_mode_prepared_key() warn: inconsistent returns '&fscrypt_mode_key_setup_mutex'.

vim +300 fs/crypto/keysetup.c

a03cf25a20f748 Sweet Tea Dorminy 2023-08-08  238  static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08  239  				       struct fscrypt_prepared_key *prep_key,
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  240  				       const struct fscrypt_info *ci)
5dae460c2292db Eric Biggers      2019-08-04  241  {
b103fb7653fff0 Eric Biggers      2019-10-24  242  	const struct inode *inode = ci->ci_inode;
b103fb7653fff0 Eric Biggers      2019-10-24  243  	const struct super_block *sb = inode->i_sb;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  244  	unsigned int policy_flags = fscrypt_policy_flags(&ci->ci_policy);
5dae460c2292db Eric Biggers      2019-08-04  245  	struct fscrypt_mode *mode = ci->ci_mode;
85af90e57ce969 Eric Biggers      2019-12-09  246  	const u8 mode_num = mode - fscrypt_modes;
5dae460c2292db Eric Biggers      2019-08-04  247  	u8 mode_key[FSCRYPT_MAX_KEY_SIZE];
b103fb7653fff0 Eric Biggers      2019-10-24  248  	u8 hkdf_info[sizeof(mode_num) + sizeof(sb->s_uuid)];
b103fb7653fff0 Eric Biggers      2019-10-24  249  	unsigned int hkdf_infolen = 0;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  250  	u8 hkdf_context = 0;
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08  251  	int err = 0;
e3b1078bedd323 Eric Biggers      2020-05-15  252  
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  253  	switch (policy_flags & FSCRYPT_POLICY_FLAGS_KEY_MASK) {
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  254  	case FSCRYPT_POLICY_FLAG_DIRECT_KEY:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  255  		hkdf_context = HKDF_CONTEXT_DIRECT_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  256  		break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  257  	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  258  		hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_64_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  259  		break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  260  	case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  261  		hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_32_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  262  		break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  263  	}
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  264  
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  265  	/*
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  266  	 * For DIRECT_KEY policies: instead of deriving per-file encryption
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  267  	 * keys, the per-file nonce will be included in all the IVs.  But
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  268  	 * unlike v1 policies, for v2 policies in this case we don't encrypt
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  269  	 * with the master key directly but rather derive a per-mode encryption
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  270  	 * key.  This ensures that the master key is consistently used only for
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  271  	 * HKDF, avoiding key reuse issues.
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  272  	 *
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  273  	 * For IV_INO_LBLK policies: encryption keys are derived from
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  274  	 * (master_key, mode_num, filesystem_uuid), and inode number is
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  275  	 * included in the IVs.  This format is optimized for use with inline
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  276  	 * encryption hardware compliant with the UFS standard.
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  277  	 */
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  278  
e3b1078bedd323 Eric Biggers      2020-05-15  279  	mutex_lock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers      2020-05-15  280  
5fee36095cda45 Satya Tangirala   2020-07-02  281  	if (fscrypt_is_key_prepared(prep_key, ci))
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08  282  		goto out_unlock;
5dae460c2292db Eric Biggers      2019-08-04  283  
5dae460c2292db Eric Biggers      2019-08-04  284  	BUILD_BUG_ON(sizeof(mode_num) != 1);
b103fb7653fff0 Eric Biggers      2019-10-24  285  	BUILD_BUG_ON(sizeof(sb->s_uuid) != 16);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  286  	BUILD_BUG_ON(sizeof(hkdf_info) != MAX_MODE_KEY_HKDF_INFO_SIZE);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  287  	hkdf_infolen = fill_hkdf_info_for_mode_key(ci, hkdf_info);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08  288  
5dae460c2292db Eric Biggers      2019-08-04  289  	err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
b103fb7653fff0 Eric Biggers      2019-10-24  290  				  hkdf_context, hkdf_info, hkdf_infolen,
5dae460c2292db Eric Biggers      2019-08-04  291  				  mode_key, mode->keysize);
5dae460c2292db Eric Biggers      2019-08-04  292  	if (err)
3bd6d42474f3a9 Sweet Tea Dorminy 2023-08-08  293  		return err;

Originally this was goto out_unlock;  Not sure why it was changed to a
direct return.

3bd6d42474f3a9 Sweet Tea Dorminy 2023-08-08  294  	prep_key->type = FSCRYPT_KEY_MASTER_KEY;
5fee36095cda45 Satya Tangirala   2020-07-02  295  	err = fscrypt_prepare_key(prep_key, mode_key, ci);
5dae460c2292db Eric Biggers      2019-08-04  296  	memzero_explicit(mode_key, mode->keysize);
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08  297  
e3b1078bedd323 Eric Biggers      2020-05-15  298  out_unlock:
e3b1078bedd323 Eric Biggers      2020-05-15  299  	mutex_unlock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers      2020-05-15 @300  	return err;
5dae460c2292db Eric Biggers      2019-08-04  301  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:08PM -0400, Sweet Tea Dorminy wrote:
> +/**
> + * 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;

FSCRYPT_KEY_MASTER_KEY seems misnamed, since it's not for master keys.  It's for
what the code elsewhere calls a per-mode key.  So maybe FSCRYPT_KEY_PER_MODE?

I think your intent was for the name to reflect the struct that the
fscrypt_prepared_key is embedded in.  I don't think that's obvious as-is.  If
you want to name it that way, it should be made super clear, like this:

    enum fscrypt_prepared_key_owner {
            FSCRYPT_KEY_OWNED_BY_INFO = 1,
            FSCRYPT_KEY_OWNED_BY_DIRECT_V1,
            FSCRYPT_KEY_OWNED_BY_MASTER_KEY,
    };

But, I think I'm leaning towards your proposal with
s/FSCRYPT_KEY_MASTER_KEY/FSCRYPT_KEY_PER_MODE/.

- Eric