fscrypt: fix keyring memory leak on mount failure

Commit Message

Eric Biggers Oct. 11, 2022, 9:38 p.m. UTC

From: Eric Biggers <[e-mail]>

Commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
fscrypt_master_key") moved the keyring destruction from __put_super() to
generic_shutdown_super() so that the filesystem's block device(s) are
still available.  Unfortunately, this causes a memory leak in the case
where a mount is attempted with the test_dummy_encryption mount option,
but the mount fails after the option has already been processed.

To fix this, attempt the keyring destruction in both places.

Reported-by: [e-mail]
Fixes: d7e7b9af104c ("fscrypt: stop using keyrings subsystem for fscrypt_master_key")
Signed-off-by: Eric Biggers <[e-mail]>
---
 fs/crypto/keyring.c     | 17 +++++++++++------
 fs/super.c              |  3 ++-
 include/linux/fscrypt.h |  4 ++--
 3 files changed, 15 insertions(+), 9 deletions(-)


base-commit: 041bc24d867a2a577a06534d6d25e500b24a01ef

Comments

Eric Biggers Oct. 18, 2022, 12:52 a.m. UTC | #1

On Tue, Oct 11, 2022 at 02:38:38PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> Commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
> fscrypt_master_key") moved the keyring destruction from __put_super() to
> generic_shutdown_super() so that the filesystem's block device(s) are
> still available.  Unfortunately, this causes a memory leak in the case
> where a mount is attempted with the test_dummy_encryption mount option,
> but the mount fails after the option has already been processed.
> 
> To fix this, attempt the keyring destruction in both places.
> 
> Reported-by: [e-mail]
> Fixes: d7e7b9af104c ("fscrypt: stop using keyrings subsystem for fscrypt_master_key")
> Signed-off-by: Eric Biggers <[e-mail]>

Applied to fscrypt.git#for-stable for 6.1.

As usual, I'd greatly appreciate reviews though...

- Eric

Christian Brauner Oct. 19, 2022, 11:36 a.m. UTC | #2

On Tue, Oct 11, 2022 at 02:38:38PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> Commit d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
> fscrypt_master_key") moved the keyring destruction from __put_super() to
> generic_shutdown_super() so that the filesystem's block device(s) are
> still available.  Unfortunately, this causes a memory leak in the case
> where a mount is attempted with the test_dummy_encryption mount option,
> but the mount fails after the option has already been processed.
> 
> To fix this, attempt the keyring destruction in both places.
> 
> Reported-by: [e-mail]
> Fixes: d7e7b9af104c ("fscrypt: stop using keyrings subsystem for fscrypt_master_key")
> Signed-off-by: Eric Biggers <[e-mail]>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[e-mail]>

Patch

diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 1cca09aa43f8b..2a24b1f0ae688 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -205,14 +205,19 @@  static int allocate_filesystem_keyring(struct super_block *sb)
 }
 
 /*
- * This is called at unmount time to release all encryption keys that have been
- * added to the filesystem, along with the keyring that contains them.
+ * Release all encryption keys that have been added to the filesystem, along
+ * with the keyring that contains them.
  *
- * Note that besides clearing and freeing memory, this might need to evict keys
- * from the keyslots of an inline crypto engine.  Therefore, this must be called
- * while the filesystem's underlying block device(s) are still available.
+ * This is called at unmount time.  The filesystem's underlying block device(s)
+ * are still available at this time; this is important because after user file
+ * accesses have been allowed, this function may need to evict keys from the
+ * keyslots of an inline crypto engine, which requires the block device(s).
+ *
+ * This is also called when the super_block is being freed.  This is needed to
+ * avoid a memory leak if mounting fails after the "test_dummy_encryption"
+ * option was processed, as in that case the unmount-time call isn't made.
  */
-void fscrypt_sb_delete(struct super_block *sb)
+void fscrypt_destroy_keyring(struct super_block *sb)
 {
 	struct fscrypt_keyring *keyring = sb->s_master_keys;
 	size_t i;
diff --git a/fs/super.c b/fs/super.c
index 6a82660e1adba..8d39e4f11cfa3 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -291,6 +291,7 @@  static void __put_super(struct super_block *s)
 		WARN_ON(s->s_inode_lru.node);
 		WARN_ON(!list_empty(&s->s_mounts));
 		security_sb_free(s);
+		fscrypt_destroy_keyring(s);
 		put_user_ns(s->s_user_ns);
 		kfree(s->s_subtype);
 		call_rcu(&s->rcu, destroy_super_rcu);
@@ -479,7 +480,7 @@  void generic_shutdown_super(struct super_block *sb)
 		evict_inodes(sb);
 		/* only nonzero refcount inodes can have marks */
 		fsnotify_sb_delete(sb);
-		fscrypt_sb_delete(sb);
+		fscrypt_destroy_keyring(sb);
 		security_sb_delete(sb);
 
 		if (sb->s_dio_done_wq) {
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index cad78b569c7ef..4f5f8a6512132 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -307,7 +307,7 @@  fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
 }
 
 /* keyring.c */
-void fscrypt_sb_delete(struct super_block *sb);
+void fscrypt_destroy_keyring(struct super_block *sb);
 int fscrypt_ioctl_add_key(struct file *filp, void __user *arg);
 int fscrypt_add_test_dummy_key(struct super_block *sb,
 			       const struct fscrypt_dummy_policy *dummy_policy);
@@ -521,7 +521,7 @@  fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
 }
 
 /* keyring.c */
-static inline void fscrypt_sb_delete(struct super_block *sb)
+static inline void fscrypt_destroy_keyring(struct super_block *sb)
 {
 }