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

[Stefan Berger]

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@xxxxxxxxxxxxxxxxxxx/
>
> 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