Re: [PATCH v7 04/43] fscrypt: conditionally don't wipe mk secret until the last active user is done

[Daniel Vacek]

On Tue, 2 Jun 2026 at 01:04, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> On Wed, May 13, 2026 at 10:52:38AM +0200, Daniel Vacek wrote:
> > From: Josef Bacik <josef@xxxxxxxxxxxxxx>
> >
> > 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 seems to be describing the state prior to commit 15baf55481de, not
> the current tip of tree.  We do still wipe on
> FS_IOC_REMOVE_ENCRYPTION_KEY, but there's a separate boolean that
> maintains the status and everyone looks at that now.

I see, I'll clear that out.

> >   * FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED
> >   *   Removal of this key has been initiated, but some inodes that were
> > - *   unlocked with it are still in-use.  Like ABSENT, ->mk_secret is wiped,
> > - *   and the key can no longer be used to unlock inodes.  Unlike ABSENT, the
> > - *   key is still in the keyring; ->mk_decrypted_inodes is nonempty; and
> > + *   unlocked with it are still in-use.
> > + *   For filesystems using per-extent encryption ->mk_secret is still
> > + *   being kept as the per-extent keys are derived at writeout time.
> > + *   Otherwise, like ABSENT, ->mk_secret is wiped, and the key can
> > + *   no longer be used to unlock inodes.  Unlike ABSENT, the key is
> > + *   still in the keyring; ->mk_decrypted_inodes is nonempty; and
> >   *   ->mk_active_refs > 0, being equal to the size of ->mk_decrypted_inodes.
>
> This is saying that a bunch of things don't apply to extent-based
> encryption, due to being after the "Otherwise," when in fact they
> actually still do.  So the wording here needs improvement.  How about:
>
>     ->mk_secret exists only if the filesystem uses extent-based
>     encryption, to support key derivation during file data writeback;
>     otherwise it is wiped.  Either way, the key can no longer be used to
>     unlock inodes.  Unlike ABSENT, the key is still in the keyring;
>     ->mk_decrypted_inodes is nonempty; and ->mk_active_refs
>     > 0, being equal to the size of ->mk_decrypted_inodes.

Agreed, this sounds better. I'll use that.

> >   *
> >   *   This state transitions to ABSENT if ->mk_decrypted_inodes becomes empty,
> > diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> > index be8e6e8011f2..796e02a0db25 100644
> > --- a/fs/crypto/keyring.c
> > +++ b/fs/crypto/keyring.c
> > @@ -110,6 +110,14 @@ 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));
> >
> > +     /* We can't wipe the master key secret until the last activeref is
> > +      * dropped on the master key with per-extent encryption since the key
> > +      * derivation continues to happen as long as there are active refs.
> > +      * Wipe it here now that we're done using it.
> > +      */
> > +     if (sb->s_cop->has_per_extent_encryption)
> > +             wipe_master_key_secret(&mk->mk_secret);
>
> wipe_master_key_secret() is idempotent, so we might as well just do it
> unconditionally here.

While this is true, it also (kind of) documents that the secret should
already be wiped out by now in case of !extent encryption.

But I can wipe the secret unconditionally here if you insist.

--nX

> - Eric