[PATCH v7 03/15] ecdsa: initial support of ecdsa using mbedtls

[Simon Glass]

Hi Philippe,

On 2026-05-28T08:19:01, Philippe Reynes <philippe.reynes at softathome.com> wrote:
> ecdsa: initial support of ecdsa using mbedtls
>
> Adds an initial support of ecdsa verify using mbedtls.
>
> Reviewed-by: Raymond Mao <raymondmaoca at gmail.com>
> Signed-off-by: Philippe Reynes <philippe.reynes at softathome.com>
>
> configs/sandbox_defconfig       |   1 +
>  include/crypto/ecdsa-uclass.h   |  15 +----
>  include/crypto/internal/ecdsa.h |  39 +++++++++++
>  lib/mbedtls/Kconfig             |   7 ++
>  lib/mbedtls/Makefile            |   3 +
>  lib/mbedtls/ecdsa.c             | 146 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 197 insertions(+), 14 deletions(-)

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

nits below

> diff --git a/include/crypto/internal/ecdsa.h b/include/crypto/internal/ecdsa.h
> @@ -0,0 +1,39 @@
> +/**
> + *
> + * ecdsa_hash_verify() - Verify the ecdsa signature of a hash
> + *
> + * @pubkey: ecdsa public key
> + * @hash: Hash
> + * @hash_len: Size of the hash
> + * @signature: Signature
> + * @sig_len: Size of the signature
> + * Return: 0 if all verified ok, <0 on error
> + */

Please drop the stray blank line at the start - the function
description should be on the first line after /**. Also a blank line
is missing before Return: so it gets folded into the @sig_len
description.

> diff --git a/lib/mbedtls/ecdsa.c b/lib/mbedtls/ecdsa.c
> @@ -0,0 +1,146 @@
> +static mbedtls_ecp_group_id ecdsa_search_group_id(const char *curve_name)
> +{
> +     mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE;
> +     const mbedtls_ecp_curve_info *info;
> +
> +     if (!curve_name)
> +             goto out;
> +
> +     if (!strcmp(curve_name, 'prime256v1'))
> +             return MBEDTLS_ECP_DP_SECP256R1;

A brief comment would help here  it is not obvious why prime256v1 is
singled out. My understanding is that it is the OpenSSL alias for
secp256r1 (which is what FIT keys carry), while
mbedtls_ecp_curve_list() only exposes the secp256r1 name. Worth saying
so.

Also, since the only function of the local grp_id is to return
MBEDTLS_ECP_DP_NONE on the !curve_name path, this could be simplified
to an early return MBEDTLS_ECP_DP_NONE; and dropping the out: label
entirely.

> diff --git a/lib/mbedtls/ecdsa.c b/lib/mbedtls/ecdsa.c
> @@ -0,0 +1,146 @@
> +     mbedtls_mpi r, s;
> +     int key_len;
> +     int err = -1;
> +
> +     key_len = DIV_ROUND_UP(pubkey->size_bits, 8);

The err = -1 initialiser is dead - every path to out: sets err
explicitly first. Please drop it, or use a meaningful -Exxx value as a
safety default.

> diff --git a/lib/mbedtls/ecdsa.c b/lib/mbedtls/ecdsa.c
> @@ -0,0 +1,146 @@
> + out4:
> +     mbedtls_mpi_free(&s);
> + out3:
> +     mbedtls_mpi_free(&r);
> + out2:
> +     mbedtls_ecp_point_free(&Q);
> + out1:
> +     mbedtls_ecp_group_free(&grp);
> + out:
> +
> +     return err;
> +}

Please name the labels after what they do rather than counting them -
e.g. free_s, free_r, free_q, free_grp, out. As-is, every edit has to
renumber them, and a reader has to scroll up to figure out what out3
protects. Also drop the blank line between out: and return err;.

Regards,
Simon