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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 29 Jul 2024 15:50:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Unlike the rsa driver, which separates signature decoding and
> signature verification into two steps, the ecdsa driver does both in one.
> 
> This restricts users to the one signature format currently supported
> (X9.62) and prevents addition of others such as P1363, which is needed
> by the forthcoming SPDM library (Security Protocol and Data Model) for
> PCI device authentication.
> 
> Per Herbert's suggestion, change ecdsa to use a "raw" signature encoding
> and then implement X9.62 and P1363 as templates which convert their
> respective encodings to the raw one.  One may then specify
> "x962(ecdsa-nist-XXX)" or "p1363(ecdsa-nist-XXX)" to pick the encoding.
> 
> The present commit moves X9.62 decoding to a template.  A separate
> commit is going to introduce another template for P1363 decoding.
> 
> The ecdsa driver internally represents a signature as two u64 arrays of
> size ECC_MAX_BYTES.  This appears to be the most natural choice for the
> raw format as it can directly be used for verification without having to
> further decode signature data or copy it around.
> 
> Repurpose all the existing test vectors for "x962(ecdsa-nist-XXX)" and
> create a duplicate of them to test the raw encoding.
> 
> Link: https://lore.kernel.org/all/ZoHXyGwRzVvYkcTP@gondor.apana.org.au/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Trivial stuff inline.


> ---
>  crypto/Makefile                     |   3 +-
>  crypto/asymmetric_keys/public_key.c |   3 +
>  crypto/ecdsa-x962.c                 | 211 ++++++++
>  crypto/ecdsa.c                      |  86 +--
>  crypto/testmgr.c                    |  27 +
>  crypto/testmgr.h                    | 813 +++++++++++++++++++++++++++-
>  include/crypto/internal/ecc.h       |   1 +
>  7 files changed, 1077 insertions(+), 67 deletions(-)
>  create mode 100644 crypto/ecdsa-x962.c
> 

> diff --git a/crypto/ecdsa-x962.c b/crypto/ecdsa-x962.c
> new file mode 100644
> index 000000000000..ff2da5be1355
> --- /dev/null
> +++ b/crypto/ecdsa-x962.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ECDSA X9.62 signature encoding
> + *
> + * Copyright (c) 2021 IBM Corporation
> + * Copyright (c) 2024 Intel Corporation
> + */
> +
> +#include <linux/asn1_decoder.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <crypto/akcipher.h>
> +#include <crypto/algapi.h>
> +#include <crypto/internal/akcipher.h>
> +#include <crypto/internal/ecc.h>
> +
> +#include "ecdsasignature.asn1.h"
> +
> +struct ecdsa_x962_ctx {
> +	struct crypto_akcipher *child;
> +};
> +
> +struct ecdsa_x962_request {
> +	u64 r[ECC_MAX_DIGITS];
> +	u64 s[ECC_MAX_DIGITS];
> +	struct akcipher_request child_req;
> +};
> +
> +/* Get the r and s components of a signature from the X.509 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 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.


> +		return -EINVAL;
> +
> +	if (vlen > bufsize) {
> +		/* skip over leading zeros that make 'value' a positive int */
> +		if (*d == 0) {
> +			vlen -= 1;
> +			d++;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ecc_digits_from_bytes(d, vlen, dest, ndigits);
> +
> +	return 0;
> +}
> +

...

> +static int ecdsa_x962_create(struct crypto_template *tmpl, struct rtattr **tb)
> +{
> +	struct crypto_akcipher_spawn *spawn;
> +	struct akcipher_instance *inst;
> +	struct akcipher_alg *ecdsa_alg;
> +	u32 mask;
> +	int err;
> +
> +	err = crypto_check_attr_type(tb, CRYPTO_ALG_TYPE_AKCIPHER, &mask);
> +	if (err)
> +		return err;
> +
> +	inst = kzalloc(sizeof(*inst) + sizeof(*spawn), GFP_KERNEL);
> +	if (!inst)
> +		return -ENOMEM;
> +
> +	spawn = akcipher_instance_ctx(inst);
> +
> +	err = crypto_grab_akcipher(spawn, akcipher_crypto_instance(inst),
> +				   crypto_attr_alg_name(tb[1]), 0, mask);
> +	if (err)
> +		goto err_free_inst;
> +
> +	ecdsa_alg = crypto_spawn_akcipher_alg(spawn);
> +
> +	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.


> +	err = crypto_inst_setname(akcipher_crypto_instance(inst), tmpl->name,
> +				  &ecdsa_alg->base);
> +	if (err)
> +		goto err_free_inst;
> +
> +	inst->alg.base.cra_priority = ecdsa_alg->base.cra_priority;
> +	inst->alg.base.cra_ctxsize = sizeof(struct ecdsa_x962_ctx);
> +
> +	inst->alg.init = ecdsa_x962_init_tfm;
> +	inst->alg.exit = ecdsa_x962_exit_tfm;
> +
> +	inst->alg.verify = ecdsa_x962_verify;
> +	inst->alg.max_size = ecdsa_x962_max_size;
> +	inst->alg.set_pub_key = ecdsa_x962_set_pub_key;
> +
> +	inst->free = ecdsa_x962_free;
> +
> +	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;
> +}
> +