[PATCH 0/2] crypto: ecdh & ecc: Fix private key byte ordering issues

[PATCH 0/2] crypto: ecdh & ecc: Fix private key byte ordering issues

[Stefan Berger]

The 1st patch fixes a byte ordering issue where ctx->private_key is
currently passed to ecc_is_key_valid but the key is in reverse byte order.
To solve this issue it introduces the variable 'priv' that is already used
throughout the ecc and ecdh code bases for a private key in proper byte
order and calls the function with 'priv'.

The 2nd patch gets rid of the 'priv' variable wherever it is used to hold
a private key (byte-swapped initialized from ctx->private_key) in proper
byte order and uses ctx->private_key directly that is now initialized in
proper byte order.

Regards,
  Stefan


Stefan Berger (2):
  crypto: ecdh - Pass private key in proper byte order to check valid
    key
  crypto: ecdh & ecc - Initialize ctx->private_key in proper byte order

 crypto/ecc.c                  | 29 ++++++++++-------------------
 crypto/ecdh.c                 |  3 ++-
 include/crypto/internal/ecc.h |  3 ++-
 3 files changed, 14 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Stefan Berger]

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 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, const void *buf,
 			   unsigned int len)
 {
 	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
+	u64 priv[ECC_MAX_DIGITS];
 	struct ecdh params;
 
 	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);
 
 	memcpy(ctx->private_key, params.key, params.key_size);
+	ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);
 
 	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;
 	}
-- 
2.43.0

[PATCH 2/2] crypto: ecdh & ecc - Initialize ctx->private_key in proper byte order

[Stefan Berger]

ctx->private_key is currently initialized in reverse byte order in
ecdh_set_secret and whenever it is needed in proper byte order the variable
priv is introduced and the bytes from ctx->private_key are copied into priv
while being byte-swapped (ecc_swap_digits). To get rid of the unnecessary
byte swapping initialize ctx->private_key in proper byte order and clean up
all functions that were previously using priv or were called with
ctx->private_key:

- ecc_gen_privkey: Directly initialize the passed ctx->private_key with
  random bytes and get rid of the priv variable. This function only has
  ecdh_set_secret as a caller.

- crypto_ecdh_shared_secret: Called only from ecdh_compute_value with
  ctx->private_key. Get rid of the priv variable and work with the passed
  private_key directly.

- ecc_make_pub_key: Called only from ecdh_compute_value with
  ctx->private_key. Get rid of the priv variable and work with the passed
  private_key directly.

Cc: Salvatore Benedetto <salvatore.benedetto@intel.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 crypto/ecc.c                  | 29 ++++++++++-------------------
 crypto/ecdh.c                 |  7 +++----
 include/crypto/internal/ecc.h |  3 ++-
 3 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 2e05387b9499..c1d2e884be1e 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1497,10 +1497,10 @@ EXPORT_SYMBOL(ecc_is_key_valid);
  * This method generates a private key uniformly distributed in the range
  * [2, n-3].
  */
-int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
+int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits,
+		    u64 *private_key)
 {
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
-	u64 priv[ECC_MAX_DIGITS];
 	unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
 	unsigned int nbits = vli_num_bits(curve->n, ndigits);
 	int err;
@@ -1509,7 +1509,7 @@ int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
 	 * Step 1 & 2: check that N is included in Table 1 of FIPS 186-5,
 	 * section 6.1.1.
 	 */
-	if (nbits < 224 || ndigits > ARRAY_SIZE(priv))
+	if (nbits < 224)
 		return -EINVAL;
 
 	/*
@@ -1527,17 +1527,16 @@ int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
 		return -EFAULT;
 
 	/* Step 3: obtain N returned_bits from the DRBG. */
-	err = crypto_rng_get_bytes(crypto_default_rng, (u8 *)priv, nbytes);
+	err = crypto_rng_get_bytes(crypto_default_rng,
+				   (u8 *)private_key, nbytes);
 	crypto_put_default_rng();
 	if (err)
 		return err;
 
 	/* Step 4: make sure the private key is in the valid range. */
-	if (__ecc_is_key_valid(curve, priv, ndigits))
+	if (__ecc_is_key_valid(curve, private_key, ndigits))
 		return -EINVAL;
 
-	ecc_swap_digits(priv, privkey, ndigits);
-
 	return 0;
 }
 EXPORT_SYMBOL(ecc_gen_privkey);
@@ -1547,23 +1546,20 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 {
 	int ret = 0;
 	struct ecc_point *pk;
-	u64 priv[ECC_MAX_DIGITS];
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
-	if (!private_key || ndigits > ARRAY_SIZE(priv)) {
+	if (!private_key) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	ecc_swap_digits(private_key, priv, ndigits);
-
 	pk = ecc_alloc_point(ndigits);
 	if (!pk) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	ecc_point_mult(pk, &curve->g, priv, NULL, curve, ndigits);
+	ecc_point_mult(pk, &curve->g, private_key, NULL, curve, ndigits);
 
 	/* SP800-56A rev 3 5.6.2.1.3 key check */
 	if (ecc_is_pubkey_valid_full(curve, pk)) {
@@ -1647,13 +1643,11 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 {
 	int ret = 0;
 	struct ecc_point *product, *pk;
-	u64 priv[ECC_MAX_DIGITS];
 	u64 rand_z[ECC_MAX_DIGITS];
 	unsigned int nbytes;
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
-	if (!private_key || !public_key ||
-	    ndigits > ARRAY_SIZE(priv) || ndigits > ARRAY_SIZE(rand_z)) {
+	if (!private_key || !public_key || ndigits > ARRAY_SIZE(rand_z)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1674,15 +1668,13 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 	if (ret)
 		goto err_alloc_product;
 
-	ecc_swap_digits(private_key, priv, ndigits);
-
 	product = ecc_alloc_point(ndigits);
 	if (!product) {
 		ret = -ENOMEM;
 		goto err_alloc_product;
 	}
 
-	ecc_point_mult(product, pk, priv, rand_z, curve, ndigits);
+	ecc_point_mult(product, pk, private_key, rand_z, curve, ndigits);
 
 	if (ecc_point_is_zero(product)) {
 		ret = -EFAULT;
@@ -1692,7 +1684,6 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 	ecc_swap_digits(product->x, secret, ndigits);
 
 err_validity:
-	memzero_explicit(priv, sizeof(priv));
 	memzero_explicit(rand_z, sizeof(rand_z));
 	ecc_free_point(product);
 err_alloc_product:
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index a73853bd44de..c217f2d2d218 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -27,7 +27,6 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
 			   unsigned int len)
 {
 	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
-	u64 priv[ECC_MAX_DIGITS];
 	struct ecdh params;
 
 	if (crypto_ecdh_decode_key(buf, len, &params) < 0 ||
@@ -40,11 +39,11 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
 		return ecc_gen_privkey(ctx->curve_id, ctx->ndigits,
 				       ctx->private_key);
 
-	memcpy(ctx->private_key, params.key, params.key_size);
-	ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);
+	ecc_digits_from_bytes(params.key, params.key_size,
+			      ctx->private_key, ctx->ndigits);
 
 	if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
-			     priv, params.key_size) < 0) {
+			     ctx->private_key, params.key_size) < 0) {
 		memzero_explicit(ctx->private_key, params.key_size);
 		return -EINVAL;
 	}
diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h
index 4e2f5f938e91..7ca1f463d1ec 100644
--- a/include/crypto/internal/ecc.h
+++ b/include/crypto/internal/ecc.h
@@ -103,7 +103,8 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * Returns 0 if the private key was generated successfully, a negative value
  * if an error occurred.
  */
-int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey);
+int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits,
+		    u64 *private_key);
 
 /**
  * ecc_make_pub_key() - Compute an ECC public key
-- 
2.43.0

Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Jarkko Sakkinen]

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 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, const void *buf,
>  			   unsigned int len)
>  {
>  	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
> +	u64 priv[ECC_MAX_DIGITS];
>  	struct ecdh params;
>  
>  	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);
>  
>  	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.

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

Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Stefan Berger]

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 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, const void *buf,
>>   			   unsigned int len)
>>   {
>>   	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
>> +	u64 priv[ECC_MAX_DIGITS];
>>   	struct ecdh params;
>>   
>>   	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);
>>   
>>   	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?
The goal of this particular patch is to fix an issue with the byte order 
(as description says) and, as you can see in the 2nd patch, private key 
is always copied into priv using ecc_swap_digits before priv is being 
used instead of ctx->private_key (or whatever it is called in the 
function it was passed to). This patch here has nothing to do with speed 
up but a) fixing an issue and b) using priv here as well, so fixing this 
'outlier' here. The speed-up comes in the 2nd patch when the bytes in 
ctx->private_key are put into proper order right away and we can get rid 
if priv, taking the swapped bytes of ctx->private_key, everywhere and we 
can use ctx->private_key directly.

The test harness (testmgr.c) runs through part of this code here 
providing the private key that is copied into ctx->private_key, so it's 
being used and when you make a mistake (making the changes I did) the 
ecdh test cases will fail.



> 
> Just a question to understand what is going on, not actual review
> feedback.
> 
>>   
>>   	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

Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Jarkko Sakkinen]

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 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, const void *buf,
> >>   			   unsigned int len)
> >>   {
> >>   	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
> >> +	u64 priv[ECC_MAX_DIGITS];
> >>   	struct ecdh params;
> >>   
> >>   	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);
> >>   
> >>   	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?
> The goal of this particular patch is to fix an issue with the byte order 
> (as description says) and, as you can see in the 2nd patch, private key 
> is always copied into priv using ecc_swap_digits before priv is being 
> used instead of ctx->private_key (or whatever it is called in the 
> function it was passed to). This patch here has nothing to do with speed 
> up but a) fixing an issue and b) using priv here as well, so fixing this 
> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in 
> ctx->private_key are put into proper order right away and we can get rid 
> if priv, taking the swapped bytes of ctx->private_key, everywhere and we 
> can use ctx->private_key directly.
>
> The test harness (testmgr.c) runs through part of this code here 
> providing the private key that is copied into ctx->private_key, so it's 
> being used and when you make a mistake (making the changes I did) the 
> 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

Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Joachim Vandersmissen]

Hi,

Apologies for hijacking this thread, but I don't have access to the 
older emails.

Should the new priv variable not be zeroized before the end of the 
function? As it contains private keying material?

Kind regards,
Joachim

On 4/16/24 9:25 AM, Jarkko Sakkinen wrote:
> 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 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, const void *buf,
>>>>    			   unsigned int len)
>>>>    {
>>>>    	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
>>>> +	u64 priv[ECC_MAX_DIGITS];
>>>>    	struct ecdh params;
>>>>    
>>>>    	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);
>>>>    
>>>>    	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?
>> The goal of this particular patch is to fix an issue with the byte order
>> (as description says) and, as you can see in the 2nd patch, private key
>> is always copied into priv using ecc_swap_digits before priv is being
>> used instead of ctx->private_key (or whatever it is called in the
>> function it was passed to). This patch here has nothing to do with speed
>> up but a) fixing an issue and b) using priv here as well, so fixing this
>> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in
>> ctx->private_key are put into proper order right away and we can get rid
>> if priv, taking the swapped bytes of ctx->private_key, everywhere and we
>> can use ctx->private_key directly.
>>
>> The test harness (testmgr.c) runs through part of this code here
>> providing the private key that is copied into ctx->private_key, so it's
>> being used and when you make a mistake (making the changes I did) the
>> 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
>

Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Stefan Berger]

On 4/16/24 22:12, Joachim Vandersmissen wrote:
> Hi,
> 
> Apologies for hijacking this thread, but I don't have access to the 
> older emails.
> 
> Should the new priv variable not be zeroized before the end of the 
> function? As it contains private keying material?

Yes, fixed in v2.

    Stefan

> 
> Kind regards,
> Joachim
> 
> On 4/16/24 9:25 AM, Jarkko Sakkinen wrote:
>> 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 
>>>>> 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, const void *buf,
>>>>>                   unsigned int len)
>>>>>    {
>>>>>        struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
>>>>> +    u64 priv[ECC_MAX_DIGITS];
>>>>>        struct ecdh params;
>>>>>        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);
>>>>>        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?
>>> The goal of this particular patch is to fix an issue with the byte order
>>> (as description says) and, as you can see in the 2nd patch, private key
>>> is always copied into priv using ecc_swap_digits before priv is being
>>> used instead of ctx->private_key (or whatever it is called in the
>>> function it was passed to). This patch here has nothing to do with speed
>>> up but a) fixing an issue and b) using priv here as well, so fixing this
>>> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in
>>> ctx->private_key are put into proper order right away and we can get rid
>>> if priv, taking the swapped bytes of ctx->private_key, everywhere and we
>>> can use ctx->private_key directly.
>>>
>>> The test harness (testmgr.c) runs through part of this code here
>>> providing the private key that is copied into ctx->private_key, so it's
>>> being used and when you make a mistake (making the changes I did) the
>>> 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
>>