Hi Philippe, On 2026-05-28T08:19:01, Philippe Reynes <[email protected]> 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 <[email protected]> > Signed-off-by: Philippe Reynes <[email protected]> > > 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 <[email protected]> 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
