| Message ID | 64c47243cea5a8eca15538b51f88c0a6d53799cf.1691505830.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fscrypt: preliminary rearrangmeents of key setup | expand |
On Tue, Aug 08, 2023 at 01:08:08PM -0400, Sweet Tea Dorminy wrote: > Right now fscrypt_infos have two fields dedicated solely to recording > what type of prepared key the info has: whether it solely owns the > prepared key, or has borrowed it from a master key, or from a direct > key. > > The ci_direct_key field is only used for v1 direct key policies, > recording the direct key that needs to have its refcount reduced when > the crypt_info is freed. However, now that crypt_info->ci_enc_key is a > pointer to the authoritative prepared key -- embedded in the direct key, > in this case, we no longer need to keep a full pointer to the direct key > -- we can use container_of() to go from the prepared key to its > surrounding direct key. > > The key ownership information doesn't change during the lifetime of a > prepared key. Since at worst there's a prepared key per info, and at > best many infos share a single prepared key, it can be slightly more > efficient to store this ownership info in the prepared key instead of in > the fscrypt_info, especially since we can squash both fields down into > a single enum. > > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
Hi Sweet,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Sweet-Tea-Dorminy/fscrypt-move-inline-crypt-decision-to-info-setup/20230809-030251
base: 54d2161835d828a9663f548f61d1d9c3d3482122
patch link: https://lore.kernel.org/r/64c47243cea5a8eca15538b51f88c0a6d53799cf.1691505830.git.sweettea-kernel%40dorminy.me
patch subject: [PATCH v6 8/8] fscrypt: make prepared keys record their type
config: x86_64-randconfig-m001-20230808 (https://download.01.org/0day-ci/archive/20230809/202308092324.d0OCNA1O-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230809/202308092324.d0OCNA1O-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202308092324.d0OCNA1O-lkp@intel.com/
smatch warnings:
fs/crypto/keysetup.c:300 setup_new_mode_prepared_key() warn: inconsistent returns '&fscrypt_mode_key_setup_mutex'.
vim +300 fs/crypto/keysetup.c
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08 238 static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08 239 struct fscrypt_prepared_key *prep_key,
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 240 const struct fscrypt_info *ci)
5dae460c2292db Eric Biggers 2019-08-04 241 {
b103fb7653fff0 Eric Biggers 2019-10-24 242 const struct inode *inode = ci->ci_inode;
b103fb7653fff0 Eric Biggers 2019-10-24 243 const struct super_block *sb = inode->i_sb;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 244 unsigned int policy_flags = fscrypt_policy_flags(&ci->ci_policy);
5dae460c2292db Eric Biggers 2019-08-04 245 struct fscrypt_mode *mode = ci->ci_mode;
85af90e57ce969 Eric Biggers 2019-12-09 246 const u8 mode_num = mode - fscrypt_modes;
5dae460c2292db Eric Biggers 2019-08-04 247 u8 mode_key[FSCRYPT_MAX_KEY_SIZE];
b103fb7653fff0 Eric Biggers 2019-10-24 248 u8 hkdf_info[sizeof(mode_num) + sizeof(sb->s_uuid)];
b103fb7653fff0 Eric Biggers 2019-10-24 249 unsigned int hkdf_infolen = 0;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 250 u8 hkdf_context = 0;
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08 251 int err = 0;
e3b1078bedd323 Eric Biggers 2020-05-15 252
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 253 switch (policy_flags & FSCRYPT_POLICY_FLAGS_KEY_MASK) {
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 254 case FSCRYPT_POLICY_FLAG_DIRECT_KEY:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 255 hkdf_context = HKDF_CONTEXT_DIRECT_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 256 break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 257 case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 258 hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_64_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 259 break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 260 case FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32:
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 261 hkdf_context = HKDF_CONTEXT_IV_INO_LBLK_32_KEY;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 262 break;
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 263 }
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 264
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 265 /*
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 266 * For DIRECT_KEY policies: instead of deriving per-file encryption
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 267 * keys, the per-file nonce will be included in all the IVs. But
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 268 * unlike v1 policies, for v2 policies in this case we don't encrypt
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 269 * with the master key directly but rather derive a per-mode encryption
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 270 * key. This ensures that the master key is consistently used only for
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 271 * HKDF, avoiding key reuse issues.
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 272 *
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 273 * For IV_INO_LBLK policies: encryption keys are derived from
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 274 * (master_key, mode_num, filesystem_uuid), and inode number is
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 275 * included in the IVs. This format is optimized for use with inline
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 276 * encryption hardware compliant with the UFS standard.
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 277 */
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 278
e3b1078bedd323 Eric Biggers 2020-05-15 279 mutex_lock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers 2020-05-15 280
5fee36095cda45 Satya Tangirala 2020-07-02 281 if (fscrypt_is_key_prepared(prep_key, ci))
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08 282 goto out_unlock;
5dae460c2292db Eric Biggers 2019-08-04 283
5dae460c2292db Eric Biggers 2019-08-04 284 BUILD_BUG_ON(sizeof(mode_num) != 1);
b103fb7653fff0 Eric Biggers 2019-10-24 285 BUILD_BUG_ON(sizeof(sb->s_uuid) != 16);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 286 BUILD_BUG_ON(sizeof(hkdf_info) != MAX_MODE_KEY_HKDF_INFO_SIZE);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 287 hkdf_infolen = fill_hkdf_info_for_mode_key(ci, hkdf_info);
78265b33a56a52 Sweet Tea Dorminy 2023-08-08 288
5dae460c2292db Eric Biggers 2019-08-04 289 err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
b103fb7653fff0 Eric Biggers 2019-10-24 290 hkdf_context, hkdf_info, hkdf_infolen,
5dae460c2292db Eric Biggers 2019-08-04 291 mode_key, mode->keysize);
5dae460c2292db Eric Biggers 2019-08-04 292 if (err)
3bd6d42474f3a9 Sweet Tea Dorminy 2023-08-08 293 return err;
Originally this was goto out_unlock; Not sure why it was changed to a
direct return.
3bd6d42474f3a9 Sweet Tea Dorminy 2023-08-08 294 prep_key->type = FSCRYPT_KEY_MASTER_KEY;
5fee36095cda45 Satya Tangirala 2020-07-02 295 err = fscrypt_prepare_key(prep_key, mode_key, ci);
5dae460c2292db Eric Biggers 2019-08-04 296 memzero_explicit(mode_key, mode->keysize);
a03cf25a20f748 Sweet Tea Dorminy 2023-08-08 297
e3b1078bedd323 Eric Biggers 2020-05-15 298 out_unlock:
e3b1078bedd323 Eric Biggers 2020-05-15 299 mutex_unlock(&fscrypt_mode_key_setup_mutex);
e3b1078bedd323 Eric Biggers 2020-05-15 @300 return err;
5dae460c2292db Eric Biggers 2019-08-04 301 }
On Tue, Aug 08, 2023 at 01:08:08PM -0400, Sweet Tea Dorminy wrote: > +/** > + * enum fscrypt_prepared_key_type - records a prepared key's ownership > + * > + * @FSCRYPT_KEY_PER_INFO: this prepared key is allocated for a specific info > + * and is never shared. > + * @FSCRYPT_KEY_DIRECT_V1: this prepared key is embedded in a fscrypt_direct_key > + * used in v1 direct key policies. > + * @FSCRYPT_KEY_MASTER_KEY: this prepared key is a per-mode and policy key, > + * part of a fscrypt_master_key, shared between all > + * users of this master key having this mode and > + * policy. > + */ > +enum fscrypt_prepared_key_type { > + FSCRYPT_KEY_PER_INFO = 1, > + FSCRYPT_KEY_DIRECT_V1, > + FSCRYPT_KEY_MASTER_KEY, > +} __packed; FSCRYPT_KEY_MASTER_KEY seems misnamed, since it's not for master keys. It's for what the code elsewhere calls a per-mode key. So maybe FSCRYPT_KEY_PER_MODE? I think your intent was for the name to reflect the struct that the fscrypt_prepared_key is embedded in. I don't think that's obvious as-is. If you want to name it that way, it should be made super clear, like this: enum fscrypt_prepared_key_owner { FSCRYPT_KEY_OWNED_BY_INFO = 1, FSCRYPT_KEY_OWNED_BY_DIRECT_V1, FSCRYPT_KEY_OWNED_BY_MASTER_KEY, }; But, I think I'm leaning towards your proposal with s/FSCRYPT_KEY_MASTER_KEY/FSCRYPT_KEY_PER_MODE/. - Eric
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 20b8ea1e3518..e2acd8894ea7 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -174,18 +174,39 @@ struct fscrypt_symlink_data { char encrypted_path[]; } __packed; +/** + * enum fscrypt_prepared_key_type - records a prepared key's ownership + * + * @FSCRYPT_KEY_PER_INFO: this prepared key is allocated for a specific info + * and is never shared. + * @FSCRYPT_KEY_DIRECT_V1: this prepared key is embedded in a fscrypt_direct_key + * used in v1 direct key policies. + * @FSCRYPT_KEY_MASTER_KEY: this prepared key is a per-mode and policy key, + * part of a fscrypt_master_key, shared between all + * users of this master key having this mode and + * policy. + */ +enum fscrypt_prepared_key_type { + FSCRYPT_KEY_PER_INFO = 1, + FSCRYPT_KEY_DIRECT_V1, + FSCRYPT_KEY_MASTER_KEY, +} __packed; + /** * struct fscrypt_prepared_key - a key prepared for actual encryption/decryption * @tfm: crypto API transform object * @blk_key: key for blk-crypto + * @type: records the ownership type of the prepared key * - * Normally only one of the fields will be non-NULL. + * Normally only one of @tfm and @blk_key will be non-NULL, although it is + * possible if @type is FSCRYPT_KEY_MASTER_KEY. */ struct fscrypt_prepared_key { struct crypto_skcipher *tfm; #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT struct blk_crypto_key *blk_key; #endif + enum fscrypt_prepared_key_type type; }; /* @@ -233,12 +254,6 @@ struct fscrypt_info { */ struct list_head ci_master_key_link; - /* - * If non-NULL, then encryption is done using the master key directly - * and ci_enc_key will equal ci_direct_key->dk_key. - */ - struct fscrypt_direct_key *ci_direct_key; - /* * This inode's hash key for filenames. This is a 128-bit SipHash-2-4 * key. This is only set for directories that use a keyed dirhash over @@ -641,7 +656,7 @@ static inline int fscrypt_require_key(struct inode *inode) /* keysetup_v1.c */ -void fscrypt_put_direct_key(struct fscrypt_direct_key *dk); +void fscrypt_put_direct_key(struct fscrypt_prepared_key *prep_key); int fscrypt_setup_v1_file_key(struct fscrypt_info *ci, const u8 *raw_master_key); diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 4f04999ecfd1..a19650f954e2 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -191,11 +191,11 @@ void fscrypt_destroy_prepared_key(struct super_block *sb, /* Given a per-file encryption key, set up the file's crypto transform object */ int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key) { - ci->ci_owns_key = true; ci->ci_enc_key = kzalloc(sizeof(*ci->ci_enc_key), GFP_KERNEL); if (!ci->ci_enc_key) return -ENOMEM; + ci->ci_enc_key->type = FSCRYPT_KEY_PER_INFO; return fscrypt_prepare_key(ci->ci_enc_key, raw_key, ci); } @@ -290,7 +290,8 @@ static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk, hkdf_context, hkdf_info, hkdf_infolen, mode_key, mode->keysize); if (err) - goto out_unlock; + return err; + prep_key->type = FSCRYPT_KEY_MASTER_KEY; err = fscrypt_prepare_key(prep_key, mode_key, ci); memzero_explicit(mode_key, mode->keysize); @@ -584,12 +585,16 @@ static void put_crypt_info(struct fscrypt_info *ci) if (!ci) return; - if (ci->ci_direct_key) - 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); - kfree_sensitive(ci->ci_enc_key); + if (ci->ci_enc_key) { + enum fscrypt_prepared_key_type type = ci->ci_enc_key->type; + + if (type == FSCRYPT_KEY_DIRECT_V1) + fscrypt_put_direct_key(ci->ci_enc_key); + if (type == FSCRYPT_KEY_PER_INFO) { + fscrypt_destroy_prepared_key(ci->ci_inode->i_sb, + ci->ci_enc_key); + kfree_sensitive(ci->ci_enc_key); + } } mk = ci->ci_master_key; diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c index e1d761e8067f..1e785cedead0 100644 --- a/fs/crypto/keysetup_v1.c +++ b/fs/crypto/keysetup_v1.c @@ -160,8 +160,11 @@ static void free_direct_key(struct fscrypt_direct_key *dk) } } -void fscrypt_put_direct_key(struct fscrypt_direct_key *dk) +void fscrypt_put_direct_key(struct fscrypt_prepared_key *prep_key) { + struct fscrypt_direct_key *dk = + container_of(prep_key, struct fscrypt_direct_key, dk_key); + if (!refcount_dec_and_lock(&dk->dk_refcount, &fscrypt_direct_keys_lock)) return; hash_del(&dk->dk_node); @@ -235,6 +238,7 @@ fscrypt_get_direct_key(const struct fscrypt_info *ci, const u8 *raw_key) dk->dk_sb = ci->ci_inode->i_sb; refcount_set(&dk->dk_refcount, 1); dk->dk_mode = ci->ci_mode; + dk->dk_key.type = FSCRYPT_KEY_DIRECT_V1; err = fscrypt_prepare_key(&dk->dk_key, raw_key, ci); if (err) goto err_free_dk; @@ -258,7 +262,6 @@ static int setup_v1_file_key_direct(struct fscrypt_info *ci, dk = fscrypt_get_direct_key(ci, raw_master_key); if (IS_ERR(dk)) return PTR_ERR(dk); - ci->ci_direct_key = dk; ci->ci_enc_key = &dk->dk_key; return 0; }
Right now fscrypt_infos have two fields dedicated solely to recording what type of prepared key the info has: whether it solely owns the prepared key, or has borrowed it from a master key, or from a direct key. The ci_direct_key field is only used for v1 direct key policies, recording the direct key that needs to have its refcount reduced when the crypt_info is freed. However, now that crypt_info->ci_enc_key is a pointer to the authoritative prepared key -- embedded in the direct key, in this case, we no longer need to keep a full pointer to the direct key -- we can use container_of() to go from the prepared key to its surrounding direct key. The key ownership information doesn't change during the lifetime of a prepared key. Since at worst there's a prepared key per info, and at best many infos share a single prepared key, it can be slightly more efficient to store this ownership info in the prepared key instead of in the fscrypt_info, especially since we can squash both fields down into a single enum. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> --- fs/crypto/fscrypt_private.h | 31 +++++++++++++++++++++++-------- fs/crypto/keysetup.c | 21 +++++++++++++-------- fs/crypto/keysetup_v1.c | 7 +++++-- 3 files changed, 41 insertions(+), 18 deletions(-)