Skip to contentSkip to conversation

FAILED: patch "[PATCH] fscrypt: Fix key setup in edge case with multiple data unit" failed to apply to 6.18-stable tree

PATCH NAK [email protected] 3 participants 3 messages Jul 2

Subsystems (2)
Stable branch fscrypt: file system level encryption support
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 6.18-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.18.y
git checkout FETCH_HEAD
git cherry-pick -x dd015b566d505d698386103e9c80b739c7336eb8
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2026070242-action-undermine-e5ce@gregkh' --subject-prefix 'PATCH 6.18.y' 'HEAD^..'

Possible dependencies:

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From dd015b566d505d698386103e9c80b739c7336eb8 Mon Sep 17 00:00:00 2001
From: Eric Biggers <[email protected]>
Date: Thu, 18 Jun 2026 11:06:51 -0700
Subject: [PATCH] fscrypt: Fix key setup in edge case with multiple data unit
sizes

The addition of support for customizable data unit sizes introduced an edge case where a file's contents can be en/decrypted with the wrong data unit size. It occurs when there are multiple v2 policies that:

- Have *different* data unit sizes, via the log2_data_unit_size field

- Share the same master_key_identifier, contents_encryption_mode, and either FSCRYPT_POLICY_FLAG_DIRECT_KEY, FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32, or FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64

- Are being used on the same filesystem, which also must be mounted with the "inlinecrypt" mount option.

Fortunately this edge case doesn't actually occur in practice. I just found it via code review. But it needs to be fixed regardless.

The bug is caused by the data unit size not being fully considered when blk_crypto_keys are cached in mk_direct_keys, mk_iv_ino_lblk_32_keys, and mk_iv_ino_lblk_64_keys. They're differentiated only by master key, encryption mode, and flag. However, each one actually has a data unit size too. Only the first data unit size that is cached is used.

To fix this, start using the data unit size to differentiate the cached keys. For several reasons, including avoiding increasing the size of struct fscrypt_master_key, just replace all three arrays with a single linked list instead of changing them into two-dimensional arrays. This works well when considering that in practice at most 2 entries are used across all three arrays, so it was already mostly wasted space.

For simplicity, make the list also take over the publish/subscribe of the prepared key itself. That is, create separate list nodes for blk_crypto_keys vs crypto_skciphers, and add nodes to the list only when their key is actually prepared. (Note that the legacy fscrypt_direct_keys table in fs/crypto/keysetup_v1.c already works this way.) This eliminates the need for the additional memory barriers when reading and writing the fields of struct fscrypt_prepared_key.

Note that I technically should have included the data unit size in the HKDF info string as well. But it's too late to change that.

Fixes 5b1188847180 ("fscrypt: support crypto data unit size less than filesystem block size")
Cc [email protected]
Link https://patch.msgid.link/[email protected] open here
Signed-off-by Eric Biggers <[email protected]>
fs/crypto/fscrypt_private.h +31 -21
index 8d3c278a7591..4263cac24b32 100644
@@ -236,7 +236,7 @@ struct fscrypt_symlink_data {
  * @tfm: crypto API transform object
  * @blk_key: key for blk-crypto
  *
- * Normally only one of the fields will be non-NULL.
+ * Only one of the fields is non-NULL.
  */
 struct fscrypt_prepared_key {
 	struct crypto_sync_skcipher *tfm;

@@ -245,6 +245,15 @@ struct fscrypt_prepared_key {
 #endif
 };
 
+/* An entry in the linked list ->mk_mode_keys */
+struct fscrypt_mode_key {
+	struct fscrypt_prepared_key key;
+	struct list_head link;
+	u8 hkdf_context;
+	u8 mode_num;
+	u8 data_unit_bits;
+};
+
 /*
  * fscrypt_inode_info - the "encryption key" for an inode
  *

@@ -430,20 +439,12 @@ int fscrypt_derive_sw_secret(struct super_block *sb,
  * @prep_key, depending on which encryption implementation the file will use.
  */
 static inline bool
-fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
+fscrypt_is_key_prepared(const struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_inode_info *ci)
 {
-	/*
-	 * The two smp_load_acquire()'s here pair with the smp_store_release()'s
-	 * in fscrypt_prepare_inline_crypt_key() and fscrypt_prepare_key().
-	 * I.e., in some cases (namely, if this prep_key is a per-mode
-	 * encryption key) another task can publish blk_key or tfm concurrently,
-	 * executing a RELEASE barrier.  We need to use smp_load_acquire() here
-	 * to safely ACQUIRE the memory the other task published.
-	 */
 	if (fscrypt_using_inline_encryption(ci))
-		return smp_load_acquire(&prep_key->blk_key) != NULL;
-	return smp_load_acquire(&prep_key->tfm) != NULL;
+		return prep_key->blk_key != NULL;
+	return prep_key->tfm != NULL;
 }
 
 #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */

@@ -486,10 +487,10 @@ fscrypt_derive_sw_secret(struct super_block *sb,
 }
 
 static inline bool
-fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
+fscrypt_is_key_prepared(const struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_inode_info *ci)
 {
-	return smp_load_acquire(&prep_key->tfm) != NULL;
+	return prep_key->tfm != NULL;
 }
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 

@@ -577,8 +578,8 @@ struct fscrypt_master_key {
 	/*
 	 * Active and structural reference counts.  An active ref guarantees
 	 * that the struct continues to exist, continues to be in the keyring
-	 * ->s_master_keys, and that any embedded subkeys (e.g.
-	 * ->mk_direct_keys) that have been prepared continue to exist.
+	 * ->s_master_keys, and that any non-file-scoped subkeys (e.g.
+	 * ->mk_mode_keys) that have been prepared continue to exist.
 	 * A structural ref only guarantees that the struct continues to exist.
 	 *
 	 * There is one active ref associated with ->mk_present being true, and

@@ -632,12 +633,21 @@ struct fscrypt_master_key {
 	spinlock_t		mk_decrypted_inodes_lock;
 
 	/*
-	 * Per-mode encryption keys for the various types of encryption policies
-	 * that use them.  Allocated and derived on-demand.
+	 * A list of 'struct fscrypt_mode_key' for the (hkdf_context, mode_num,
+	 * data_unit_bits, inlinecrypt) combinations that are in use for this
+	 * master key, for hkdf_context in [HKDF_CONTEXT_DIRECT_KEY,
+	 * HKDF_CONTEXT_IV_INO_LBLK_32_KEY, HKDF_CONTEXT_IV_INO_LBLK_64_KEY].
+	 *
+	 * This is a linked list and not a hash table because in practice
+	 * there's just a single encryption policy per master key, using
+	 * _at most_ 2 nodes in this list.  Per-file keys don't use this at all.
+	 *
+	 * This list is append-only until the master key is fully removed, at
+	 * which time the list is cleared.  Before then,
+	 * fscrypt_mode_key_setup_mutex synchronizes appends, and searches use
+	 * the RCU read lock together with ->mk_sem held for read.
 	 */
-	struct fscrypt_prepared_key mk_direct_keys[FSCRYPT_MODE_MAX + 1];
-	struct fscrypt_prepared_key mk_iv_ino_lblk_64_keys[FSCRYPT_MODE_MAX + 1];
-	struct fscrypt_prepared_key mk_iv_ino_lblk_32_keys[FSCRYPT_MODE_MAX + 1];
+	struct list_head	mk_mode_keys;
 
 	/* Hash key for inode numbers.  Initialized only when needed. */
 	siphash_key_t		mk_ino_hash_key;
fs/crypto/inline_crypt.c +1 -7
index 37d42d357925..47324062fee5 100644
@@ -198,13 +198,7 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 		goto fail;
 	}
 
-	/*
-	 * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared().
-	 * I.e., here we publish ->blk_key with a RELEASE barrier so that
-	 * concurrent tasks can ACQUIRE it.  Note that this concurrency is only
-	 * possible for per-mode keys, not for per-file keys.
-	 */
-	smp_store_release(&prep_key->blk_key, blk_key);
+	prep_key->blk_key = blk_key;
 	return 0;
 
 fail:
fs/crypto/keyring.c +14 -9
index be8e6e8011f2..5fe0d985a58d 100644
@@ -87,14 +87,14 @@ void fscrypt_put_master_key(struct fscrypt_master_key *mk)
 void fscrypt_put_master_key_activeref(struct super_block *sb,
 				      struct fscrypt_master_key *mk)
 {
-	size_t i;
+	struct fscrypt_mode_key *node, *tmp;
 
 	if (!refcount_dec_and_test(&mk->mk_active_refs))
 		return;
 	/*
 	 * No active references left, so complete the full removal of this
 	 * fscrypt_master_key struct by removing it from the keyring and
-	 * destroying any subkeys embedded in it.
+	 * destroying any non-file-scoped subkeys.
 	 */
 
 	if (WARN_ON_ONCE(!sb->s_master_keys))

@@ -110,13 +110,16 @@ void fscrypt_put_master_key_activeref(struct super_block *sb,
 	WARN_ON_ONCE(mk->mk_present);
 	WARN_ON_ONCE(!list_empty(&mk->mk_decrypted_inodes));
 
-	for (i = 0; i <= FSCRYPT_MODE_MAX; i++) {
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_direct_keys[i]);
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_64_keys[i]);
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_32_keys[i]);
+	/*
+	 * Destroy any non-file-scoped subkeys.  Since ->mk_active_refs == 0,
+	 * they're no longer referenced by any inodes.  Nor can key setup run
+	 * and use them again.  So they're no longer needed.  (This implies no
+	 * concurrent readers, so we don't need list_del_rcu() for example.)
+	 */
+	list_for_each_entry_safe(node, tmp, &mk->mk_mode_keys, link) {
+		fscrypt_destroy_prepared_key(sb, &node->key);
+		list_del(&node->link);
+		kfree(node);
 	}
 	memzero_explicit(&mk->mk_ino_hash_key,
 			 sizeof(mk->mk_ino_hash_key));

@@ -445,6 +448,8 @@ static int add_new_master_key(struct super_block *sb,
 	INIT_LIST_HEAD(&mk->mk_decrypted_inodes);
 	spin_lock_init(&mk->mk_decrypted_inodes_lock);
 
+	INIT_LIST_HEAD(&mk->mk_mode_keys);
+
 	if (mk_spec->type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) {
 		err = allocate_master_key_users_keyring(mk);
 		if (err)
fs/crypto/keysetup.c +74 -44

176 lines not on this page · raw message

maintainer
#1 lore raw

PATCH 1/1 fscrypt: Fix key setup in edge case with multiple data unit sizes

From: Eric Biggers <[email protected]>

[ Upstream commit dd015b566d505d698386103e9c80b739c7336eb8 ]

The addition of support for customizable data unit sizes introduced an edge case where a file's contents can be en/decrypted with the wrong data unit size. It occurs when there are multiple v2 policies that:

- Have *different* data unit sizes, via the log2_data_unit_size field

- Share the same master_key_identifier, contents_encryption_mode, and either FSCRYPT_POLICY_FLAG_DIRECT_KEY, FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32, or FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64

- Are being used on the same filesystem, which also must be mounted with the "inlinecrypt" mount option.

Fortunately this edge case doesn't actually occur in practice. I just found it via code review. But it needs to be fixed regardless.

The bug is caused by the data unit size not being fully considered when blk_crypto_keys are cached in mk_direct_keys, mk_iv_ino_lblk_32_keys, and mk_iv_ino_lblk_64_keys. They're differentiated only by master key, encryption mode, and flag. However, each one actually has a data unit size too. Only the first data unit size that is cached is used.

To fix this, start using the data unit size to differentiate the cached keys. For several reasons, including avoiding increasing the size of struct fscrypt_master_key, just replace all three arrays with a single linked list instead of changing them into two-dimensional arrays. This works well when considering that in practice at most 2 entries are used across all three arrays, so it was already mostly wasted space.

For simplicity, make the list also take over the publish/subscribe of the prepared key itself. That is, create separate list nodes for blk_crypto_keys vs crypto_skciphers, and add nodes to the list only when their key is actually prepared. (Note that the legacy fscrypt_direct_keys table in fs/crypto/keysetup_v1.c already works this way.) This eliminates the need for the additional memory barriers when reading and writing the fields of struct fscrypt_prepared_key.

Note that I technically should have included the data unit size in the HKDF info string as well. But it's too late to change that.

Fixes 5b1188847180 ("fscrypt: support crypto data unit size less than filesystem block size")
Cc [email protected]
Link https://patch.msgid.link/[email protected] open here
Signed-off-by Eric Biggers <[email protected]>
[ added explicit GFP_KERNEL argument to kzalloc_obj() since the 6.18 macro lacks the variadic default-gfp form ]
Signed-off-by Sasha Levin <[email protected]>
---
 fs/crypto/fscrypt_private.h |  52 +++++++++-------
 fs/crypto/inline_crypt.c    |   8 +--
 fs/crypto/keyring.c         |  23 ++++---
 fs/crypto/keysetup.c        | 118 ++++++++++++++++++++++--------------
 4 files changed, 120 insertions(+), 81 deletions(-)
fs/crypto/fscrypt_private.h +31 -21
index 4e8e82a9ccf9a3..3bcde4b59efb0e 100644
@@ -236,7 +236,7 @@ struct fscrypt_symlink_data {
  * @tfm: crypto API transform object
  * @blk_key: key for blk-crypto
  *
- * Normally only one of the fields will be non-NULL.
+ * Only one of the fields is non-NULL.
  */
 struct fscrypt_prepared_key {
 	struct crypto_sync_skcipher *tfm;

@@ -245,6 +245,15 @@ struct fscrypt_prepared_key {
 #endif
 };
 
+/* An entry in the linked list ->mk_mode_keys */
+struct fscrypt_mode_key {
+	struct fscrypt_prepared_key key;
+	struct list_head link;
+	u8 hkdf_context;
+	u8 mode_num;
+	u8 data_unit_bits;
+};
+
 /*
  * fscrypt_inode_info - the "encryption key" for an inode
  *

@@ -433,20 +442,12 @@ int fscrypt_derive_sw_secret(struct super_block *sb,
  * @prep_key, depending on which encryption implementation the file will use.
  */
 static inline bool
-fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
+fscrypt_is_key_prepared(const struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_inode_info *ci)
 {
-	/*
-	 * The two smp_load_acquire()'s here pair with the smp_store_release()'s
-	 * in fscrypt_prepare_inline_crypt_key() and fscrypt_prepare_key().
-	 * I.e., in some cases (namely, if this prep_key is a per-mode
-	 * encryption key) another task can publish blk_key or tfm concurrently,
-	 * executing a RELEASE barrier.  We need to use smp_load_acquire() here
-	 * to safely ACQUIRE the memory the other task published.
-	 */
 	if (fscrypt_using_inline_encryption(ci))
-		return smp_load_acquire(&prep_key->blk_key) != NULL;
-	return smp_load_acquire(&prep_key->tfm) != NULL;
+		return prep_key->blk_key != NULL;
+	return prep_key->tfm != NULL;
 }
 
 #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */

@@ -489,10 +490,10 @@ fscrypt_derive_sw_secret(struct super_block *sb,
 }
 
 static inline bool
-fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
+fscrypt_is_key_prepared(const struct fscrypt_prepared_key *prep_key,
 			const struct fscrypt_inode_info *ci)
 {
-	return smp_load_acquire(&prep_key->tfm) != NULL;
+	return prep_key->tfm != NULL;
 }
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 

@@ -580,8 +581,8 @@ struct fscrypt_master_key {
 	/*
 	 * Active and structural reference counts.  An active ref guarantees
 	 * that the struct continues to exist, continues to be in the keyring
-	 * ->s_master_keys, and that any embedded subkeys (e.g.
-	 * ->mk_direct_keys) that have been prepared continue to exist.
+	 * ->s_master_keys, and that any non-file-scoped subkeys (e.g.
+	 * ->mk_mode_keys) that have been prepared continue to exist.
 	 * A structural ref only guarantees that the struct continues to exist.
 	 *
 	 * There is one active ref associated with ->mk_present being true, and

@@ -635,12 +636,21 @@ struct fscrypt_master_key {
 	spinlock_t		mk_decrypted_inodes_lock;
 
 	/*
-	 * Per-mode encryption keys for the various types of encryption policies
-	 * that use them.  Allocated and derived on-demand.
+	 * A list of 'struct fscrypt_mode_key' for the (hkdf_context, mode_num,
+	 * data_unit_bits, inlinecrypt) combinations that are in use for this
+	 * master key, for hkdf_context in [HKDF_CONTEXT_DIRECT_KEY,
+	 * HKDF_CONTEXT_IV_INO_LBLK_32_KEY, HKDF_CONTEXT_IV_INO_LBLK_64_KEY].
+	 *
+	 * This is a linked list and not a hash table because in practice
+	 * there's just a single encryption policy per master key, using
+	 * _at most_ 2 nodes in this list.  Per-file keys don't use this at all.
+	 *
+	 * This list is append-only until the master key is fully removed, at
+	 * which time the list is cleared.  Before then,
+	 * fscrypt_mode_key_setup_mutex synchronizes appends, and searches use
+	 * the RCU read lock together with ->mk_sem held for read.
 	 */
-	struct fscrypt_prepared_key mk_direct_keys[FSCRYPT_MODE_MAX + 1];
-	struct fscrypt_prepared_key mk_iv_ino_lblk_64_keys[FSCRYPT_MODE_MAX + 1];
-	struct fscrypt_prepared_key mk_iv_ino_lblk_32_keys[FSCRYPT_MODE_MAX + 1];
+	struct list_head	mk_mode_keys;
 
 	/* Hash key for inode numbers.  Initialized only when needed. */
 	siphash_key_t		mk_ino_hash_key;
fs/crypto/inline_crypt.c +1 -7
index ed6e926226b51d..645cc493607294 100644
@@ -198,13 +198,7 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 		goto fail;
 	}
 
-	/*
-	 * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared().
-	 * I.e., here we publish ->blk_key with a RELEASE barrier so that
-	 * concurrent tasks can ACQUIRE it.  Note that this concurrency is only
-	 * possible for per-mode keys, not for per-file keys.
-	 */
-	smp_store_release(&prep_key->blk_key, blk_key);
+	prep_key->blk_key = blk_key;
 	return 0;
 
 fail:
fs/crypto/keyring.c +14 -9
index 3adbd7167055a9..29bea5f3c98234 100644
@@ -87,14 +87,14 @@ void fscrypt_put_master_key(struct fscrypt_master_key *mk)
 void fscrypt_put_master_key_activeref(struct super_block *sb,
 				      struct fscrypt_master_key *mk)
 {
-	size_t i;
+	struct fscrypt_mode_key *node, *tmp;
 
 	if (!refcount_dec_and_test(&mk->mk_active_refs))
 		return;
 	/*
 	 * No active references left, so complete the full removal of this
 	 * fscrypt_master_key struct by removing it from the keyring and
-	 * destroying any subkeys embedded in it.
+	 * destroying any non-file-scoped subkeys.
 	 */
 
 	if (WARN_ON_ONCE(!sb->s_master_keys))

@@ -110,13 +110,16 @@ void fscrypt_put_master_key_activeref(struct super_block *sb,
 	WARN_ON_ONCE(mk->mk_present);
 	WARN_ON_ONCE(!list_empty(&mk->mk_decrypted_inodes));
 
-	for (i = 0; i <= FSCRYPT_MODE_MAX; i++) {
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_direct_keys[i]);
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_64_keys[i]);
-		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_32_keys[i]);
+	/*
+	 * Destroy any non-file-scoped subkeys.  Since ->mk_active_refs == 0,
+	 * they're no longer referenced by any inodes.  Nor can key setup run
+	 * and use them again.  So they're no longer needed.  (This implies no
+	 * concurrent readers, so we don't need list_del_rcu() for example.)
+	 */
+	list_for_each_entry_safe(node, tmp, &mk->mk_mode_keys, link) {
+		fscrypt_destroy_prepared_key(sb, &node->key);
+		list_del(&node->link);
+		kfree(node);
 	}
 	memzero_explicit(&mk->mk_ino_hash_key,
 			 sizeof(mk->mk_ino_hash_key));

@@ -445,6 +448,8 @@ static int add_new_master_key(struct super_block *sb,
 	INIT_LIST_HEAD(&mk->mk_decrypted_inodes);
 	spin_lock_init(&mk->mk_decrypted_inodes_lock);
 
+	INIT_LIST_HEAD(&mk->mk_mode_keys);
+
 	if (mk_spec->type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) {
 		err = allocate_master_key_users_keyring(mk);
 		if (err)
fs/crypto/keysetup.c +74 -44

176 lines not on this page · raw message

Signature · 1 line
2.53.0
maintainer Nacked-by on v1 1/1 NAK
#2 lore raw

Re: [PATCH 6.18.y] fscrypt: Fix key setup in edge case with multiple data unit sizes

Quoted 2 lines · Sasha Levin
[ added explicit GFP_KERNEL argument to kzalloc_obj() since the 6.18 macro lacks the variadic default-gfp form ]
Signed-off-by Sasha Levin <[email protected]>

At this point I just have to nack any patch that isn't Cc'ed to the subsystem mailing list and isn't a clean cherry-pick. I always mention to include the mailing list, but clearly that's not working, as I just get ignored.

Nacked-by Eric Biggers <[email protected]>

It would also be a good idea to make k[mz]alloc_obj() on 6.18 have the same behavior as on mainline, as every patch using those is going to have the same issue.

- Eric