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

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

Hi,

On 05.08.24 14:44, Sascha Hauer wrote:
> 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.

Ok, let's leave it to behave like the RSA options then.

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

This looks certainly odd. It's probably worth looking into.

Cheers,
Ahmad

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