Re: [PATCH v2 17/19] crypto: add ECDSA support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 01.08.24 07:57, Sascha Hauer wrote:
> This adds ECDSA signature verification support. The code is based on the
> Linux code as of Linux-6.10. The Linux code expects the key to be in
> ASN.1 encoded format. We don't need this in barebox as directly compile
> the x and y key values into the binary, so this is left out.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  crypto/Kconfig                    |  20 ++++
>  crypto/Makefile                   |  19 ++++
>  crypto/ecdsa.c                    | 169 ++++++++++++++++++++++++++++++
>  include/asm-generic/barebox.lds.h |   7 ++
>  include/ecdsa.h                   |  21 ++++
>  5 files changed, 236 insertions(+)
>  create mode 100644 crypto/ecdsa.c
>  create mode 100644 include/ecdsa.h
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index e953ef5e15..eeacd9ffb7 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -156,4 +156,24 @@ config JWT
>  config CRYPTO_ECC
>  	bool
>  
> +config CRYPTO_ECDSA
> +	bool "ECDSA support"
> +	select CRYPTO_ECC
> +
> +config CRYPTO_ECDSA_BUILTIN_KEYS
> +	bool
> +	default y if CRYPTO_ECDSA_KEY != ""
> +	select KEYTOC
> +
> +config CRYPTO_ECDSA_KEY
> +	depends on CRYPTO_ECDSA
> +	string "ECDSA key to compile in"
> +	help
> +	  This option should be a filename of a PEM-formatted file containing
> +	  X.509 certificates to be included into barebox. If the string starts
> +	  with "pkcs11:" it is interpreted as a PKCS#11 URI rather than a file.
> +
> +	  This avoids the mkimage dependency of CONFIG_BOOTM_FITIMAGE_PUBKEY
> +	  at the cost of an openssl build-time dependency.

Why can't this option take multiple space-separated paths?

> +static struct ecdsa_public_key *ecdsa_key_dup(const struct ecdsa_public_key *key)
> +{
> +	struct ecdsa_public_key *new;
> +	int key_size_bits;
> +
> +	key_size_bits = ecdsa_key_size(key->curve_name);
> +	if (!key_size_bits)
> +		return NULL;
> +
> +	new = xmemdup(key, sizeof(*key));
> +	new->x = xmemdup(key->x, key_size_bits / 8);
> +	new->y = xmemdup(key->y, key_size_bits / 8);
> +	new->size_bits = key_size_bits;

I think you'll want to explicitly initialize the linked list member here
instead of leaving it dangling.

> +
> +	return new;
> +}
> +
> +extern const struct ecdsa_public_key * const __ecdsa_keys_start;
> +extern const struct ecdsa_public_key * const __ecdsa_keys_end;

This looks odd. Shouldn't this be array of unknown size [].

> +
> +static int ecdsa_init_keys(void)
> +{
> +	const struct ecdsa_public_key * const *iter;

and this would be a single level pointer?

> +	struct ecdsa_public_key *key;
> +	int ret;
> +
> +	for (iter = &__ecdsa_keys_start; iter != &__ecdsa_keys_end; iter++) {
> +		key = ecdsa_key_dup(*iter);
> +		if (!key) {
> +			pr_err("Ignoring key with unknown curve_name %s\n", key->curve_name);
> +			continue;
> +		}
> +
> +		ret = ecdsa_key_add(key);
> +		if (ret)
> +			pr_err("Cannot add rsa key: %pe\n", ERR_PTR(ret));
> +	}
> +
> +	return 0;
> +}
ntifier: GPL-2.0-only
> +#ifndef _ECDSA_H
> +#define _ECDSA_H
> +
> +struct ecdsa_public_key {
> +	const char *curve_name;	/* Name of curve, e.g. "prime256v1" */
> +	const void *x;		/* x coordinate of public key */
> +	const void *y;		/* y coordinate of public key */

Why void and not a specific type?


Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |