Hi Simon,
Le 29/05/2026 à 09:16, Simon Glass a écrit :
This Mail comes from Outside of SoftAtHome: Do not answer, click links or open
attachments unless you recognize the sender and know the content is safe.
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?
Without this serie, there is an initial (broken) support of ecdsa 521.
It was added only to tools/image-sig-host.c, but with the name if
"secp521r1" instead of "ecdsa521".
In my first proposal, I have changed this "secp521r1" to "ecdsa521".
Then, in the review, You and Raymond
were worried about breaking existing code (see:
https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/).
So so avoid any issue, I have added the support of secp521r1 and ecdsa521.
Best regards,
Philippe
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
