| Message ID | 20240722142122.128258-2-hare@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | nvme: implement secure concatenation | expand |
On Mon, Jul 22, 2024 at 04:21:14PM +0200, Hannes Reinecke wrote: > diff --git a/crypto/Makefile b/crypto/Makefile > index edbbaa3ffef5..b77fc360f0ff 100644 > --- a/crypto/Makefile > +++ b/crypto/Makefile > @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o > > crypto_hash-y += ahash.o > crypto_hash-y += shash.o > +crypto_hash-y += hkdf.o > obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o This should go under a kconfig option CONFIG_CRYPTO_HKDF that is selected by the users that need it. That way the code will be built only when needed. Including a self-test would also be desirable. > diff --git a/crypto/hkdf.c b/crypto/hkdf.c > new file mode 100644 > index 000000000000..22e343851c0b > --- /dev/null > +++ b/crypto/hkdf.c > @@ -0,0 +1,112 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation > + * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): > + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". > + * > + * This is used to derive keys from the fscrypt master keys. This is no longer in fs/crypto/, so the part about fscrypt should be removed. > +/* > + * HKDF consists of two steps: > + * > + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from > + * the input keying material and optional salt. It doesn't make sense to refer to HKDF_HASHLEN here since it is specific to fs/crypto/. > +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */ > +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm, > + unsigned int ikmlen, u8 *prk) Needs kerneldoc now that this is a library interface. > +{ > + unsigned int prklen = crypto_shash_digestsize(hmac_tfm); > + u8 *default_salt; > + int err; > + > + default_salt = kzalloc(prklen, GFP_KERNEL); > + if (!default_salt) > + return -ENOMEM; Now that this is a library interface, it should take the salt as a parameter, and the users who want the default salt should explicitly specify that. If we only provide support for unsalted use, that might inadventently discourage the use of a salt in future code. As the function is named hkdf_extract(), people might also overlook that it's unsalted and doesn't actually match the RFC's definition of HKDF-Extract. The use of kzalloc here is also inefficient, as the maximum length of a digest is known (HKDF_HASHLEN in fs/crypto/ case, HASH_MAX_DIGESTSIZE in general). > + err = crypto_shash_setkey(hmac_tfm, default_salt, prklen); > + if (!err) > + err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk); > + > + kfree(default_salt); > + return err; > +} > +EXPORT_SYMBOL_GPL(hkdf_extract); > + > +/* > + * HKDF-Expand (RFC 5869 section 2.3). > + * This expands the pseudorandom key, which was already keyed into @hmac_tfm, > + * into @okmlen bytes of output keying material parameterized by the > + * application-specific @info of length @infolen bytes. > + * This is thread-safe and may be called by multiple threads in parallel. > + */ > +int hkdf_expand(struct crypto_shash *hmac_tfm, > + const u8 *info, unsigned int infolen, > + u8 *okm, unsigned int okmlen) Needs kerneldoc now that this is a library interface. > diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c > index 5a384dad2c72..9c2f9aca9412 100644 > --- a/fs/crypto/hkdf.c > +++ b/fs/crypto/hkdf.c >// SPDX-License-Identifier: GPL-2.0 >/* > * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation > * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): > * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". > * > * This is used to derive keys from the fscrypt master keys. > * > * Copyright 2019 Google LLC > */ The above file comment should be adjusted now that this file doesn't contain the actual HKDF implementation. > @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, > u8 *okm, unsigned int okmlen) > { > SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm); > - u8 prefix[9]; > - unsigned int i; > + u8 *prefix; > int err; > - const u8 *prev = NULL; > - u8 counter = 1; > - u8 tmp[HKDF_HASHLEN]; > > if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN)) > return -EINVAL; > > + prefix = kzalloc(okmlen + 9, GFP_KERNEL); > + if (!prefix) > + return -ENOMEM; > desc->tfm = hkdf->hmac_tfm; > > memcpy(prefix, "fscrypt\0", 8); > prefix[8] = context; > + memcpy(prefix + 9, info, infolen); This makes the variable called 'prefix' no longer be the prefix, but rather the full info string. A better name for it would be 'full_info'. Also, it's being allocated with the wrong length. It should be 9 + infolen. > + err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8, > + okm, okmlen); > + kfree(prefix); kfree_sensitive() > diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h > new file mode 100644 > index 000000000000..bf06c080d7ed > --- /dev/null > +++ b/include/crypto/hkdf.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869 > + * > + * Extracted from fs/crypto/hkdf.c, which has > + * Copyright 2019 Google LLC > + */ If this is keeping the copyright of fs/crypto/hkdf.c, the license needs to stay the same (GPL-2.0, not GPL-2.0-or-later). - Eric
On 7/23/24 03:36, Eric Biggers wrote: > On Mon, Jul 22, 2024 at 04:21:14PM +0200, Hannes Reinecke wrote: >> diff --git a/crypto/Makefile b/crypto/Makefile >> index edbbaa3ffef5..b77fc360f0ff 100644 >> --- a/crypto/Makefile >> +++ b/crypto/Makefile >> @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o >> >> crypto_hash-y += ahash.o >> crypto_hash-y += shash.o >> +crypto_hash-y += hkdf.o >> obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o > > This should go under a kconfig option CONFIG_CRYPTO_HKDF that is selected by the > users that need it. That way the code will be built only when needed. > Okay. > Including a self-test would also be desirable. > First time for me, but ok. >> diff --git a/crypto/hkdf.c b/crypto/hkdf.c >> new file mode 100644 >> index 000000000000..22e343851c0b >> --- /dev/null >> +++ b/crypto/hkdf.c >> @@ -0,0 +1,112 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation >> + * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): >> + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". >> + * >> + * This is used to derive keys from the fscrypt master keys. > > This is no longer in fs/crypto/, so the part about fscrypt should be removed. > Will be removing this line. >> +/* >> + * HKDF consists of two steps: >> + * >> + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from >> + * the input keying material and optional salt. > > It doesn't make sense to refer to HKDF_HASHLEN here since it is specific to > fs/crypto/. > Ok. >> +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */ >> +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm, >> + unsigned int ikmlen, u8 *prk) > > Needs kerneldoc now that this is a library interface. > Indeed, will be updating the comment. >> +{ >> + unsigned int prklen = crypto_shash_digestsize(hmac_tfm); >> + u8 *default_salt; >> + int err; >> + >> + default_salt = kzalloc(prklen, GFP_KERNEL); >> + if (!default_salt) >> + return -ENOMEM; > > Now that this is a library interface, it should take the salt as a parameter, > and the users who want the default salt should explicitly specify that. If we > only provide support for unsalted use, that might inadventently discourage the > use of a salt in future code. As the function is named hkdf_extract(), people > might also overlook that it's unsalted and doesn't actually match the RFC's > definition of HKDF-Extract. > > The use of kzalloc here is also inefficient, as the maximum length of a digest > is known (HKDF_HASHLEN in fs/crypto/ case, HASH_MAX_DIGESTSIZE in general). > I was trying to keep the changes to the actual code to a minimum, so I didn't modify the calling sequence of the original code. But sure, passing in the 'salt' as a parameter is sensible. >> + err = crypto_shash_setkey(hmac_tfm, default_salt, prklen); >> + if (!err) >> + err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk); >> + >> + kfree(default_salt); >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(hkdf_extract); >> + >> +/* >> + * HKDF-Expand (RFC 5869 section 2.3). >> + * This expands the pseudorandom key, which was already keyed into @hmac_tfm, >> + * into @okmlen bytes of output keying material parameterized by the >> + * application-specific @info of length @infolen bytes. >> + * This is thread-safe and may be called by multiple threads in parallel. >> + */ >> +int hkdf_expand(struct crypto_shash *hmac_tfm, >> + const u8 *info, unsigned int infolen, >> + u8 *okm, unsigned int okmlen) > > Needs kerneldoc now that this is a library interface. > Ok. >> diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c >> index 5a384dad2c72..9c2f9aca9412 100644 >> --- a/fs/crypto/hkdf.c >> +++ b/fs/crypto/hkdf.c >> // SPDX-License-Identifier: GPL-2.0 >> /* >> * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation >> * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): >> * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". >> * >> * This is used to derive keys from the fscrypt master keys. >> * >> * Copyright 2019 Google LLC >> */ > > The above file comment should be adjusted now that this file doesn't contain the > actual HKDF implementation. > Ok. >> @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, >> u8 *okm, unsigned int okmlen) >> { >> SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm); >> - u8 prefix[9]; >> - unsigned int i; >> + u8 *prefix; >> int err; >> - const u8 *prev = NULL; >> - u8 counter = 1; >> - u8 tmp[HKDF_HASHLEN]; >> >> if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN)) >> return -EINVAL; >> >> + prefix = kzalloc(okmlen + 9, GFP_KERNEL); >> + if (!prefix) >> + return -ENOMEM; >> desc->tfm = hkdf->hmac_tfm; >> >> memcpy(prefix, "fscrypt\0", 8); >> prefix[8] = context; >> + memcpy(prefix + 9, info, infolen); > > This makes the variable called 'prefix' no longer be the prefix, but rather the > full info string. A better name for it would be 'full_info'. > > Also, it's being allocated with the wrong length. It should be 9 + infolen. > Will fix it up. >> + err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8, >> + okm, okmlen); >> + kfree(prefix); > > kfree_sensitive() > >> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h >> new file mode 100644 >> index 000000000000..bf06c080d7ed >> --- /dev/null >> +++ b/include/crypto/hkdf.h >> @@ -0,0 +1,18 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869 >> + * >> + * Extracted from fs/crypto/hkdf.c, which has >> + * Copyright 2019 Google LLC >> + */ > > If this is keeping the copyright of fs/crypto/hkdf.c, the license needs to stay > the same (GPL-2.0, not GPL-2.0-or-later). > Will be modifying it. Thanks a lot for your review! Cheers, Hannes
diff --git a/crypto/Makefile b/crypto/Makefile index edbbaa3ffef5..b77fc360f0ff 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o crypto_hash-y += ahash.o crypto_hash-y += shash.o +crypto_hash-y += hkdf.o obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o diff --git a/crypto/hkdf.c b/crypto/hkdf.c new file mode 100644 index 000000000000..22e343851c0b --- /dev/null +++ b/crypto/hkdf.c @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation + * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010): + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme". + * + * This is used to derive keys from the fscrypt master keys. + * + * Copyright 2019 Google LLC + */ + +#include <crypto/hash.h> +#include <crypto/sha2.h> +#include <crypto/hkdf.h> + +/* + * HKDF consists of two steps: + * + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from + * the input keying material and optional salt. + * 2. HKDF-Expand: expand the pseudorandom key into output keying material of + * any length, parameterized by an application-specific info string. + * + */ + +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */ +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm, + unsigned int ikmlen, u8 *prk) +{ + unsigned int prklen = crypto_shash_digestsize(hmac_tfm); + u8 *default_salt; + int err; + + default_salt = kzalloc(prklen, GFP_KERNEL); + if (!default_salt) + return -ENOMEM; + err = crypto_shash_setkey(hmac_tfm, default_salt, prklen); + if (!err) + err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk); + + kfree(default_salt); + return err; +} +EXPORT_SYMBOL_GPL(hkdf_extract); + +/* + * HKDF-Expand (RFC 5869 section 2.3). + * This expands the pseudorandom key, which was already keyed into @hmac_tfm, + * into @okmlen bytes of output keying material parameterized by the + * application-specific @info of length @infolen bytes. + * This is thread-safe and may be called by multiple threads in parallel. + */ +int hkdf_expand(struct crypto_shash *hmac_tfm, + const u8 *info, unsigned int infolen, + u8 *okm, unsigned int okmlen) +{ + SHASH_DESC_ON_STACK(desc, hmac_tfm); + unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm); + int err; + const u8 *prev = NULL; + u8 counter = 1; + u8 *tmp; + + if (WARN_ON(okmlen > 255 * hashlen)) + return -EINVAL; + + tmp = kzalloc(hashlen, GFP_KERNEL); + if (!tmp) + return -ENOMEM; + + desc->tfm = hmac_tfm; + + for (i = 0; i < okmlen; i += hashlen) { + + err = crypto_shash_init(desc); + if (err) + goto out; + + if (prev) { + err = crypto_shash_update(desc, prev, hashlen); + if (err) + goto out; + } + + err = crypto_shash_update(desc, info, infolen); + if (err) + goto out; + + BUILD_BUG_ON(sizeof(counter) != 1); + if (okmlen - i < 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); + kfree(tmp); + return err; +} +EXPORT_SYMBOL_GPL(hkdf_expand); diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c index 5a384dad2c72..9c2f9aca9412 100644 --- a/fs/crypto/hkdf.c +++ b/fs/crypto/hkdf.c @@ -11,6 +11,7 @@ #include <crypto/hash.h> #include <crypto/sha2.h> +#include <crypto/hkdf.h> #include "fscrypt_private.h" @@ -44,20 +45,6 @@ * 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. @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, u8 *okm, unsigned int okmlen) { SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm); - u8 prefix[9]; - unsigned int i; + u8 *prefix; int err; - const u8 *prev = NULL; - u8 counter = 1; - u8 tmp[HKDF_HASHLEN]; if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN)) return -EINVAL; + prefix = kzalloc(okmlen + 9, GFP_KERNEL); + if (!prefix) + return -ENOMEM; desc->tfm = hkdf->hmac_tfm; memcpy(prefix, "fscrypt\0", 8); prefix[8] = context; + memcpy(prefix + 9, info, infolen); - for (i = 0; i < okmlen; i += HKDF_HASHLEN) { - - 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); + err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8, + okm, okmlen); + kfree(prefix); return err; } diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h new file mode 100644 index 000000000000..bf06c080d7ed --- /dev/null +++ b/include/crypto/hkdf.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869 + * + * Extracted from fs/crypto/hkdf.c, which has + * Copyright 2019 Google LLC + */ + +#ifndef _CRYPTO_HKDF_H +#define _CRYPTO_HKDF_H + +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm, + unsigned int ikmlen, u8 *prk); +int hkdf_expand(struct crypto_shash *hmac_tfm, + const u8 *info, unsigned int infolen, + u8 *okm, unsigned int okmlen); + +#endif
Separate out the HKDF functions into a separate file to make them available to other callers. Signed-off-by: Hannes Reinecke <hare@kernel.org> Cc: Eric Biggers <ebiggers@kernel.org> Cc: linux-crypto@vger.kernel.org --- crypto/Makefile | 1 + crypto/hkdf.c | 112 ++++++++++++++++++++++++++++++++++++++++++ fs/crypto/hkdf.c | 68 ++++--------------------- include/crypto/hkdf.h | 18 +++++++ 4 files changed, 140 insertions(+), 59 deletions(-) create mode 100644 crypto/hkdf.c create mode 100644 include/crypto/hkdf.h