Hi Philippe, On 2026-05-28T08:19:01, Philippe Reynes <[email protected]> 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 <[email protected]> > > test/dm/ecdsa.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 99 insertions(+), 10 deletions(-)
Thanks for the test! Reviewed-by: Simon Glass <[email protected]> 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
