Re: [PATCH v2 02/36] fscrypt: don't wipe mk secret until the last active user is gone

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Oct 10, 2023 at 04:40:17PM -0400, Josef Bacik wrote:
> Previously we were wiping the master key secret when we do
> FS_IOC_REMOVE_ENCRYPTION_KEY, and then using the fact that it was
> cleared as the mechanism from keeping new users from being setup.  This
> works with inode based encryption, as the per-inode key is derived at
> setup time, so the secret disappearing doesn't affect any currently open
> files from being able to continue working.
> 
> However for extent based encryption we do our key derivation at page
> writeout and readpage time, which means we need the master key secret to
> be available while we still have our file open.
> 
> Since the master key lifetime is controlled by a flag, move the clearing
> of the secret to the mk_active_users cleanup stage.  This counter
> represents the actively open files that still exist on the file system,
> and thus should still be able to operate normally.  Once the last user
> is closed we can clear the secret.  Until then no new users are allowed,
> and this allows currently open files to continue to operate until
> they're closed.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/crypto/keyring.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index e0e311ed6b88..31ea81d97075 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -116,6 +116,7 @@ void fscrypt_put_master_key_activeref(struct super_block *sb,
>  	memzero_explicit(&mk->mk_ino_hash_key,
>  			 sizeof(mk->mk_ino_hash_key));
>  	mk->mk_ino_hash_key_initialized = false;
> +	wipe_master_key_secret(&mk->mk_secret);
>  
>  	/* Drop the structural ref associated with the active refs. */
>  	fscrypt_put_master_key(mk);
> @@ -245,7 +246,6 @@ void fscrypt_destroy_keyring(struct super_block *sb)
>  			WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 1);
>  			WARN_ON_ONCE(refcount_read(&mk->mk_struct_refs) != 1);
>  			WARN_ON_ONCE(!is_master_key_secret_present(mk));
> -			wipe_master_key_secret(&mk->mk_secret);
>  			set_bit(FSCRYPT_MK_FLAG_EVICTED, &mk->mk_flags);
>  			fscrypt_put_master_key_activeref(sb, mk);
>  		}
> @@ -1064,7 +1064,6 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
>  	/* No user claims remaining.  Go ahead and wipe the secret. */
>  	err = -ENOKEY;
>  	if (!test_and_set_bit(FSCRYPT_MK_FLAG_EVICTED, &mk->mk_flags)) {
> -		wipe_master_key_secret(&mk->mk_secret);
>  		fscrypt_put_master_key_activeref(sb, mk);
>  		err = 0;
>  	}
> -- 
> 2.41.0
> 

I think we should do this only on filesystems that use extent-based encryption.
Yes, wiping the key in the "incompletely removed" state is not essential, for
the reasons we've discussed before, but we might as well do the best we can do
on each filesystem.

- Eric