Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API
From: Eric Biggers <ebiggers@kernel.org>
Date: 2021-01-07 07:20:04
Also in:
keyrings, linux-fscrypt, lkml
On Mon, Jan 04, 2021 at 10:50:49PM +0100, Stephan Müller wrote:
quoted hunk ↗ jump to hunk
As the kernel crypto API implements HKDF, replace the file-system-specific HKDF implementation with the generic HKDF implementation. Signed-off-by: Stephan Mueller <redacted> --- fs/crypto/Kconfig | 2 +- fs/crypto/fscrypt_private.h | 4 +- fs/crypto/hkdf.c | 108 +++++++++--------------------------- 3 files changed, 30 insertions(+), 84 deletions(-)diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig index a5f5c30368a2..9450e958f1d1 100644 --- a/fs/crypto/Kconfig +++ b/fs/crypto/Kconfig@@ -2,7 +2,7 @@ config FS_ENCRYPTION bool "FS Encryption (Per-file encryption)" select CRYPTO - select CRYPTO_HASH + select CRYPTO_HKDF select CRYPTO_SKCIPHER select CRYPTO_LIB_SHA256 select KEYSdiff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 3fa965eb3336..0d6871838099 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h@@ -304,7 +304,7 @@ struct fscrypt_hkdf { struct crypto_shash *hmac_tfm; }; -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key, unsigned int master_key_size);
It shouldn't be necessary to remove const here.
quoted hunk ↗ jump to hunk
/*@@ -323,7 +323,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, #define HKDF_CONTEXT_INODE_HASH_KEY 7 /* info=<empty> */ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, - const u8 *info, unsigned int infolen, + u8 *info, unsigned int infolen, u8 *okm, unsigned int okmlen);
Likewise. In fact some callers rely on 'info' not being modified.
quoted hunk ↗ jump to hunk
-/* + * * Compute HKDF-Extract using the given master key as the input keying material, * and prepare an HMAC transform object keyed by the resulting pseudorandom key. * * Afterwards, the keyed HMAC transform object can be used for HKDF-Expand many * times without having to recompute HKDF-Extract each time. */ -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key, unsigned int master_key_size) { + /* HKDF-Extract (RFC 5869 section 2.2), unsalted */ + const struct kvec seed[] = { { + .iov_base = NULL, + .iov_len = 0 + }, { + .iov_base = master_key, + .iov_len = master_key_size + } }; struct crypto_shash *hmac_tfm; - u8 prk[HKDF_HASHLEN]; int err; hmac_tfm = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);@@ -74,16 +65,12 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, return PTR_ERR(hmac_tfm); } - if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != sizeof(prk))) { + if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != HKDF_HASHLEN)) { err = -EINVAL; goto err_free_tfm; } - err = hkdf_extract(hmac_tfm, master_key, master_key_size, prk); - if (err) - goto err_free_tfm; - - err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk)); + err = crypto_hkdf_setkey(hmac_tfm, seed, ARRAY_SIZE(seed)); if (err) goto err_free_tfm;
It's weird that the salt and key have to be passed in a kvec. Why not just have normal function parameters like: int crypto_hkdf_setkey(struct crypto_shash *hmac_tfm, const u8 *key, size_t keysize, const u8 *salt, size_t saltsize);
int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
- const u8 *info, unsigned int infolen,
+ u8 *info, unsigned int infolen,
u8 *okm, unsigned int okmlen)
{
- SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
- u8 prefix[9];
- unsigned int i;
- int err;
- const u8 *prev = NULL;
- u8 counter = 1;
- u8 tmp[HKDF_HASHLEN];
-
- if (WARN_ON(okmlen > 255 * HKDF_HASHLEN))
- return -EINVAL;
-
- desc->tfm = hkdf->hmac_tfm;
-
- memcpy(prefix, "fscrypt\0", 8);
- prefix[8] = context;
-
- for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
+ const struct kvec info_iov[] = { {
+ .iov_base = "fscrypt\0",
+ .iov_len = 8,
+ }, {
+ .iov_base = &context,
+ .iov_len = 1,
+ }, {
+ .iov_base = info,
+ .iov_len = infolen,
+ } };
+ int err = crypto_hkdf_generate(hkdf->hmac_tfm,
+ info_iov, ARRAY_SIZE(info_iov),
+ okm, okmlen);
- err = crypto_shash_init(desc);
- if (err)
- goto out;
-
- if (prev) {
- err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
- if (err)
- goto out;
- }
-
- err = crypto_shash_update(desc, prefix, sizeof(prefix));
- if (err)
- goto out;
-
- err = crypto_shash_update(desc, info, infolen);
- if (err)
- goto out;
-
- BUILD_BUG_ON(sizeof(counter) != 1);
- if (okmlen - i < HKDF_HASHLEN) {
- err = crypto_shash_finup(desc, &counter, 1, tmp);
- if (err)
- goto out;
- memcpy(&okm[i], tmp, okmlen - i);
- memzero_explicit(tmp, sizeof(tmp));
- } else {
- err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
- if (err)
- goto out;
- }
- counter++;
- prev = &okm[i];
- }
- err = 0;
-out:
if (unlikely(err))
memzero_explicit(okm, okmlen); /* so caller doesn't need to */
- shash_desc_zero(desc);Shouldn't crypto_hkdf_generate() handle the above memzero_explicit() of the output buffer on error, so that all callers don't need to do it? - Eric