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@xxxxxxxxx> 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
