[PATCH v7 06/15] test: dm: ecdsa.c: clean this test as software ecdsa is now implemented

[Simon Glass]

Hi Philippe,

On 2026-05-28T08:19:01, Philippe Reynes <philippe.reynes at softathome.com> wrote:
> test: dm: ecdsa.c: clean this test as software ecdsa is now implemented
>
> The test ecdsa was done when ecdsa was only supported by hardware.
> So it wasn't possible to test ecdsa on sandbox, and there is a test
> to check that ecdsa is not supported on sandbox.
> Now, there is a software implementation of ecdsa. So we add a test
> to verify that ecdsa_verify may be used on sandbox.
>
> Signed-off-by: Philippe Reynes <philippe.reynes at softathome.com>
>
> test/dm/ecdsa.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 99 insertions(+), 10 deletions(-)

Thanks for the test!

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

nits / thoughts below

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +#define CHECK(op) ({                                                         \
> +             int err = op;                                                   \
> +             if (err < 0) {                                                  \
> +                     printf("%s: function %s returns an error (%d)\n",       \
> +                            __func__, #op, err);                             \
> +                     return err;                                             \
> +             }                                                               \
> +                                                                             \
> +             err;                                                            \
> +     })

We normally handle this short of thing just with a ut_assertok() call.
It reports the error number, line number and the code that fails so it
should be good enough.

In other words, instead of:

+       CHECK(fdt_create(fdt, size));

you can do:

   ut_assertok(fdt_create(fdt, size));

Also this:
+       /* create a fdt with the public key */
+       ut_asserteq(0, create_fdt_with_ecdsa_key(fdt, sizeof(fdt),
name, curve, x, y));
+

should really use utt_assertok() - the asserteq is for when we are
checking for a particular integer value. Same with ecdsa_verify()

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +static int set_fdt_ecdsa_point(char *fdt, char *name, const char *data, size_t len)
> +{
> +     char *value = NULL;
> +     int ret = 0;
> +
> +     if (!fdt || !name || !data) {
> +             ret = -EINVAL;
> +             goto out;
> +     }

name should be const char * since every caller passes a string
literal. The !fdt / !name / !data check is dead code but OK if you
want to keep it.

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +     value = malloc(len / 2);
> +     if (!value) {
> +             ret = -ENOMEM;
> +             goto out;
> +     }
> +
> +     ret = hex2bin(value, data, len / 2);

Passing the hex-string length and dividing by two inside the function
is an odd interface - the binary length is what actually matters. I'd
rather you took the binary length (or just took data and called
strlen() internally) so the /2 doesn't appear three times. What do you
think?

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +     if (ret)
> +             goto out;
> +
> + out:
> +     free(value);
> +     return ret;
> +}

Label should sit in column 1, i.e. with no space before

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +     const char *r = "EFD48B2AACB6A8FD1140DD9CD45E81D69D2C877B56AAF991C34D0EA84EAF3716";
> +     const char *s = "F7CB1C942D657C41D436C7A1B6E29F65F3E900DBB9AFF4064DC4AB2F843ACDA8";
> +     u8 sig[64];
> +     char fdt[FDT_MAX_SIZE];

You could put those in a shared header? Also please use lower-case hex
if you can.

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
> +     /* prepare the signature */
> +     ut_asserteq(0, hex2bin(sig +  0, r, strlen(r) / 2));
> +     ut_asserteq(0, hex2bin(sig + 32, s, strlen(s) / 2));

Stray double space in sig +  0

> diff --git a/test/dm/ecdsa.c b/test/dm/ecdsa.c
> @@ -3,36 +3,125 @@
>  /*
>   * Basic test of the ECDSA uclass and ecdsa_verify()
>   *
> - * ECDSA implementations in u-boot are hardware-dependent. Until we have a
> - * software implementation that can be compiled into the sandbox, all we can
> - * test is the uclass support.
> + * ECDSA software implementation is tested in another test,
> + * so we only check that the class UCLASS_ECDSA may be used.
>   *
> - * The uclass_get() test is redundant since ecdsa_verify() would also fail. We
> - * run both functions in order to isolate the cause more clearly. i.e. is
> - * ecdsa_verify() failing because the UCLASS is absent/broken?
> + * The data used in this test come from RFC6979

Nit: 'the class UCLASS_ECDSA' reads oddly - 'the UCLASS_ECDSA uclass'
is the usual phrasing. Also please mention which curve/vector (P-256,
SHA-256 sample) so a future reader knows where the magic hex came from
without chasing the RFC.

Regards,
Simon