Re: [PATCH 4/5] crypto: ecdsa - Move X9.62 signature decoding into template

[Lukas Wunner <lukas@wunner.de>]

On Thu, Aug 01, 2024 at 05:58:13PM +0100, Jonathan Cameron wrote:
> On Mon, 29 Jul 2024 15:50:00 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > +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 bufsize = ndigits * sizeof(u64);
> > +	const char *d = value;
> > +
> > +	if (!value || !vlen || vlen > bufsize + 1)
> 
> Assuming previous musing correct middle test isn't needed.
> Maybe want to keep it though. Up to you.

The kernel wouldn't crash in the !vlen case because
ecc_digits_from_bytes() can cope with a zero length argument.

However an integer ASN.1 tag with zero length would be illegal I think.
The integer R or S could be very short, but ought to be at least 1 byte
long.  asn1_decoder.c does not seem to error out on zero length,
I assume that would be legal at least for *some* tags.

So it does seem prudent to keep the !vlen check.


> > +	err = -EINVAL;
> > +	if (strncmp(ecdsa_alg->base.cra_name, "ecdsa", 5) != 0)
> > +		goto err_free_inst;
> > +
> 
> 	if (cmp(ecdsa_alg->base.cra_name, "ecdsa", 5) != 0) {
> 		err = -EINVAL;
> 		goto err_free_inst;
> 	}
> 
> Seems more readable to me.

I'm aware that it looks unusual but in the crypto subsystem
it appears to be a fairly common pattern, so I followed it
to blend in:

  First set the return variable to an error code,
  then check for an error condition followed by goto.

See e.g.:

  __crypto_register_alg() in crypto/algapi.c
  software_key_eds_op() in crypto/asymmetric_keys/public_key.c
  x509_get_sig_params() in crypto/asymmetric_keys/x509_public_key.c
  x509_check_for_self_signed() in crypto/asymmetric_keys/x509_public_key.c


> > +	err = akcipher_register_instance(tmpl, inst);
> > +	if (err) {
> > +err_free_inst:
> > +		ecdsa_x962_free(inst);
> > +	}
> > +	return err;
> 
> I'd rather see a separate error path even if it's a little more code.
> 
> 	if (err)
> 		goto err_free_inst;
> 
> 	return 0;
> 
> err_free_inst:
> 	ecdsa_x862_free(inst)
> 	return err;
> > +}

It seems all the existing crypto_templates uniformly follow that
pattern of jumping to an "err_free_inst" label in the
*_register_instance() error path:

$ git grep -C 4 "err_free_inst:" -- crypto/

It seemed unwise to go against the current, so I followed the
existing pattern.  Upstream diplomacy. ;)

Thanks,

Lukas