Commit ccd30a47 authored by Eric Biggers's avatar Eric Biggers
Browse files

fscrypt: fix keyring memory leak on mount failure 



Commit d7e7b9af ("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: default avatar <syzbot+104c2a89561289cec13e@syzkaller.appspotmail.com>
Fixes: d7e7b9af ("fscrypt: stop using keyrings subsystem for fscrypt_master_key")
Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
Reviewed-by: default avatarChristian Brauner (Microsoft) <brauner@kernel.org>
Link: https://lore.kernel.org/r/20221011213838.209879-1-ebiggers@kernel.org
parent 9abf2313

fs/crypto/keyring.c

+11 −6
Original line number Diff line number Diff line
@@ -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;

fs/super.c

+2 −1
Original line number Diff line number Diff line
@@ -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) {

include/linux/fscrypt.h

+2 −2
Original line number Diff line number Diff line
@@ -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)

{

}