fscrypt fixes for stable
Subsystems (2)
Eric Biggers
maintainercover fscrypt fixes for stable
Please apply these to 5.15-stable.
Eric Biggers (2):
fscrypt: stop using keyrings subsystem for fscrypt_master_key
fscrypt: fix keyring memory leak on mount failure
fs/crypto/fscrypt_private.h | 71 ++++-- fs/crypto/hooks.c | 10 +- fs/crypto/keyring.c | 491 +++++++++++++++++++----------------- fs/crypto/keysetup.c | 81 +++--- fs/crypto/policy.c | 8 +- fs/super.c | 3 +- include/linux/fs.h | 2 +- include/linux/fscrypt.h | 4 +- 8 files changed, 359 insertions(+), 311 deletions(-)
base-commit: 793d8378b74ac283a4dd7cef1b304553c8a42260
Signature · 1 line
2.38.1
Eric Biggers
maintainerPATCH 1/2 fscrypt: stop using keyrings subsystem for fscrypt_master_key
From: Eric Biggers <[email protected]>
commit d7e7b9af104c7b389a0c21eb26532511bce4b510 upstream.
The approach of fs/crypto/ internally managing the fscrypt_master_key structs as the payloads of "struct key" objects contained in a "struct key" keyring has outlived its usefulness. The original idea was to simplify the code by reusing code from the keyrings subsystem. However, several issues have arisen that can't easily be resolved:
- When a master key struct is destroyed, blk_crypto_evict_key() must be called on any per-mode keys embedded in it. (This started being the case when inline encryption support was added.) Yet, the keyrings subsystem can arbitrarily delay the destruction of keys, even past the time the filesystem was unmounted. Therefore, currently there is no easy way to call blk_crypto_evict_key() when a master key is destroyed. Currently, this is worked around by holding an extra reference to the filesystem's request_queue(s). But it was overlooked that the request_queue reference is *not* guaranteed to pin the corresponding blk_crypto_profile too; for device-mapper devices that support inline crypto, it doesn't. This can cause a use-after-free.
- When the last inode that was using an incompletely-removed master key is evicted, the master key removal is completed by removing the key struct from the keyring. Currently this is done via key_invalidate(). Yet, key_invalidate() takes the key semaphore. This can deadlock when called from the shrinker, since in fscrypt_ioctl_add_key(), memory is allocated with GFP_KERNEL under the same semaphore.
- More generally, the fact that the keyrings subsystem can arbitrarily delay the destruction of keys (via garbage collection delay, or via random processes getting temporary key references) is undesirable, as it means we can't strictly guarantee that all secrets are ever wiped.
- Doing the master key lookups via the keyrings subsystem results in the key_permission LSM hook being called. fscrypt doesn't want this, as all access control for encrypted files is designed to happen via the files themselves, like any other files. The workaround which SELinux users are using is to change their SELinux policy to grant key search access to all domains. This works, but it is an odd extra step that shouldn't really have to be done.
The fix for all these issues is to change the implementation to what I should have done originally: don't use the keyrings subsystem to keep track of the filesystem's fscrypt_master_key structs. Instead, just store them in a regular kernel data structure, and rework the reference counting, locking, and lifetime accordingly. Retain support for RCU-mode key lookups by using a hash table. Replace fscrypt_sb_free() with fscrypt_sb_delete(), which releases the keys synchronously and runs a bit earlier during unmount, so that block devices are still available.
A side effect of this patch is that neither the master keys themselves nor the filesystem keyrings will be listed in /proc/keys anymore. ("Master key users" and the master key users keyrings will still be listed.) However, this was mostly an implementation detail, and it was intended just for debugging purposes. I don't know of anyone using it.
This patch does *not* change how "master key users" (->mk_users) works; that still uses the keyrings subsystem. That is still needed for key quotas, and changing that isn't necessary to solve the issues listed above. If we decide to change that too, it would be a separate patch.
I've marked this as fixing the original commit that added the fscrypt keyring, but as noted above the most important issue that this patch fixes wasn't introduced until the addition of inline encryption support.
--- fs/crypto/fscrypt_private.h | 71 ++++-- fs/crypto/hooks.c | 10 +- fs/crypto/keyring.c | 486 +++++++++++++++++++----------------- fs/crypto/keysetup.c | 81 +++--- fs/crypto/policy.c | 8 +- fs/super.c | 2 +- include/linux/fs.h | 2 +- include/linux/fscrypt.h | 4 +- 8 files changed, 353 insertions(+), 311 deletions(-)
fs/crypto/fscrypt_private.h +51 -20
@@ -220,7 +220,7 @@ struct fscrypt_info {* will be NULL if the master key was found in a process-subscribed * keyring rather than in the filesystem-level keyring. */ -struct key *ci_master_key; +struct fscrypt_master_key *ci_master_key;/* * Link in list of inodes that were unlocked with the master key.
@@ -430,6 +430,40 @@ struct fscrypt_master_key_secret {*/ struct fscrypt_master_key { +/* +* Back-pointer to the super_block of the filesystem to which this +* master key has been added. Only valid if ->mk_active_refs > 0. +*/ +struct super_block *mk_sb; + +/* +* Link in ->mk_sb->s_master_keys->key_hashtable. +* Only valid if ->mk_active_refs > 0. +*/ +struct hlist_node mk_node; + +/* Semaphore that protects ->mk_secret and ->mk_users */ +struct rw_semaphore mk_sem; + +/* +* Active and structural reference counts. An active ref guarantees +* that the struct continues to exist, continues to be in the keyring +* ->mk_sb->s_master_keys, and that any embedded subkeys (e.g. +* ->mk_direct_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_secret being present, +* and one active ref for each inode in ->mk_decrypted_inodes. +* +* There is one structural ref associated with the active refcount being +* nonzero. Finding a key in the keyring also takes a structural ref, +* which is then held temporarily while the key is operated on. +*/ +refcount_t mk_active_refs; +refcount_t mk_struct_refs; + +struct rcu_head mk_rcu_head; +/* * The secret key material. After FS_IOC_REMOVE_ENCRYPTION_KEY is * executed, this is wiped and no new inodes can be unlocked with this
@@ -438,7 +472,10 @@ struct fscrypt_master_key {* FS_IOC_REMOVE_ENCRYPTION_KEY can be retried, or * FS_IOC_ADD_ENCRYPTION_KEY can add the secret again. * -* Locking: protected by this master key's key->sem. +* While ->mk_secret is present, one ref in ->mk_active_refs is held. +* +* Locking: protected by ->mk_sem. The manipulation of ->mk_active_refs +* associated with this field is protected by ->mk_sem as well. */ struct fscrypt_master_key_secret mk_secret;
@@ -459,22 +496,12 @@ struct fscrypt_master_key {* * This is NULL for v1 policy keys; those can only be added by root. * -* Locking: in addition to this keyring's own semaphore, this is -* protected by this master key's key->sem, so we can do atomic -* search+insert. It can also be searched without taking any locks, but -* in that case the returned key may have already been removed. +* Locking: protected by ->mk_sem. (We don't just rely on the keyrings +* subsystem semaphore ->mk_users->sem, as we need support for atomic +* search+insert along with proper synchronization with ->mk_secret.) */ struct key *mk_users; -/* -* Length of ->mk_decrypted_inodes, plus one if mk_secret is present. -* Once this goes to 0, the master key is removed from ->s_master_keys. -* The 'struct fscrypt_master_key' will continue to live as long as the -* 'struct key' whose payload it is, but we won't let this reference -* count rise again. -*/ -refcount_t mk_refcount; -/* * List of inodes that were unlocked using this key. This allows the * inodes to be evicted efficiently if the key is removed.
@@ -500,10 +527,10 @@ static inline bool is_master_key_secret_present(const struct fscrypt_master_key_secret *secret) {/* -* The READ_ONCE() is only necessary for fscrypt_drop_inode() and -* fscrypt_key_describe(). These run in atomic context, so they can't -* take the key semaphore and thus 'secret' can change concurrently -* which would be a data race. But they only need to know whether the +* The READ_ONCE() is only necessary for fscrypt_drop_inode(). +* fscrypt_drop_inode() runs in atomic context, so it can't take the key +* semaphore and thus 'secret' can change concurrently which would be a +* data race. But fscrypt_drop_inode() only need to know whether the * secret *was* present at the time of check, so READ_ONCE() suffices. */ return READ_ONCE (secret->size) !=0 ;
@@ -532,7 +559,11 @@ static inline int master_key_spec_len(const struct fscrypt_key_specifier *spec)return 0 ; } -struct key * +void fscrypt_put_master_key (struct fscrypt_master_key *mk); + +void fscrypt_put_master_key_activeref (struct fscrypt_master_key *mk); + +struct fscrypt_master_key *fscrypt_find_master_key (struct super_block *sb,const struct fscrypt_key_specifier *mk_spec);
fs/crypto/hooks.c +3 -7
@@ -5,8 +5,6 @@* Encryption hooks for higher-level filesystem operations. */ -#include <linux/key.h> -#include "fscrypt_private.h" /**
@@ -142,7 +140,6 @@ int fscrypt_prepare_setflags(struct inode *inode, unsigned int oldflags, unsigned int flags) {struct fscrypt_info *ci; -struct key *key;struct fscrypt_master_key *mk;int err;
@@ -158,14 +155,13 @@ int fscrypt_prepare_setflags(struct inode *inode, ci = inode->i_crypt_info;if (ci->ci_policy.version !=FSCRYPT_POLICY_V2 )return -EINVAL ; - key = ci->ci_master_key; - mk = key->payload.data[0 ]; -down_read (&key->sem); + mk = ci->ci_master_key; +down_read (&mk->mk_sem);if (is_master_key_secret_present (&mk->mk_secret)) err =fscrypt_derive_dirhash_key (ci, mk);else err = -ENOKEY ; -up_read (&key->sem); +up_read (&mk->mk_sem);return err; }return 0 ;
fs/crypto/keyring.c +261 -225
751 lines not on this page · raw message
fs/crypto/keysetup.c +32 -49
183 lines not on this page · raw message
fs/crypto/policy.c +2 -6
@@ -692,12 +692,8 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)* delayed key setup that requires the inode number. */ if (ci->ci_policy.version ==FSCRYPT_POLICY_V2 && - (ci->ci_policy.v2.flags &FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 )) { -const struct fscrypt_master_key *mk = - ci->ci_master_key->payload.data[0 ]; - -fscrypt_hash_inode_number (ci, mk); - } + (ci->ci_policy.v2.flags &FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 )) +fscrypt_hash_inode_number (ci, ci->ci_master_key);return inode->i_sb->s_cop->set_context (inode, &ctx, ctxsize, fs_data); }
fs/super.c +1 -1
@@ -293,7 +293,6 @@ 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_sb_free (s);put_user_ns (s->s_user_ns);kfree (s->s_subtype);call_rcu (&s->rcu, destroy_super_rcu);
@@ -454,6 +453,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);security_sb_delete (sb);if (sb->s_dio_done_wq) {
include/linux/fs.h +1 -1
@@ -1487,7 +1487,7 @@ struct super_block {const struct xattr_handler **s_xattr;#ifdef CONFIG_FS_ENCRYPTION const struct fscrypt_operations *s_cop; -struct key *s_master_keys;/* master crypto keys in use */ +struct fscrypt_keyring *s_master_keys;/* master crypto keys in use */ #endif #ifdef CONFIG_FS_VERITY const struct fsverity_operations *s_vop;
include/linux/fscrypt.h +2 -2
@@ -294,7 +294,7 @@ fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy) }/* keyring.c */ -void fscrypt_sb_free (struct super_block *sb); +void fscrypt_sb_delete (struct super_block *sb);int fscrypt_ioctl_add_key (struct file *filp,void __user *arg);int fscrypt_ioctl_remove_key (struct file *filp,void __user *arg);int fscrypt_ioctl_remove_key_all_users (struct file *filp,void __user *arg);
@@ -482,7 +482,7 @@ fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy) }/* keyring.c */ -static inline void fscrypt_sb_free (struct super_block *sb) +static inline void fscrypt_sb_delete (struct super_block *sb) { }
Signature · 1 line
2.38.1
Eric Biggers
maintainerPATCH 2/2 fscrypt: fix keyring memory leak on mount failure
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
commit ccd30a476f8e864732de220bd50e6f372f5ebcab upstream.
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.
--- fs/crypto/keyring.c | 17 +++++++++++------ fs/super.c | 3 ++- include/linux/fscrypt.h | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-)
fs/crypto/keyring.c +11 -6
@@ -202,14 +202,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
@@ -293,6 +293,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);
@@ -453,7 +454,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
@@ -294,7 +294,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_ioctl_remove_key (struct file *filp,void __user *arg);int fscrypt_ioctl_remove_key_all_users (struct file *filp,void __user *arg);
@@ -482,7 +482,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) { }
Signature · 1 line
2.38.1
Quoted 1 line · Eric Biggers
Please apply these to 5.15-stable.
Now queued up, thanks.
greg k-h