* [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test)
@ 2020-12-30 21:00 Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c Alexandru Gagniuc
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
# Introduction
This series is part of a larger effort to implement verified boot
on STM32MP1.The purpose of this series is to let people know I'm
looking into ECDSA.
## Purpose and intent
The ROM code on the STM32MP requires an ECDSA-signed FSBL. Maintaining
verified boot through FIT images would require switching to an RSA key
after SPL. This would be stupid, so this series is focused on enabling
ECDSA signing. The use case that I am focused on is signing an
existing FIT image:
mkimage -F some-existing.fit --signing-key some/key.pem
I don't care about signing while assembling the FIT. The reason is
that I want the machine that builds things to be separate from the
machine that has access to the super-secret-key.pem.
Astute readers may have noticed the "uselessness" of this series due
to the lack of a device-side implementation. I don't plan to write out
the algorithm for ECDSA, but instead use the CRYP engine of the stm32mp,
or the ROM services. This is a matter for another series.
# Implementation
I initially tried to model this after the RSA implementation
(rsa-sign.c), but that didn't go well for a few reasons:
(a) The openssl/libcrypto API is a pain in the ass
(b) The RSA path doesn't have a way to pass a specific key file.
On point (a), I don't want to spend too much time battling a C API for
crypto. I find pyCryptodomex to be vastly superior, but that is not
available for mkimage. I am thus focusing on the simple case of
key in, signature out.
On point (b), the RSA path takes the FDT property 'key-name-hint' to
decide which key file to read from disk. In the context of "which fdt
node describes my signing key", this makes sense. On the other hand,
'key-name-hint' is also used as the basename of where the key is on the
filesystem. This leads to some funny search paths, such as
"some/dir/(null).key"
So I am using the -K option to mkimage as the _full_ path to the key
file. It doesn't have to be named .key, it doesn't have to be named
.crt, and it doesn't have to exist in a particular directory (as is
the case for the RSA path). Take that as is for here -- we can discuss
the merits of this in a separate thread.
A bonus point is that I have decided to keep signin/verifying in the
same source file. This allows me to reuse some helper functions. I'm
only adding 300 lines of code, so I don't see the point in splitting
it up.
# Testing
test/py/tests/test_fit_ecdsa.py is implementing a test for mkimage. It
lets mkimage run wild, tehn verifies the signature against
pyCryptodomex -- see earlier point on for I didn't use openssl.
Alexandru Gagniuc (5):
lib: Rename rsa-checksum.c to hash-checksum.c
lib/rsa: Make fdt_add_bignum() available outside of RSA code
lib: Add support for ECDSA image signing
doc: signature.txt: Document devicetree format for ECDSA keys
test/py: ecdsa: Add test for mkimage ECDSA signing
common/image-fit-sig.c | 2 +-
common/image-sig.c | 16 +-
doc/uImage.FIT/signature.txt | 7 +-
include/image.h | 2 +-
include/u-boot/ecdsa.h | 27 ++
include/u-boot/fdt-libcrypto.h | 15 +
.../{rsa-checksum.h => hash-checksum.h} | 0
lib/Makefile | 1 +
lib/crypto/pkcs7_verify.c | 2 +-
lib/crypto/x509_public_key.c | 2 +-
lib/ecdsa/ecdsa-libcrypto.c | 300 ++++++++++++++++++
lib/fdt-libcrypto.c | 72 +++++
lib/{rsa/rsa-checksum.c => hash-checksum.c} | 3 +-
lib/rsa/Makefile | 2 +-
lib/rsa/rsa-sign.c | 65 +---
test/py/tests/test_fit_ecdsa.py | 111 +++++++
tools/Makefile | 7 +-
17 files changed, 559 insertions(+), 75 deletions(-)
create mode 100644 include/u-boot/ecdsa.h
create mode 100644 include/u-boot/fdt-libcrypto.h
rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
create mode 100644 lib/ecdsa/ecdsa-libcrypto.c
create mode 100644 lib/fdt-libcrypto.c
rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)
create mode 100644 test/py/tests/test_fit_ecdsa.py
--
2.26.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
@ 2020-12-30 21:00 ` Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code Alexandru Gagniuc
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
rsa-checksum.c sontains the hash_calculate() implementations. Despite
the "rsa-" file prefix, this function is useful for other algorithms.
To prevent confusion, move this file to lib/crypto, and rename it to
hash-checksum.c, to give it a more "generic" feel.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
common/image-fit-sig.c | 2 +-
common/image-sig.c | 2 +-
include/image.h | 2 +-
include/u-boot/{rsa-checksum.h => hash-checksum.h} | 0
lib/Makefile | 1 +
lib/crypto/pkcs7_verify.c | 2 +-
lib/crypto/x509_public_key.c | 2 +-
lib/{rsa/rsa-checksum.c => hash-checksum.c} | 3 ++-
lib/rsa/Makefile | 2 +-
tools/Makefile | 3 ++-
10 files changed, 11 insertions(+), 8 deletions(-)
rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 5401d9411b..7fcbb47235 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -15,7 +15,7 @@ DECLARE_GLOBAL_DATA_PTR;
#include <fdt_region.h>
#include <image.h>
#include <u-boot/rsa.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
#define IMAGE_MAX_HASHED_NODES 100
diff --git a/common/image-sig.c b/common/image-sig.c
index f3c209ae8b..21dafe6b91 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -16,7 +16,7 @@ DECLARE_GLOBAL_DATA_PTR;
#endif /* !USE_HOSTCC*/
#include <image.h>
#include <u-boot/rsa.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
#define IMAGE_MAX_HASHED_NODES 100
diff --git a/include/image.h b/include/image.h
index 00bc03bebe..d1a0b3d9f6 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1258,7 +1258,7 @@ struct image_region {
};
#if IMAGE_ENABLE_VERIFY
-# include <u-boot/rsa-checksum.h>
+# include <u-boot/hash-checksum.h>
#endif
struct checksum_algo {
const char *name;
diff --git a/include/u-boot/rsa-checksum.h b/include/u-boot/hash-checksum.h
similarity index 100%
rename from include/u-boot/rsa-checksum.h
rename to include/u-boot/hash-checksum.h
diff --git a/lib/Makefile b/lib/Makefile
index 851a80ef3b..cf64188ba5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -60,6 +60,7 @@ endif
obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
obj-$(CONFIG_$(SPL_)MD5) += md5.o
obj-$(CONFIG_$(SPL_)RSA) += rsa/
+obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
obj-$(CONFIG_SHA1) += sha1.o
obj-$(CONFIG_SHA256) += sha256.o
obj-$(CONFIG_SHA512_ALGO) += sha512.o
diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
index 320ba49f79..3c411f651f 100644
--- a/lib/crypto/pkcs7_verify.c
+++ b/lib/crypto/pkcs7_verify.c
@@ -15,7 +15,7 @@
#include <linux/bitops.h>
#include <linux/compat.h>
#include <linux/asn1.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
#include <crypto/public_key.h>
#include <crypto/pkcs7_parser.h>
#else
diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c
index 91810a8640..d557ab27ae 100644
--- a/lib/crypto/x509_public_key.c
+++ b/lib/crypto/x509_public_key.c
@@ -19,7 +19,7 @@
#include <linux/kernel.h>
#ifdef __UBOOT__
#include <crypto/x509_parser.h>
-#include <u-boot/rsa-checksum.h>
+#include <u-boot/hash-checksum.h>
#else
#include <linux/slab.h>
#include <keys/asymmetric-subtype.h>
diff --git a/lib/rsa/rsa-checksum.c b/lib/hash-checksum.c
similarity index 96%
rename from lib/rsa/rsa-checksum.c
rename to lib/hash-checksum.c
index e60debb7df..d732ecc38f 100644
--- a/lib/rsa/rsa-checksum.c
+++ b/lib/hash-checksum.c
@@ -13,7 +13,8 @@
#else
#include "fdt_host.h"
#endif
-#include <u-boot/rsa.h>
+#include <hash.h>
+#include <image.h>
int hash_calculate(const char *name,
const struct image_region region[],
diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
index 8b75d41f04..c9ac72c1e2 100644
--- a/lib/rsa/Makefile
+++ b/lib/rsa/Makefile
@@ -5,6 +5,6 @@
# (C) Copyright 2000-2007
# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
-obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
+obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY) += rsa-verify.o
obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
diff --git a/tools/Makefile b/tools/Makefile
index 253a6b9706..b1595ad814 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -67,7 +67,7 @@ LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \
fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o)
RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
- rsa-sign.o rsa-verify.o rsa-checksum.o \
+ rsa-sign.o rsa-verify.o \
rsa-mod-exp.o)
AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
@@ -105,6 +105,7 @@ dumpimage-mkimage-objs := aisimage.o \
$(ROCKCHIP_OBS) \
socfpgaimage.o \
lib/crc16.o \
+ lib/hash-checksum.o \
lib/sha1.o \
lib/sha256.o \
lib/sha512.o \
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c Alexandru Gagniuc
@ 2020-12-30 21:00 ` Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing Alexandru Gagniuc
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
fdt_add_bignum() is useful for algorithms other than just RSA. To
allow its use for ECDSA, move it to a common file under lib/.
The new file is suffixed with '-libcrypto' because it has a direct
dependency on openssl. This is due to the use of the "BIGNUM *" type.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
include/u-boot/fdt-libcrypto.h | 15 +++++++
lib/fdt-libcrypto.c | 72 ++++++++++++++++++++++++++++++++++
lib/rsa/rsa-sign.c | 65 +-----------------------------
tools/Makefile | 1 +
4 files changed, 89 insertions(+), 64 deletions(-)
create mode 100644 include/u-boot/fdt-libcrypto.h
create mode 100644 lib/fdt-libcrypto.c
diff --git a/include/u-boot/fdt-libcrypto.h b/include/u-boot/fdt-libcrypto.h
new file mode 100644
index 0000000000..a6873487ed
--- /dev/null
+++ b/include/u-boot/fdt-libcrypto.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#ifndef _FDT_LIBCRYPTO_H
+#define _FDT_LIBCRYPTO_H
+
+#include <openssl/bn.h>
+
+int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
+ BIGNUM *num, int num_bits);
+
+#endif /* _FDT_LIBCRYPTO_H */
diff --git a/lib/fdt-libcrypto.c b/lib/fdt-libcrypto.c
new file mode 100644
index 0000000000..ecb0344c8f
--- /dev/null
+++ b/lib/fdt-libcrypto.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#include <libfdt.h>
+#include <u-boot/fdt-libcrypto.h>
+
+int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
+ BIGNUM *num, int num_bits)
+{
+ int nwords = num_bits / 32;
+ int size;
+ uint32_t *buf, *ptr;
+ BIGNUM *tmp, *big2, *big32, *big2_32;
+ BN_CTX *ctx;
+ int ret;
+
+ tmp = BN_new();
+ big2 = BN_new();
+ big32 = BN_new();
+ big2_32 = BN_new();
+
+ /*
+ * Note: This code assumes that all of the above succeed, or all fail.
+ * In practice memory allocations generally do not fail (unless the
+ * process is killed), so it does not seem worth handling each of these
+ * as a separate case. Technicaly this could leak memory on failure,
+ * but a) it won't happen in practice, and b) it doesn't matter as we
+ * will immediately exit with a failure code.
+ */
+ if (!tmp || !big2 || !big32 || !big2_32) {
+ fprintf(stderr, "Out of memory (bignum)\n");
+ return -ENOMEM;
+ }
+ ctx = BN_CTX_new();
+ if (!ctx) {
+ fprintf(stderr, "Out of memory (bignum context)\n");
+ return -ENOMEM;
+ }
+ BN_set_word(big2, 2L);
+ BN_set_word(big32, 32L);
+ BN_exp(big2_32, big2, big32, ctx); /* B = 2^32 */
+
+ size = nwords * sizeof(uint32_t);
+ buf = malloc(size);
+ if (!buf) {
+ fprintf(stderr, "Out of memory (%d bytes)\n", size);
+ return -ENOMEM;
+ }
+
+ /* Write out modulus as big endian array of integers */
+ for (ptr = buf + nwords - 1; ptr >= buf; ptr--) {
+ BN_mod(tmp, num, big2_32, ctx); /* n = N mod B */
+ *ptr = cpu_to_fdt32(BN_get_word(tmp));
+ BN_rshift(num, num, 32); /* N = N/B */
+ }
+
+ /*
+ * We try signing with successively increasing size values, so this
+ * might fail several times
+ */
+ ret = fdt_setprop(blob, noffset, prop_name, buf, size);
+ free(buf);
+ BN_free(tmp);
+ BN_free(big2);
+ BN_free(big32);
+ BN_free(big2_32);
+
+ return ret ? -FDT_ERR_NOSPACE : 0;
+}
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index 1f0d81bd7a..557c690a6d 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -9,6 +9,7 @@
#include <string.h>
#include <image.h>
#include <time.h>
+#include <u-boot/fdt-libcrypto.h>
#include <openssl/bn.h>
#include <openssl/rsa.h>
#include <openssl/pem.h>
@@ -680,70 +681,6 @@ int rsa_get_params(RSA *key, uint64_t *exponent, uint32_t *n0_invp,
return ret;
}
-static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
- BIGNUM *num, int num_bits)
-{
- int nwords = num_bits / 32;
- int size;
- uint32_t *buf, *ptr;
- BIGNUM *tmp, *big2, *big32, *big2_32;
- BN_CTX *ctx;
- int ret;
-
- tmp = BN_new();
- big2 = BN_new();
- big32 = BN_new();
- big2_32 = BN_new();
-
- /*
- * Note: This code assumes that all of the above succeed, or all fail.
- * In practice memory allocations generally do not fail (unless the
- * process is killed), so it does not seem worth handling each of these
- * as a separate case. Technicaly this could leak memory on failure,
- * but a) it won't happen in practice, and b) it doesn't matter as we
- * will immediately exit with a failure code.
- */
- if (!tmp || !big2 || !big32 || !big2_32) {
- fprintf(stderr, "Out of memory (bignum)\n");
- return -ENOMEM;
- }
- ctx = BN_CTX_new();
- if (!ctx) {
- fprintf(stderr, "Out of memory (bignum context)\n");
- return -ENOMEM;
- }
- BN_set_word(big2, 2L);
- BN_set_word(big32, 32L);
- BN_exp(big2_32, big2, big32, ctx); /* B = 2^32 */
-
- size = nwords * sizeof(uint32_t);
- buf = malloc(size);
- if (!buf) {
- fprintf(stderr, "Out of memory (%d bytes)\n", size);
- return -ENOMEM;
- }
-
- /* Write out modulus as big endian array of integers */
- for (ptr = buf + nwords - 1; ptr >= buf; ptr--) {
- BN_mod(tmp, num, big2_32, ctx); /* n = N mod B */
- *ptr = cpu_to_fdt32(BN_get_word(tmp));
- BN_rshift(num, num, 32); /* N = N/B */
- }
-
- /*
- * We try signing with successively increasing size values, so this
- * might fail several times
- */
- ret = fdt_setprop(blob, noffset, prop_name, buf, size);
- free(buf);
- BN_free(tmp);
- BN_free(big2);
- BN_free(big32);
- BN_free(big2_32);
-
- return ret ? -FDT_ERR_NOSPACE : 0;
-}
-
int rsa_add_verify_data(struct image_sign_info *info, void *keydest)
{
BIGNUM *modulus, *r_squared;
diff --git a/tools/Makefile b/tools/Makefile
index b1595ad814..af7698fd01 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -106,6 +106,7 @@ dumpimage-mkimage-objs := aisimage.o \
socfpgaimage.o \
lib/crc16.o \
lib/hash-checksum.o \
+ lib/fdt-libcrypto.o \
lib/sha1.o \
lib/sha256.o \
lib/sha512.o \
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code Alexandru Gagniuc
@ 2020-12-30 21:00 ` Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing Alexandru Gagniuc
4 siblings, 1 reply; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
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@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@gmail.com>.
+ */
+
+#ifndef _ECDSA_H
+#define _ECDSA_H
+
+#include <errno.h>
+#include <image.h>
+
+/**
+ * crypto_algo API impementation for ECDSA;
+ * @see "struct crypto_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);
+/** @} */
+
+#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.
+ * - Handling of keys and key paths:
+ * - The '-K' key directory option must contain path to the key file,
+ * instead of the key directory.
+ * - 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@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() !*/
+};
+
+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)
+{
+ 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;
+ }
+
+ 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;
+ }
+
+ 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)
+ 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;
+}
+
+static int do_sign(struct signer *ctx, struct image_sign_info *info,
+ const struct image_region region[], int region_count)
+{
+ const struct checksum_algo *algo = info->checksum;
+ ECDSA_SIG *sig;
+
+ algo->calculate(algo->name, region, region_count, ctx->hash);
+ sig = ECDSA_do_sign(ctx->hash, algo->checksum_len, ctx->ecdsa_key);
+
+ ecdsa_sig_encode_raw(ctx->signature, sig, info->crypto->key_len);
+
+ return 0;
+}
+
+static int ecdsa_check_signature(struct signer *ctx, struct image_sign_info *info)
+{
+ ECDSA_SIG *sig;
+ int okay;
+
+ sig = ecdsa_sig_from_raw(ctx->signature, info->crypto->key_len);
+ if (!sig)
+ return -1;
+
+ okay = ECDSA_do_verify(ctx->hash, info->checksum->checksum_len,
+ sig, ctx->ecdsa_key);
+ if (!okay)
+ fprintf(stderr, "WARNING: Signature is fake news!\n");
+
+ ECDSA_SIG_free(sig);
+ return !okay;
+}
+
+static int do_verify(struct signer *ctx, struct image_sign_info *info,
+ const struct image_region region[], int region_count,
+ uint8_t *raw_sig, uint sig_len)
+{
+ const struct checksum_algo *algo = info->checksum;
+
+ if (sig_len != info->crypto->key_len * 2) {
+ fprintf(stderr, "Signature has wrong length\n");
+ return -1;
+ }
+
+ memcpy(ctx->signature, raw_sig, sig_len);
+ algo->calculate(algo->name, region, region_count, ctx->hash);
+
+ return ecdsa_check_signature(ctx, info);
+}
+
+int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
+ int region_count, uint8_t **sigp, uint *sig_len)
+{
+ struct signer ctx;
+ int ret;
+
+ ret = prepare_ctx(&ctx, info);
+ if (ret >= 0) {
+ do_sign(&ctx, info, region, region_count);
+ *sigp = ctx.signature;
+ *sig_len = info->crypto->key_len * 2;
+
+ ret = ecdsa_check_signature(&ctx, info);
+ }
+
+ free_ctx(&ctx);
+ return ret;
+}
+
+int ecdsa_verify(struct image_sign_info *info,
+ const struct image_region region[], int region_count,
+ uint8_t *sig, uint sig_len)
+{
+ struct signer ctx;
+ int ret;
+
+ ret = prepare_ctx(&ctx, info);
+ if (ret >= 0)
+ ret = do_verify(&ctx, info, region, region_count, sig, sig_len);
+
+ free_ctx(&ctx);
+ return ret;
+}
+
+static int do_add(struct signer *ctx, void *fdt, const char *key_node_name)
+{
+ int signature_node, key_node, ret, key_bits;
+ const char *curve_name;
+ const EC_GROUP *group;
+ const EC_POINT *point;
+ BIGNUM *x, *y;
+
+ signature_node = fdt_subnode_offset(fdt, 0, FIT_SIG_NODENAME);
+ if (signature_node < 0) {
+ fprintf(stderr, "Could not find 'signature node: %s\n",
+ fdt_strerror(signature_node));
+ return signature_node;
+ }
+
+ key_node = fdt_add_subnode(fdt, signature_node, key_node_name);
+ if (key_node < 0) {
+ fprintf(stderr, "Could not create '%s' node: %s\n",
+ key_node_name, fdt_strerror(key_node));
+ return key_node;
+ }
+
+ group = EC_KEY_get0_group(ctx->ecdsa_key);
+ key_bits = EC_GROUP_order_bits(group);
+ curve_name = OBJ_nid2sn(EC_GROUP_get_curve_name(group));
+ /* Let 'x' and 'y' memory leak by not BN_free()'ing them. */
+ x = BN_new();
+ y = BN_new();
+ point = EC_KEY_get0_public_key(ctx->ecdsa_key);
+ EC_POINT_get_affine_coordinates(group, point, x, y, NULL);
+
+ ret = fdt_setprop_string(fdt, key_node, "ecdsa,curve", curve_name);
+ if (ret < 0)
+ return ret;
+
+ ret = fdt_add_bignum(fdt, key_node, "ecdsa,x-point", x, key_bits);
+ if (ret < 0)
+ return ret;
+
+ ret = fdt_add_bignum(fdt, key_node, "ecdsa,y-point", y, key_bits);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+int ecdsa_add_verify_data(struct image_sign_info *info, void *fdt)
+{
+ const char *fdt_key_name;
+ struct signer ctx;
+ int ret;
+
+ fdt_key_name = info->keyname ? info->keyname : "default-key";
+ ret = prepare_ctx(&ctx, info);
+ if (ret >= 0)
+ do_add(&ctx, fdt, fdt_key_name);
+
+ free_ctx(&ctx);
+ return ret;
+}
diff --git a/tools/Makefile b/tools/Makefile
index af7698fd01..a4f7b7e80c 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
rsa-sign.o rsa-verify.o \
rsa-mod-exp.o)
+ECDSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o)
+
AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
aes-encrypt.o aes-decrypt.o)
@@ -119,6 +121,7 @@ dumpimage-mkimage-objs := aisimage.o \
gpimage.o \
gpimage-common.o \
mtk_image.o \
+ $(ECDSA_OBJS-y) \
$(RSA_OBJS-y) \
$(AES_OBJS-y)
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
` (2 preceding siblings ...)
2020-12-30 21:00 ` [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing Alexandru Gagniuc
@ 2020-12-30 21:00 ` Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing Alexandru Gagniuc
4 siblings, 1 reply; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
doc/uImage.FIT/signature.txt | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt
index a3455889ed..0139295d33 100644
--- a/doc/uImage.FIT/signature.txt
+++ b/doc/uImage.FIT/signature.txt
@@ -142,7 +142,7 @@ public key in U-Boot's control FDT (using CONFIG_OF_CONTROL).
Public keys should be stored as sub-nodes in a /signature node. Required
properties are:
-- algo: Algorithm name (e.g. "sha1,rsa2048")
+- algo: Algorithm name (e.g. "sha1,rsa2048" or "sha256,ecdsa256")
Optional properties are:
@@ -167,6 +167,11 @@ For RSA the following are mandatory:
- rsa,r-squared: (2^num-bits)^2 as a big-endian multi-word integer
- rsa,n0-inverse: -1 / modulus[0] mod 2^32
+For ECDSA the following are mandatory:
+- ecdsa,curve: Name of ECDSA curve (e.g. "prime256v1")
+- ecdsa,x-point: Public key X coordinate as a big-endian multi-word integer
+- ecdsa,y-point: Public key Y coordinate as a big-endian multi-word integer
+
These parameters can be added to a binary device tree using parameter -K of the
mkimage command::
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
` (3 preceding siblings ...)
2020-12-30 21:00 ` [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys Alexandru Gagniuc
@ 2020-12-30 21:00 ` Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
4 siblings, 1 reply; 19+ messages in thread
From: Alexandru Gagniuc @ 2020-12-30 21:00 UTC (permalink / raw)
To: u-boot
Add a test to make sure that the ECDSA signatures generated by
mkimage can be verified successfully. pyCryptodomex was chosen as the
crypto library because it integrates much better with python code.
Using openssl would have been unnecessarily painful.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
create mode 100644 test/py/tests/test_fit_ecdsa.py
diff --git a/test/py/tests/test_fit_ecdsa.py b/test/py/tests/test_fit_ecdsa.py
new file mode 100644
index 0000000000..957964d329
--- /dev/null
+++ b/test/py/tests/test_fit_ecdsa.py
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
+
+"""
+Test ECDSA signing of FIT images
+
+This test uses mkimage to sign an existing FIT image with an ECDSA key. The
+signature is then extracted, and verified against pyCryptodome.
+This test doesn't run the sandbox. It only checks the host tool 'mkimage'
+"""
+
+import pytest
+import u_boot_utils as util
+from Cryptodome.Hash import SHA256
+from Cryptodome.PublicKey import ECC
+from Cryptodome.Signature import DSS
+
+class SignableFitImage(object):
+ """ Helper to manipulate a FIT image on disk """
+ def __init__(self, cons, file_name):
+ self.fit = file_name
+ self.cons = cons
+ self.signable_nodes = set()
+
+ def __fdt_list(self, path):
+ return util.run_and_log(self.cons, f'fdtget -l {self.fit} {path}')
+
+ def __fdt_set(self, node, **prop_value):
+ for prop, value in prop_value.items():
+ util.run_and_log(self.cons, f'fdtput -ts {self.fit} {node} {prop} {value}')
+
+ def __fdt_get_binary(self, node, prop):
+ numbers = util.run_and_log(self.cons, f'fdtget -tbi {self.fit} {node} {prop}')
+
+ bignum = bytearray()
+ for little_num in numbers.split():
+ bignum.append(int(little_num))
+
+ return bignum
+
+ def find_signable_image_nodes(self):
+ for node in self.__fdt_list('/images').split():
+ image = f'/images/{node}'
+ if 'signature' in self.__fdt_list(image):
+ self.signable_nodes.add(image)
+
+ return self.signable_nodes
+
+ def change_signature_algo_to_ecdsa(self):
+ for image in self.signable_nodes:
+ self.__fdt_set(f'{image}/signature', algo='sha256,ecdsa256')
+
+ def sign(self, mkimage, key_file):
+ util.run_and_log(self.cons, [mkimage, '-F', self.fit, f'-k{key_file}'])
+
+ def check_signatures(self, key):
+ for image in self.signable_nodes:
+ raw_sig = self.__fdt_get_binary(f'{image}/signature', 'value')
+ raw_bin = self.__fdt_get_binary(image, 'data')
+
+ sha = SHA256.new(raw_bin)
+ verifier = DSS.new(key, 'fips-186-3')
+ verifier.verify(sha, bytes(raw_sig))
+
+
+ at pytest.mark.buildconfigspec('fit_signature')
+ at pytest.mark.requiredtool('dtc')
+ at pytest.mark.requiredtool('fdtget')
+ at pytest.mark.requiredtool('fdtput')
+def test_fit_ecdsa(u_boot_console):
+ """TODO: Document me
+ """
+ def generate_ecdsa_key():
+ return ECC.generate(curve='prime256v1')
+
+ def assemble_fit_image(dest_fit, its, destdir):
+ dtc_args = f'-I dts -O dtb -i {destdir}'
+ util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f', its, dest_fit])
+
+ def dtc(dts):
+ dtb = dts.replace('.dts', '.dtb')
+ util.run_and_log(cons, f'dtc {datadir}/{dts} -O dtb -o {tempdir}/{dtb}')
+
+ cons = u_boot_console
+ mkimage = cons.config.build_dir + '/tools/mkimage'
+ datadir = cons.config.source_dir + '/test/py/tests/vboot/'
+ tempdir = cons.config.result_dir
+ key_file = f'{tempdir}/ecdsa-test-key.pem'
+ fit_file = f'{tempdir}/test.fit'
+ dtc('sandbox-kernel.dts')
+
+ key = generate_ecdsa_key()
+
+ # Create a number kernel image with zeroes
+ with open(f'{tempdir}/test-kernel.bin', 'w') as fd:
+ fd.write(500 * chr(0))
+
+ with open(key_file, 'w') as f:
+ f.write(key.export_key(format='PEM'))
+
+ assemble_fit_image(fit_file, f'{datadir}/sign-images-sha256.its', tempdir)
+
+ fit = SignableFitImage(cons, fit_file)
+ nodes = fit.find_signable_image_nodes()
+ if len(nodes) == 0:
+ raise ValueError('FIT image has no "/image" nodes with "signature"')
+
+ fit.change_signature_algo_to_ecdsa()
+ fit.sign(mkimage, key_file)
+ fit.check_signatures(key)
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c
2020-12-30 21:00 ` [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c Alexandru Gagniuc
@ 2021-01-07 12:35 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2021-01-07 12:35 UTC (permalink / raw)
To: u-boot
Hi Alexandru,
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> rsa-checksum.c sontains the hash_calculate() implementations. Despite
> the "rsa-" file prefix, this function is useful for other algorithms.
>
> To prevent confusion, move this file to lib/crypto, and rename it to
> hash-checksum.c, to give it a more "generic" feel.
It looks like it is moving to lib/ ?
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
> common/image-fit-sig.c | 2 +-
> common/image-sig.c | 2 +-
> include/image.h | 2 +-
> include/u-boot/{rsa-checksum.h => hash-checksum.h} | 0
> lib/Makefile | 1 +
> lib/crypto/pkcs7_verify.c | 2 +-
> lib/crypto/x509_public_key.c | 2 +-
> lib/{rsa/rsa-checksum.c => hash-checksum.c} | 3 ++-
> lib/rsa/Makefile | 2 +-
> tools/Makefile | 3 ++-
> 10 files changed, 11 insertions(+), 8 deletions(-)
> rename include/u-boot/{rsa-checksum.h => hash-checksum.h} (100%)
> rename lib/{rsa/rsa-checksum.c => hash-checksum.c} (96%)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code
2020-12-30 21:00 ` [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code Alexandru Gagniuc
@ 2021-01-07 12:35 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2021-01-07 12:35 UTC (permalink / raw)
To: u-boot
Hi Alexandru,
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> fdt_add_bignum() is useful for algorithms other than just RSA. To
> allow its use for ECDSA, move it to a common file under lib/.
>
> The new file is suffixed with '-libcrypto' because it has a direct
> dependency on openssl. This is due to the use of the "BIGNUM *" type.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
> include/u-boot/fdt-libcrypto.h | 15 +++++++
> lib/fdt-libcrypto.c | 72 ++++++++++++++++++++++++++++++++++
> lib/rsa/rsa-sign.c | 65 +-----------------------------
> tools/Makefile | 1 +
> 4 files changed, 89 insertions(+), 64 deletions(-)
> create mode 100644 include/u-boot/fdt-libcrypto.h
> create mode 100644 lib/fdt-libcrypto.c
Reviewed-by: Simon Glass <sjg@chromium.org>
>
> diff --git a/include/u-boot/fdt-libcrypto.h b/include/u-boot/fdt-libcrypto.h
> new file mode 100644
> index 0000000000..a6873487ed
> --- /dev/null
> +++ b/include/u-boot/fdt-libcrypto.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
> + * Copyright (c) 2013, Google Inc.
> + */
> +
> +#ifndef _FDT_LIBCRYPTO_H
> +#define _FDT_LIBCRYPTO_H
> +
> +#include <openssl/bn.h>
> +
> +int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
> + BIGNUM *num, int num_bits);
Please add a full function comment.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2020-12-30 21:00 ` [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing Alexandru Gagniuc
@ 2021-01-07 12:35 ` Simon Glass
2021-01-07 16:27 ` Alex G.
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-01-07 12:35 UTC (permalink / raw)
To: u-boot
Hi Alexandru,
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@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@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@gmail.com>.
> + */
> +
> +#ifndef _ECDSA_H
> +#define _ECDSA_H
> +
> +#include <errno.h>
> +#include <image.h>
> +
> +/**
> + * crypto_algo API impementation for ECDSA;
> + * @see "struct crypto_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.
> +/** @} */
> +
> +#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?
> + * - 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.
> + * - 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@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
> +};
> +
> +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.
> +{
> + 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?
> + }
> +
> + 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.
> + }
> +
> + 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.
> + 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys
2020-12-30 21:00 ` [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys Alexandru Gagniuc
@ 2021-01-07 12:35 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2021-01-07 12:35 UTC (permalink / raw)
To: u-boot
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
> doc/uImage.FIT/signature.txt | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
2020-12-30 21:00 ` [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing Alexandru Gagniuc
@ 2021-01-07 12:35 ` Simon Glass
2021-01-07 16:44 ` Alex G.
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-01-07 12:35 UTC (permalink / raw)
To: u-boot
Hi Alexandru,
On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Add a test to make sure that the ECDSA signatures generated by
> mkimage can be verified successfully. pyCryptodomex was chosen as the
> crypto library because it integrates much better with python code.
> Using openssl would have been unnecessarily painful.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
> test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
> create mode 100644 test/py/tests/test_fit_ecdsa.py
>
This test looks fine but the functions need full comments. I do think
it might be worth putting the code in test_vboot, particularly when
you get to the sandbox implementation.
For installing the Python library, you might need to update the docs
in test/ and perhaps install things in .gitlab-ci.yml and .azure...
> diff --git a/test/py/tests/test_fit_ecdsa.py b/test/py/tests/test_fit_ecdsa.py
> new file mode 100644
> index 0000000000..957964d329
> --- /dev/null
> +++ b/test/py/tests/test_fit_ecdsa.py
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
> +
> +"""
> +Test ECDSA signing of FIT images
> +
> +This test uses mkimage to sign an existing FIT image with an ECDSA key. The
> +signature is then extracted, and verified against pyCryptodome.
> +This test doesn't run the sandbox. It only checks the host tool 'mkimage'
> +"""
> +
> +import pytest
> +import u_boot_utils as util
> +from Cryptodome.Hash import SHA256
> +from Cryptodome.PublicKey import ECC
> +from Cryptodome.Signature import DSS
> +
> +class SignableFitImage(object):
> + """ Helper to manipulate a FIT image on disk """
> + def __init__(self, cons, file_name):
> + self.fit = file_name
> + self.cons = cons
> + self.signable_nodes = set()
> +
> + def __fdt_list(self, path):
> + return util.run_and_log(self.cons, f'fdtget -l {self.fit} {path}')
> +
> + def __fdt_set(self, node, **prop_value):
> + for prop, value in prop_value.items():
> + util.run_and_log(self.cons, f'fdtput -ts {self.fit} {node} {prop} {value}')
> +
> + def __fdt_get_binary(self, node, prop):
> + numbers = util.run_and_log(self.cons, f'fdtget -tbi {self.fit} {node} {prop}')
> +
> + bignum = bytearray()
> + for little_num in numbers.split():
> + bignum.append(int(little_num))
> +
> + return bignum
> +
> + def find_signable_image_nodes(self):
> + for node in self.__fdt_list('/images').split():
> + image = f'/images/{node}'
> + if 'signature' in self.__fdt_list(image):
> + self.signable_nodes.add(image)
> +
> + return self.signable_nodes
> +
> + def change_signature_algo_to_ecdsa(self):
> + for image in self.signable_nodes:
> + self.__fdt_set(f'{image}/signature', algo='sha256,ecdsa256')
> +
> + def sign(self, mkimage, key_file):
> + util.run_and_log(self.cons, [mkimage, '-F', self.fit, f'-k{key_file}'])
> +
> + def check_signatures(self, key):
> + for image in self.signable_nodes:
> + raw_sig = self.__fdt_get_binary(f'{image}/signature', 'value')
> + raw_bin = self.__fdt_get_binary(image, 'data')
> +
> + sha = SHA256.new(raw_bin)
> + verifier = DSS.new(key, 'fips-186-3')
> + verifier.verify(sha, bytes(raw_sig))
> +
> +
> + at pytest.mark.buildconfigspec('fit_signature')
> + at pytest.mark.requiredtool('dtc')
> + at pytest.mark.requiredtool('fdtget')
> + at pytest.mark.requiredtool('fdtput')
> +def test_fit_ecdsa(u_boot_console):
> + """TODO: Document me
> + """
> + def generate_ecdsa_key():
> + return ECC.generate(curve='prime256v1')
> +
> + def assemble_fit_image(dest_fit, its, destdir):
> + dtc_args = f'-I dts -O dtb -i {destdir}'
> + util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f', its, dest_fit])
> +
> + def dtc(dts):
> + dtb = dts.replace('.dts', '.dtb')
> + util.run_and_log(cons, f'dtc {datadir}/{dts} -O dtb -o {tempdir}/{dtb}')
> +
> + cons = u_boot_console
> + mkimage = cons.config.build_dir + '/tools/mkimage'
> + datadir = cons.config.source_dir + '/test/py/tests/vboot/'
> + tempdir = cons.config.result_dir
> + key_file = f'{tempdir}/ecdsa-test-key.pem'
> + fit_file = f'{tempdir}/test.fit'
> + dtc('sandbox-kernel.dts')
> +
> + key = generate_ecdsa_key()
> +
> + # Create a number kernel image with zeroes
> + with open(f'{tempdir}/test-kernel.bin', 'w') as fd:
> + fd.write(500 * chr(0))
> +
> + with open(key_file, 'w') as f:
> + f.write(key.export_key(format='PEM'))
> +
> + assemble_fit_image(fit_file, f'{datadir}/sign-images-sha256.its', tempdir)
> +
> + fit = SignableFitImage(cons, fit_file)
> + nodes = fit.find_signable_image_nodes()
> + if len(nodes) == 0:
> + raise ValueError('FIT image has no "/image" nodes with "signature"')
> +
> + fit.change_signature_algo_to_ecdsa()
> + fit.sign(mkimage, key_file)
> + fit.check_signatures(key)
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2021-01-07 12:35 ` Simon Glass
@ 2021-01-07 16:27 ` Alex G.
2021-01-07 17:25 ` Tom Rini
2021-01-07 17:29 ` Simon Glass
0 siblings, 2 replies; 19+ messages in thread
From: Alex G. @ 2021-01-07 16:27 UTC (permalink / raw)
To: u-boot
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@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@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@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@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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
2021-01-07 12:35 ` Simon Glass
@ 2021-01-07 16:44 ` Alex G.
2021-01-07 17:31 ` Simon Glass
0 siblings, 1 reply; 19+ messages in thread
From: Alex G. @ 2021-01-07 16:44 UTC (permalink / raw)
To: u-boot
On 1/7/21 6:35 AM, Simon Glass wrote:
> Hi Alexandru,
>
> On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> Add a test to make sure that the ECDSA signatures generated by
>> mkimage can be verified successfully. pyCryptodomex was chosen as the
>> crypto library because it integrates much better with python code.
>> Using openssl would have been unnecessarily painful.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>> test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
>> 1 file changed, 111 insertions(+)
>> create mode 100644 test/py/tests/test_fit_ecdsa.py
>>
>
> This test looks fine but the functions need full comments. I do think
> it might be worth putting the code in test_vboot, particularly when
> you get to the sandbox implementation.
test_vboot seems to be testing the bootm command, while with this test
I'm only looking to test the host-side (mkimage). In the next series, I
won't have a software implementation of ECDSA, like RSA_MOD_EXP. I will
use the ROM on the stm32mp. So there won't be somthing testable in the
sandbox.
> For installing the Python library, you might need to update the docs
> in test/ and perhaps install things in .gitlab-ci.yml and .azure...
>
Sure, let me look into this.
>
>> diff --git a/test/py/tests/test_fit_ecdsa.py b/test/py/tests/test_fit_ecdsa.py
>> new file mode 100644
>> index 0000000000..957964d329
>> --- /dev/null
>> +++ b/test/py/tests/test_fit_ecdsa.py
>> @@ -0,0 +1,111 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +# Copyright (c) 2020, Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> +
>> +"""
>> +Test ECDSA signing of FIT images
>> +
>> +This test uses mkimage to sign an existing FIT image with an ECDSA key. The
>> +signature is then extracted, and verified against pyCryptodome.
>> +This test doesn't run the sandbox. It only checks the host tool 'mkimage'
>> +"""
>> +
>> +import pytest
>> +import u_boot_utils as util
>> +from Cryptodome.Hash import SHA256
>> +from Cryptodome.PublicKey import ECC
>> +from Cryptodome.Signature import DSS
>> +
>> +class SignableFitImage(object):
>> + """ Helper to manipulate a FIT image on disk """
>> + def __init__(self, cons, file_name):
>> + self.fit = file_name
>> + self.cons = cons
>> + self.signable_nodes = set()
>> +
>> + def __fdt_list(self, path):
>> + return util.run_and_log(self.cons, f'fdtget -l {self.fit} {path}')
>> +
>> + def __fdt_set(self, node, **prop_value):
>> + for prop, value in prop_value.items():
>> + util.run_and_log(self.cons, f'fdtput -ts {self.fit} {node} {prop} {value}')
>> +
>> + def __fdt_get_binary(self, node, prop):
>> + numbers = util.run_and_log(self.cons, f'fdtget -tbi {self.fit} {node} {prop}')
>> +
>> + bignum = bytearray()
>> + for little_num in numbers.split():
>> + bignum.append(int(little_num))
>> +
>> + return bignum
>> +
>> + def find_signable_image_nodes(self):
>> + for node in self.__fdt_list('/images').split():
>> + image = f'/images/{node}'
>> + if 'signature' in self.__fdt_list(image):
>> + self.signable_nodes.add(image)
>> +
>> + return self.signable_nodes
>> +
>> + def change_signature_algo_to_ecdsa(self):
>> + for image in self.signable_nodes:
>> + self.__fdt_set(f'{image}/signature', algo='sha256,ecdsa256')
>> +
>> + def sign(self, mkimage, key_file):
>> + util.run_and_log(self.cons, [mkimage, '-F', self.fit, f'-k{key_file}'])
>> +
>> + def check_signatures(self, key):
>> + for image in self.signable_nodes:
>> + raw_sig = self.__fdt_get_binary(f'{image}/signature', 'value')
>> + raw_bin = self.__fdt_get_binary(image, 'data')
>> +
>> + sha = SHA256.new(raw_bin)
>> + verifier = DSS.new(key, 'fips-186-3')
>> + verifier.verify(sha, bytes(raw_sig))
>> +
>> +
>> + at pytest.mark.buildconfigspec('fit_signature')
>> + at pytest.mark.requiredtool('dtc')
>> + at pytest.mark.requiredtool('fdtget')
>> + at pytest.mark.requiredtool('fdtput')
>> +def test_fit_ecdsa(u_boot_console):
>> + """TODO: Document me
>> + """
>> + def generate_ecdsa_key():
>> + return ECC.generate(curve='prime256v1')
>> +
>> + def assemble_fit_image(dest_fit, its, destdir):
>> + dtc_args = f'-I dts -O dtb -i {destdir}'
>> + util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f', its, dest_fit])
>> +
>> + def dtc(dts):
>> + dtb = dts.replace('.dts', '.dtb')
>> + util.run_and_log(cons, f'dtc {datadir}/{dts} -O dtb -o {tempdir}/{dtb}')
>> +
>> + cons = u_boot_console
>> + mkimage = cons.config.build_dir + '/tools/mkimage'
>> + datadir = cons.config.source_dir + '/test/py/tests/vboot/'
>> + tempdir = cons.config.result_dir
>> + key_file = f'{tempdir}/ecdsa-test-key.pem'
>> + fit_file = f'{tempdir}/test.fit'
>> + dtc('sandbox-kernel.dts')
>> +
>> + key = generate_ecdsa_key()
>> +
>> + # Create a number kernel image with zeroes
>> + with open(f'{tempdir}/test-kernel.bin', 'w') as fd:
>> + fd.write(500 * chr(0))
>> +
>> + with open(key_file, 'w') as f:
>> + f.write(key.export_key(format='PEM'))
>> +
>> + assemble_fit_image(fit_file, f'{datadir}/sign-images-sha256.its', tempdir)
>> +
>> + fit = SignableFitImage(cons, fit_file)
>> + nodes = fit.find_signable_image_nodes()
>> + if len(nodes) == 0:
>> + raise ValueError('FIT image has no "/image" nodes with "signature"')
>> +
>> + fit.change_signature_algo_to_ecdsa()
>> + fit.sign(mkimage, key_file)
>> + fit.check_signatures(key)
>> --
>> 2.26.2
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2021-01-07 16:27 ` Alex G.
@ 2021-01-07 17:25 ` Tom Rini
2021-01-07 22:24 ` Alex G.
2021-01-07 17:29 ` Simon Glass
1 sibling, 1 reply; 19+ messages in thread
From: Tom Rini @ 2021-01-07 17:25 UTC (permalink / raw)
To: u-boot
On Thu, Jan 07, 2021 at 10:27:50AM -0600, Alex G. wrote:
> 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.
Comments for comments sake are bad. Comments so that we can also have
reasonable generated documentation are good. Function prototypes fall
in to that second category to me, with few exceptions (and that we have
lots of bad examples isn't a good reason). The function names may well
make it obvious what's going on but the comments allow for generated
documentation to include that when explaining the not so obvious parts.
Thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210107/b8a3621c/attachment.sig>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2021-01-07 16:27 ` Alex G.
2021-01-07 17:25 ` Tom Rini
@ 2021-01-07 17:29 ` Simon Glass
2021-01-07 19:56 ` Alex G.
1 sibling, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-01-07 17:29 UTC (permalink / raw)
To: u-boot
Hi Alex,
On Thu, 7 Jan 2021 at 09:27, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> 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.
>
Don't get me started on comments in the kernel...there seems to almost
be a ban on them :-)
We used to follow the same approach but now we have comments for
non-trivial code. Comments and tests are closely related.
- if there is no comment, we don't know what the function is supposed
to do so we can't change it (there is no contract), we are not sure if
line 5 is a bug or a real quirk, casual readers can't understand what
is going on, the automated docs don't produce anything, no one wants
to refactor it, etc.
- if there is no test, presumably the code doesn't work now, if it ever did
> >
> > On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@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@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@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.
Please add full comments on exported function, no exceptions. Again,
this is not Linux and people don't have as much time to cogitate on
code. They are just trying to get their device working.
>
> >> +/** @} */
> >> +
> >> +#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.
OK I'm just a bit confused as to how this tests anything. But sure it
can come later.
>
> >> + * - 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.
It's just that the filename is defined by the code not the parameter.
There is a public key file and a certificate file in the same dir, so
it is convenient to specify the dir rather than the filename. But I
don't see a big problem with changing it. Everyone will have to
update their scripts I suppose, so there is that...
Another option would be to add a new arg which specifies the exact file.
>
> >> + * - 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@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.
What does hash point to and what is it for? Same with signature. Why
are there two keys and what is the difference? Please, if you are
contributing to U-Boot, add comments.
>[..]
> >> +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.
I would still like the comments please. If it's really trivial that's
fine, but this code is not obvious to me. We need to make it easy to
contribute to U-Boot.
>
> >
> >> +{
> >> + 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?
Sure, or perhaps a different error if this could mean that the file is corrupt.
[..]
> >> +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.
U-Boot generally returns 0 for success and -ve for error. The +ve
numbers are used when the function needs to return a value, if valid.
>
> >> + return -1;
-ENOMEM
> >> +
> >> + if (read_key(ctx, kname) < 0)
> >> + return -1;
I recommend handling errors like this:
int ret;
ret = read_key(ctx, kname);
if (ret)
return log_msg_ret("read", ret);
so that people can quickly figure out exactly what is failing either
by looking at the error number (propagated to the top) or turning on
CONFIG_LOG_ERROR_RETURN
> >> +
> >> + 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
> >
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
2021-01-07 16:44 ` Alex G.
@ 2021-01-07 17:31 ` Simon Glass
2021-01-07 18:44 ` Alex G.
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-01-07 17:31 UTC (permalink / raw)
To: u-boot
Hi Alex,
On Thu, 7 Jan 2021 at 09:44, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 1/7/21 6:35 AM, Simon Glass wrote:
> > Hi Alexandru,
> >
> > On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>
> >> Add a test to make sure that the ECDSA signatures generated by
> >> mkimage can be verified successfully. pyCryptodomex was chosen as the
> >> crypto library because it integrates much better with python code.
> >> Using openssl would have been unnecessarily painful.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >> test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
> >> 1 file changed, 111 insertions(+)
> >> create mode 100644 test/py/tests/test_fit_ecdsa.py
> >>
> >
> > This test looks fine but the functions need full comments. I do think
> > it might be worth putting the code in test_vboot, particularly when
> > you get to the sandbox implementation.
>
> test_vboot seems to be testing the bootm command, while with this test
It also runs fit_check_sign to check the signature.
> I'm only looking to test the host-side (mkimage). In the next series, I
> won't have a software implementation of ECDSA, like RSA_MOD_EXP. I will
> use the ROM on the stm32mp. So there won't be somthing testable in the
> sandbox.
I'm not sure that is a good idea. With driver model you'll end up
creating a ECDSA driver I suppose, so implementing it for sandbox
should be possible. Is it a complicated algorithm? Without that, I'm
not even sure how fit_check_sign could work?
>
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing
2021-01-07 17:31 ` Simon Glass
@ 2021-01-07 18:44 ` Alex G.
0 siblings, 0 replies; 19+ messages in thread
From: Alex G. @ 2021-01-07 18:44 UTC (permalink / raw)
To: u-boot
On 1/7/21 11:31 AM, Simon Glass wrote:
> Hi Alex,
>
> On Thu, 7 Jan 2021 at 09:44, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>>
>>
>> On 1/7/21 6:35 AM, Simon Glass wrote:
>>> Hi Alexandru,
>>>
>>> On Wed, 30 Dec 2020 at 14:00, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>>>
>>>> Add a test to make sure that the ECDSA signatures generated by
>>>> mkimage can be verified successfully. pyCryptodomex was chosen as the
>>>> crypto library because it integrates much better with python code.
>>>> Using openssl would have been unnecessarily painful.
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>> test/py/tests/test_fit_ecdsa.py | 111 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 111 insertions(+)
>>>> create mode 100644 test/py/tests/test_fit_ecdsa.py
>>>>
>>>
>>> This test looks fine but the functions need full comments. I do think
>>> it might be worth putting the code in test_vboot, particularly when
>>> you get to the sandbox implementation.
>>
>> test_vboot seems to be testing the bootm command, while with this test
>
> It also runs fit_check_sign to check the signature.
Hmm, it backends on tools/check_fit_sign. Would be an interesting
execrise to extend it ecdsa signatures, but that would take
significantly more effort than the simple test I am proposing here.
>> I'm only looking to test the host-side (mkimage). In the next series, I
>> won't have a software implementation of ECDSA, like RSA_MOD_EXP. I will
>> use the ROM on the stm32mp. So there won't be somthing testable in the
>> sandbox.
>
> I'm not sure that is a good idea. With driver model you'll end up
> creating a ECDSA driver I suppose, so implementing it for sandbox
> should be possible. Is it a complicated algorithm?
A software implementation of ECDSA is outside the scope of my project.
> Without that, I'm not even sure how fit_check_sign could work?
It uses the ops->verify in ecdsa-libcrypto, does it not?
>
> [..]
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2021-01-07 17:29 ` Simon Glass
@ 2021-01-07 19:56 ` Alex G.
0 siblings, 0 replies; 19+ messages in thread
From: Alex G. @ 2021-01-07 19:56 UTC (permalink / raw)
To: u-boot
On 1/7/21 11:29 AM, Simon Glass wrote:
> Hi Alex,
>
> On Thu, 7 Jan 2021 at 09:27, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>> 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.
>>
>
> Don't get me started on comments in the kernel...there seems to almost
> be a ban on them :-)
>
> We used to follow the same approach but now we have comments for
> non-trivial code. Comments and tests are closely related.
>
> - if there is no comment, we don't know what the function is supposed
> to do so we can't change it (there is no contract), we are not sure if
> line 5 is a bug or a real quirk, casual readers can't understand what
> is going on, the automated docs don't produce anything, no one wants
> to refactor it, etc.
> - if there is no test, presumably the code doesn't work now, if it ever did
I will add each of the comments you are requesting, but not for the
reasons quoted. The comments that I could write won't add anything of
value -- they would just make the code larger, and increase the
cognitive load. You are the maintainer, and starting an argument would
be pointless. In the end, comments don't get compiled, and the code
works just the same :)
[snip]
>
> Again,
> this is not Linux and people don't have as much time to cogitate on
> code.
I resent that statement. It takes me longer to do a task in u-boot than
it would take me to do a similar task in linux. In the context of
comments (note I intentionally did not say 'documentation'), does
plastering the same information over and over in a way that hides the
essence really help? I think the current path is misguided, and will
only aggravate the problem. (I'll add the comments, though)
> They are just trying to get their device working.
That's true for linux too.
[snip]
All other comments will be addressed in v3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing
2021-01-07 17:25 ` Tom Rini
@ 2021-01-07 22:24 ` Alex G.
0 siblings, 0 replies; 19+ messages in thread
From: Alex G. @ 2021-01-07 22:24 UTC (permalink / raw)
To: u-boot
On 1/7/21 11:25 AM, Tom Rini wrote:
> On Thu, Jan 07, 2021 at 10:27:50AM -0600, Alex G. wrote:
>> 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.
>
> Comments for comments sake are bad. Comments so that we can also have
> reasonable generated documentation are good. Function prototypes fall
> in to that second category to me, with few exceptions (and that we have
> lots of bad examples isn't a good reason). The function names may well
> make it obvious what's going on but the comments allow for generated
> documentation to include that when explaining the not so obvious parts.
> Thanks!
>
The problem with generated documentation is that it's not very useful.
People add comments for the sake of comments to have something
generated. Most often you end up with a detailed description of all the
details, but almost never the big picture. Nowadays, I don't even waste
my time reading generated documentation.
I am getting ready to send the new series with the goodies requested by
Simon. I don't find the new comments to be useful, and I find some to
spread out the code such that it hurts readability. They will be there
because you and Simon asked nicely, although I think you're wrong.
Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-01-07 22:24 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30 21:00 [PATCH RFC v2 0/5] Add support for ECDSA image signing (with test) Alexandru Gagniuc
2020-12-30 21:00 ` [PATCH RFC v2 1/5] lib: Rename rsa-checksum.c to hash-checksum.c Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 2/5] lib/rsa: Make fdt_add_bignum() available outside of RSA code Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 3/5] lib: Add support for ECDSA image signing Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2021-01-07 16:27 ` Alex G.
2021-01-07 17:25 ` Tom Rini
2021-01-07 22:24 ` Alex G.
2021-01-07 17:29 ` Simon Glass
2021-01-07 19:56 ` Alex G.
2020-12-30 21:00 ` [PATCH RFC v2 4/5] doc: signature.txt: Document devicetree format for ECDSA keys Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2020-12-30 21:00 ` [PATCH RFC v2 5/5] test/py: ecdsa: Add test for mkimage ECDSA signing Alexandru Gagniuc
2021-01-07 12:35 ` Simon Glass
2021-01-07 16:44 ` Alex G.
2021-01-07 17:31 ` Simon Glass
2021-01-07 18:44 ` Alex G.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.