On Tue, Aug 08, 2023 at 01:08:02PM -0400, Sweet Tea Dorminy wrote:
> +/*
> + * Find or create the appropriate prepared key for an info.
> + */
> +static int fscrypt_setup_file_key(struct fscrypt_info *ci,
> + struct fscrypt_master_key *mk,
> + bool need_dirhash_key)
> +{
> + int err;
> +
> + if (!mk) {
> + if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
> + return -ENOKEY;
> +
> + /*
> + * 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);
> + }
> +
> + switch (ci->ci_policy.version) {
> + case FSCRYPT_POLICY_V1:
> + err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
> + break;
> + case FSCRYPT_POLICY_V2:
> + err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + err = -EINVAL;
> + break;
> + }
> + return err;
> +}
'err' is not needed. The switch statement should look like:
switch (ci->ci_policy.version) {
case FSCRYPT_POLICY_V1:
return fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
case FSCRYPT_POLICY_V2:
return fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
default:
WARN_ON_ONCE(1);
return -EINVAL;
}
> /*
> - * Find the master key, then set up the inode's actual encryption key.
> + * Find and lock the master key.
> *
> * If the master key is found in the filesystem-level keyring, then it is
> * returned in *mk_ret with its semaphore read-locked. This is needed to ensure
> @@ -434,9 +471,8 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
> * multiple tasks may race to create an fscrypt_info for the same inode), and to
> * synchronize the master key being removed with a new inode starting to use it.
> */
> -static int setup_file_encryption_key(struct fscrypt_info *ci,
> - bool need_dirhash_key,
> - struct fscrypt_master_key **mk_ret)
> +static int find_and_lock_master_key(const struct fscrypt_info *ci,
> + struct fscrypt_master_key **mk_ret)
I think it would be a bit cleaner if this returned
'struct fscrypt_master_key *'. Use NULL for not found, ERR_PTR() for errors.
> {
> struct super_block *sb = ci->ci_inode->i_sb;
> struct fscrypt_key_specifier mk_spec;
> @@ -466,17 +502,19 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
> mk = fscrypt_find_master_key(sb, &mk_spec);
> }
> }
> +
> if (unlikely(!mk)) {
> if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
> return -ENOKEY;
>
> /*
> - * 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.
> + * This might be the case of a v1 policy using a process
> + * subscribed keyring to get the key, so there may not be
> + * a relevant master key.
> */
> - return fscrypt_setup_v1_file_key_via_subscribed_keyrings(ci);
> +
> + *mk_ret = NULL;
> + return 0;
> }
'ci->ci_policy.version != FSCRYPT_POLICY_V1' is duplicated with
fscrypt_setup_file_key(). The problem is really that this patch makes the
handling of "master key not found" happen in two different places.
I think find_and_lock_master_key() should just return NULL for the master key
when it's not found. Then fscrypt_setup_file_key() decides what to do about it.
Also, the comment for find_and_lock_master_key() needs to be updated. The last
sentence in particular is not necessary anymore. I think your refactoring fixes
the reason why that explanation was needed in the first place. With my
suggestion to return a pointer, I think a good comment would be:
/*
* Find the master key for ci_policy in the filesystem-level keyring. Returns
* the read-locked key if found, NULL if not found, or an ERR_PTR on error. The
* caller is responsible for unlocking and putting the key if found.
*/
- Eric
|