Hi Philippe, On Mon, May 25, 2026 at 9:52 AM Philippe Reynes <[email protected]> wrote: > > 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. > > Signed-off-by: Philippe Reynes <[email protected]> > --- > v2: > - intitial version > v3: > - fix typo in comments > v4: > - fix commit message > - clean code with DIV_ROUND_UP- > - duplicate data before shifting > - support ecdsa521 and secp521r1 > - clean code > v5: > - check that x and y are not null before using them > - free keys when keys not found > -v6: > - free x and y if x and y are not null > - do not free key when an error has occured > > 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(-) >
Looks good to me, thanks! Reviewed-by: Raymond Mao <[email protected]> > diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c > index c4bfb2cec61..89c1851b71d 100644 > --- a/lib/ecdsa/ecdsa-libcrypto.c > +++ b/lib/ecdsa/ecdsa-libcrypto.c > @@ -26,6 +26,8 @@ > #include <openssl/ec.h> > #include <openssl/bn.h> > > +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > + > /* Image signing context for openssl-libcrypto */ > struct signer { > EVP_PKEY *evp_key; /* Pointer to EVP_PKEY object */ > @@ -41,10 +43,26 @@ struct ecdsa_public_key { > int size_bits; > }; > > +static char *memdup(char *buf, size_t size) > +{ > + char *dup; > + > + dup = malloc(size); > + if (dup) > + memcpy(dup, buf, size); > + > + return dup; > +} > + > static int fdt_get_key(struct ecdsa_public_key *key, const void *fdt, int > node) > { > + const char *x; > + const char *y; > int x_len; > int y_len; > + int expected_len; > + > + memset(key, 0, sizeof(*key)); > > key->curve_name = fdt_getprop(fdt, node, "ecdsa,curve", NULL); > if (!key->curve_name) > @@ -54,6 +72,8 @@ static int fdt_get_key(struct ecdsa_public_key *key, const > void *fdt, int node) > key->size_bits = 256; > else if (!strcmp(key->curve_name, "secp384r1")) > key->size_bits = 384; > + else if (!strcmp(key->curve_name, "secp521r1")) > + key->size_bits = 521; > else > return -EINVAL; > > @@ -63,12 +83,45 @@ static int fdt_get_key(struct ecdsa_public_key *key, > const void *fdt, int node) > if (!key->x || !key->y) > return -EINVAL; > > - 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) > return -EINVAL; > > + x = memdup((char *)key->x + (x_len - expected_len), expected_len); > + if (!x) { > + fprintf(stderr, "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) { > + fprintf(stderr, "Cannot allocate memory for point Y"); > + free((char *)x); > + return -ENOMEM; > + } > + key->y = (const uint8_t *)y; > + > return 0; > } > > +static void fdt_free_key(struct ecdsa_public_key *key) > +{ > + if (!key) > + return; > + if (key->x) > + free((char *)key->x); > + if (key->y) > + free((char *)key->y); > +} > + > static int read_key_from_fdt(struct signer *ctx, const void *fdt, int node) > { > struct ecdsa_public_key pubkey; > @@ -89,8 +142,11 @@ static int read_key_from_fdt(struct signer *ctx, const > void *fdt, int node) > nid = NID_X9_62_prime256v1; > } else if (!strcmp(pubkey.curve_name, "secp384r1")) { > nid = NID_secp384r1; > + } else if (!strcmp(pubkey.curve_name, "secp521r1")) { > + nid = NID_secp521r1; > } else { > fprintf(stderr, "Unsupported curve name: '%s'\n", > pubkey.curve_name); > + fdt_free_key(&pubkey); > return -EINVAL; > } > > @@ -100,6 +156,7 @@ static int read_key_from_fdt(struct signer *ctx, const > void *fdt, int node) > ec_key = EC_KEY_new_by_curve_name(nid); > if (!ec_key) { > fprintf(stderr, "Failed to allocate EC_KEY for curve %s\n", > pubkey.curve_name); > + fdt_free_key(&pubkey); > return -ENOMEM; > } > > @@ -108,10 +165,11 @@ static int read_key_from_fdt(struct signer *ctx, const > void *fdt, int node) > if (!point) { > fprintf(stderr, "Failed to allocate EC_POINT\n"); > EC_KEY_free(ec_key); > + fdt_free_key(&pubkey); > return -ENOMEM; > } > > - len = pubkey.size_bits / 8; > + len = DIV_ROUND_UP(pubkey.size_bits, 8); > > uint8_t buf[1 + len * 2]; > > @@ -123,6 +181,7 @@ static int read_key_from_fdt(struct signer *ctx, const > void *fdt, int node) > fprintf(stderr, "Failed to convert (x,y) point to > EC_POINT\n"); > EC_POINT_free(point); > EC_KEY_free(ec_key); > + fdt_free_key(&pubkey); > return -EINVAL; > } > > @@ -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; > } > > fprintf(stderr, "Successfully loaded ECDSA key from FDT node %d\n", > node); > EC_POINT_free(point); > + fdt_free_key(&pubkey); > ctx->ecdsa_key = ec_key; > > return 0; > diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c > index 629b662cf6c..e58b8892b98 100644 > --- a/lib/ecdsa/ecdsa-verify.c > +++ b/lib/ecdsa/ecdsa-verify.c > @@ -10,6 +10,7 @@ > > #include <crypto/ecdsa-uclass.h> > #include <dm/uclass.h> > +#include <malloc.h> > #include <u-boot/ecdsa.h> > > /* > @@ -24,13 +25,19 @@ static int ecdsa_key_size(const char *curve_name) > return 256; > else if (!strcmp(curve_name, "secp384r1")) > return 384; > + else if (!strcmp(curve_name, "secp521r1")) > + return 521; > > return 0; > } > > static int fdt_get_key(struct ecdsa_public_key *key, const void *fdt, int > node) > { > - int x_len, y_len; > + int expected_len, x_len, y_len; > + const char *x; > + const char *y; > + > + memset(key, 0, sizeof(*key)); > > key->curve_name = fdt_getprop(fdt, node, "ecdsa,curve", NULL); > if (!key->curve_name) { > @@ -50,15 +57,48 @@ static int fdt_get_key(struct ecdsa_public_key *key, > const void *fdt, int node) > if (!key->x || !key->y) > return -EINVAL; > > - 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) { > printf("%s: node=%d, curve@%p x@%p+%i y@%p+%i\n", __func__, > node, key->curve_name, key->x, x_len, key->y, y_len); > return -EINVAL; > } > > + 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; > + > return 0; > } > > +static void fdt_free_key(struct ecdsa_public_key *key) > +{ > + if (!key) > + return; > + if (key->x) > + free((char *)key->x); > + if (key->y) > + free((char *)key->y); > +} > + > static int ecdsa_verify_hash(struct udevice *dev, > const struct image_sign_info *info, > const void *hash, const void *sig, uint sig_len) > @@ -76,8 +116,11 @@ static int ecdsa_verify_hash(struct udevice *dev, > if (ret < 0) > return ret; > > - return ops->verify(dev, &key, hash, algo->checksum_len, > - sig, sig_len); > + ret = ops->verify(dev, &key, hash, algo->checksum_len, > + sig, sig_len); > + fdt_free_key(&key); > + > + return ret; > } > > sig_node = fdt_subnode_offset(info->fdt_blob, 0, FIT_SIG_NODENAME); > @@ -93,6 +136,8 @@ static int ecdsa_verify_hash(struct udevice *dev, > ret = ops->verify(dev, &key, hash, algo->checksum_len, > sig, sig_len); > > + fdt_free_key(&key); > + > /* On success, don't worry about remaining keys */ > if (!ret) > return 0; > @@ -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, > +}; > + > /* > * uclass definition for ECDSA API > * > diff --git a/lib/fdt-libcrypto.c b/lib/fdt-libcrypto.c > index ecb0344c8f6..090246b44e9 100644 > --- a/lib/fdt-libcrypto.c > +++ b/lib/fdt-libcrypto.c > @@ -10,7 +10,7 @@ > int fdt_add_bignum(void *blob, int noffset, const char *prop_name, > BIGNUM *num, int num_bits) > { > - int nwords = num_bits / 32; > + int nwords = (num_bits + 31) / 32; > int size; > uint32_t *buf, *ptr; > BIGNUM *tmp, *big2, *big32, *big2_32; > diff --git a/tools/image-sig-host.c b/tools/image-sig-host.c > index 5285263c616..285547994ca 100644 > --- a/tools/image-sig-host.c > +++ b/tools/image-sig-host.c > @@ -83,6 +83,13 @@ struct crypto_algo crypto_algos[] = { > .add_verify_data = ecdsa_add_verify_data, > .verify = ecdsa_verify, > }, > + { > + .name = "ecdsa521", > + .key_len = ECDSA521_BYTES, > + .sign = ecdsa_sign, > + .add_verify_data = ecdsa_add_verify_data, > + .verify = ecdsa_verify, > + }, > { > .name = "secp521r1", > .key_len = ECDSA521_BYTES, > -- > 2.43.0 >
