[PATCH 0/5] Add KDF implementations to crypto API

[PATCH 0/5] Add KDF implementations to crypto API

[Stephan Müller]

Hi,

The key derviation functions are considered to be a cryptographic
operation. As cryptographic operations are provided via the kernel
crypto API, this patch set consolidates the KDF implementations into the
crypto API.

The KDF implementations are provided as service functions. Yet, the
interface to the two provided KDFs are identical with the goal to allow
them to be transformed into a crypto API template eventually.

The KDFs execute a power-on self test with test vectors from commonly
known sources.

Tbe SP800-108 KDF implementation is used to replace the implementation
in the keys subsystem. The implementation was verified using the
keyutils command line test code provided in
tests/keyctl/dh_compute/valid. All tests show that the expected values
are calculated with the new code.

The HKDF addition is used to replace the implementation in the filesystem
crypto extension. This code was tested by using an EXT4 encrypted file
system that was created and contains files written to by the current
implementation. Using the new implementation a successful read of the
existing files was possible and new files / directories were created
and read successfully. These newly added file system objects could be
successfully read using the current code. Yet if there is a test suite
to validate whether the invokcation of the HKDF calculates the same
result as the existing implementation, I would be happy to validate
the implementation accordingly.

Stephan Mueller (5):
  crypto: Add key derivation self-test support code
  crypto: add SP800-108 counter key derivation function
  crypto: add RFC5869 HKDF
  security: DH - use KDF implementation from crypto API
  fs: use HKDF implementation from kernel crypto API

 crypto/Kconfig                         |  14 ++
 crypto/Makefile                        |   6 +
 crypto/hkdf.c                          | 226 +++++++++++++++++++++++++
 crypto/kdf_sp800108.c                  | 149 ++++++++++++++++
 fs/crypto/Kconfig                      |   2 +-
 fs/crypto/fscrypt_private.h            |   4 +-
 fs/crypto/hkdf.c                       | 108 +++---------
 include/crypto/hkdf.h                  |  48 ++++++
 include/crypto/internal/kdf_selftest.h |  68 ++++++++
 include/crypto/kdf_sp800108.h          |  59 +++++++
 security/keys/Kconfig                  |   2 +-
 security/keys/dh.c                     | 118 ++-----------
 12 files changed, 617 insertions(+), 187 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 crypto/kdf_sp800108.c
 create mode 100644 include/crypto/hkdf.h
 create mode 100644 include/crypto/internal/kdf_selftest.h
 create mode 100644 include/crypto/kdf_sp800108.h

-- 
2.26.2

[PATCH 1/5] crypto: Add key derivation self-test support code

[Stephan Müller]

As a preparation to add the key derivation implementations, the
self-test data structure definition and the common test code is made
available.

The test framework follows the testing applied by the NIST CAVP test
approach.

The structure of the test code follows the implementations found in
crypto/testmgr.c|h. In case the KDF implementations will be made
available via a kernel crypto API templates, the test code is intended
to be merged into testmgr.c|h.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/crypto/internal/kdf_selftest.h | 68 ++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 include/crypto/internal/kdf_selftest.h

diff --git a/include/crypto/internal/kdf_selftest.h b/include/crypto/internal/kdf_selftest.h
new file mode 100644
index 000000000000..c4f80d2cc61c
--- /dev/null
+++ b/include/crypto/internal/kdf_selftest.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
+ */
+
+#ifndef _CRYPTO_KDF_SELFTEST_H
+#define _CRYPTO_KDF_SELFTEST_H
+
+#include <crypto/hash.h>
+#include <linux/uio.h>
+
+struct kdf_testvec {
+	struct kvec seed[2];
+	unsigned int seed_nvec;
+	struct kvec info;
+	unsigned char *expected;
+	size_t expectedlen;
+};
+
+static inline int
+kdf_test(const struct kdf_testvec *test, const char *name,
+	 int (*crypto_kdf_setkey)(struct crypto_shash *kmd,
+				  const struct kvec *seed,
+				  unsigned int seed_nvec),
+	 int (*crypto_kdf_generate)(struct crypto_shash *kmd,
+				    const struct kvec *info,
+				    unsigned int info_nvec,
+				    u8 *dst, unsigned int dlen))
+{
+	struct crypto_shash *kmd;
+	int ret;
+	u8 *buf = kzalloc(test->expectedlen, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	kmd = crypto_alloc_shash(name, 0, 0);
+	if (IS_ERR(kmd)) {
+		pr_err("alg: kdf: could not allocate cipher handle for %s\n",
+		       name);
+		kfree(buf);
+		return -ENOMEM;
+	}
+
+	ret = crypto_kdf_setkey(kmd, test->seed, test->seed_nvec);
+	if (ret) {
+		pr_err("alg: kdf: could not set key derivation key\n");
+		goto err;
+	}
+
+	ret = crypto_kdf_generate(kmd, &test->info, 1, buf, test->expectedlen);
+	if (ret) {
+		pr_err("alg: kdf: could not obtain key data\n");
+		goto err;
+	}
+
+	ret = memcmp(test->expected, buf, test->expectedlen);
+	if (ret)
+		ret = -EINVAL;
+
+err:
+	crypto_free_shash(kmd);
+	kfree(buf);
+	return ret;
+}
+
+#endif /* _CRYPTO_KDF_SELFTEST_H */
-- 
2.26.2

[PATCH 2/5] crypto: add SP800-108 counter key derivation function

[Stephan Müller]

SP800-108 defines three KDFs - this patch provides the counter KDF
implementation.

The KDF is implemented as a service function where the caller has to
maintain the hash / HMAC state. Apart from this hash/HMAC state, no
additional state is required to be maintained by either the caller or
the KDF implementation.

The key for the KDF is set with the crypto_kdf108_setkey function which
is intended to be invoked before the caller requests a key derivation
operation via crypto_kdf108_ctr_generate.

SP800-108 allows the use of either a HMAC or a hash as crypto primitive
for the KDF. When a HMAC cipher primtive is intended to be used,
crypto_kdf108_setkey must be used to set the HMAC key. Otherwise, for a
hash crypto primitve crypto_kdf108_ctr_generate can be used immediately
after allocating the cipher handle.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/Kconfig                |   7 ++
 crypto/Makefile               |   5 ++
 crypto/kdf_sp800108.c         | 149 ++++++++++++++++++++++++++++++++++
 include/crypto/kdf_sp800108.h |  59 ++++++++++++++
 4 files changed, 220 insertions(+)
 create mode 100644 crypto/kdf_sp800108.c
 create mode 100644 include/crypto/kdf_sp800108.h

diff --git a/crypto/Kconfig b/crypto/Kconfig
index a367fcfeb5d4..9f375c2350f5 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1862,6 +1862,13 @@ config CRYPTO_JITTERENTROPY
 	  random numbers. This Jitterentropy RNG registers with
 	  the kernel crypto API and can be used by any caller.
 
+config CRYPTO_KDF800108_CTR
+	tristate "Counter KDF (SP800-108)"
+	select CRYPTO_HASH
+	help
+	  Enable the key derivation function in counter mode compliant to
+	  SP800-108.
+
 config CRYPTO_USER_API
 	tristate
 
diff --git a/crypto/Makefile b/crypto/Makefile
index b279483fba50..46845a70850d 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -197,3 +197,8 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys/
 obj-$(CONFIG_CRYPTO_HASH_INFO) += hash_info.o
 crypto_simd-y := simd.o
 obj-$(CONFIG_CRYPTO_SIMD) += crypto_simd.o
+
+#
+# Key derivation function
+#
+obj-$(CONFIG_CRYPTO_KDF800108_CTR) += kdf_sp800108.o
diff --git a/crypto/kdf_sp800108.c b/crypto/kdf_sp800108.c
new file mode 100644
index 000000000000..325dbd9dba38
--- /dev/null
+++ b/crypto/kdf_sp800108.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * SP800-108 Key-derivation function
+ *
+ * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
+ */
+
+#include <linux/module.h>
+#include <crypto/kdf_sp800108.h>
+#include <crypto/internal/kdf_selftest.h>
+
+/*
+ * SP800-108 CTR KDF implementation
+ */
+int crypto_kdf108_ctr_generate(struct crypto_shash *kmd,
+			       const struct kvec *info, unsigned int info_nvec,
+			       u8 *dst, unsigned int dlen)
+{
+	SHASH_DESC_ON_STACK(desc, kmd);
+	__be32 counter = cpu_to_be32(1);
+	const unsigned int h = crypto_shash_digestsize(kmd);
+	unsigned int i;
+	int err = 0;
+	u8 *dst_orig = dst;
+
+	desc->tfm = kmd;
+
+	while (dlen) {
+		err = crypto_shash_init(desc);
+		if (err)
+			goto out;
+
+		err = crypto_shash_update(desc, (u8 *)&counter, sizeof(__be32));
+		if (err)
+			goto out;
+
+		for (i = 0; i < info_nvec; i++) {
+			err = crypto_shash_update(desc, info[i].iov_base,
+						  info[i].iov_len);
+			if (err)
+				goto out;
+		}
+
+		if (dlen < h) {
+			u8 tmpbuffer[HASH_MAX_DIGESTSIZE];
+
+			err = crypto_shash_final(desc, tmpbuffer);
+			if (err)
+				goto out;
+			memcpy(dst, tmpbuffer, dlen);
+			memzero_explicit(tmpbuffer, h);
+			goto out;
+		}
+
+		err = crypto_shash_final(desc, dst);
+		if (err)
+			goto out;
+
+		dlen -= h;
+		dst += h;
+		counter = cpu_to_be32(be32_to_cpu(counter) + 1);
+	}
+
+out:
+	if (err)
+		memzero_explicit(dst_orig, dlen);
+	shash_desc_zero(desc);
+	return err;
+}
+EXPORT_SYMBOL(crypto_kdf108_ctr_generate);
+
+/*
+ * The seeding of the KDF
+ */
+int crypto_kdf108_setkey(struct crypto_shash *kmd,
+			 const struct kvec *seed, unsigned int seed_nvec)
+{
+	unsigned int ds = crypto_shash_digestsize(kmd);
+
+	if (seed_nvec != 1)
+		return -EINVAL;
+
+	/* Check according to SP800-108 section 7.2 */
+	if (ds > seed[0].iov_len)
+		return -EINVAL;
+
+	/*
+	 * We require that we operate on a MAC -- if we do not operate on a
+	 * MAC, this function returns an error.
+	 */
+	return crypto_shash_setkey(kmd, seed[0].iov_base, seed[0].iov_len);
+}
+EXPORT_SYMBOL(crypto_kdf108_setkey);
+
+/*
+ * Test vector obtained from
+ * http://csrc.nist.gov/groups/STM/cavp/documents/KBKDF800-108/CounterMode.zip
+ */
+static const struct kdf_testvec kdf_ctr_hmac_sha256_tv_template[] = {
+	{
+		.seed = { {
+			/* K1 */
+			.iov_base = "\xdd\x1d\x91\xb7\xd9\x0b\x2b\xd3"
+				    "\x13\x85\x33\xce\x92\xb2\x72\xfb"
+				    "\xf8\xa3\x69\x31\x6a\xef\xe2\x42"
+				    "\xe6\x59\xcc\x0a\xe2\x38\xaf\xe0",
+			.iov_len  = 32,
+		} },
+		.seed_nvec	  = 1,
+		.info = {
+			.iov_base = "\x01\x32\x2b\x96\xb3\x0a\xcd\x19"
+				    "\x79\x79\x44\x4e\x46\x8e\x1c\x5c"
+				    "\x68\x59\xbf\x1b\x1c\xf9\x51\xb7"
+				    "\xe7\x25\x30\x3e\x23\x7e\x46\xb8"
+				    "\x64\xa1\x45\xfa\xb2\x5e\x51\x7b"
+				    "\x08\xf8\x68\x3d\x03\x15\xbb\x29"
+				    "\x11\xd8\x0a\x0e\x8a\xba\x17\xf3"
+				    "\xb4\x13\xfa\xac",
+			.iov_len  = 60
+		},
+		.expected	  = "\x10\x62\x13\x42\xbf\xb0\xfd\x40"
+				    "\x04\x6c\x0e\x29\xf2\xcf\xdb\xf0",
+		.expectedlen	  = 16
+	}
+};
+
+static int __init crypto_kdf108_init(void)
+{
+	int ret = kdf_test(&kdf_ctr_hmac_sha256_tv_template[0], "hmac(sha256)",
+			   crypto_kdf108_setkey, crypto_kdf108_ctr_generate);
+
+	if (ret)
+		pr_warn("alg: self-tests for CTR-KDF (hmac(sha256)) failed (rc=%d)\n",
+			ret);
+	else
+		pr_info("alg: self-tests for CTR-KDF (hmac(sha256)) passed\n");
+
+	return ret;
+}
+
+static void __exit crypto_kdf108_exit(void) { }
+
+module_init(crypto_kdf108_init);
+module_exit(crypto_kdf108_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Stephan Mueller <smueller@chronox.de>");
+MODULE_DESCRIPTION("Key Derivation Function conformant to SP800-108");
diff --git a/include/crypto/kdf_sp800108.h b/include/crypto/kdf_sp800108.h
new file mode 100644
index 000000000000..c13efe68bc7e
--- /dev/null
+++ b/include/crypto/kdf_sp800108.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
+ */
+
+#ifndef _CRYPTO_KDF108_H
+#define _CRYPTO_KDF108_H
+
+#include <crypto/hash.h>
+#include <linux/uio.h>
+
+/**
+ * Counter KDF generate operation according to SP800-108 section 5.1
+ * as well as SP800-56A section 5.8.1 (Single-step KDF).
+ *
+ * @kmd Keyed message digest whose key was set with crypto_kdf108_setkey or
+ *	unkeyed message digest
+ * @info optional context and application specific information - this may be
+ *	 NULL
+ * @info_vec number of optional context/application specific information entries
+ * @dst destination buffer that the caller already allocated
+ * @dlen length of the destination buffer - the KDF derives that amount of
+ *	 bytes.
+ *
+ * To comply with SP800-108, the caller must provide Label || 0x00 || Context
+ * in the info parameter.
+ *
+ * @return 0 on success, < 0 on error
+ */
+int crypto_kdf108_ctr_generate(struct crypto_shash *kmd,
+			       const struct kvec *info, unsigned int info_nvec,
+			       u8 *dst, unsigned int dlen);
+
+/**
+ * Counter KDF setkey operation
+ *
+ * @kmd Keyed message digest allocated by the caller. The key should not have
+ *	been set.
+ * @seed Seed key to be used to initialize the keyed message digest context.
+ *	 This kvec must contain exactly one entry pointing to the key value.
+ * @seed_nvec This value must be one.
+ *
+ * According to SP800-108 section 7.2, the seed key must be at least as large as
+ * the message digest size of the used keyed message digest. This limitation
+ * is enforced by the implementation.
+ *
+ * SP800-108 allows the use of either a HMAC or a hash cipher primitive. When
+ * the caller intends to use a hash cipher primitive, the call to
+ * crypto_kdf108_setkey is not required and the key derivation operation can
+ * immediately performed using crypto_kdf108_ctr_generate after allocating
+ * a cipher handle.
+ *
+ * @return 0 on success, < 0 on error
+ */
+int crypto_kdf108_setkey(struct crypto_shash *kmd,
+			 const struct kvec *seed, unsigned int seed_nvec);
+
+#endif /* _CRYPTO_KDF108_H */
-- 
2.26.2

[PATCH 3/5] crypto: add RFC5869 HKDF

[Stephan Müller]

RFC5869 specifies an extract and expand two-step key derivation
function. The HKDF implementation is provided as a service function that
operates on a caller-provided HMAC cipher handle. The caller has to
allocate the HMAC cipher and then can invoke the HKDF service functions.
The HKDF implementation ensures that the entire state is kept with the
HMAC cipher handle which implies that no additional state is required to
be maintained by the HKDF implementation.

The extract function is invoked via the crypto_hkdf_setkey call. RFC5869
allows two optional parameters to be provided to the extract operation:
the salt and additional information. Both are to be provided with the
seed parameter where the salt is the first entry of the seed parameter
and all subsequent entries are handled as additional information. If
the caller intends to invoke the HKDF without salt, it has to provide a
NULL/0 entry as first entry in seed.

The expand function is invoked via the crypto_hkdf_generate and can be
invoked multiple times. This function allows the caller to provide a
context for the key derivation operation. As specified in RFC5869, it is
optional. In case such context is not provided, the caller must provide
NULL / 0 for the info / info_nvec parameters.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/Kconfig        |   7 ++
 crypto/Makefile       |   1 +
 crypto/hkdf.c         | 226 ++++++++++++++++++++++++++++++++++++++++++
 include/crypto/hkdf.h |  48 +++++++++
 4 files changed, 282 insertions(+)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 9f375c2350f5..661287d7283b 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1862,6 +1862,13 @@ config CRYPTO_JITTERENTROPY
 	  random numbers. This Jitterentropy RNG registers with
 	  the kernel crypto API and can be used by any caller.
 
+config CRYPTO_HKDF
+	tristate "Extract and Expand HKDF (RFC 5869)"
+	select CRYPTO_HASH
+	help
+	  Enable the extract and expand key derivation function compliant
+	  to RFC 5869.
+
 config CRYPTO_KDF800108_CTR
 	tristate "Counter KDF (SP800-108)"
 	select CRYPTO_HASH
diff --git a/crypto/Makefile b/crypto/Makefile
index 46845a70850d..55a4d8c31a45 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -201,4 +201,5 @@ obj-$(CONFIG_CRYPTO_SIMD) += crypto_simd.o
 #
 # Key derivation function
 #
+obj-$(CONFIG_CRYPTO_HKDF) += hkdf.o
 obj-$(CONFIG_CRYPTO_KDF800108_CTR) += kdf_sp800108.o
diff --git a/crypto/hkdf.c b/crypto/hkdf.c
new file mode 100644
index 000000000000..a3bf6d6b07ea
--- /dev/null
+++ b/crypto/hkdf.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * HMAC-based Extract-and-Expand Key Derivation Function (conformant to RFC5869)
+ *
+ * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
+ */
+
+#include <linux/module.h>
+#include <crypto/hkdf.h>
+#include <crypto/internal/kdf_selftest.h>
+
+/*
+ * HKDF expand phase
+ */
+int crypto_hkdf_generate(struct crypto_shash *kmd,
+			 const struct kvec *info, unsigned int info_nvec,
+			 u8 *dst, unsigned int dlen)
+{
+	SHASH_DESC_ON_STACK(desc, kmd);
+	const unsigned int h = crypto_shash_digestsize(kmd);
+	unsigned int i;
+	int err = 0;
+	u8 *dst_orig = dst;
+	const u8 *prev = NULL;
+	u8 ctr = 0x01;
+
+	if (dlen > h * 255)
+		return -EINVAL;
+
+	desc->tfm = kmd;
+
+	/* T(1) and following */
+	while (dlen) {
+		err = crypto_shash_init(desc);
+		if (err)
+			goto out;
+
+		if (prev) {
+			err = crypto_shash_update(desc, prev, h);
+			if (err)
+				goto out;
+		}
+
+		for (i = 0; i < info_nvec; i++) {
+			err = crypto_shash_update(desc, info[i].iov_base,
+						  info[i].iov_len);
+			if (err)
+				goto out;
+		}
+
+		if (dlen < h) {
+			u8 tmpbuffer[HASH_MAX_DIGESTSIZE];
+
+			err = crypto_shash_finup(desc, &ctr, 1, tmpbuffer);
+			if (err)
+				goto out;
+			memcpy(dst, tmpbuffer, dlen);
+			memzero_explicit(tmpbuffer, h);
+			goto out;
+		}
+
+		err = crypto_shash_finup(desc, &ctr, 1, dst);
+		if (err)
+			goto out;
+
+		prev = dst;
+		dst += h;
+		dlen -= h;
+		ctr++;
+	}
+
+out:
+	if (err)
+		memzero_explicit(dst_orig, dlen);
+	shash_desc_zero(desc);
+	return err;
+}
+EXPORT_SYMBOL(crypto_hkdf_generate);
+
+/*
+ * HKDF extract phase.
+ */
+int crypto_hkdf_setkey(struct crypto_shash *kmd,
+		       const struct kvec *seed, unsigned int seed_nvec)
+{
+	SHASH_DESC_ON_STACK(desc, kmd);
+	unsigned int h = crypto_shash_digestsize(kmd);
+	int err;
+	static const u8 null_salt[HASH_MAX_DIGESTSIZE] = { 0 };
+	u8 prk[HASH_MAX_DIGESTSIZE];
+
+	/* Require at least the salt information */
+	if (seed_nvec < 1)
+		return -EINVAL;
+
+	desc->tfm = kmd;
+
+	if (seed[0].iov_len) {
+		/* Set the salt as HMAC key */
+		err = crypto_shash_setkey(kmd,
+					  seed[0].iov_base, seed[0].iov_len);
+	} else {
+		/* Set the default HMAC key as none was provided */
+		err = crypto_shash_setkey(kmd, null_salt, h);
+	}
+
+	if (err)
+		return err;
+
+	/* Extract the PRK */
+	if (seed_nvec > 1) {
+		unsigned int i;
+
+		err = crypto_shash_init(desc);
+		if (err)
+			goto err;
+
+		for (i = 1; i < seed_nvec; i++) {
+			err = crypto_shash_update(desc, seed[i].iov_base,
+						  seed[i].iov_len);
+			if (err)
+				goto err;
+		}
+
+		err = crypto_shash_final(desc, prk);
+	} else {
+		err = crypto_shash_digest(desc, NULL, 0, prk);
+	}
+	if (err)
+		goto err;
+
+	/* Set the PRK for the expand phase */
+	err = crypto_shash_setkey(kmd, prk, h);
+
+err:
+	shash_desc_zero(desc);
+	memzero_explicit(prk, h);
+	return err;
+}
+EXPORT_SYMBOL(crypto_hkdf_setkey);
+
+/* Test vectors from RFC 5869 appendix A */
+static const struct kdf_testvec hkdf_hmac_sha256_tv_template[] = {
+	{
+		.seed = { {
+			/* salt */
+			.iov_base = "\x00\x01\x02\x03\x04\x05\x06\x07"
+				    "\x08\x09\x0a\x0b\x0c",
+			.iov_len  = 13,
+		}, {
+			/* IKM */
+			.iov_base = "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
+				    "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
+				    "\x0b\x0b\x0b\x0b\x0b\x0b",
+			.iov_len  = 22,
+		} },
+		.seed_nvec	  = 2,
+		.info = {
+			.iov_base = "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7"
+				    "\xf8\xf9",
+			.iov_len  = 10
+		},
+		.expected	  = "\x3c\xb2\x5f\x25\xfa\xac\xd5\x7a"
+				    "\x90\x43\x4f\x64\xd0\x36\x2f\x2a"
+				    "\x2d\x2d\x0a\x90\xcf\x1a\x5a\x4c"
+				    "\x5d\xb0\x2d\x56\xec\xc4\xc5\xbf"
+				    "\x34\x00\x72\x08\xd5\xb8\x87\x18"
+				    "\x58\x65",
+		.expectedlen	  = 42
+	}, {
+		.seed = { {
+			/* salt */
+			.iov_base = NULL,
+			.iov_len  = 0,
+		}, {
+			/* IKM */
+			.iov_base = "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
+				    "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
+				    "\x0b\x0b\x0b\x0b\x0b\x0b",
+			.iov_len  = 22,
+		} },
+		.seed_nvec	  = 2,
+		.info = {
+			.iov_base = NULL,
+			.iov_len  = 0
+		},
+		.expected	  = "\x8d\xa4\xe7\x75\xa5\x63\xc1\x8f"
+				    "\x71\x5f\x80\x2a\x06\x3c\x5a\x31"
+				    "\xb8\xa1\x1f\x5c\x5e\xe1\x87\x9e"
+				    "\xc3\x45\x4e\x5f\x3c\x73\x8d\x2d"
+				    "\x9d\x20\x13\x95\xfa\xa4\xb6\x1a"
+				    "\x96\xc8",
+		.expectedlen	  = 42
+	}
+};
+
+static int __init crypto_hkdf_init(void)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(hkdf_hmac_sha256_tv_template); i++) {
+		ret = kdf_test(&hkdf_hmac_sha256_tv_template[i], "hmac(sha256)",
+			       crypto_hkdf_setkey, crypto_hkdf_generate);
+
+		if (ret) {
+			pr_warn("alg: self-tests for HKDF (hmac(sha256)) failed (rc=%d)\n",
+				ret);
+			return ret;
+		}
+	}
+
+	pr_info("alg: self-tests for HKDF (hmac(sha256)) passed\n");
+
+	return 0;
+}
+
+static void __exit crypto_hkdf_exit(void) { }
+
+module_init(crypto_hkdf_init);
+module_exit(crypto_hkdf_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Stephan Mueller <smueller@chronox.de>");
+MODULE_DESCRIPTION("HKDF HMAC-based Extract-and-Expand Key Derivation Function  (conformant to RFC5869)");
diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
new file mode 100644
index 000000000000..4e4b5444a7a6
--- /dev/null
+++ b/include/crypto/hkdf.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
+ */
+
+#ifndef _CRYPTO_HKDF_H
+#define _CRYPTO_HKDF_H
+
+#include <crypto/hash.h>
+#include <linux/uio.h>
+
+/**
+ * RFC 5869 HKDF expand operation
+ *
+ * @kmd Keyed message digest whose key was set with crypto_hkdf_setkey
+ * @info optional context and application specific information - this may be
+ *	 NULL
+ * @info_vec number of optional context/application specific information entries
+ * @dst destination buffer that the caller already allocated
+ * @dlen length of the destination buffer - the KDF derives that amount of
+ *	 bytes.
+ *
+ * @return 0 on success, < 0 on error
+ */
+int crypto_hkdf_generate(struct crypto_shash *kmd,
+			 const struct kvec *info, unsigned int info_nvec,
+			 u8 *dst, unsigned int dlen);
+
+/**
+ * RFC 5869 HKDF extract operation
+ *
+ * @kmd Keyed message digest allocated by the caller. The key should not have
+ *	been set.
+ * @seed Seed data to extract the HKDF state from - for HKDF, the seed is
+ *	 partitioned as follows:
+ *	 seed[0] is the salt - this kvec MUST be filled in. It is permissible
+ *	 to provide a NULL seed where .iov_base = NULL and .iov_len = 0.
+ *	 seed[>0] is the additional information data. It is permissible to
+ *	 provide zero or more kvec entries.
+ * @seed_nvec number of seed entries (must be at least 1)
+ *
+ * @return 0 on success, < 0 on error
+ */
+int crypto_hkdf_setkey(struct crypto_shash *kmd,
+		       const struct kvec *seed, unsigned int seed_nvec);
+
+#endif /* _CRYPTO_HKDF_H */
-- 
2.26.2

[PATCH 4/5] security: DH - use KDF implementation from crypto API

[Stephan Müller]

The kernel crypto API provides the SP800-108 counter KDF implementation.
Thus, the separate implementation provided as part of the keys subsystem
can be replaced with calls to the KDF offered by the kernel crypto API.

The keys subsystem uses the counter KDF with a hash cipher primitive.
Thus, it only uses the call to crypto_kdf108_ctr_generate.

The change removes the specific code that adds a zero padding that was
intended to be invoked when the DH operation result was smaller than the
modulus. However, this cannot occur any more these days because the
function mpi_write_to_sgl is used in the code path that calculates the
shared secret in dh_compute_value. This MPI service function guarantees
that leading zeros are introduced as needed to ensure the resulting data
is exactly as long as the modulus. This implies that the specific code
to add zero padding is dead code which can be safely removed.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 security/keys/Kconfig |   2 +-
 security/keys/dh.c    | 118 ++++++------------------------------------
 2 files changed, 17 insertions(+), 103 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 83bc23409164..e6604499f0a8 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -106,7 +106,7 @@ config KEY_DH_OPERATIONS
        bool "Diffie-Hellman operations on retained keys"
        depends on KEYS
        select CRYPTO
-       select CRYPTO_HASH
+       select CRYPTO_KDF800108_CTR
        select CRYPTO_DH
        help
 	 This option provides support for calculating Diffie-Hellman
diff --git a/security/keys/dh.c b/security/keys/dh.c
index 1abfa70ed6e1..46fa442b81ec 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -11,6 +11,7 @@
 #include <crypto/hash.h>
 #include <crypto/kpp.h>
 #include <crypto/dh.h>
+#include <crypto/kdf_sp800108.h>
 #include <keys/user-type.h>
 #include "internal.h"
 
@@ -79,16 +80,9 @@ static void dh_crypto_done(struct crypto_async_request *req, int err)
 	complete(&compl->completion);
 }
 
-struct kdf_sdesc {
-	struct shash_desc shash;
-	char ctx[];
-};
-
-static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
+static int kdf_alloc(struct crypto_shash **hash, char *hashname)
 {
 	struct crypto_shash *tfm;
-	struct kdf_sdesc *sdesc;
-	int size;
 	int err;
 
 	/* allocate synchronous hash */
@@ -102,14 +96,7 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
 	if (crypto_shash_digestsize(tfm) == 0)
 		goto out_free_tfm;
 
-	err = -ENOMEM;
-	size = sizeof(struct shash_desc) + crypto_shash_descsize(tfm);
-	sdesc = kmalloc(size, GFP_KERNEL);
-	if (!sdesc)
-		goto out_free_tfm;
-	sdesc->shash.tfm = tfm;
-
-	*sdesc_ret = sdesc;
+	*hash = tfm;
 
 	return 0;
 
@@ -118,92 +105,20 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
 	return err;
 }
 
-static void kdf_dealloc(struct kdf_sdesc *sdesc)
-{
-	if (!sdesc)
-		return;
-
-	if (sdesc->shash.tfm)
-		crypto_free_shash(sdesc->shash.tfm);
-
-	kfree_sensitive(sdesc);
-}
-
-/*
- * Implementation of the KDF in counter mode according to SP800-108 section 5.1
- * as well as SP800-56A section 5.8.1 (Single-step KDF).
- *
- * SP800-56A:
- * The src pointer is defined as Z || other info where Z is the shared secret
- * from DH and other info is an arbitrary string (see SP800-56A section
- * 5.8.1.2).
- *
- * 'dlen' must be a multiple of the digest size.
- */
-static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
-		   u8 *dst, unsigned int dlen, unsigned int zlen)
+static void kdf_dealloc(struct crypto_shash *hash)
 {
-	struct shash_desc *desc = &sdesc->shash;
-	unsigned int h = crypto_shash_digestsize(desc->tfm);
-	int err = 0;
-	u8 *dst_orig = dst;
-	__be32 counter = cpu_to_be32(1);
-
-	while (dlen) {
-		err = crypto_shash_init(desc);
-		if (err)
-			goto err;
-
-		err = crypto_shash_update(desc, (u8 *)&counter, sizeof(__be32));
-		if (err)
-			goto err;
-
-		if (zlen && h) {
-			u8 tmpbuffer[32];
-			size_t chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
-			memset(tmpbuffer, 0, chunk);
-
-			do {
-				err = crypto_shash_update(desc, tmpbuffer,
-							  chunk);
-				if (err)
-					goto err;
-
-				zlen -= chunk;
-				chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
-			} while (zlen);
-		}
-
-		if (src && slen) {
-			err = crypto_shash_update(desc, src, slen);
-			if (err)
-				goto err;
-		}
-
-		err = crypto_shash_final(desc, dst);
-		if (err)
-			goto err;
-
-		dlen -= h;
-		dst += h;
-		counter = cpu_to_be32(be32_to_cpu(counter) + 1);
-	}
-
-	return 0;
-
-err:
-	memzero_explicit(dst_orig, dlen);
-	return err;
+	if (hash)
+		crypto_free_shash(hash);
 }
 
-static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
+static int keyctl_dh_compute_kdf(struct crypto_shash *hash,
 				 char __user *buffer, size_t buflen,
-				 uint8_t *kbuf, size_t kbuflen, size_t lzero)
+				 uint8_t *kbuf, size_t kbuflen)
 {
+	struct kvec kbuf_iov = { .iov_base = kbuf, .iov_len = kbuflen };
 	uint8_t *outbuf = NULL;
 	int ret;
-	size_t outbuf_len = roundup(buflen,
-				    crypto_shash_digestsize(sdesc->shash.tfm));
+	size_t outbuf_len = roundup(buflen, crypto_shash_digestsize(hash));
 
 	outbuf = kmalloc(outbuf_len, GFP_KERNEL);
 	if (!outbuf) {
@@ -211,7 +126,7 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
 		goto err;
 	}
 
-	ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len, lzero);
+	ret = crypto_kdf108_ctr_generate(hash, &kbuf_iov, 1, outbuf, outbuf_len);
 	if (ret)
 		goto err;
 
@@ -240,7 +155,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
 	struct kpp_request *req;
 	uint8_t *secret;
 	uint8_t *outbuf;
-	struct kdf_sdesc *sdesc = NULL;
+	struct crypto_shash *hash = NULL;
 
 	if (!params || (!buffer && buflen)) {
 		ret = -EINVAL;
@@ -273,7 +188,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
 		}
 
 		/* allocate KDF from the kernel crypto API */
-		ret = kdf_alloc(&sdesc, hashname);
+		ret = kdf_alloc(&hash, hashname);
 		kfree(hashname);
 		if (ret)
 			goto out1;
@@ -383,9 +298,8 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
 			goto out6;
 		}
 
-		ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, outbuf,
-					    req->dst_len + kdfcopy->otherinfolen,
-					    outlen - req->dst_len);
+		ret = keyctl_dh_compute_kdf(hash, buffer, buflen, outbuf,
+					    req->dst_len + kdfcopy->otherinfolen);
 	} else if (copy_to_user(buffer, outbuf, req->dst_len) == 0) {
 		ret = req->dst_len;
 	} else {
@@ -403,7 +317,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
 out2:
 	dh_free_data(&dh_inputs);
 out1:
-	kdf_dealloc(sdesc);
+	kdf_dealloc(hash);
 	return ret;
 }
 
-- 
2.26.2

[PATCH 5/5] fs: use HKDF implementation from kernel crypto API

[Stephan Müller]

As the kernel crypto API implements HKDF, replace the
file-system-specific HKDF implementation with the generic HKDF
implementation.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 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);
 
 /*
@@ -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);
 
 void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf);
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index e0ec21055505..f837cb8ec0a5 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -9,7 +9,7 @@
  * Copyright 2019 Google LLC
  */
 
-#include <crypto/hash.h>
+#include <crypto/hkdf.h>
 #include <crypto/sha2.h>
 
 #include "fscrypt_private.h"
@@ -37,34 +37,25 @@
  * unnecessarily long master keys.  Thus fscrypt still does HKDF-Extract.  No
  * salt is used, since fscrypt master keys should already be pseudorandom and
  * there's no way to persist a random salt per master key from kernel mode.
- */
-
-/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
-static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
-			unsigned int ikmlen, u8 prk[HKDF_HASHLEN])
-{
-	static const u8 default_salt[HKDF_HASHLEN];
-	int err;
-
-	err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN);
-	if (err)
-		return err;
-
-	return crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
-}
-
-/*
+ *
  * 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;
 
@@ -93,7 +80,6 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
 err_free_tfm:
 	crypto_free_shash(hmac_tfm);
 out:
-	memzero_explicit(prk, sizeof(prk));
 	return err;
 }
 
@@ -109,65 +95,25 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
  * accidentally repeat an info string when using HKDF for different purposes.)
  */
 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);
 	return err;
 }
 
-- 
2.26.2

Re: [PATCH 0/5] Add KDF implementations to crypto API

[Eric Biggers]

On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote:
> The HKDF addition is used to replace the implementation in the filesystem
> crypto extension. This code was tested by using an EXT4 encrypted file
> system that was created and contains files written to by the current
> implementation. Using the new implementation a successful read of the
> existing files was possible and new files / directories were created
> and read successfully. These newly added file system objects could be
> successfully read using the current code. Yet if there is a test suite
> to validate whether the invokcation of the HKDF calculates the same
> result as the existing implementation, I would be happy to validate
> the implementation accordingly.

See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests
for how to run the fscrypt tests.  'kvm-xfstests -c ext4 generic/582' should be
enough for this, though you could run all the tests if you want.

- Eric

Re: [PATCH 0/5] Add KDF implementations to crypto API

[Stephan Mueller]

Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers:
> On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote:
> > The HKDF addition is used to replace the implementation in the filesystem
> > crypto extension. This code was tested by using an EXT4 encrypted file
> > system that was created and contains files written to by the current
> > implementation. Using the new implementation a successful read of the
> > existing files was possible and new files / directories were created
> > and read successfully. These newly added file system objects could be
> > successfully read using the current code. Yet if there is a test suite
> > to validate whether the invokcation of the HKDF calculates the same
> > result as the existing implementation, I would be happy to validate
> > the implementation accordingly.
> 
> See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests
> for how to run the fscrypt tests.  'kvm-xfstests -c ext4 generic/582' should
> be
> enough for this, though you could run all the tests if you want.

I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my
HKDF changes. I.e. the testing shows the same results for both kernels which
seems to imply that my HKDF changes do not change the behavior.

I get the following errors in both occasions - let me know if I should dig a
bit more.


[failed, exit status 1] [06:19:21]- output mismatch (see
/results/ext4/results-encrypt/ext4/023.out.bad)
    --- tests/ext4/023.out      2020-03-20 02:31:32.000000000 +0000
    +++ /results/ext4/results-encrypt/ext4/023.out.bad  2021-01-07
06:19:21.292339438 +0000
    @@ -1,3 +1,2 @@
     QA output created by 023
     Format and populate
    -Mount
    ...
    (Run 'diff -u /root/xfstests/tests/ext4/023.out /results/ext4/results-
encrypt/ext4/023.out.bad'  to see the entire )

[failed, exit status 1] [06:19:28]- output mismatch (see
/results/ext4/results-encrypt/ext4/028.out.bad)
    --- tests/ext4/028.out      2020-03-20 02:31:32.000000000 +0000
    +++ /results/ext4/results-encrypt/ext4/028.out.bad  2021-01-07
06:19:28.762339424 +0000
    @@ -1,3 +1,2 @@
     QA output created by 028
     Format and mount
    -Compare fsmap
    ...
    (Run 'diff -u /root/xfstests/tests/ext4/028.out /results/ext4/results-
encrypt/ext4/028.out.bad'  to see the entire )

[failed, exit status 1] [06:21:02]- output mismatch (see
/results/ext4/results-encrypt/ext4/044.out.bad)
    --- tests/ext4/044.out      2020-03-20 02:31:32.000000000 +0000
    +++ /results/ext4/results-encrypt/ext4/044.out.bad  2021-01-07
06:21:02.215672727 +0000
    @@ -1,2 +1,5 @@
     QA output created by 044
     Silence is golden
    +mount: /vdc: wrong fs type, bad option, bad superblock on /dev/vdc,
missing codepage or helper program, or other e.
    +ext3 mount failed
    +(see /results/ext4/results-encrypt/ext4/044.full for details)
    ...
    (Run 'diff -u /root/xfstests/tests/ext4/044.out /results/ext4/results-
encrypt/ext4/044.out.bad'  to see the entire )


generic/085             [06:32:40][  849.654788] run fstests generic/085 at
2021-01-07 06:32:40
[  849.903286] EXT4-fs (vdd): Test dummy encryption mode enabled
[  849.915355] EXT4-fs (vdd): mounted filesystem with ordered data mode. Opts:
acl,user_xattr,block_validity,test_dummy.
[  850.267282] dm-0: detected capacity change from 524288 to 0
[  850.369101] EXT4-fs (dm-0): mounted filesystem with ordered data mode.
Opts: (null). Quota mode: none.
[  850.370106] ext4 filesystem being mounted at /vdc supports timestamps until
2038 (0x7fffffff)
[  850.479981] EXT4-fs (dm-0): mounted filesystem with ordered data mode.
Opts: (null). Quota mode: none.
[  850.480782] ext4 filesystem being mounted at /vdc supports timestamps until
2038 (0x7fffffff)
[  850.530734] BUG: kernel NULL pointer dereference, address: 0000000000000058
[  850.531241] #PF: supervisor read access in kernel mode
[  850.531613] #PF: error_code(0x0000) - not-present page
[  850.532020] PGD 2a496067 P4D 2a496067 PUD 0 
[  850.532336] Oops: 0000 [#1] SMP NOPTI
[  850.532604] CPU: 1 PID: 19542 Comm: dmsetup Not tainted 5.11.0-rc2-xfstests
#8
[  850.533156] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.14.0-1.fc33 04/01/2014
[  850.533780] RIP: 0010:thaw_bdev+0x47/0x90
[  850.534106] Code: 8b 83 d8 04 00 00 85 c0 74 57 83 e8 01 45 31 e4 85 c0 89
83 d8 04 00 00 7f 2d 48 8b bb 80 05 00 007
[  850.535447] RSP: 0018:ffffb97586c2bcd8 EFLAGS: 00010286
[  850.535822] RAX: 0000000000000000 RBX: ffff9df4a2e74240 RCX:
ffffb97586c2bbdc
[  850.536361] RDX: ffff9df4fdc17e80 RSI: ffff9df4a2e74790 RDI:
ffff9df48b0bf000
[  850.536864] RBP: ffff9df4a2e74720 R08: 0000000000000000 R09:
0000000000040216
[  850.537410] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[  850.537950] R13: 0000000000000000 R14: 0000000000000006 R15:
0000000000000001
[  850.538455] FS:  0000000000000000(0000) GS:ffff9df4fdc00000(0063)
knlGS:00000000f7a487c0
[  850.539063] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[  850.539474] CR2: 0000000000000058 CR3: 00000000072ec002 CR4:
0000000000770ee0
[  850.540017] PKRU: 55555554
[  850.540215] Call Trace:
[  850.540395]  __dm_resume+0x70/0x90
[  850.540649]  dm_resume+0x10d/0x120
[  850.540934]  do_resume+0x17b/0x210
[  850.541182]  ctl_ioctl+0x163/0x260
[  850.541428]  ? dev_set_geometry+0x180/0x180
[  850.541730]  dm_compat_ctl_ioctl+0xc/0x10
[  850.542056]  __do_compat_sys_ioctl+0x137/0x160
[  850.542375]  __do_fast_syscall_32+0x5c/0x90
[  850.542682]  do_fast_syscall_32+0x2f/0x70
[  850.543007]  entry_SYSENTER_compat_after_hwframe+0x4d/0x5f
[  850.543404] RIP: 0023:0xf7f05549
[  850.543640] Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8
01 00 00 00 00 00 00 00 00 00 00 00 00 000
[  850.545029] RSP: 002b:00000000ffaed778 EFLAGS: 00000296 ORIG_RAX:
0000000000000036
[  850.545566] RAX: ffffffffffffffda RBX: 0000000000000003 RCX:
00000000c138fd06
[  850.546107] RDX: 00000000573ca4f0 RSI: 00000000f7eea266 RDI:
00000000f7eea266
[  850.546613] RBP: 00000000ffaed828 R08: 0000000000000000 R09:
0000000000000000
[  850.547154] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[  850.547651] R13: 0000000000000000 R14: 0000000000000000 R15:
0000000000000000
[  850.548191] CR2: 0000000000000058
[  850.548431] ---[ end trace ec81eda86d2573e7 ]---
[  850.548761] RIP: 0010:thaw_bdev+0x47/0x90
[  850.549089] Code: 8b 83 d8 04 00 00 85 c0 74 57 83 e8 01 45 31 e4 85 c0 89
83 d8 04 00 00 7f 2d 48 8b bb 80 05 00 007
[  850.550444] RSP: 0018:ffffb97586c2bcd8 EFLAGS: 00010286
[  850.550816] RAX: 0000000000000000 RBX: ffff9df4a2e74240 RCX:
ffffb97586c2bbdc
[  850.551359] RDX: ffff9df4fdc17e80 RSI: ffff9df4a2e74790 RDI:
ffff9df48b0bf000
[  850.551863] RBP: ffff9df4a2e74720 R08: 0000000000000000 R09:
0000000000040216
[  850.552405] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[  850.552944] R13: 0000000000000000 R14: 0000000000000006 R15:
0000000000000001
[  850.553450] FS:  0000000000000000(0000) GS:ffff9df4fdc00000(0063)
knlGS:00000000f7a487c0
[  850.554060] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[  850.554466] CR2: 0000000000000058 CR3: 00000000072ec002 CR4:
0000000000770ee0
[  850.555008] PKRU: 55555554
[  850.555206] BUG: sleeping function called from invalid context at
include/linux/percpu-rwsem.h:49
[  850.555833] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 19542,
name: dmsetup
[  850.556436] INFO: lockdep is turned off.
[  850.556718] irq event stamp: 8602
[  850.556997] hardirqs last  enabled at (8601): [<ffffffff95cd4ed6>]
queue_work_on+0x56/0x70
[  850.557588] hardirqs last disabled at (8602): [<ffffffff969453c5>]
exc_page_fault+0x35/0x240
[  850.558220] softirqs last  enabled at (8568): [<ffffffff964a651a>]
__rhashtable_insert_fast.constprop.0+0x2ca/0x540
[  850.558997] softirqs last disabled at (8566): [<ffffffff964a6317>]
__rhashtable_insert_fast.constprop.0+0xc7/0x540
[  850.559729] CPU: 1 PID: 19542 Comm: dmsetup Tainted: G      D          
5.11.0-rc2-xfstests #8
[  850.560373] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.14.0-1.fc33 04/01/2014
[  850.561030] Call Trace:
[  850.561211]  dump_stack+0x77/0x97
[  850.561454]  ___might_sleep.cold+0xa6/0xb6
[  850.561753]  exit_signals+0x1c/0x2c0
[  850.562048]  do_exit+0xb4/0x3d0
[  850.562278]  rewind_stack_do_exit+0x17/0x20
[  850.562578] RIP: 0023:0xf7f05549
[  850.562818] Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8
01 00 00 00 00 00 00 00 00 00 00 00 00 000
[  850.564185] RSP: 002b:00000000ffaed778 EFLAGS: 00000296 ORIG_RAX:
0000000000000036
[  850.564716] RAX: ffffffffffffffda RBX: 0000000000000003 RCX:
00000000c138fd06
[  850.565248] RDX: 00000000573ca4f0 RSI: 00000000f7eea266 RDI:
00000000f7eea266
[  850.565747] RBP: 00000000ffaed828 R08: 0000000000000000 R09:
0000000000000000
[  850.566280] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[  850.566779] R13: 0000000000000000 R14: 0000000000000000 R15:
0000000000000000

Ciao
Stephan
> 
> - Eric

Re: [PATCH 0/5] Add KDF implementations to crypto API

[Eric Biggers]

On Thu, Jan 07, 2021 at 07:37:05AM +0100, Stephan Mueller wrote:
> Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers:
> > On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote:
> > > The HKDF addition is used to replace the implementation in the filesystem
> > > crypto extension. This code was tested by using an EXT4 encrypted file
> > > system that was created and contains files written to by the current
> > > implementation. Using the new implementation a successful read of the
> > > existing files was possible and new files / directories were created
> > > and read successfully. These newly added file system objects could be
> > > successfully read using the current code. Yet if there is a test suite
> > > to validate whether the invokcation of the HKDF calculates the same
> > > result as the existing implementation, I would be happy to validate
> > > the implementation accordingly.
> > 
> > See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests
> > for how to run the fscrypt tests.  'kvm-xfstests -c ext4 generic/582' should
> > be
> > enough for this, though you could run all the tests if you want.
> 
> I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my
> HKDF changes. I.e. the testing shows the same results for both kernels which
> seems to imply that my HKDF changes do not change the behavior.
> 
> I get the following errors in both occasions - let me know if I should dig a
> bit more.

The command you ran runs almost all xfstests with the test_dummy_encryption
mount option enabled, which is different from running the encryption tests --
and in fact it skips the real encryption tests, so it doesn't test the
correctness of HKDF at all.  It looks like you saw some unrelated test failures.
Sorry if I wasn't clear -- by "all tests" I meant all encryption tests, i.e.
'kvm-xfstests -c ext4 -g encrypt'.  Also, even the single test generic/582
should be sufficient to test HKDF, as I mentioned.

- Eric

Re: [PATCH 0/5] Add KDF implementations to crypto API

[Eric Biggers]

On Wed, Jan 06, 2021 at 10:59:24PM -0800, Eric Biggers wrote:
> On Thu, Jan 07, 2021 at 07:37:05AM +0100, Stephan Mueller wrote:
> > Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers:
> > > On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote:
> > > > The HKDF addition is used to replace the implementation in the filesystem
> > > > crypto extension. This code was tested by using an EXT4 encrypted file
> > > > system that was created and contains files written to by the current
> > > > implementation. Using the new implementation a successful read of the
> > > > existing files was possible and new files / directories were created
> > > > and read successfully. These newly added file system objects could be
> > > > successfully read using the current code. Yet if there is a test suite
> > > > to validate whether the invokcation of the HKDF calculates the same
> > > > result as the existing implementation, I would be happy to validate
> > > > the implementation accordingly.
> > > 
> > > See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests
> > > for how to run the fscrypt tests.  'kvm-xfstests -c ext4 generic/582' should
> > > be
> > > enough for this, though you could run all the tests if you want.
> > 
> > I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my
> > HKDF changes. I.e. the testing shows the same results for both kernels which
> > seems to imply that my HKDF changes do not change the behavior.
> > 
> > I get the following errors in both occasions - let me know if I should dig a
> > bit more.
> 
> The command you ran runs almost all xfstests with the test_dummy_encryption
> mount option enabled, which is different from running the encryption tests --
> and in fact it skips the real encryption tests, so it doesn't test the
> correctness of HKDF at all.  It looks like you saw some unrelated test failures.
> Sorry if I wasn't clear -- by "all tests" I meant all encryption tests, i.e.
> 'kvm-xfstests -c ext4 -g encrypt'.  Also, even the single test generic/582
> should be sufficient to test HKDF, as I mentioned.
> 

I just did it myself and the tests pass.

- Eric

Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API

[Eric Biggers]

On Mon, Jan 04, 2021 at 10:50:49PM +0100, Stephan Müller wrote:
> As the kernel crypto API implements HKDF, replace the
> file-system-specific HKDF implementation with the generic HKDF
> implementation.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  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.

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

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

Re: [PATCH 3/5] crypto: add RFC5869 HKDF

[Eric Biggers]

On Mon, Jan 04, 2021 at 10:49:13PM +0100, Stephan Müller wrote:
> RFC5869 specifies an extract and expand two-step key derivation
> function. The HKDF implementation is provided as a service function that
> operates on a caller-provided HMAC cipher handle.

HMAC isn't a "cipher".

> The extract function is invoked via the crypto_hkdf_setkey call.

Any reason not to call this crypto_hkdf_extract(), to match the specification?

> RFC5869
> allows two optional parameters to be provided to the extract operation:
> the salt and additional information. Both are to be provided with the
> seed parameter where the salt is the first entry of the seed parameter
> and all subsequent entries are handled as additional information. If
> the caller intends to invoke the HKDF without salt, it has to provide a
> NULL/0 entry as first entry in seed.

Where does "additional information" for extract come from?  RFC 5869 has:

	HKDF-Extract(salt, IKM) -> PRK

	Inputs:
	      salt     optional salt value (a non-secret random value);
		       if not provided, it is set to a string of HashLen zeros.
	      IKM      input keying material

There's no "additional information".

> 
> The expand function is invoked via the crypto_hkdf_generate and can be
> invoked multiple times. This function allows the caller to provide a
> context for the key derivation operation. As specified in RFC5869, it is
> optional. In case such context is not provided, the caller must provide
> NULL / 0 for the info / info_nvec parameters.

Any reason not to call this crypto_hkdf_expand() to match the specification?

- Eric

Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API

[Stephan Mueller]

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:
> > As the kernel crypto API implements HKDF, replace the
> > file-system-specific HKDF implementation with the generic HKDF
> > implementation.
> > 
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> >  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
> 
> >  
> >  /*
> > @@ -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.
> 
> > -/*
> > + *
> >   * 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.

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

Re: [PATCH 3/5] crypto: add RFC5869 HKDF

[Stephan Mueller]

Am Mittwoch, dem 06.01.2021 um 23:30 -0800 schrieb Eric Biggers:
> On Mon, Jan 04, 2021 at 10:49:13PM +0100, Stephan Müller wrote:
> > RFC5869 specifies an extract and expand two-step key derivation
> > function. The HKDF implementation is provided as a service function that
> > operates on a caller-provided HMAC cipher handle.
> 
> HMAC isn't a "cipher".
> 
> > The extract function is invoked via the crypto_hkdf_setkey call.
> 
> Any reason not to call this crypto_hkdf_extract(), to match the
> specification?

I named it to match the other KDF implementation. But you are right, I will
name it accordingly.

> 
> > RFC5869
> > allows two optional parameters to be provided to the extract operation:
> > the salt and additional information. Both are to be provided with the
> > seed parameter where the salt is the first entry of the seed parameter
> > and all subsequent entries are handled as additional information. If
> > the caller intends to invoke the HKDF without salt, it has to provide a
> > NULL/0 entry as first entry in seed.
> 
> Where does "additional information" for extract come from?  RFC 5869 has:
> 
>         HKDF-Extract(salt, IKM) -> PRK
> 
>         Inputs:
>               salt     optional salt value (a non-secret random value);
>                        if not provided, it is set to a string of HashLen
> zeros.
>               IKM      input keying material
> 
> There's no "additional information".

I used the terminology from SP800-108. I will update the description
accordingly. 
> 
> > 
> > The expand function is invoked via the crypto_hkdf_generate and can be
> > invoked multiple times. This function allows the caller to provide a
> > context for the key derivation operation. As specified in RFC5869, it is
> > optional. In case such context is not provided, the caller must provide
> > NULL / 0 for the info / info_nvec parameters.
> 
> Any reason not to call this crypto_hkdf_expand() to match the specification?

I will update the function name.

Thanks
Stephan
> 
> - Eric

Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API

[Eric Biggers]

On Thu, Jan 07, 2021 at 08:49:52AM +0100, Stephan Mueller wrote:
> > > -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
> > 
> > >  
> > >  /*
> > > @@ -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.

If the HKDF API will have a quirk like this, it's better not to "leak" it into
the prototypes of these fscrypt functions.  Just add the needed casts in
fscrypt_init_hkdf() and fscrypt_hkdf_expand().

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

But the API being provided is a library function specifically for HKDF.
So there's no need to make it conform to some other API.

- Eric

Re: [PATCH 3/5] crypto: add RFC5869 HKDF

[Eric Biggers]

On Thu, Jan 07, 2021 at 08:53:15AM +0100, Stephan Mueller wrote:
> > 
> > > RFC5869
> > > allows two optional parameters to be provided to the extract operation:
> > > the salt and additional information. Both are to be provided with the
> > > seed parameter where the salt is the first entry of the seed parameter
> > > and all subsequent entries are handled as additional information. If
> > > the caller intends to invoke the HKDF without salt, it has to provide a
> > > NULL/0 entry as first entry in seed.
> > 
> > Where does "additional information" for extract come from?  RFC 5869 has:
> > 
> >         HKDF-Extract(salt, IKM) -> PRK
> > 
> >         Inputs:
> >               salt     optional salt value (a non-secret random value);
> >                        if not provided, it is set to a string of HashLen
> > zeros.
> >               IKM      input keying material
> > 
> > There's no "additional information".
> 
> I used the terminology from SP800-108. I will update the description
> accordingly. 

For HKDF, it would be better to stick to the terminology used in RFC 5869
(https://tools.ietf.org/html/rfc5869), as generally that's what people are most
familiar with for HKDF.  It also matches the HKDF paper
(https://eprint.iacr.org/2010/264.pdf) more closely.

- Eric

Re: [PATCH 4/5] security: DH - use KDF implementation from crypto API

[Jarkko Sakkinen]

On Mon, 2021-01-04 at 22:49 +0100, Stephan Müller wrote:
> The kernel crypto API provides the SP800-108 counter KDF implementation.
> Thus, the separate implementation provided as part of the keys subsystem
> can be replaced with calls to the KDF offered by the kernel crypto API.
> 
> The keys subsystem uses the counter KDF with a hash cipher primitive.
> Thus, it only uses the call to crypto_kdf108_ctr_generate.
> 
> The change removes the specific code that adds a zero padding that was
> intended to be invoked when the DH operation result was smaller than the
> modulus. However, this cannot occur any more these days because the
> function mpi_write_to_sgl is used in the code path that calculates the
> shared secret in dh_compute_value. This MPI service function guarantees
> that leading zeros are introduced as needed to ensure the resulting data
> is exactly as long as the modulus. This implies that the specific code
> to add zero padding is dead code which can be safely removed.

Should be thn split into two patches, i.e. prepended with a patch
removing the dead code.

/Jarkko

> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  security/keys/Kconfig |   2 +-
>  security/keys/dh.c    | 118 ++++++------------------------------------
>  2 files changed, 17 insertions(+), 103 deletions(-)
> 
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 83bc23409164..e6604499f0a8 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -106,7 +106,7 @@ config KEY_DH_OPERATIONS
>         bool "Diffie-Hellman operations on retained keys"
>         depends on KEYS
>         select CRYPTO
> -       select CRYPTO_HASH
> +       select CRYPTO_KDF800108_CTR
>         select CRYPTO_DH
>         help
>          This option provides support for calculating Diffie-Hellman
> diff --git a/security/keys/dh.c b/security/keys/dh.c
> index 1abfa70ed6e1..46fa442b81ec 100644
> --- a/security/keys/dh.c
> +++ b/security/keys/dh.c
> @@ -11,6 +11,7 @@
>  #include <crypto/hash.h>
>  #include <crypto/kpp.h>
>  #include <crypto/dh.h>
> +#include <crypto/kdf_sp800108.h>
>  #include <keys/user-type.h>
>  #include "internal.h"
>  
> @@ -79,16 +80,9 @@ static void dh_crypto_done(struct crypto_async_request *req, int err)
>         complete(&compl->completion);
>  }
>  
> -struct kdf_sdesc {
> -       struct shash_desc shash;
> -       char ctx[];
> -};
> -
> -static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
> +static int kdf_alloc(struct crypto_shash **hash, char *hashname)
>  {
>         struct crypto_shash *tfm;
> -       struct kdf_sdesc *sdesc;
> -       int size;
>         int err;
>  
>         /* allocate synchronous hash */
> @@ -102,14 +96,7 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
>         if (crypto_shash_digestsize(tfm) == 0)
>                 goto out_free_tfm;
>  
> -       err = -ENOMEM;
> -       size = sizeof(struct shash_desc) + crypto_shash_descsize(tfm);
> -       sdesc = kmalloc(size, GFP_KERNEL);
> -       if (!sdesc)
> -               goto out_free_tfm;
> -       sdesc->shash.tfm = tfm;
> -
> -       *sdesc_ret = sdesc;
> +       *hash = tfm;
>  
>         return 0;
>  
> @@ -118,92 +105,20 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname)
>         return err;
>  }
>  
> -static void kdf_dealloc(struct kdf_sdesc *sdesc)
> -{
> -       if (!sdesc)
> -               return;
> -
> -       if (sdesc->shash.tfm)
> -               crypto_free_shash(sdesc->shash.tfm);
> -
> -       kfree_sensitive(sdesc);
> -}
> -
> -/*
> - * Implementation of the KDF in counter mode according to SP800-108 section 5.1
> - * as well as SP800-56A section 5.8.1 (Single-step KDF).
> - *
> - * SP800-56A:
> - * The src pointer is defined as Z || other info where Z is the shared secret
> - * from DH and other info is an arbitrary string (see SP800-56A section
> - * 5.8.1.2).
> - *
> - * 'dlen' must be a multiple of the digest size.
> - */
> -static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
> -                  u8 *dst, unsigned int dlen, unsigned int zlen)
> +static void kdf_dealloc(struct crypto_shash *hash)
>  {
> -       struct shash_desc *desc = &sdesc->shash;
> -       unsigned int h = crypto_shash_digestsize(desc->tfm);
> -       int err = 0;
> -       u8 *dst_orig = dst;
> -       __be32 counter = cpu_to_be32(1);
> -
> -       while (dlen) {
> -               err = crypto_shash_init(desc);
> -               if (err)
> -                       goto err;
> -
> -               err = crypto_shash_update(desc, (u8 *)&counter, sizeof(__be32));
> -               if (err)
> -                       goto err;
> -
> -               if (zlen && h) {
> -                       u8 tmpbuffer[32];
> -                       size_t chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
> -                       memset(tmpbuffer, 0, chunk);
> -
> -                       do {
> -                               err = crypto_shash_update(desc, tmpbuffer,
> -                                                         chunk);
> -                               if (err)
> -                                       goto err;
> -
> -                               zlen -= chunk;
> -                               chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
> -                       } while (zlen);
> -               }
> -
> -               if (src && slen) {
> -                       err = crypto_shash_update(desc, src, slen);
> -                       if (err)
> -                               goto err;
> -               }
> -
> -               err = crypto_shash_final(desc, dst);
> -               if (err)
> -                       goto err;
> -
> -               dlen -= h;
> -               dst += h;
> -               counter = cpu_to_be32(be32_to_cpu(counter) + 1);
> -       }
> -
> -       return 0;
> -
> -err:
> -       memzero_explicit(dst_orig, dlen);
> -       return err;
> +       if (hash)
> +               crypto_free_shash(hash);
>  }
>  
> -static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
> +static int keyctl_dh_compute_kdf(struct crypto_shash *hash,
>                                  char __user *buffer, size_t buflen,
> -                                uint8_t *kbuf, size_t kbuflen, size_t lzero)
> +                                uint8_t *kbuf, size_t kbuflen)
>  {
> +       struct kvec kbuf_iov = { .iov_base = kbuf, .iov_len = kbuflen };
>         uint8_t *outbuf = NULL;
>         int ret;
> -       size_t outbuf_len = roundup(buflen,
> -                                   crypto_shash_digestsize(sdesc->shash.tfm));
> +       size_t outbuf_len = roundup(buflen, crypto_shash_digestsize(hash));
>  
>         outbuf = kmalloc(outbuf_len, GFP_KERNEL);
>         if (!outbuf) {
> @@ -211,7 +126,7 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
>                 goto err;
>         }
>  
> -       ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len, lzero);
> +       ret = crypto_kdf108_ctr_generate(hash, &kbuf_iov, 1, outbuf, outbuf_len);
>         if (ret)
>                 goto err;
>  
> @@ -240,7 +155,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
>         struct kpp_request *req;
>         uint8_t *secret;
>         uint8_t *outbuf;
> -       struct kdf_sdesc *sdesc = NULL;
> +       struct crypto_shash *hash = NULL;
>  
>         if (!params || (!buffer && buflen)) {
>                 ret = -EINVAL;
> @@ -273,7 +188,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
>                 }
>  
>                 /* allocate KDF from the kernel crypto API */
> -               ret = kdf_alloc(&sdesc, hashname);
> +               ret = kdf_alloc(&hash, hashname);
>                 kfree(hashname);
>                 if (ret)
>                         goto out1;
> @@ -383,9 +298,8 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
>                         goto out6;
>                 }
>  
> -               ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, outbuf,
> -                                           req->dst_len + kdfcopy->otherinfolen,
> -                                           outlen - req->dst_len);
> +               ret = keyctl_dh_compute_kdf(hash, buffer, buflen, outbuf,
> +                                           req->dst_len + kdfcopy->otherinfolen);
>         } else if (copy_to_user(buffer, outbuf, req->dst_len) == 0) {
>                 ret = req->dst_len;
>         } else {
> @@ -403,7 +317,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
>  out2:
>         dh_free_data(&dh_inputs);
>  out1:
> -       kdf_dealloc(sdesc);
> +       kdf_dealloc(hash);
>         return ret;
>  }
>