Received-SPF: pass (google.com: domain of linux-kernel+bounces-106764-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <a8385505-b3f2-40f8-b603-fd3435c5ba24@linux.ibm.com>
Date: Mon, 18 Mar 2024 18:42:13 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 12/13] crypto: asymmetric_keys - Adjust signature size
 calculation for NIST P521
Content-Language: en-US
To: Jarkko Sakkinen <jarkko@kernel.org>,
        Stefan Berger <stefanb@linux.vnet.ibm.com>, keyrings@vger.kernel.org,
        linux-crypto@vger.kernel.org, herbert@gondor.apana.org.au,
        davem@davemloft.net
Cc: linux-kernel@vger.kernel.org, saulo.alessandre@tse.jus.br, lukas@wunner.de,
        bbhushan2@marvell.com
References: <20240312183618.1211745-1-stefanb@linux.vnet.ibm.com>
 <20240312183618.1211745-13-stefanb@linux.vnet.ibm.com>
 <CZX6OOKP5RQD.3BN0EDSAF7QXR@kernel.org>
From: Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <CZX6OOKP5RQD.3BN0EDSAF7QXR@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 3/18/24 17:12, Jarkko Sakkinen wrote:
> On Tue Mar 12, 2024 at 8:36 PM EET, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Adjust the calculation of the maximum signature size for support of
>> NIST P521. While existing curves may prepend a 0 byte to their coordinates
>> (to make the number positive), NIST P521 will not do this since only the
>> first bit in the most significant byte is used.
>>
>> If the encoding of the x & y coordinates requires at least 128 bytes then
>> an additional byte is needed for the encoding of the length. Take this into
>> account when calculating the maximum signature size.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>> Tested-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>   crypto/asymmetric_keys/public_key.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
>> index e5f22691febd..16cc0be28929 100644
>> --- a/crypto/asymmetric_keys/public_key.c
>> +++ b/crypto/asymmetric_keys/public_key.c
>> @@ -233,6 +233,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>>   	info->key_size = len * 8;
>>   
>>   	if (strncmp(pkey->pkey_algo, "ecdsa", 5) == 0) {
>> +		int slen = len;
>>   		/*
>>   		 * ECDSA key sizes are much smaller than RSA, and thus could
>>   		 * operate on (hashed) inputs that are larger than key size.
>> @@ -246,8 +247,19 @@ static int software_key_query(const struct kernel_pkey_params *params,
>>   		 * Verify takes ECDSA-Sig (described in RFC 5480) as input,
>>   		 * which is actually 2 'key_size'-bit integers encoded in
>>   		 * ASN.1.  Account for the ASN.1 encoding overhead here.
>> +		 *
>> +		 * NIST P192/256/384 may prepend a '0' to a coordinate to
>> +		 * indicate a positive integer. NIST P521 never needs it.
>>   		 */
>> -		info->max_sig_size = 2 * (len + 3) + 2;
>> +		if (strcmp(pkey->pkey_algo, "ecdsa-nist-p521") != 0)
>> +			slen += 1;
> 
> Just wondering the logic of picking between these:
> 
> 1. "strncmp"
> 2. "strcmp"
> 

strncmp: prefix-matching
strcmp: full string matching

> Now the "ecdsa" is matched with strncmp and "ecdsa-nist-p521" is
> compared with strcmp.

That's prefix matching vs. full string match.

. and indeed 'ecdsa' is a prefix of 'ecdsa-nist-p521'.

> 
> So is there a good reason to use different function in these
> cases?

Yes, there is.

> 
> I'd guess both could be using strcmp since comparing against
> constant...

No, prefix versus full string matching requires different function calls.

> 
>> +		/* Length of encoding the x & y coordinates */
>> +		slen = 2 * (slen + 2);
>> +		/*
>> +		 * If coordinate encoding takes at least 128 bytes then an
>> +		 * additional byte for length encoding is needed.
>> +		 */
>> +		info->max_sig_size = 1 + (slen >= 128) + 1 + slen;
>>   	} else {
>>   		info->max_data_size = len;
>>   		info->max_sig_size = len;
> 
> 
> BR, Jarkko
>