Received-SPF: pass (google.com: domain of linux-crypto+bounces-3567-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6442f387-e45e-4019-8af0-6ca309e4ce4f@linux.ibm.com>
Date: Mon, 15 Apr 2024 20:51:02 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order
 to check valid key
Content-Language: en-US
To: Jarkko Sakkinen <jarkko@kernel.org>, linux-crypto@vger.kernel.org,
        herbert@gondor.apana.org.au, davem@davemloft.net
Cc: linux-kernel@vger.kernel.org, ardb@kernel.org,
        salvatore.benedetto@intel.com
References: <20240415003026.2661270-1-stefanb@linux.ibm.com>
 <20240415003026.2661270-2-stefanb@linux.ibm.com>
 <D0KX9NQPXKO1.2RXZU000DD1BB@kernel.org>
From: Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <D0KX9NQPXKO1.2RXZU000DD1BB@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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