Re: [PATCH v12 02/10] crypto: Add support for ECDSA signature verification

[Stefan Berger <stefanb@linux.ibm.com>]

On 7/17/24 12:17, Lukas Wunner wrote:
> Hi Stefan,
> 
> On Tue, Mar 16, 2021 at 05:07:32PM -0400, Stefan Berger wrote:
>> +/*
>> + * Get the r and s components of a signature from the X509 certificate.
>> + */
>> +static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag,
>> +				  const void *value, size_t vlen, unsigned int ndigits)
>> +{
>> +	size_t keylen = ndigits * sizeof(u64);
>> +	ssize_t diff = vlen - keylen;
>> +	const char *d = value;
>> +	u8 rs[ECC_MAX_BYTES];
>> +
>> +	if (!value || !vlen)
>> +		return -EINVAL;
>> +
>> +	/* diff = 0: 'value' has exacly the right size
>> +	 * diff > 0: 'value' has too many bytes; one leading zero is allowed that
>> +	 *           makes the value a positive integer; error on more
>> +	 * diff < 0: 'value' is missing leading zeros, which we add
>> +	 */
>> +	if (diff > 0) {
>> +		/* skip over leading zeros that make 'value' a positive int */
>> +		if (*d == 0) {
>> +			vlen -= 1;
>> +			diff--;
>> +			d++;
>> +		}
>> +		if (diff)
>> +			return -EINVAL;
>> +	}
>> +	if (-diff >= keylen)
>> +		return -EINVAL;
> 
> I'm in the process of creating a crypto_template for decoding an x962
> signature as requested by Herbert:
> 
> https://lore.kernel.org/all/ZoHXyGwRzVvYkcTP@gondor.apana.org.au/
> 
> I intend to move the above code to the template and to do so I'm
> trying to understand what it's doing.
> 
> There's an oddity in the above-quoted function.  The check ...
> 
> +	if (-diff >= keylen)
> +		return -EINVAL;
> 
> ... seems superfluous. diff is assigned the following value at the
> top of the function:
> 
> +	ssize_t diff = vlen - keylen;
> 
> This means that:  -diff == keylen - vlen.
> 
> Now, if vlen is zero, -diff would equal keylen and then the
> "-diff >= keylen" check would be true.  However at the top of
> the function, there's already a !vlen check.  No need to check
> the same thing again!

You're right, this check is not necessary.

    Stefan