[PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
Alex G.
mr.nuke.me at gmail.com
Thu Jan 7 17:27:50 CET 2021
On 1/7/21 6:35 AM, Simon Glass wrote:
> Hi Alexandru,
Hi Simon,
(pun alert!) A lot of your comments have to do with comments. I use
comments as a tool to add something of value to code. When the code is
self-documenting, comments don't help much.
See kernel coding style chapter 8.
What comments can do, is increase code size and break code flow -- there
is a cost to having them. I'm certain you've seen functions that need to
be scrolled up and down because of a huge comment smack in the middle.
I'll address individual comment comments below.
>
> On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me at gmail.com> wrote:
>>
>> mkimage supports rsa2048, and rsa4096 signatures. With newer silicon
>> now supporting hardware-accelerated ECDSA, it makes sense to expand
>> signing support to elliptic curves.
>>
>> Implement host-side ECDSA signing and verification with libcrypto.
>> Device-side implementation of signature verification is beyond the
>> scope of this patch.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>
>> ---
>> common/image-sig.c | 14 +-
>> include/u-boot/ecdsa.h | 27 ++++
>> lib/ecdsa/ecdsa-libcrypto.c | 300 ++++++++++++++++++++++++++++++++++++
>> tools/Makefile | 3 +
>> 4 files changed, 342 insertions(+), 2 deletions(-)
>> create mode 100644 include/u-boot/ecdsa.h
>> create mode 100644 lib/ecdsa/ecdsa-libcrypto.c
>>
>> diff --git a/common/image-sig.c b/common/image-sig.c
>> index 21dafe6b91..2548b3eb0f 100644
>> --- a/common/image-sig.c
>> +++ b/common/image-sig.c
>> @@ -15,6 +15,7 @@
>> DECLARE_GLOBAL_DATA_PTR;
>> #endif /* !USE_HOSTCC*/
>> #include <image.h>
>> +#include <u-boot/ecdsa.h>
>> #include <u-boot/rsa.h>
>> #include <u-boot/hash-checksum.h>
>>
>> @@ -82,8 +83,17 @@ struct crypto_algo crypto_algos[] = {
>> .sign = rsa_sign,
>> .add_verify_data = rsa_add_verify_data,
>> .verify = rsa_verify,
>> - }
>> -
>> + },
>> +#if defined(USE_HOSTCC)
>> + /* Currently, only host support exists for ECDSA */
>> + {
>> + .name = "ecdsa256",
>> + .key_len = ECDSA256_BYTES,
>> + .sign = ecdsa_sign,
>> + .add_verify_data = ecdsa_add_verify_data,
>> + .verify = ecdsa_verify,
>> + },
>> +#endif
>> };
>>
>> struct padding_algo padding_algos[] = {
>> diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
>> new file mode 100644
>> index 0000000000..a13a7267e1
>> --- /dev/null
>> +++ b/include/u-boot/ecdsa.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me at gmail.com>.
>> + */
>> +
>> +#ifndef _ECDSA_H
>> +#define _ECDSA_H
>> +
>> +#include <errno.h>
>> +#include <image.h>
>> +
>> +/**
>> + * crypto_algo API impementation for ECDSA;
>> + * @see "struct crypt_algo"
>> + * @{
>> + */
>> +int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
>> + int region_count, uint8_t **sigp, uint *sig_len);
>> +int ecdsa_verify(struct image_sign_info *info,
>> + const struct image_region region[], int region_count,
>> + uint8_t *sig, uint sig_len);
>> +int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
>
> Please always add full comments when you export functions, or have a
> non-trivial static function.
I disagree. These functions implement and are designed to be used via
the crypt_algo API. One should understand the crypt_algo API, and any
deviations in behavior would be a bug. So there is no scenario in which
comments here would be useful.
>> +/** @} */
>> +
>> +#define ECDSA256_BYTES (256 / 8)
>> +
>> +#endif
>> diff --git a/lib/ecdsa/ecdsa-libcrypto.c b/lib/ecdsa/ecdsa-libcrypto.c
>> new file mode 100644
>> index 0000000000..ff491411d0
>> --- /dev/null
>> +++ b/lib/ecdsa/ecdsa-libcrypto.c
>> @@ -0,0 +1,300 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * ECDSA image signing implementation using libcrypto backend
>> + *
>> + * The signature is a binary representation of the (R, S) points, padded to the
>> + * key size. The signature will be (2 * key_size_bits) / 8 bytes.
>> + *
>> + * Deviations from behavior of RSA equivalent:
>> + * - Verification uses private key. This is not technically required, but a
>> + * limitation on how clumsy the openssl API is to use.
>
> I'm not quite sure what the implications are on this. If this is
> public key crypto, the private key is not available, so how can you
> verify with it?
I presume this is fixable, but only as an academic exercise. This file
is for mkimage, which doesn't do standalone verification. The way you
verify is in u-boot. That is the subject of another series.
>> + * - Handling of keys and key paths:
>> + * - The '-K' key directory option must contain path to the key file,
>> + * instead of the key directory.
>
> If we make this change we should update RSA to do the same.
Of course, but is there agreement as to this direction? There seem to be
some hidden subtleties to key-name-hint that I don't fully understand yet.
>> + * - No assumptions are made about the file extension of the key
>> + * - The 'key-name-hint' property is only used for naming devicetree nodes,
>> + * but is not used for looking up keys on the filesystem.
>> + *
>> + * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me at gmail.com>
>> + */
>> +
>> +#include <u-boot/ecdsa.h>
>> +#include <u-boot/fdt-libcrypto.h>
>> +#include <openssl/ssl.h>
>> +#include <openssl/ec.h>
>> +#include <openssl/bn.h>
>> +
>> +struct signer {
>> + EVP_PKEY *evp_key;
>> + EC_KEY *ecdsa_key;
>> + void *hash;
>> + void *signature; /* Do not free() !*/
>
> need comments
Is this for the sake of having comments, or is there something of value
that I've missed? The only member that could be confusing is evp_key,
but that becomes obvious in the context of libcrypto.
>> +};
>> +
>> +static int alloc_ctx(struct signer *ctx, const struct image_sign_info *info)
>> +{
>> + memset(ctx, 0, sizeof(*ctx));
>> +
>> + if (!OPENSSL_init_ssl(0, NULL)) {
>> + fprintf(stderr, "Failure to init SSL library\n");
>> + return -1;
>> + }
>> +
>> + ctx->hash = malloc(info->checksum->checksum_len);
>> + ctx->signature = malloc(info->crypto->key_len * 2);
>> +
>> + if (!ctx->hash || !ctx->signature)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +static void free_ctx(struct signer *ctx)
>> +{
>> + if (ctx->ecdsa_key)
>> + EC_KEY_free(ctx->ecdsa_key);
>> +
>> + if (ctx->evp_key)
>> + EVP_PKEY_free(ctx->evp_key);
>> +
>> + if (ctx->hash)
>> + free(ctx->hash);
>> +}
>> +
>> +/*
>> + * Convert an ECDSA signature to raw format
>> + *
>> + * openssl DER-encodes 'binary' signatures. We want the signature in a raw
>> + * (R, S) point pair. So we have to dance a bit.
>> + */
>> +static void ecdsa_sig_encode_raw(void *buf, const ECDSA_SIG *sig, size_t order)
>> +{
>> + int point_bytes = order;
>> + const BIGNUM *r, *s;
>> + uintptr_t s_buf;
>> +
>> + ECDSA_SIG_get0(sig, &r, &s);
>> + s_buf = (uintptr_t)buf + point_bytes;
>> + BN_bn2binpad(r, buf, point_bytes);
>> + BN_bn2binpad(s, (void *)s_buf, point_bytes);
>> +}
>> +
>> +static ECDSA_SIG *ecdsa_sig_from_raw(void *buf, size_t order)
>> +{
>> + int point_bytes = order;
>> + uintptr_t s_buf;
>> + ECDSA_SIG *sig;
>> + BIGNUM *r, *s;
>> +
>> + sig = ECDSA_SIG_new();
>> + if (!sig)
>> + return NULL;
>> +
>> + s_buf = (uintptr_t)buf + point_bytes;
>> + r = BN_bin2bn(buf, point_bytes, NULL);
>> + s = BN_bin2bn((void *)s_buf, point_bytes, NULL);
>> + ECDSA_SIG_set0(sig, r, s);
>> +
>> + return sig;
>> +}
>> +
>> +static size_t ecdsa_key_size_bytes(const EC_KEY *key)
>
> I think all of these functions need a comment.
The function names are self-explanatory. One point of confusion would be
the meaning of raw signature -- this is already explained at the top.
>
>> +{
>> + const EC_GROUP *group;
>> +
>> + group = EC_KEY_get0_group(key);
>> + return EC_GROUP_order_bits(group) / 8;
>> +}
>> +
>> +static int read_key(struct signer *ctx, const char *key_name)
>> +{
>> + FILE *f = fopen(key_name, "r");
>> +
>> + if (!f) {
>> + fprintf(stderr, "Can not get key file '%s'\n", key_name);
>> + return -1;
>
> return -EIO perhaps?
Good idea!
>> + }
>> +
>> + ctx->evp_key = PEM_read_PrivateKey(f, NULL, NULL, NULL);
>> + fclose(f);
>> + if (!ctx->evp_key) {
>> + fprintf(stderr, "Can not read key from '%s'\n", key_name);
>> + return -1;
>
> These should return useful error number: -1 is -EPERM which doesn't
> seem right here.
-EIO again?
>> + }
>> +
>> + if (EVP_PKEY_id(ctx->evp_key) != EVP_PKEY_EC) {
>> + fprintf(stderr, "'%s' is not an ECDSA key\n", key_name);
>> + return -1;
>> + }
>> +
>> + ctx->ecdsa_key = EVP_PKEY_get1_EC_KEY(ctx->evp_key);
>> + if (!ctx->ecdsa_key)
>> + fprintf(stderr, "Can not extract ECDSA key\n");
>> +
>> + return (ctx->ecdsa_key) ? 0 : -1;
>> +}
>> +
>> +static int prepare_ctx(struct signer *ctx, const struct image_sign_info *info)
>> +{
>> + const char *kname = info->keydir;
>> + int key_len_bytes;
>> +
>> + if (alloc_ctx(ctx, info) < 0)
>
> That function returns 0 on success, so you don't need the < 0. A
> comment on the above function would make that clear.
I think the following would be less clear:
if (alloc_ctx())
error();
Does alloc_ctx() return an int? Does it return memory? It has (m)alloc
in the name. By having a '< 0' in the predicate, it's now clear that
this can't return a pointer. So yes, you might scratch your head as to
why there's a '< 0' that's not needed. Also, you know that this function
returns an error code without needing to scroll up to its definition. A
comment above the function wouldn't eliminate the need to scroll up.
>> + return -1;
>> +
>> + if (read_key(ctx, kname) < 0)
>> + return -1;
>> +
>> + key_len_bytes = ecdsa_key_size_bytes(ctx->ecdsa_key);
>> + if (key_len_bytes != info->crypto->key_len) {
>> + fprintf(stderr, "Expected a %u-bit key, got %u-bit key\n",
>> + info->crypto->key_len * 8, key_len_bytes * 8);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> [..]
>
> Regards,
> Simon
>
More information about the U-Boot
mailing list