Received-SPF: pass (google.com: domain of linux-crypto+bounces-3546-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 15 Apr 2024 21:53:22 +0300
Message-Id: <D0KX9NQPXKO1.2RXZU000DD1BB@kernel.org>
Cc: <linux-kernel@vger.kernel.org>, <ardb@kernel.org>,
 <salvatore.benedetto@intel.com>
Subject: Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte
 order to check valid key
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Stefan Berger" <stefanb@linux.ibm.com>, <linux-crypto@vger.kernel.org>,
 <herbert@gondor.apana.org.au>, <davem@davemloft.net>
References: <20240415003026.2661270-1-stefanb@linux.ibm.com>
 <20240415003026.2661270-2-stefanb@linux.ibm.com>
In-Reply-To: <20240415003026.2661270-2-stefanb@linux.ibm.com>

On Mon Apr 15, 2024 at 3:30 AM EEST, Stefan Berger wrote:
> ecc_is_key_valid expects a key with the most significant digit in the las=
t
> entry of the digit array. Currently ecdh_set_secret passes a reversed key
> to ecc_is_key_valid that then passes the rather simple test checking
> whether the private key is in range [2, n-3]. For all current ecdh-
> supported curves (NIST P192/256/384) the 'n' parameter is a rather large
> number, therefore easily passing this test.
>
> Throughout the ecdh and ecc codebase the variable 'priv' is used for a
> private_key holding the bytes in proper byte order. Therefore, introduce
> priv in ecdh_set_secret and copy the bytes from ctx->private_key into
> priv in proper byte order by using ecc_swap_digits. Pass priv to
> ecc_is_valid_key.
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  crypto/ecdh.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/ecdh.c b/crypto/ecdh.c
> index 3049f147e011..a73853bd44de 100644
> --- a/crypto/ecdh.c
> +++ b/crypto/ecdh.c
> @@ -27,6 +27,7 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, cons=
t void *buf,
>  			   unsigned int len)
>  {
>  	struct ecdh_ctx *ctx =3D ecdh_get_ctx(tfm);
> +	u64 priv[ECC_MAX_DIGITS];
>  	struct ecdh params;
> =20
>  	if (crypto_ecdh_decode_key(buf, len, &params) < 0 ||
> @@ -40,9 +41,10 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, con=
st void *buf,
>  				       ctx->private_key);
> =20
>  	memcpy(ctx->private_key, params.key, params.key_size);
> +	ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);

Does swapping speed up the test that follows are what effect does it
have to the ecc_is_key_valid() call?

Just a question to understand what is going on, not actual review
feedback.

> =20
>  	if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
> -			     ctx->private_key, params.key_size) < 0) {
> +			     priv, params.key_size) < 0) {
>  		memzero_explicit(ctx->private_key, params.key_size);
>  		return -EINVAL;
>  	}

BR, Jarkko