| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1a332942-c5ec-b317-ceac-a48dcdf45e46@linux.intel.com>
Date: Wed, 17 Nov 2021 13:45:35 -0800 (PST)
From: Mat Martineau <mathew.j.martineau@...ux.intel.com>
To: Stephan Müller <smueller@...onox.de>
cc: herbert@...dor.apana.org.au, ebiggers@...nel.org,
Jarkko Sakkinen <jarkko@...nel.org>,
"dhowells@...hat.com" <dhowells@...hat.com>,
linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
keyrings <keyrings@...r.kernel.org>, simo@...hat.com
Subject: Re: [PATCH v3 4/4] security: DH - use KDF implementation from crypto
API
On Mon, 15 Nov 2021, 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 primitive. Thus,
> it only uses the call to crypto_kdf108_ctr_generate.
>
> Signed-off-by: Stephan Mueller <smueller@...onox.de>
> ---
> security/keys/Kconfig | 2 +-
> security/keys/dh.c | 97 +++++++------------------------------------
> 2 files changed, 15 insertions(+), 84 deletions(-)
>
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 64b81abd087e..969122c7b92f 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -109,7 +109,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 56e12dae4534..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;
With the sdesc allocation failure path removed below, there's only one use
of the 'err' variable and the out_free_tfm code path. I suggest furthering
the cleanup by removing err and the goto path, and moving the
crypto_free_shash() and return -EINVAL here (where the goto currently is).
-Mat
>
> - 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,76 +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)
> +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 (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)
> {
> + 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) {
> @@ -195,7 +126,7 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
> goto err;
> }
>
> - ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len);
> + ret = crypto_kdf108_ctr_generate(hash, &kbuf_iov, 1, outbuf, outbuf_len);
> if (ret)
> goto err;
>
> @@ -224,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;
> @@ -257,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;
> @@ -367,7 +298,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
> goto out6;
> }
>
> - ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, outbuf,
> + 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;
> @@ -386,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.33.1
>
>
>
>
>
--
Mat Martineau
Intel
Powered by blists - more mailing lists