Received-SPF: pass (google.com: domain of linux-kernel+bounces-147009-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 16 Apr 2024 17:25:27 +0300
Message-Id: <D0LM72MR4LDH.3QN2WEXU4QEEJ@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>
 <D0KX9NQPXKO1.2RXZU000DD1BB@kernel.org>
 <6442f387-e45e-4019-8af0-6ca309e4ce4f@linux.ibm.com>
In-Reply-To: <6442f387-e45e-4019-8af0-6ca309e4ce4f@linux.ibm.com>

On Tue Apr 16, 2024 at 3:51 AM EEST, Stefan Berger wrote:
>
>
> On 4/15/24 14:53, Jarkko Sakkinen wrote:
> > 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 =
last
> >> 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 lar=
ge
> >> 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, introdu=
ce
> >> 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, c=
onst 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, =
const void *buf,
> >>   				       ctx->private_key);
> >>  =20
> >>   	memcpy(ctx->private_key, params.key, params.key_size);
> >> +	ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);
> >=20
> > Does swapping speed up the test that follows are what effect does it
> > have to the ecc_is_key_valid() call?
> The goal of this particular patch is to fix an issue with the byte order=
=20
> (as description says) and, as you can see in the 2nd patch, private key=
=20
> is always copied into priv using ecc_swap_digits before priv is being=20
> used instead of ctx->private_key (or whatever it is called in the=20
> function it was passed to). This patch here has nothing to do with speed=
=20
> up but a) fixing an issue and b) using priv here as well, so fixing this=
=20
> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in=20
> ctx->private_key are put into proper order right away and we can get rid=
=20
> if priv, taking the swapped bytes of ctx->private_key, everywhere and we=
=20
> can use ctx->private_key directly.
>
> The test harness (testmgr.c) runs through part of this code here=20
> providing the private key that is copied into ctx->private_key, so it's=
=20
> being used and when you make a mistake (making the changes I did) the=20
> ecdh test cases will fail.

OK, thanks for the explanation :-) No opposition on the change itself.

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko