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

Stefan Berger
2024-07-22T12:20:08Z

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