Re: [PATCH] fscrypt: track master key presence separately from secret

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 15, 2023 at 2:12 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> Master keys can be in one of three states: present, incompletely
> removed, and absent (as per FSCRYPT_KEY_STATUS_* used in the UAPI).
> Currently, the way that "present" is distinguished from "incompletely
> removed" internally is by whether ->mk_secret exists or not.
>
> With extent-based encryption, it will be necessary to allow per-extent
> keys to be derived while the master key is incompletely removed, so that
> I/O on open files will reliably continue working after removal of the
> key has been initiated.  (We could allow I/O to sometimes fail in that
> case, but that seems problematic for reasons such as writes getting
> silently thrown away and diverging from the existing fscrypt semantics.)
> Therefore, when the filesystem is using extent-based encryption,
> ->mk_secret can't be wiped when the key becomes incompletely removed.
>
> As a prerequisite for doing that, this patch makes the "present" state
> be tracked using a new field, ->mk_present.  No behavior is changed yet.
>
> The basic idea here is borrowed from Josef Bacik's patch
> "fscrypt: use a flag to indicate that the master key is being evicted"
> (https://lore.kernel.org/r/e86c16dddc049ff065f877d793ad773e4c6bfad9.1696970227.git.josef@xxxxxxxxxxxxxx).
> I reimplemented it using a "present" bool instead of an "evicted" flag,
> fixed a couple bugs, and tried to update everything to be consistent.
>
> Note: I considered adding a ->mk_status field instead, holding one of
> FSCRYPT_KEY_STATUS_*.  At first that seemed nice, but it ended up being
> more complex (despite simplifying FS_IOC_GET_ENCRYPTION_KEY_STATUS),
> since it would have introduced redundancy and had weird locking rules.
>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
>  Documentation/filesystems/fscrypt.rst |  4 +-
>  fs/crypto/fscrypt_private.h           | 66 +++++++++++++----------
>  fs/crypto/hooks.c                     |  2 +-
>  fs/crypto/keyring.c                   | 78 ++++++++++++++++-----------
>  fs/crypto/keysetup.c                  | 13 ++---
>  5 files changed, 95 insertions(+), 68 deletions(-)
>
> diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> index 28700fb41a00b..1b84f818e574e 100644
> --- a/Documentation/filesystems/fscrypt.rst
> +++ b/Documentation/filesystems/fscrypt.rst
> @@ -1127,22 +1127,22 @@ The caller must zero all input fields, then fill in ``key_spec``:
>        ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR and fill
>        in ``key_spec.u.descriptor``.
>
>      - To get the status of a key for v2 encryption policies, set
>        ``key_spec.type`` to FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER and fill
>        in ``key_spec.u.identifier``.
>
>  On success, 0 is returned and the kernel fills in the output fields:
>
>  - ``status`` indicates whether the key is absent, present, or
> -  incompletely removed.  Incompletely removed means that the master
> -  secret has been removed, but some files are still in use; i.e.,
> +  incompletely removed.  Incompletely removed means that removal has
> +  been initiated, but some files are still in use; i.e.,
>    `FS_IOC_REMOVE_ENCRYPTION_KEY`_ returned 0 but set the informational
>    status flag FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY.
>
>  - ``status_flags`` can contain the following flags:
>
>      - ``FSCRYPT_KEY_STATUS_FLAG_ADDED_BY_SELF`` indicates that the key
>        has added by the current user.  This is only set for keys
>        identified by ``identifier`` rather than by ``descriptor``.
>
>  - ``user_count`` specifies the number of users who have added the key.
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 2fb4ba435d27d..1892356cf924a 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -468,65 +468,78 @@ struct fscrypt_master_key_secret {
>
>         /* For v1 policy keys: the raw key.  Wiped for v2 policy keys. */
>         u8                      raw[FSCRYPT_MAX_KEY_SIZE];
>
>  } __randomize_layout;
>
>  /*
>   * fscrypt_master_key - an in-use master key
>   *
>   * This represents a master encryption key which has been added to the
> - * filesystem and can be used to "unlock" the encrypted files which were
> - * encrypted with it.
> + * filesystem.  There are three high-level states that a key can be in:
> + *
> + * FSCRYPT_KEY_STATUS_PRESENT
> + *     Key is fully usable; it can be used to unlock inodes that are encrypted
> + *     with it (this includes being able to create new inodes).  ->mk_present
> + *     indicates whether the key is in this state.  ->mk_secret exists, the key
> + *     is in the keyring, and ->mk_active_refs > 0 due to ->mk_present.
> + *
> + * 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
> + *     ->mk_active_refs > 0, being equal to the size of ->mk_decrypted_inodes.
> + *
> + *     This state transitions to ABSENT if ->mk_decrypted_inodes becomes empty,
> + *     or to PRESENT if FS_IOC_ADD_ENCRYPTION_KEY is called again for this key.
> + *
> + * FSCRYPT_KEY_STATUS_ABSENT
> + *     Key is fully removed.  The key is no longer in the keyring,
> + *     ->mk_decrypted_inodes is empty, ->mk_active_refs == 0, ->mk_secret is
> + *     wiped, and the key can no longer be used to unlock inodes.
>   */
>  struct fscrypt_master_key {
>
>         /*
>          * Link in ->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 */
> +       /* Semaphore that protects ->mk_secret, ->mk_users, and ->mk_present */
>         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
>          * ->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 active ref associated with ->mk_present being true, 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
> -        * key; however, there may still be inodes in ->mk_decrypted_inodes
> -        * which could not be evicted.  As long as some inodes still remain,
> -        * FS_IOC_REMOVE_ENCRYPTION_KEY can be retried, or
> -        * FS_IOC_ADD_ENCRYPTION_KEY can add the secret again.
> +        * The secret key material.  Wiped as soon as it is no longer needed;
> +        * for details, see the fscrypt_master_key struct comment.
>          *
> -        * 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.
> +        * Locking: protected by ->mk_sem.
>          */
>         struct fscrypt_master_key_secret        mk_secret;
>
>         /*
>          * For v1 policy keys: an arbitrary key descriptor which was assigned by
>          * userspace (->descriptor).
>          *
>          * For v2 policy keys: a cryptographic hash of this key (->identifier).
>          */
>         struct fscrypt_key_specifier            mk_spec;
> @@ -535,21 +548,21 @@ struct fscrypt_master_key {
>          * Keyring which contains a key of type 'key_type_fscrypt_user' for each
>          * user who has added this key.  Normally each key will be added by just
>          * one user, but it's possible that multiple users share a key, and in
>          * that case we need to keep track of those users so that one user can't
>          * remove the key before the others want it removed too.
>          *
>          * This is NULL for v1 policy keys; those can only be added by root.
>          *
>          * 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.)
> +        * search+insert along with proper synchronization with other fields.)
>          */
>         struct key              *mk_users;
>
>         /*
>          * List of inodes that were unlocked using this key.  This allows the
>          * inodes to be evicted efficiently if the key is removed.
>          */
>         struct list_head        mk_decrypted_inodes;
>         spinlock_t              mk_decrypted_inodes_lock;
>
> @@ -558,34 +571,31 @@ struct fscrypt_master_key {
>          * that use them.  Allocated and derived on-demand.
>          */
>         struct fscrypt_prepared_key mk_direct_keys[FSCRYPT_MODE_MAX + 1];
>         struct fscrypt_prepared_key mk_iv_ino_lblk_64_keys[FSCRYPT_MODE_MAX + 1];
>         struct fscrypt_prepared_key mk_iv_ino_lblk_32_keys[FSCRYPT_MODE_MAX + 1];
>
>         /* Hash key for inode numbers.  Initialized only when needed. */
>         siphash_key_t           mk_ino_hash_key;
>         bool                    mk_ino_hash_key_initialized;
>
> -} __randomize_layout;
> -
> -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().
> -        * 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.
> +        * Whether this key is in the "present" state, i.e. fully usable.  For
> +        * details, see the fscrypt_master_key struct comment.
> +        *
> +        * Locking: protected by ->mk_sem, but can be read locklessly using
> +        * READ_ONCE().  Writers must use WRITE_ONCE() when concurrent readers
> +        * are possible.
>          */
> -       return READ_ONCE(secret->size) != 0;
> -}
> +       bool                    mk_present;
> +
> +} __randomize_layout;
>
>  static inline const char *master_key_spec_type(
>                                 const struct fscrypt_key_specifier *spec)
>  {
>         switch (spec->type) {
>         case FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR:
>                 return "descriptor";
>         case FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER:
>                 return "identifier";
>         }
> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
> index 85d2975b69b78..52504dd478d31 100644
> --- a/fs/crypto/hooks.c
> +++ b/fs/crypto/hooks.c
> @@ -180,21 +180,21 @@ int fscrypt_prepare_setflags(struct inode *inode,
>          */
>         if (IS_ENCRYPTED(inode) && (flags & ~oldflags & FS_CASEFOLD_FL)) {
>                 err = fscrypt_require_key(inode);
>                 if (err)
>                         return err;
>                 ci = inode->i_crypt_info;
>                 if (ci->ci_policy.version != FSCRYPT_POLICY_V2)
>                         return -EINVAL;
>                 mk = ci->ci_master_key;
>                 down_read(&mk->mk_sem);
> -               if (is_master_key_secret_present(&mk->mk_secret))
> +               if (mk->mk_present)
>                         err = fscrypt_derive_dirhash_key(ci, mk);
>                 else
>                         err = -ENOKEY;
>                 up_read(&mk->mk_sem);
>                 return err;
>         }
>         return 0;
>  }
>
>  /**
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index a51fa6a33de10..f34a9b0b9e922 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -92,42 +92,54 @@ void fscrypt_put_master_key_activeref(struct super_block *sb,
>          * destroying any subkeys embedded in it.
>          */
>
>         if (WARN_ON_ONCE(!sb->s_master_keys))
>                 return;
>         spin_lock(&sb->s_master_keys->lock);
>         hlist_del_rcu(&mk->mk_node);
>         spin_unlock(&sb->s_master_keys->lock);
>
>         /*
> -        * ->mk_active_refs == 0 implies that ->mk_secret is not present and
> -        * that ->mk_decrypted_inodes is empty.
> +        * ->mk_active_refs == 0 implies that ->mk_present is false and
> +        * ->mk_decrypted_inodes is empty.
>          */
> -       WARN_ON_ONCE(is_master_key_secret_present(&mk->mk_secret));
> +       WARN_ON_ONCE(mk->mk_present);
>         WARN_ON_ONCE(!list_empty(&mk->mk_decrypted_inodes));
>
>         for (i = 0; i <= FSCRYPT_MODE_MAX; i++) {
>                 fscrypt_destroy_prepared_key(
>                                 sb, &mk->mk_direct_keys[i]);
>                 fscrypt_destroy_prepared_key(
>                                 sb, &mk->mk_iv_ino_lblk_64_keys[i]);
>                 fscrypt_destroy_prepared_key(
>                                 sb, &mk->mk_iv_ino_lblk_32_keys[i]);
>         }
>         memzero_explicit(&mk->mk_ino_hash_key,
>                          sizeof(mk->mk_ino_hash_key));
>         mk->mk_ino_hash_key_initialized = false;
>
>         /* Drop the structural ref associated with the active refs. */
>         fscrypt_put_master_key(mk);
>  }
>
> +/*
> + * This transitions the key state from present to incompletely removed, and then
> + * potentially to absent (depending on whether inodes remain).
> + */
> +static void fscrypt_initiate_key_removal(struct super_block *sb,
> +                                        struct fscrypt_master_key *mk)
> +{
> +       WRITE_ONCE(mk->mk_present, false);
> +       wipe_master_key_secret(&mk->mk_secret);
> +       fscrypt_put_master_key_activeref(sb, mk);
> +}
> +
>  static inline bool valid_key_spec(const struct fscrypt_key_specifier *spec)
>  {
>         if (spec->__reserved)
>                 return false;
>         return master_key_spec_len(spec) != 0;
>  }
>
>  static int fscrypt_user_key_instantiate(struct key *key,
>                                         struct key_preparsed_payload *prep)
>  {
> @@ -227,28 +239,27 @@ void fscrypt_destroy_keyring(struct super_block *sb)
>                 struct hlist_head *bucket = &keyring->key_hashtable[i];
>                 struct fscrypt_master_key *mk;
>                 struct hlist_node *tmp;
>
>                 hlist_for_each_entry_safe(mk, tmp, bucket, mk_node) {
>                         /*
>                          * Since all potentially-encrypted inodes were already
>                          * evicted, every key remaining in the keyring should
>                          * have an empty inode list, and should only still be in
>                          * the keyring due to the single active ref associated
> -                        * with ->mk_secret.  There should be no structural refs
> -                        * beyond the one associated with the active ref.
> +                        * with ->mk_present.  There should be no structural
> +                        * refs beyond the one associated with the active ref.
>                          */
>                         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->mk_secret));
> -                       wipe_master_key_secret(&mk->mk_secret);
> -                       fscrypt_put_master_key_activeref(sb, mk);
> +                       WARN_ON_ONCE(!mk->mk_present);
> +                       fscrypt_initiate_key_removal(sb, mk);
>                 }
>         }
>         kfree_sensitive(keyring);
>         sb->s_master_keys = NULL;
>  }
>
>  static struct hlist_head *
>  fscrypt_mk_hash_bucket(struct fscrypt_keyring *keyring,
>                        const struct fscrypt_key_specifier *mk_spec)
>  {
> @@ -432,21 +443,22 @@ static int add_new_master_key(struct super_block *sb,
>         if (mk_spec->type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) {
>                 err = allocate_master_key_users_keyring(mk);
>                 if (err)
>                         goto out_put;
>                 err = add_master_key_user(mk);
>                 if (err)
>                         goto out_put;
>         }
>
>         move_master_key_secret(&mk->mk_secret, secret);
> -       refcount_set(&mk->mk_active_refs, 1); /* ->mk_secret is present */
> +       mk->mk_present = true;
> +       refcount_set(&mk->mk_active_refs, 1); /* ->mk_present is true */
>
>         spin_lock(&keyring->lock);
>         hlist_add_head_rcu(&mk->mk_node,
>                            fscrypt_mk_hash_bucket(keyring, mk_spec));
>         spin_unlock(&keyring->lock);
>         return 0;
>
>  out_put:
>         fscrypt_put_master_key(mk);
>         return err;
> @@ -471,25 +483,32 @@ static int add_existing_master_key(struct fscrypt_master_key *mk,
>                         if (IS_ERR(mk_user))
>                                 return PTR_ERR(mk_user);
>                         key_put(mk_user);
>                         return 0;
>                 }
>                 err = add_master_key_user(mk);
>                 if (err)
>                         return err;
>         }
>
> -       /* Re-add the secret if needed. */
> -       if (!is_master_key_secret_present(&mk->mk_secret)) {
> -               if (!refcount_inc_not_zero(&mk->mk_active_refs))
> +       /* If the key is incompletely removed, make it present again. */
> +       if (!mk->mk_present) {
> +               if (!refcount_inc_not_zero(&mk->mk_active_refs)) {
> +                       /*
> +                        * Raced with the last active ref being dropped, so the
> +                        * key has become, or is about to become, "absent".
> +                        * Therefore, we need to allocate a new key struct.
> +                        */
>                         return KEY_DEAD;
> +               }
>                 move_master_key_secret(&mk->mk_secret, secret);
> +               WRITE_ONCE(mk->mk_present, true);
>         }
>
>         return 0;
>  }
>
>  static int do_add_master_key(struct super_block *sb,
>                              struct fscrypt_master_key_secret *secret,
>                              const struct fscrypt_key_specifier *mk_spec)
>  {
>         static DEFINE_MUTEX(fscrypt_add_key_mutex);
> @@ -499,22 +518,22 @@ static int do_add_master_key(struct super_block *sb,
>         mutex_lock(&fscrypt_add_key_mutex); /* serialize find + link */
>
>         mk = fscrypt_find_master_key(sb, mk_spec);
>         if (!mk) {
>                 /* Didn't find the key in ->s_master_keys.  Add it. */
>                 err = allocate_filesystem_keyring(sb);
>                 if (!err)
>                         err = add_new_master_key(sb, secret, mk_spec);
>         } else {
>                 /*
> -                * Found the key in ->s_master_keys.  Re-add the secret if
> -                * needed, and add the user to ->mk_users if needed.
> +                * Found the key in ->s_master_keys.  Add the user to ->mk_users
> +                * if needed, and make the key "present" again if possible.
>                  */
>                 down_write(&mk->mk_sem);
>                 err = add_existing_master_key(mk, secret);
>                 up_write(&mk->mk_sem);
>                 if (err == KEY_DEAD) {
>                         /*
>                          * We found a key struct, but it's already been fully
>                          * removed.  Ignore the old struct and add a new one.
>                          * fscrypt_add_key_mutex means we don't need to worry
>                          * about concurrent adds.
> @@ -982,23 +1001,22 @@ static int try_to_lock_encrypted_files(struct super_block *sb,
>   * claim to the key, then removes the key itself if no other users have claims.
>   * FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS (all_users=true) always removes the
>   * key itself.
>   *
>   * To "remove the key itself", first we wipe the actual master key secret, so
>   * that no more inodes can be unlocked with it.  Then we try to evict all cached
>   * inodes that had been unlocked with the key.
>   *
>   * If all inodes were evicted, then we unlink the fscrypt_master_key from the
>   * keyring.  Otherwise it remains in the keyring in the "incompletely removed"
> - * state (without the actual secret key) where it tracks the list of remaining
> - * inodes.  Userspace can execute the ioctl again later to retry eviction, or
> - * alternatively can re-add the secret key again.
> + * state where it tracks the list of remaining inodes.  Userspace can execute
> + * the ioctl again later to retry eviction, or alternatively can re-add the key.
>   *
>   * For more details, see the "Removing keys" section of
>   * Documentation/filesystems/fscrypt.rst.
>   */
>  static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
>  {
>         struct super_block *sb = file_inode(filp)->i_sb;
>         struct fscrypt_remove_key_arg __user *uarg = _uarg;
>         struct fscrypt_remove_key_arg arg;
>         struct fscrypt_master_key *mk;
> @@ -1046,44 +1064,43 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
>                          * can't remove the key itself.
>                          */
>                         status_flags |=
>                                 FSCRYPT_KEY_REMOVAL_STATUS_FLAG_OTHER_USERS;
>                         err = 0;
>                         up_write(&mk->mk_sem);
>                         goto out_put_key;
>                 }
>         }
>
> -       /* No user claims remaining.  Go ahead and wipe the secret. */
> +       /* No user claims remaining.  Initiate removal of the key. */
>         err = -ENOKEY;
> -       if (is_master_key_secret_present(&mk->mk_secret)) {
> -               wipe_master_key_secret(&mk->mk_secret);
> -               fscrypt_put_master_key_activeref(sb, mk);
> +       if (mk->mk_present) {
> +               fscrypt_initiate_key_removal(sb, mk);
>                 err = 0;
>         }
>         inodes_remain = refcount_read(&mk->mk_active_refs) > 0;
>         up_write(&mk->mk_sem);
>
>         if (inodes_remain) {
>                 /* Some inodes still reference this key; try to evict them. */
>                 err = try_to_lock_encrypted_files(sb, mk);
>                 if (err == -EBUSY) {
>                         status_flags |=
>                                 FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY;
>                         err = 0;
>                 }
>         }
>         /*
>          * We return 0 if we successfully did something: removed a claim to the
> -        * key, wiped the secret, or tried locking the files again.  Users need
> -        * to check the informational status flags if they care whether the key
> -        * has been fully removed including all files locked.
> +        * key, initiated removal of the key, or tried locking the files again.
> +        * Users need to check the informational status flags if they care
> +        * whether the key has been fully removed including all files locked.
>          */
>  out_put_key:
>         fscrypt_put_master_key(mk);
>         if (err == 0)
>                 err = put_user(status_flags, &uarg->removal_status_flags);
>         return err;
>  }
>
>  int fscrypt_ioctl_remove_key(struct file *filp, void __user *uarg)
>  {
> @@ -1096,26 +1113,25 @@ int fscrypt_ioctl_remove_key_all_users(struct file *filp, void __user *uarg)
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EACCES;
>         return do_remove_key(filp, uarg, true);
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_ioctl_remove_key_all_users);
>
>  /*
>   * Retrieve the status of an fscrypt master encryption key.
>   *
>   * We set ->status to indicate whether the key is absent, present, or
> - * incompletely removed.  "Incompletely removed" means that the master key
> - * secret has been removed, but some files which had been unlocked with it are
> - * still in use.  This field allows applications to easily determine the state
> - * of an encrypted directory without using a hack such as trying to open a
> - * regular file in it (which can confuse the "incompletely removed" state with
> - * absent or present).
> + * incompletely removed.  (For an explanation of what these statuses mean and
> + * how they are represented internally, see struct fscrypt_master_key.)  This
> + * field allows applications to easily determine the status of an encrypted
> + * directory without using a hack such as trying to open a regular file in it
> + * (which can confuse the "incompletely removed" status with absent or present).
>   *
>   * In addition, for v2 policy keys we allow applications to determine, via
>   * ->status_flags and ->user_count, whether the key has been added by the
>   * current user, by other users, or by both.  Most applications should not need
>   * this, since ordinarily only one user should know a given key.  However, if a
>   * secret key is shared by multiple users, applications may wish to add an
>   * already-present key to prevent other users from removing it.  This ioctl can
>   * be used to check whether that really is the case before the work is done to
>   * add the key --- which might e.g. require prompting the user for a passphrase.
>   *
> @@ -1143,21 +1159,21 @@ int fscrypt_ioctl_get_key_status(struct file *filp, void __user *uarg)
>         memset(arg.__out_reserved, 0, sizeof(arg.__out_reserved));
>
>         mk = fscrypt_find_master_key(sb, &arg.key_spec);
>         if (!mk) {
>                 arg.status = FSCRYPT_KEY_STATUS_ABSENT;
>                 err = 0;
>                 goto out;
>         }
>         down_read(&mk->mk_sem);
>
> -       if (!is_master_key_secret_present(&mk->mk_secret)) {
> +       if (!mk->mk_present) {
>                 arg.status = refcount_read(&mk->mk_active_refs) > 0 ?
>                         FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED :
>                         FSCRYPT_KEY_STATUS_ABSENT /* raced with full removal */;
>                 err = 0;
>                 goto out_release_key;
>         }
>
>         arg.status = FSCRYPT_KEY_STATUS_PRESENT;
>         if (mk->mk_users) {
>                 struct key *mk_user;
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index 094d1b7a1ae61..d71f7c799e79e 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -479,22 +479,22 @@ static int setup_file_encryption_key(struct fscrypt_inode_info *ci,
>                 /*
>                  * As a legacy fallback for v1 policies, search for the key in
>                  * the current task's subscribed keyrings too.  Don't move this
>                  * to before the search of ->s_master_keys, since users
>                  * shouldn't be able to override filesystem-level keys.
>                  */
>                 return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
>         }
>         down_read(&mk->mk_sem);
>
> -       /* Has the secret been removed (via FS_IOC_REMOVE_ENCRYPTION_KEY)? */
> -       if (!is_master_key_secret_present(&mk->mk_secret)) {
> +       if (!mk->mk_present) {
> +               /* FS_IOC_REMOVE_ENCRYPTION_KEY has been executed on this key */
>                 err = -ENOKEY;
>                 goto out_release_key;
>         }
>
>         if (!fscrypt_valid_master_key_size(mk, ci)) {
>                 err = -ENOKEY;
>                 goto out_release_key;
>         }
>
>         switch (ci->ci_policy.version) {
> @@ -532,22 +532,22 @@ static void put_crypt_info(struct fscrypt_inode_info *ci)
>                 fscrypt_put_direct_key(ci->ci_direct_key);
>         else if (ci->ci_owns_key)
>                 fscrypt_destroy_prepared_key(ci->ci_inode->i_sb,
>                                              &ci->ci_enc_key);
>
>         mk = ci->ci_master_key;
>         if (mk) {
>                 /*
>                  * Remove this inode from the list of inodes that were unlocked
>                  * with the master key.  In addition, if we're removing the last
> -                * inode from a master key struct that already had its secret
> -                * removed, then complete the full removal of the struct.
> +                * inode from an incompletely removed key, then complete the
> +                * full removal of the key.
>                  */
>                 spin_lock(&mk->mk_decrypted_inodes_lock);
>                 list_del(&ci->ci_master_key_link);
>                 spin_unlock(&mk->mk_decrypted_inodes_lock);
>                 fscrypt_put_master_key_activeref(ci->ci_inode->i_sb, mk);
>         }
>         memzero_explicit(ci, sizeof(*ci));
>         kmem_cache_free(fscrypt_inode_info_cachep, ci);
>  }
>
> @@ -794,20 +794,21 @@ int fscrypt_drop_inode(struct inode *inode)
>         /*
>          * With proper, non-racy use of FS_IOC_REMOVE_ENCRYPTION_KEY, all inodes
>          * protected by the key were cleaned by sync_filesystem().  But if
>          * userspace is still using the files, inodes can be dirtied between
>          * then and now.  We mustn't lose any writes, so skip dirty inodes here.
>          */
>         if (inode->i_state & I_DIRTY_ALL)
>                 return 0;
>
>         /*
> -        * Note: since we aren't holding the key semaphore, the result here can
> +        * We can't take ->mk_sem here, since this runs in atomic context.
> +        * Therefore, ->mk_present can change concurrently, and our result may
>          * immediately become outdated.  But there's no correctness problem with
>          * unnecessarily evicting.  Nor is there a correctness problem with not
>          * evicting while iput() is racing with the key being removed, since
>          * then the thread removing the key will either evict the inode itself
>          * or will correctly detect that it wasn't evicted due to the race.
>          */
> -       return !is_master_key_secret_present(&ci->ci_master_key->mk_secret);
> +       return !READ_ONCE(ci->ci_master_key->mk_present);
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_drop_inode);
>
> base-commit: 3e7807d5a7d770c59837026e9967fe99ad043174
> --
> 2.42.0
>

This patch looks reasonable to me. The updated comment details help a lot. :)

Reviewed-by: Neal Gompa <neal@xxxxxxxxx>



--
真実はいつも一つ!/ Always, there's only one truth!



[Index of Archives]     [linux Cryptography]     [Asterisk App Development]     [PJ SIP]     [Gnu Gatekeeper]     [IETF Sipping]     [Info Cyrus]     [ALSA User]     [Fedora Linux Users]     [Linux SCTP]     [DCCP]     [Gimp]     [Yosemite News]     [Deep Creek Hot Springs]     [Yosemite Campsites]     [ISDN Cause Codes]

  Powered by Linux