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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Aug 05, 2024 at 01:57:36PM +0200, Ahmad Fatoum wrote:
> 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?

The code added for ECDSA is mostly a copy from the existing RSA code.

It's less than ideal. What I'd really like to have is a single list of
keys which can include both RSA and ECDSA keys instead of maintaining
multiple lists. Likewise for the Kconfig options, it would be better to
have a CRYPTO_PUBLIC_KEYS option which holds multiple RSA and/or
ECDSA keys. Unfortunately my time budget for this task is over, so I
think we'll have to stick with this until the next cleanup round.

> > +#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?

No specific reason, it's copied from U-Boot. One reason might be that
keytoc prints the values as array of uint32_t whereas the values in
barebox are interpreted as array of uint64_t. Using void * avoids casts
and covers some interesting endianess problems when the barebox
endianess differs from the endianess of the build machine.

Sascha

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