Am Mittwoch, dem 06.01.2021 um 23:19 -0800 schrieb Eric Biggers:

On Mon, Jan 04, 2021 at 10:50:49PM +0100, Stephan Müller wrote:

quoted

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 KEYS

diff --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.

Unfortunately it is when adding the pointer to struct kvec

quoted

 
 /*

@@ -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.

Same here.

quoted

-/*
+ *
  * 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);

I wanted to have an identical interface for all types of KDFs to allow turning
them into a template eventually. For example, SP800-108 KDFs only have one
parameter. Hence the use of a kvec.

quoted

 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?

Yes, I will move it to HKDF (and the SP800-108 KDF as well).

Thanks for the review
Stephan

- Eric