[PATCH v7 01/15] ecdsa: fix support of secp521r1

[Simon Glass]

Hi Philippe,

On 2026-05-28T08:19:01, Philippe Reynes <philippe.reynes at softathome.com> wrote:
> ecdsa: fix support of secp521r1
>
> Current implementation of ecdsa only supports key len aligned on
> 8 bits. But the curve secp521r1 uses a key of 521 bits which is not
> aligned on 8 bits. In this commit, we update the keys management
> for ecdsa to support keys that are not aligned on 8 bits.
>
> Reviewed-by: Raymond Mao <raymondmaoca at gmail.com>
> Signed-off-by: Philippe Reynes <philippe.reynes at softathome.com>
>
> lib/ecdsa/ecdsa-libcrypto.c | 65 +++++++++++++++++++++++++++++++++++++++++++--
>  lib/ecdsa/ecdsa-verify.c    | 65 ++++++++++++++++++++++++++++++++++++++++++---
>  lib/fdt-libcrypto.c         |  2 +-
>  tools/image-sig-host.c      |  7 +++++
>  4 files changed, 132 insertions(+), 7 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

questions / nits below

> diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
> @@ -41,10 +43,26 @@ struct ecdsa_public_key {
> +static char *memdup(char *buf, size_t size)
> +{
> +     char *dup;
> +
> +     dup = malloc(size);
> +     if (dup)
> +             memcpy(dup, buf, size);
> +
> +     return dup;
> +}

Please match the U-Boot signature: void *memdup(const void *src,
size_t len) (see include/linux/string.h). Making buf const lets the
call sites lose the (char *) casts. Also note that fdt_get_key() now
hands back malloc'd buffers - please spell out the caller's free
responsibility in a function comment.

> diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
> @@ -135,6 +180,18 @@ U_BOOT_CRYPTO_ALGO(ecdsa384) = {
>       .verify = ecdsa_verify,
>  };
>
> +U_BOOT_CRYPTO_ALGO(ecdsa521) = {
> +     .name = 'ecdsa521',
> +     .key_len = ECDSA521_BYTES,
> +     .verify = ecdsa_verify,
> +};
> +
> +U_BOOT_CRYPTO_ALGO(secp521r1) = {
> +     .name = 'secp521r1',
> +     .key_len = ECDSA521_BYTES,
> +     .verify = ecdsa_verify,
> +};

I'm not entirely clear why there are two entries here?

> diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
> @@ -50,15 +57,48 @@ static int fdt_get_key(struct ecdsa_public_key *key, const void *fdt, int node)
> +     x = memdup((char *)key->x + (x_len - expected_len), expected_len);
> +     if (!x) {
> +             debug("Cannot allocate memory for point X");
> +             return -ENOMEM;
> +     }
> +     key->x = (const uint8_t *)x;
> +
> +     y = memdup((char *)key->y + (y_len - expected_len), expected_len);
> +     if (!y) {
> +             debug("Cannot allocate memory for point Y");
> +             free((char *)x);
> +             return -ENOMEM;
> +     }
> +     key->y = (const uint8_t *)y;

A few things:

1. You should be able to drop the char * casts for memdup()
2. When the y allocation fails you free x but leave key->x pointing at
the freed buffer, so a later fdt_free_key() will double-free. Please
set key->x = NULL after the free.
3. The debug() messages are missing a trailing newline.

> diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
> @@ -50,15 +57,48 @@ static int fdt_get_key(struct ecdsa_public_key *key, const void *fdt, int node)
> -     if (x_len != (key->size_bits / 8) || y_len != (key->size_bits / 8)) {
> +     /*
> +      * The public key is stored as an array of u32, so if the key size is
> +      * not a multiple of 32 (for example 521), we may have extra bytes.
> +      * To avoid any issue later, we shift the x and y pointer to the first
> +      * useful byte.
> +      */
> +     expected_len = DIV_ROUND_UP(key->size_bits, 8);
> +
> +     if (x_len < expected_len || y_len < expected_len) {

The previous check was !=; you have loosened it to <

The padded length is known exactly, I think - isn't it
DIV_ROUND_UP(key->size_bits, 32) * 4. ? So could we check the exact
value to avoid silently accepting (and truncating) over-sized
properties for prime256v1/secp384r1 ?

> diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
> @@ -130,11 +189,13 @@ static int read_key_from_fdt(struct signer *ctx, const void *fdt, int node)
>               fprintf(stderr, "Failed to set EC_POINT as public key\n");
>               EC_POINT_free(point);
>               EC_KEY_free(ec_key);
> +             fdt_free_key(&pubkey);
>               return -EINVAL;
>       }

The repeated EC_POINT_free(point); EC_KEY_free(ec_key);
fdt_free_key(&pubkey); return -EINVAL; across five exit paths is
begging for a goto-style cleanup at some point in the future :-)

Regards,
Simon