From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex G. <mr.nuke.me@gmail.com>
Date: Tue, 9 Feb 2021 16:58:56 -0600
Subject: [PATCH 3/5] lib: ecdsa: Implement signature verification for
 crypto_algo API
In-Reply-To: <5b40bb7d-24e5-ad47-ae1c-de65c945fd64@foss.st.com>
References: <20210111154137.621732-1-mr.nuke.me@gmail.com>
 <20210111154137.621732-4-mr.nuke.me@gmail.com>
 <5b40bb7d-24e5-ad47-ae1c-de65c945fd64@foss.st.com>
Message-ID: <66015184-ddaf-d222-a531-bc50b1227a72@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de


On 2/9/21 9:56 AM, Patrick DELAUNAY wrote:
> Hi,

[snip]

>> diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
>> index d2e6a40f4a..d84f6eb093 100644
>> --- a/lib/ecdsa/ecdsa-verify.c
>> +++ b/lib/ecdsa/ecdsa-verify.c
>> @@ -1,13 +1,128 @@
>> ? // SPDX-License-Identifier: GPL-2.0+
>> ? /*
>> + * ECDSA signature verification for u-boot
>> + *
>> + * This implements the firmware-side wrapper for ECDSA verification. 
>> It bridges
>> + * the struct crypto_algo API to the ECDSA uclass implementations.
>> + *
>> ?? * Copyright (c) 2020, Alexandru Gagniuc<mr.nuke.me@gmail.com>
>> ?? */
> 
> http://www.denx.de/wiki/U-Boot/CodingStyle #Include files
> 
> #include <crypto/ecdsa-uclass.h>
> 
> it is normally the first in alphabetic order of directory

Thank your for catching that. I will have it fixed in the next iteration 
of this series.

>> +#include <dm/uclass.h>
>> ? #include <u-boot/ecdsa.h>
>> +#include <crypto/ecdsa-uclass.h>
>> +
>> +/*
>> + * Derive size of an ECDSA key from the curve name
>> + *
>> + * While it's possible to extract the key size by using string 
>> manipulation,
>> + * use a list of known curves for the time being.
>> + */
>> +static int ecdsa_key_size(const char *curve_name)
>> +{
>> +??? if (!strcmp(curve_name, "prime256v1"))
>> +??????? return 256;
>> +??? else
>> +??????? return 0;
>> +}
>> +
> 
> 
> To prepare the future can you parse a array of supported curves with 
> associated ID
> 
> used as parameter of ECDSA parameter = enum ECDSA_CURVES
> 
> for example: char * name, int size, enum ECDSA_CURVES
> 
> const [] = {
> {"prime256v1", 256, ECDSA_PRIME256V1 },
> 
> }

That is possible. If I were to have a longer list of curve names, I 
change things to extract the key length from the name itself. So instead 
of running strncmp() of N keyname strings, I would extract the digits 
from 'curve_name'.

I chose not to do that here because I want this patch to be didactic. 
That is, I'm trying to achieve the goal clearly, with the lowest number 
of lines of code.

[snip]

>> +static int ecdsa_verify_hash(struct udevice *dev,
>> +???????????????? const struct image_sign_info *info,
>> +???????????????? const void *hash, const void *sig, uint sig_len)
>> +{
>> +??? const struct ecdsa_ops *ops = device_get_ops(dev);
>> +??? const struct checksum_algo *algo = info->checksum;
>> +??? struct ecdsa_public_key key;
>> +??? int sig_node, key_node, ret;
>> +
>> +??? if (!ops || !ops->verify)
>> +??????? return -ENODEV;
>> +
>> +??? if (info->required_keynode > 0) {
>> +??????? ret = fdt_get_key(&key, info->fdt_blob, info->required_keynode);
>> +??????? if (ret < 0)
>> +??????????? return ret;
>> +
>> +??????? return ops->verify(dev, &key, hash, algo->checksum_len,
>> +?????????????????? sig, sig_len);
> 
> 
> Need to indicate the used curve here as parameter of the verify opts ?

The curve name is part of the key (struct ecdsa_public_key).


[snip]
>> +??????? ret = ops->verify(dev, &key, hash, algo->checksum_len,
>> +????????????????? sig, sig_len);
>> +
>> +??????? /* On success, don't worry about remaining keys */
>> +??????? if (ret == 0)
> 
> 
> issue raised by chekpatch I think
> 
> if (!ret)

Oh! I'll get this fixed. Thanks!

Alex