[PATCH v7 01/15] ecdsa: fix support of secp521r1

[Philippe Reynes]

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 <philippe.reynes at softathome.com> 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 <raymondmaoca at gmail.com>
>> Signed-off-by: Philippe Reynes <philippe.reynes at softathome.com>
>>
>> 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 <sjg at chromium.org>
>
> 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/20260331100047.34618-6-philippe.reynes@softathome.com/).
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