[U-Boot] [PATCH v1 0/3] rsa: extend rsa_verify() for UEFI secure boot

[U-Boot] [PATCH v1 0/3] rsa: extend rsa_verify() for UEFI secure boot

[AKASHI Takahiro]

The current rsa_verify() requires five parameters for a RSA public key
for efficiency while RSA, in theory, requires only two. In addition,
those parameters are expected to come from FIT image.

So this function won't fit very well when we want to use it for the purpose
of implementing UEFI secure boot, in particular, image authentication
as well as variable authentication, where the essential two parameters
are set to be retrieved from one of X509 certificates in signature
database.

So, in this patch, additional three parameters will be calculated
on the fly when rsa_verify() is called without fdt which should contain
parameters above.

This calculation heavily relies on "big-number (or multi-precision)
library." Therefore some routines from BearSSL[1] under MIT license are
imported in this implementation. See Patch#2.
# Please let me know if this is not appropriate.

# Checkpatch will complain with lots of warnings/errors, but
# I intentionally don't fix them for maximum maintainability.

  [1] https://bearssl.org/

Changes in v1 (Oct 9, 2019)
* fix a build error on pine64-lts_defconfig (reported by Heinrich)
  by defining FIT_IMAGE_ENABLE_VERIFY flag and adding
  SPL_RSA_VERIFY config (patch#1)
* remove FIT-specific code from image-sig.c and put them to new
  image-fit-sig.c to allow us to disable CONFIG_FIT_SIGNATURE (patch#1)
* compile rsa-keyprop.c only if necessary (i.e. if
  CONFIG_RSA_VERIFY_WITH_PKEY) (patch#2)
* add SPDX license identifier in rsa-keyprop.c (patch#2)
* include <common.h> instead of <stdio.h> (patch#2)
* use U-Boot's byteorder helper functions instead of BearSSL's (patch#2)

AKASHI Takahiro (3):
  lib: rsa: decouple rsa from FIT image verification
  lib: rsa: generate additional parameters for public key
  lib: rsa: add rsa_verify_with_pkey()

 Kconfig                      |   1 +
 common/Makefile              |   3 +-
 common/image-fit-sig.c       | 417 +++++++++++++++++++++++++
 common/image-fit.c           |   6 +-
 common/image-sig.c           | 396 ------------------------
 include/image.h              |  14 +-
 include/u-boot/rsa-mod-exp.h |   3 +
 lib/rsa/Kconfig              |  12 +
 lib/rsa/Makefile             |   2 +-
 lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
 lib/rsa/rsa-verify.c         |  65 +++-
 tools/Makefile               |   2 +-
 12 files changed, 1095 insertions(+), 411 deletions(-)
 create mode 100644 common/image-fit-sig.c
 create mode 100644 lib/rsa/rsa-keyprop.c

-- 
2.21.0

[U-Boot] [PATCH v1 1/3] lib: rsa: decouple rsa from FIT image verification

[AKASHI Takahiro]

Introduce new configuration, CONFIG_RSA_VERIFY which will decouple building
RSA functions from FIT verification and allow for adding a RSA-based
signature verification for other file formats, in particular PE file
for UEFI secure boot.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 Kconfig                |   1 +
 common/Makefile        |   3 +-
 common/image-fit-sig.c | 417 +++++++++++++++++++++++++++++++++++++++++
 common/image-fit.c     |   6 +-
 common/image-sig.c     | 396 --------------------------------------
 include/image.h        |  14 +-
 lib/rsa/Kconfig        |  12 ++
 lib/rsa/Makefile       |   2 +-
 lib/rsa/rsa-verify.c   |   2 +
 tools/Makefile         |   2 +-
 10 files changed, 452 insertions(+), 403 deletions(-)
 create mode 100644 common/image-fit-sig.c

diff --git a/Kconfig b/Kconfig
index 1f0904f7045e..cf885fc2750f 100644
--- a/Kconfig
+++ b/Kconfig
@@ -416,6 +416,7 @@ config SPL_FIT_SIGNATURE
 	depends on SPL_DM
 	select SPL_FIT
 	select SPL_RSA
+	select SPL_RSA_VERIFY
 
 config SPL_LOAD_FIT
 	bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
diff --git a/common/Makefile b/common/Makefile
index 302d8beaf356..5457f4738a02 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -112,7 +112,8 @@ obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
 obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
 obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
 obj-$(CONFIG_$(SPL_)MULTI_DTB_FIT) += boot_fit.o common_fit.o
-obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-sig.o
+obj-$(CONFIG_RSA_VERIFY) += image-sig.o
+obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-fit-sig.o
 obj-$(CONFIG_IO_TRACE) += iotrace.o
 obj-y += memsize.o
 obj-y += stdio.o
diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
new file mode 100644
index 000000000000..f6caeb0c5901
--- /dev/null
+++ b/common/image-fit-sig.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#ifdef USE_HOSTCC
+#include "mkimage.h"
+#include <time.h>
+#else
+#include <common.h>
+#include <malloc.h>
+DECLARE_GLOBAL_DATA_PTR;
+#endif /* !USE_HOSTCC*/
+#include <image.h>
+#include <u-boot/rsa.h>
+#include <u-boot/rsa-checksum.h>
+
+#define IMAGE_MAX_HASHED_NODES		100
+
+#ifdef USE_HOSTCC
+void *host_blob;
+
+void image_set_host_blob(void *blob)
+{
+	host_blob = blob;
+}
+
+void *image_get_host_blob(void)
+{
+	return host_blob;
+}
+#endif
+
+/**
+ * fit_region_make_list() - Make a list of image regions
+ *
+ * Given a list of fdt_regions, create a list of image_regions. This is a
+ * simple conversion routine since the FDT and image code use different
+ * structures.
+ *
+ * @fit: FIT image
+ * @fdt_regions: Pointer to FDT regions
+ * @count: Number of FDT regions
+ * @region: Pointer to image regions, which must hold @count records. If
+ * region is NULL, then (except for an SPL build) the array will be
+ * allocated.
+ * @return: Pointer to image regions
+ */
+struct image_region *fit_region_make_list(const void *fit,
+					  struct fdt_region *fdt_regions,
+					  int count,
+					  struct image_region *region)
+{
+	int i;
+
+	debug("Hash regions:\n");
+	debug("%10s %10s\n", "Offset", "Size");
+
+	/*
+	 * Use malloc() except in SPL (to save code size). In SPL the caller
+	 * must allocate the array.
+	 */
+#ifndef CONFIG_SPL_BUILD
+	if (!region)
+		region = calloc(sizeof(*region), count);
+#endif
+	if (!region)
+		return NULL;
+	for (i = 0; i < count; i++) {
+		debug("%10x %10x\n", fdt_regions[i].offset,
+		      fdt_regions[i].size);
+		region[i].data = fit + fdt_regions[i].offset;
+		region[i].size = fdt_regions[i].size;
+	}
+
+	return region;
+}
+
+static int fit_image_setup_verify(struct image_sign_info *info,
+				  const void *fit, int noffset,
+				  int required_keynode, char **err_msgp)
+{
+	char *algo_name;
+	const char *padding_name;
+
+	if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
+		*err_msgp = "Total size too large";
+		return 1;
+	}
+
+	if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
+		*err_msgp = "Can't get hash algo property";
+		return -1;
+	}
+
+	padding_name = fdt_getprop(fit, noffset, "padding", NULL);
+	if (!padding_name)
+		padding_name = RSA_DEFAULT_PADDING_NAME;
+
+	memset(info, '\0', sizeof(*info));
+	info->keyname = fdt_getprop(fit, noffset, "key-name-hint", NULL);
+	info->fit = (void *)fit;
+	info->node_offset = noffset;
+	info->name = algo_name;
+	info->checksum = image_get_checksum_algo(algo_name);
+	info->crypto = image_get_crypto_algo(algo_name);
+	info->padding = image_get_padding_algo(padding_name);
+	info->fdt_blob = gd_fdt_blob();
+	info->required_keynode = required_keynode;
+	printf("%s:%s", algo_name, info->keyname);
+
+	if (!info->checksum || !info->crypto || !info->padding) {
+		*err_msgp = "Unknown signature algorithm";
+		return -1;
+	}
+
+	return 0;
+}
+
+int fit_image_check_sig(const void *fit, int noffset, const void *data,
+			size_t size, int required_keynode, char **err_msgp)
+{
+	struct image_sign_info info;
+	struct image_region region;
+	uint8_t *fit_value;
+	int fit_value_len;
+
+	*err_msgp = NULL;
+	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
+				   err_msgp))
+		return -1;
+
+	if (fit_image_hash_get_value(fit, noffset, &fit_value,
+				     &fit_value_len)) {
+		*err_msgp = "Can't get hash value property";
+		return -1;
+	}
+
+	region.data = data;
+	region.size = size;
+
+	if (info.crypto->verify(&info, &region, 1, fit_value, fit_value_len)) {
+		*err_msgp = "Verification failed";
+		return -1;
+	}
+
+	return 0;
+}
+
+static int fit_image_verify_sig(const void *fit, int image_noffset,
+				const char *data, size_t size,
+				const void *sig_blob, int sig_offset)
+{
+	int noffset;
+	char *err_msg = "";
+	int verified = 0;
+	int ret;
+
+	/* Process all hash subnodes of the component image node */
+	fdt_for_each_subnode(noffset, fit, image_noffset) {
+		const char *name = fit_get_name(fit, noffset, NULL);
+
+		if (!strncmp(name, FIT_SIG_NODENAME,
+			     strlen(FIT_SIG_NODENAME))) {
+			ret = fit_image_check_sig(fit, noffset, data,
+						  size, -1, &err_msg);
+			if (ret) {
+				puts("- ");
+			} else {
+				puts("+ ");
+				verified = 1;
+				break;
+			}
+		}
+	}
+
+	if (noffset == -FDT_ERR_TRUNCATED || noffset == -FDT_ERR_BADSTRUCTURE) {
+		err_msg = "Corrupted or truncated tree";
+		goto error;
+	}
+
+	return verified ? 0 : -EPERM;
+
+error:
+	printf(" error!\n%s for '%s' hash node in '%s' image node\n",
+	       err_msg, fit_get_name(fit, noffset, NULL),
+	       fit_get_name(fit, image_noffset, NULL));
+	return -1;
+}
+
+int fit_image_verify_required_sigs(const void *fit, int image_noffset,
+				   const char *data, size_t size,
+				   const void *sig_blob, int *no_sigsp)
+{
+	int verify_count = 0;
+	int noffset;
+	int sig_node;
+
+	/* Work out what we need to verify */
+	*no_sigsp = 1;
+	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
+	if (sig_node < 0) {
+		debug("%s: No signature node found: %s\n", __func__,
+		      fdt_strerror(sig_node));
+		return 0;
+	}
+
+	fdt_for_each_subnode(noffset, sig_blob, sig_node) {
+		const char *required;
+		int ret;
+
+		required = fdt_getprop(sig_blob, noffset, "required", NULL);
+		if (!required || strcmp(required, "image"))
+			continue;
+		ret = fit_image_verify_sig(fit, image_noffset, data, size,
+					   sig_blob, noffset);
+		if (ret) {
+			printf("Failed to verify required signature '%s'\n",
+			       fit_get_name(sig_blob, noffset, NULL));
+			return ret;
+		}
+		verify_count++;
+	}
+
+	if (verify_count)
+		*no_sigsp = 0;
+
+	return 0;
+}
+
+int fit_config_check_sig(const void *fit, int noffset, int required_keynode,
+			 char **err_msgp)
+{
+	char * const exc_prop[] = {"data"};
+	const char *prop, *end, *name;
+	struct image_sign_info info;
+	const uint32_t *strings;
+	uint8_t *fit_value;
+	int fit_value_len;
+	int max_regions;
+	int i, prop_len;
+	char path[200];
+	int count;
+
+	debug("%s: fdt=%p, conf='%s', sig='%s'\n", __func__, gd_fdt_blob(),
+	      fit_get_name(fit, noffset, NULL),
+	      fit_get_name(gd_fdt_blob(), required_keynode, NULL));
+	*err_msgp = NULL;
+	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
+				   err_msgp))
+		return -1;
+
+	if (fit_image_hash_get_value(fit, noffset, &fit_value,
+				     &fit_value_len)) {
+		*err_msgp = "Can't get hash value property";
+		return -1;
+	}
+
+	/* Count the number of strings in the property */
+	prop = fdt_getprop(fit, noffset, "hashed-nodes", &prop_len);
+	end = prop ? prop + prop_len : prop;
+	for (name = prop, count = 0; name < end; name++)
+		if (!*name)
+			count++;
+	if (!count) {
+		*err_msgp = "Can't get hashed-nodes property";
+		return -1;
+	}
+
+	if (prop && prop_len > 0 && prop[prop_len - 1] != '\0') {
+		*err_msgp = "hashed-nodes property must be null-terminated";
+		return -1;
+	}
+
+	/* Add a sanity check here since we are using the stack */
+	if (count > IMAGE_MAX_HASHED_NODES) {
+		*err_msgp = "Number of hashed nodes exceeds maximum";
+		return -1;
+	}
+
+	/* Create a list of node names from those strings */
+	char *node_inc[count];
+
+	debug("Hash nodes (%d):\n", count);
+	for (name = prop, i = 0; name < end; name += strlen(name) + 1, i++) {
+		debug("   '%s'\n", name);
+		node_inc[i] = (char *)name;
+	}
+
+	/*
+	 * Each node can generate one region for each sub-node. Allow for
+	 * 7 sub-nodes (hash-1, signature-1, etc.) and some extra.
+	 */
+	max_regions = 20 + count * 7;
+	struct fdt_region fdt_regions[max_regions];
+
+	/* Get a list of regions to hash */
+	count = fdt_find_regions(fit, node_inc, count,
+				 exc_prop, ARRAY_SIZE(exc_prop),
+				 fdt_regions, max_regions - 1,
+				 path, sizeof(path), 0);
+	if (count < 0) {
+		*err_msgp = "Failed to hash configuration";
+		return -1;
+	}
+	if (count == 0) {
+		*err_msgp = "No data to hash";
+		return -1;
+	}
+	if (count >= max_regions - 1) {
+		*err_msgp = "Too many hash regions";
+		return -1;
+	}
+
+	/* Add the strings */
+	strings = fdt_getprop(fit, noffset, "hashed-strings", NULL);
+	if (strings) {
+		/*
+		 * The strings region offset must be a static 0x0.
+		 * This is set in tool/image-host.c
+		 */
+		fdt_regions[count].offset = fdt_off_dt_strings(fit);
+		fdt_regions[count].size = fdt32_to_cpu(strings[1]);
+		count++;
+	}
+
+	/* Allocate the region list on the stack */
+	struct image_region region[count];
+
+	fit_region_make_list(fit, fdt_regions, count, region);
+	if (info.crypto->verify(&info, region, count, fit_value,
+				fit_value_len)) {
+		*err_msgp = "Verification failed";
+		return -1;
+	}
+
+	return 0;
+}
+
+static int fit_config_verify_sig(const void *fit, int conf_noffset,
+				 const void *sig_blob, int sig_offset)
+{
+	int noffset;
+	char *err_msg = "";
+	int verified = 0;
+	int ret;
+
+	/* Process all hash subnodes of the component conf node */
+	fdt_for_each_subnode(noffset, fit, conf_noffset) {
+		const char *name = fit_get_name(fit, noffset, NULL);
+
+		if (!strncmp(name, FIT_SIG_NODENAME,
+			     strlen(FIT_SIG_NODENAME))) {
+			ret = fit_config_check_sig(fit, noffset, sig_offset,
+						   &err_msg);
+			if (ret) {
+				puts("- ");
+			} else {
+				puts("+ ");
+				verified = 1;
+				break;
+			}
+		}
+	}
+
+	if (noffset == -FDT_ERR_TRUNCATED || noffset == -FDT_ERR_BADSTRUCTURE) {
+		err_msg = "Corrupted or truncated tree";
+		goto error;
+	}
+
+	return verified ? 0 : -EPERM;
+
+error:
+	printf(" error!\n%s for '%s' hash node in '%s' config node\n",
+	       err_msg, fit_get_name(fit, noffset, NULL),
+	       fit_get_name(fit, conf_noffset, NULL));
+	return -1;
+}
+
+int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
+				    const void *sig_blob)
+{
+	int noffset;
+	int sig_node;
+
+	/* Work out what we need to verify */
+	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
+	if (sig_node < 0) {
+		debug("%s: No signature node found: %s\n", __func__,
+		      fdt_strerror(sig_node));
+		return 0;
+	}
+
+	fdt_for_each_subnode(noffset, sig_blob, sig_node) {
+		const char *required;
+		int ret;
+
+		required = fdt_getprop(sig_blob, noffset, "required", NULL);
+		if (!required || strcmp(required, "conf"))
+			continue;
+		ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
+					    noffset);
+		if (ret) {
+			printf("Failed to verify required signature '%s'\n",
+			       fit_get_name(sig_blob, noffset, NULL));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+int fit_config_verify(const void *fit, int conf_noffset)
+{
+	return fit_config_verify_required_sigs(fit, conf_noffset,
+					       gd_fdt_blob());
+}
diff --git a/common/image-fit.c b/common/image-fit.c
index 5c63c769de27..2d78f0b47da2 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1216,7 +1216,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
 	int ret;
 
 	/* Verify all required signatures */
-	if (IMAGE_ENABLE_VERIFY &&
+	if (FIT_IMAGE_ENABLE_VERIFY &&
 	    fit_image_verify_required_sigs(fit, image_noffset, data, size,
 					   gd_fdt_blob(), &verify_all)) {
 		err_msg = "Unable to verify required signature";
@@ -1238,7 +1238,7 @@ int fit_image_verify_with_data(const void *fit, int image_noffset,
 						 &err_msg))
 				goto error;
 			puts("+ ");
-		} else if (IMAGE_ENABLE_VERIFY && verify_all &&
+		} else if (FIT_IMAGE_ENABLE_VERIFY && verify_all &&
 				!strncmp(name, FIT_SIG_NODENAME,
 					strlen(FIT_SIG_NODENAME))) {
 			ret = fit_image_check_sig(fit, noffset, data,
@@ -1870,7 +1870,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		if (image_type == IH_TYPE_KERNEL)
 			images->fit_uname_cfg = fit_base_uname_config;
 
-		if (IMAGE_ENABLE_VERIFY && images->verify) {
+		if (FIT_IMAGE_ENABLE_VERIFY && images->verify) {
 			puts("   Verifying Hash Integrity ... ");
 			if (fit_config_verify(fit, cfg_noffset)) {
 				puts("Bad Data Hash\n");
diff --git a/common/image-sig.c b/common/image-sig.c
index 004fbc525b5c..0fc114b09ce9 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -17,18 +17,6 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define IMAGE_MAX_HASHED_NODES		100
 
-#ifdef USE_HOSTCC
-void *host_blob;
-void image_set_host_blob(void *blob)
-{
-	host_blob = blob;
-}
-void *image_get_host_blob(void)
-{
-	return host_blob;
-}
-#endif
-
 struct checksum_algo checksum_algos[] = {
 	{
 		.name = "sha1",
@@ -133,387 +121,3 @@ struct padding_algo *image_get_padding_algo(const char *name)
 
 	return NULL;
 }
-
-/**
- * fit_region_make_list() - Make a list of image regions
- *
- * Given a list of fdt_regions, create a list of image_regions. This is a
- * simple conversion routine since the FDT and image code use different
- * structures.
- *
- * @fit: FIT image
- * @fdt_regions: Pointer to FDT regions
- * @count: Number of FDT regions
- * @region: Pointer to image regions, which must hold @count records. If
- * region is NULL, then (except for an SPL build) the array will be
- * allocated.
- * @return: Pointer to image regions
- */
-struct image_region *fit_region_make_list(const void *fit,
-		struct fdt_region *fdt_regions, int count,
-		struct image_region *region)
-{
-	int i;
-
-	debug("Hash regions:\n");
-	debug("%10s %10s\n", "Offset", "Size");
-
-	/*
-	 * Use malloc() except in SPL (to save code size). In SPL the caller
-	 * must allocate the array.
-	 */
-#ifndef CONFIG_SPL_BUILD
-	if (!region)
-		region = calloc(sizeof(*region), count);
-#endif
-	if (!region)
-		return NULL;
-	for (i = 0; i < count; i++) {
-		debug("%10x %10x\n", fdt_regions[i].offset,
-		      fdt_regions[i].size);
-		region[i].data = fit + fdt_regions[i].offset;
-		region[i].size = fdt_regions[i].size;
-	}
-
-	return region;
-}
-
-static int fit_image_setup_verify(struct image_sign_info *info,
-		const void *fit, int noffset, int required_keynode,
-		char **err_msgp)
-{
-	char *algo_name;
-	const char *padding_name;
-
-	if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
-		*err_msgp = "Total size too large";
-		return 1;
-	}
-
-	if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
-		*err_msgp = "Can't get hash algo property";
-		return -1;
-	}
-
-	padding_name = fdt_getprop(fit, noffset, "padding", NULL);
-	if (!padding_name)
-		padding_name = RSA_DEFAULT_PADDING_NAME;
-
-	memset(info, '\0', sizeof(*info));
-	info->keyname = fdt_getprop(fit, noffset, "key-name-hint", NULL);
-	info->fit = (void *)fit;
-	info->node_offset = noffset;
-	info->name = algo_name;
-	info->checksum = image_get_checksum_algo(algo_name);
-	info->crypto = image_get_crypto_algo(algo_name);
-	info->padding = image_get_padding_algo(padding_name);
-	info->fdt_blob = gd_fdt_blob();
-	info->required_keynode = required_keynode;
-	printf("%s:%s", algo_name, info->keyname);
-
-	if (!info->checksum || !info->crypto || !info->padding) {
-		*err_msgp = "Unknown signature algorithm";
-		return -1;
-	}
-
-	return 0;
-}
-
-int fit_image_check_sig(const void *fit, int noffset, const void *data,
-		size_t size, int required_keynode, char **err_msgp)
-{
-	struct image_sign_info info;
-	struct image_region region;
-	uint8_t *fit_value;
-	int fit_value_len;
-
-	*err_msgp = NULL;
-	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
-				   err_msgp))
-		return -1;
-
-	if (fit_image_hash_get_value(fit, noffset, &fit_value,
-				     &fit_value_len)) {
-		*err_msgp = "Can't get hash value property";
-		return -1;
-	}
-
-	region.data = data;
-	region.size = size;
-
-	if (info.crypto->verify(&info, &region, 1, fit_value, fit_value_len)) {
-		*err_msgp = "Verification failed";
-		return -1;
-	}
-
-	return 0;
-}
-
-static int fit_image_verify_sig(const void *fit, int image_noffset,
-		const char *data, size_t size, const void *sig_blob,
-		int sig_offset)
-{
-	int noffset;
-	char *err_msg = "";
-	int verified = 0;
-	int ret;
-
-	/* Process all hash subnodes of the component image node */
-	fdt_for_each_subnode(noffset, fit, image_noffset) {
-		const char *name = fit_get_name(fit, noffset, NULL);
-
-		if (!strncmp(name, FIT_SIG_NODENAME,
-			     strlen(FIT_SIG_NODENAME))) {
-			ret = fit_image_check_sig(fit, noffset, data,
-							size, -1, &err_msg);
-			if (ret) {
-				puts("- ");
-			} else {
-				puts("+ ");
-				verified = 1;
-				break;
-			}
-		}
-	}
-
-	if (noffset == -FDT_ERR_TRUNCATED || noffset == -FDT_ERR_BADSTRUCTURE) {
-		err_msg = "Corrupted or truncated tree";
-		goto error;
-	}
-
-	return verified ? 0 : -EPERM;
-
-error:
-	printf(" error!\n%s for '%s' hash node in '%s' image node\n",
-	       err_msg, fit_get_name(fit, noffset, NULL),
-	       fit_get_name(fit, image_noffset, NULL));
-	return -1;
-}
-
-int fit_image_verify_required_sigs(const void *fit, int image_noffset,
-		const char *data, size_t size, const void *sig_blob,
-		int *no_sigsp)
-{
-	int verify_count = 0;
-	int noffset;
-	int sig_node;
-
-	/* Work out what we need to verify */
-	*no_sigsp = 1;
-	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
-	if (sig_node < 0) {
-		debug("%s: No signature node found: %s\n", __func__,
-		      fdt_strerror(sig_node));
-		return 0;
-	}
-
-	fdt_for_each_subnode(noffset, sig_blob, sig_node) {
-		const char *required;
-		int ret;
-
-		required = fdt_getprop(sig_blob, noffset, "required", NULL);
-		if (!required || strcmp(required, "image"))
-			continue;
-		ret = fit_image_verify_sig(fit, image_noffset, data, size,
-					sig_blob, noffset);
-		if (ret) {
-			printf("Failed to verify required signature '%s'\n",
-			       fit_get_name(sig_blob, noffset, NULL));
-			return ret;
-		}
-		verify_count++;
-	}
-
-	if (verify_count)
-		*no_sigsp = 0;
-
-	return 0;
-}
-
-int fit_config_check_sig(const void *fit, int noffset, int required_keynode,
-			 char **err_msgp)
-{
-	char * const exc_prop[] = {"data"};
-	const char *prop, *end, *name;
-	struct image_sign_info info;
-	const uint32_t *strings;
-	uint8_t *fit_value;
-	int fit_value_len;
-	int max_regions;
-	int i, prop_len;
-	char path[200];
-	int count;
-
-	debug("%s: fdt=%p, conf='%s', sig='%s'\n", __func__, gd_fdt_blob(),
-	      fit_get_name(fit, noffset, NULL),
-	      fit_get_name(gd_fdt_blob(), required_keynode, NULL));
-	*err_msgp = NULL;
-	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
-				   err_msgp))
-		return -1;
-
-	if (fit_image_hash_get_value(fit, noffset, &fit_value,
-				     &fit_value_len)) {
-		*err_msgp = "Can't get hash value property";
-		return -1;
-	}
-
-	/* Count the number of strings in the property */
-	prop = fdt_getprop(fit, noffset, "hashed-nodes", &prop_len);
-	end = prop ? prop + prop_len : prop;
-	for (name = prop, count = 0; name < end; name++)
-		if (!*name)
-			count++;
-	if (!count) {
-		*err_msgp = "Can't get hashed-nodes property";
-		return -1;
-	}
-
-	if (prop && prop_len > 0 && prop[prop_len - 1] != '\0') {
-		*err_msgp = "hashed-nodes property must be null-terminated";
-		return -1;
-	}
-
-	/* Add a sanity check here since we are using the stack */
-	if (count > IMAGE_MAX_HASHED_NODES) {
-		*err_msgp = "Number of hashed nodes exceeds maximum";
-		return -1;
-	}
-
-	/* Create a list of node names from those strings */
-	char *node_inc[count];
-
-	debug("Hash nodes (%d):\n", count);
-	for (name = prop, i = 0; name < end; name += strlen(name) + 1, i++) {
-		debug("   '%s'\n", name);
-		node_inc[i] = (char *)name;
-	}
-
-	/*
-	 * Each node can generate one region for each sub-node. Allow for
-	 * 7 sub-nodes (hash-1, signature-1, etc.) and some extra.
-	 */
-	max_regions = 20 + count * 7;
-	struct fdt_region fdt_regions[max_regions];
-
-	/* Get a list of regions to hash */
-	count = fdt_find_regions(fit, node_inc, count,
-			exc_prop, ARRAY_SIZE(exc_prop),
-			fdt_regions, max_regions - 1,
-			path, sizeof(path), 0);
-	if (count < 0) {
-		*err_msgp = "Failed to hash configuration";
-		return -1;
-	}
-	if (count == 0) {
-		*err_msgp = "No data to hash";
-		return -1;
-	}
-	if (count >= max_regions - 1) {
-		*err_msgp = "Too many hash regions";
-		return -1;
-	}
-
-	/* Add the strings */
-	strings = fdt_getprop(fit, noffset, "hashed-strings", NULL);
-	if (strings) {
-		/*
-		 * The strings region offset must be a static 0x0.
-		 * This is set in tool/image-host.c
-		 */
-		fdt_regions[count].offset = fdt_off_dt_strings(fit);
-		fdt_regions[count].size = fdt32_to_cpu(strings[1]);
-		count++;
-	}
-
-	/* Allocate the region list on the stack */
-	struct image_region region[count];
-
-	fit_region_make_list(fit, fdt_regions, count, region);
-	if (info.crypto->verify(&info, region, count, fit_value,
-				fit_value_len)) {
-		*err_msgp = "Verification failed";
-		return -1;
-	}
-
-	return 0;
-}
-
-static int fit_config_verify_sig(const void *fit, int conf_noffset,
-		const void *sig_blob, int sig_offset)
-{
-	int noffset;
-	char *err_msg = "";
-	int verified = 0;
-	int ret;
-
-	/* Process all hash subnodes of the component conf node */
-	fdt_for_each_subnode(noffset, fit, conf_noffset) {
-		const char *name = fit_get_name(fit, noffset, NULL);
-
-		if (!strncmp(name, FIT_SIG_NODENAME,
-			     strlen(FIT_SIG_NODENAME))) {
-			ret = fit_config_check_sig(fit, noffset, sig_offset,
-						   &err_msg);
-			if (ret) {
-				puts("- ");
-			} else {
-				puts("+ ");
-				verified = 1;
-				break;
-			}
-		}
-	}
-
-	if (noffset == -FDT_ERR_TRUNCATED || noffset == -FDT_ERR_BADSTRUCTURE) {
-		err_msg = "Corrupted or truncated tree";
-		goto error;
-	}
-
-	return verified ? 0 : -EPERM;
-
-error:
-	printf(" error!\n%s for '%s' hash node in '%s' config node\n",
-	       err_msg, fit_get_name(fit, noffset, NULL),
-	       fit_get_name(fit, conf_noffset, NULL));
-	return -1;
-}
-
-int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
-		const void *sig_blob)
-{
-	int noffset;
-	int sig_node;
-
-	/* Work out what we need to verify */
-	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
-	if (sig_node < 0) {
-		debug("%s: No signature node found: %s\n", __func__,
-		      fdt_strerror(sig_node));
-		return 0;
-	}
-
-	fdt_for_each_subnode(noffset, sig_blob, sig_node) {
-		const char *required;
-		int ret;
-
-		required = fdt_getprop(sig_blob, noffset, "required", NULL);
-		if (!required || strcmp(required, "conf"))
-			continue;
-		ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
-					    noffset);
-		if (ret) {
-			printf("Failed to verify required signature '%s'\n",
-			       fit_get_name(sig_blob, noffset, NULL));
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-int fit_config_verify(const void *fit, int conf_noffset)
-{
-	return fit_config_verify_required_sigs(fit, conf_noffset,
-					       gd_fdt_blob());
-}
diff --git a/include/image.h b/include/image.h
index c1065c06f9bd..e37011b7e463 100644
--- a/include/image.h
+++ b/include/image.h
@@ -18,6 +18,15 @@
 #include "compiler.h"
 #include <asm/byteorder.h>
 #include <stdbool.h>
+#if 1
+#ifndef USE_HOSTCC
+/*
+ * We should not include kconfig.h> on host, otherwise it may cause
+ * build errors due to unexpected CONFIG_* defined.
+ */
+#include <linux/kconfig.h>
+#endif
+#endif
 
 /* Define this to avoid #ifdefs later on */
 struct lmb;
@@ -1098,14 +1107,17 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
 #  define IMAGE_ENABLE_VERIFY	1
+#  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
 #  define IMAGE_ENABLE_VERIFY	0
+#  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
+# define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
+# define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
 #ifdef USE_HOSTCC
diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
index 2b33f323bccc..62b7ab9c5e5c 100644
--- a/lib/rsa/Kconfig
+++ b/lib/rsa/Kconfig
@@ -1,5 +1,6 @@
 config RSA
 	bool "Use RSA Library"
+	select RSA_VERIFY
 	select RSA_FREESCALE_EXP if FSL_CAAM && !ARCH_MX7 && !ARCH_MX6 && !ARCH_MX5
 	select RSA_SOFTWARE_EXP if !RSA_FREESCALE_EXP
 	help
@@ -17,6 +18,17 @@ if RSA
 
 config SPL_RSA
 	bool "Use RSA Library within SPL"
+	select RSA_VERIFY
+
+config SPL_RSA_VERIFY
+	bool
+	help
+	  Add RSA signature verification support in SPL.
+
+config RSA_VERIFY
+	bool
+	help
+	  Add RSA signature verification support.
 
 config RSA_SOFTWARE_EXP
 	bool "Enable driver for RSA Modular Exponentiation in software"
diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
index a51c6e1685fb..c07305188e0c 100644
--- a/lib/rsa/Makefile
+++ b/lib/rsa/Makefile
@@ -5,5 +5,5 @@
 # (C) Copyright 2000-2007
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
-obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += rsa-verify.o rsa-checksum.o
+obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
 obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index 287fcc4d234d..1df42f28c64a 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -270,6 +270,7 @@ out:
 }
 #endif
 
+#if CONFIG_IS_ENABLED(FIT_SIGNATURE) || defined(CONFIG_RSA_VERIFY_WITH_PKEY)
 /**
  * rsa_verify_key() - Verify a signature against some data using RSA Key
  *
@@ -341,6 +342,7 @@ static int rsa_verify_key(struct image_sign_info *info,
 
 	return 0;
 }
+#endif
 
 /**
  * rsa_verify_with_keynode() - Verify a signature against some data using
diff --git a/tools/Makefile b/tools/Makefile
index 24581adccd4b..1c46d699ec61 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -58,7 +58,7 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
 hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
 
 FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o
-FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o
+FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o
 
 # The following files are synced with upstream DTC.
 # Use synced versions from scripts/dtc/libfdt/.
-- 
2.21.0

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[AKASHI Takahiro]

In the current implementation of FIT_SIGNATURE, five parameters for
a RSA public key are required while only two of them are essential.
(See rsa-mod-exp.h and uImage.FIT/signature.txt)
This is a result of considering relatively limited computer power
and resources on embedded systems, while such a assumption may not
be quite practical for other use cases.

In this patch, added is a function, rsa_gen_key_prop(), which will
generate additional parameters for other uses, in particular
UEFI secure boot, on the fly.

Note: the current code uses some "big number" routines from BearSSL
for the calculation.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/u-boot/rsa-mod-exp.h |   3 +
 lib/rsa/Kconfig              |   7 +
 lib/rsa/Makefile             |   1 +
 lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
 4 files changed, 596 insertions(+)
 create mode 100644 lib/rsa/rsa-keyprop.c

diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
index 8a428c4b6a1a..ca189292d869 100644
--- a/include/u-boot/rsa-mod-exp.h
+++ b/include/u-boot/rsa-mod-exp.h
@@ -26,6 +26,9 @@ struct key_prop {
 	uint32_t exp_len;	/* Exponent length in number of uint8_t */
 };
 
+struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
+void rsa_free_key_prop(struct key_prop *prop);
+
 /**
  * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
  *
diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
index 62b7ab9c5e5c..d1743d7a4c47 100644
--- a/lib/rsa/Kconfig
+++ b/lib/rsa/Kconfig
@@ -30,6 +30,13 @@ config RSA_VERIFY
 	help
 	  Add RSA signature verification support.
 
+config RSA_VERIFY_WITH_PKEY
+	bool "Execute RSA verification without key parameters from FDT"
+	depends on RSA
+	help
+	  This options enables RSA signature verification without
+	  using public key parameters which is embedded control FDT.
+
 config RSA_SOFTWARE_EXP
 	bool "Enable driver for RSA Modular Exponentiation in software"
 	depends on DM
diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
index c07305188e0c..14ed3cb4012b 100644
--- a/lib/rsa/Makefile
+++ b/lib/rsa/Makefile
@@ -6,4 +6,5 @@
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
 obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
+obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
 obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
new file mode 100644
index 000000000000..d7d222e9bed9
--- /dev/null
+++ b/lib/rsa/rsa-keyprop.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0+ and MIT
+/*
+ * RSA library - generate parameters for a public key
+ *
+ * Copyright (c) 2019 Linaro Limited
+ * Author: AKASHI Takahiro
+ *
+ * Big number routines in this file come from BearSSL:
+ * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
+ */
+
+#include <common.h>
+#include <image.h>
+#include <malloc.h>
+#include <asm/byteorder.h>
+#include <crypto/internal/rsa.h>
+#include <u-boot/rsa-mod-exp.h>
+
+static inline unsigned
+br_dec16be(const void *src)
+{
+	return be16_to_cpup(src);
+}
+
+static inline uint32_t
+br_dec32be(const void *src)
+{
+	return be32_to_cpup(src);
+}
+
+static inline void
+br_enc32be(void *dst, uint32_t x)
+{
+	__be32 tmp;
+
+	tmp = cpu_to_be32(x);
+	memcpy(dst, &tmp, sizeof(tmp));
+}
+
+/* stripped version of src/inner.h */
+
+static inline uint32_t
+NOT(uint32_t ctl)
+{
+	return ctl ^ 1;
+}
+
+static inline uint32_t
+MUX(uint32_t ctl, uint32_t x, uint32_t y)
+{
+	return y ^ (-ctl & (x ^ y));
+}
+
+static inline uint32_t
+EQ(uint32_t x, uint32_t y)
+{
+	uint32_t q;
+
+	q = x ^ y;
+	return NOT((q | -q) >> 31);
+}
+
+static inline uint32_t
+NEQ(uint32_t x, uint32_t y)
+{
+	uint32_t q;
+
+	q = x ^ y;
+	return (q | -q) >> 31;
+}
+
+static inline uint32_t
+GT(uint32_t x, uint32_t y)
+{
+	/*
+	 * If both x < 2^31 and y < 2^31, then y-x will have its high
+	 * bit set if x > y, cleared otherwise.
+	 *
+	 * If either x >= 2^31 or y >= 2^31 (but not both), then the
+	 * result is the high bit of x.
+	 *
+	 * If both x >= 2^31 and y >= 2^31, then we can virtually
+	 * subtract 2^31 from both, and we are back to the first case.
+	 * Since (y-2^31)-(x-2^31) = y-x, the subtraction is already
+	 * fine.
+	 */
+	uint32_t z;
+
+	z = y - x;
+	return (z ^ ((x ^ y) & (x ^ z))) >> 31;
+}
+
+static inline uint32_t
+BIT_LENGTH(uint32_t x)
+{
+	uint32_t k, c;
+
+	k = NEQ(x, 0);
+	c = GT(x, 0xFFFF); x = MUX(c, x >> 16, x); k += c << 4;
+	c = GT(x, 0x00FF); x = MUX(c, x >>  8, x); k += c << 3;
+	c = GT(x, 0x000F); x = MUX(c, x >>  4, x); k += c << 2;
+	c = GT(x, 0x0003); x = MUX(c, x >>  2, x); k += c << 1;
+	k += GT(x, 0x0001);
+	return k;
+}
+
+#define GE(x, y)   NOT(GT(y, x))
+#define LT(x, y)   GT(y, x)
+#define MUL(x, y)   ((uint64_t)(x) * (uint64_t)(y))
+
+static inline uint32_t
+br_i32_word(const uint32_t *a, uint32_t off)
+{
+	size_t u;
+	unsigned j;
+
+	u = (size_t)(off >> 5) + 1;
+	j = (unsigned)off & 31;
+	if (j == 0) {
+		return a[u];
+	} else {
+		return (a[u] >> j) | (a[u + 1] << (32 - j));
+	}
+}
+
+/* src/int/i32_bitlen.c */
+
+static uint32_t
+br_i32_bit_length(uint32_t *x, size_t xlen)
+{
+	uint32_t tw, twk;
+
+	tw = 0;
+	twk = 0;
+	while (xlen -- > 0) {
+		uint32_t w, c;
+
+		c = EQ(tw, 0);
+		w = x[xlen];
+		tw = MUX(c, w, tw);
+		twk = MUX(c, (uint32_t)xlen, twk);
+	}
+	return (twk << 5) + BIT_LENGTH(tw);
+}
+
+/* src/int/i32_decode.c */
+
+static void
+br_i32_decode(uint32_t *x, const void *src, size_t len)
+{
+	const unsigned char *buf;
+	size_t u, v;
+
+	buf = src;
+	u = len;
+	v = 1;
+	for (;;) {
+		if (u < 4) {
+			uint32_t w;
+
+			if (u < 2) {
+				if (u == 0) {
+					break;
+				} else {
+					w = buf[0];
+				}
+			} else {
+				if (u == 2) {
+					w = br_dec16be(buf);
+				} else {
+					w = ((uint32_t)buf[0] << 16)
+						| br_dec16be(buf + 1);
+				}
+			}
+			x[v ++] = w;
+			break;
+		} else {
+			u -= 4;
+			x[v ++] = br_dec32be(buf + u);
+		}
+	}
+	x[0] = br_i32_bit_length(x + 1, v - 1);
+}
+
+/* src/int/i32_encode.c */
+
+static void
+br_i32_encode(void *dst, size_t len, const uint32_t *x)
+{
+	unsigned char *buf;
+	size_t k;
+
+	buf = dst;
+
+	/*
+	 * Compute the announced size of x in bytes; extra bytes are
+	 * filled with zeros.
+	 */
+	k = (x[0] + 7) >> 3;
+	while (len > k) {
+		*buf ++ = 0;
+		len --;
+	}
+
+	/*
+	 * Now we use k as index within x[]. That index starts at 1;
+	 * we initialize it to the topmost complete word, and process
+	 * any remaining incomplete word.
+	 */
+	k = (len + 3) >> 2;
+	switch (len & 3) {
+	case 3:
+		*buf ++ = x[k] >> 16;
+		/* fall through */
+	case 2:
+		*buf ++ = x[k] >> 8;
+		/* fall through */
+	case 1:
+		*buf ++ = x[k];
+		k --;
+	}
+
+	/*
+	 * Encode all complete words.
+	 */
+	while (k > 0) {
+		br_enc32be(buf, x[k]);
+		k --;
+		buf += 4;
+	}
+}
+
+/* src/int/i32_ninv32.c */
+
+static uint32_t
+br_i32_ninv32(uint32_t x)
+{
+	uint32_t y;
+
+	y = 2 - x;
+	y *= 2 - y * x;
+	y *= 2 - y * x;
+	y *= 2 - y * x;
+	y *= 2 - y * x;
+	return MUX(x & 1, -y, 0);
+}
+
+/* src/int/i32_add.c */
+
+static uint32_t
+br_i32_add(uint32_t *a, const uint32_t *b, uint32_t ctl)
+{
+	uint32_t cc;
+	size_t u, m;
+
+	cc = 0;
+	m = (a[0] + 63) >> 5;
+	for (u = 1; u < m; u ++) {
+		uint32_t aw, bw, naw;
+
+		aw = a[u];
+		bw = b[u];
+		naw = aw + bw + cc;
+
+		/*
+		 * Carry is 1 if naw < aw. Carry is also 1 if naw == aw
+		 * AND the carry was already 1.
+		 */
+		cc = (cc & EQ(naw, aw)) | LT(naw, aw);
+		a[u] = MUX(ctl, naw, aw);
+	}
+	return cc;
+}
+
+/* src/int/i32_sub.c */
+
+static uint32_t
+br_i32_sub(uint32_t *a, const uint32_t *b, uint32_t ctl)
+{
+	uint32_t cc;
+	size_t u, m;
+
+	cc = 0;
+	m = (a[0] + 63) >> 5;
+	for (u = 1; u < m; u ++) {
+		uint32_t aw, bw, naw;
+
+		aw = a[u];
+		bw = b[u];
+		naw = aw - bw - cc;
+
+		/*
+		 * Carry is 1 if naw > aw. Carry is 1 also if naw == aw
+		 * AND the carry was already 1.
+		 */
+		cc = (cc & EQ(naw, aw)) | GT(naw, aw);
+		a[u] = MUX(ctl, naw, aw);
+	}
+	return cc;
+}
+
+/* src/int/i32_div32.c */
+
+static uint32_t
+br_divrem(uint32_t hi, uint32_t lo, uint32_t d, uint32_t *r)
+{
+	/* TODO: optimize this */
+	uint32_t q;
+	uint32_t ch, cf;
+	int k;
+
+	q = 0;
+	ch = EQ(hi, d);
+	hi = MUX(ch, 0, hi);
+	for (k = 31; k > 0; k --) {
+		int j;
+		uint32_t w, ctl, hi2, lo2;
+
+		j = 32 - k;
+		w = (hi << j) | (lo >> k);
+		ctl = GE(w, d) | (hi >> k);
+		hi2 = (w - d) >> j;
+		lo2 = lo - (d << k);
+		hi = MUX(ctl, hi2, hi);
+		lo = MUX(ctl, lo2, lo);
+		q |= ctl << k;
+	}
+	cf = GE(lo, d) | hi;
+	q |= cf;
+	*r = MUX(cf, lo - d, lo);
+	return q;
+}
+
+static inline uint32_t
+br_rem(uint32_t hi, uint32_t lo, uint32_t d)
+{
+	uint32_t r;
+
+	br_divrem(hi, lo, d, &r);
+	return r;
+}
+
+static inline uint32_t
+br_div(uint32_t hi, uint32_t lo, uint32_t d)
+{
+	uint32_t r;
+
+	return br_divrem(hi, lo, d, &r);
+}
+
+/* src/int/i32_muladd.c */
+
+static void
+br_i32_muladd_small(uint32_t *x, uint32_t z, const uint32_t *m)
+{
+	uint32_t m_bitlen;
+	size_t u, mlen;
+	uint32_t a0, a1, b0, hi, g, q, tb;
+	uint32_t chf, clow, under, over;
+	uint64_t cc;
+
+	/*
+	 * We can test on the modulus bit length since we accept to
+	 * leak that length.
+	 */
+	m_bitlen = m[0];
+	if (m_bitlen == 0) {
+		return;
+	}
+	if (m_bitlen <= 32) {
+		x[1] = br_rem(x[1], z, m[1]);
+		return;
+	}
+	mlen = (m_bitlen + 31) >> 5;
+
+	/*
+	 * Principle: we estimate the quotient (x*2^32+z)/m by
+	 * doing a 64/32 division with the high words.
+	 *
+	 * Let:
+	 *   w = 2^32
+	 *   a = (w*a0 + a1) * w^N + a2
+	 *   b = b0 * w^N + b2
+	 * such that:
+	 *   0 <= a0 < w
+	 *   0 <= a1 < w
+	 *   0 <= a2 < w^N
+	 *   w/2 <= b0 < w
+	 *   0 <= b2 < w^N
+	 *   a < w*b
+	 * I.e. the two top words of a are a0:a1, the top word of b is
+	 * b0, we ensured that b0 is "full" (high bit set), and a is
+	 * such that the quotient q = a/b fits on one word (0 <= q < w).
+	 *
+	 * If a = b*q + r (with 0 <= r < q), we can estimate q by
+	 * doing an Euclidean division on the top words:
+	 *   a0*w+a1 = b0*u + v  (with 0 <= v < w)
+	 * Then the following holds:
+	 *   0 <= u <= w
+	 *   u-2 <= q <= u
+	 */
+	a0 = br_i32_word(x, m_bitlen - 32);
+	hi = x[mlen];
+	memmove(x + 2, x + 1, (mlen - 1) * sizeof *x);
+	x[1] = z;
+	a1 = br_i32_word(x, m_bitlen - 32);
+	b0 = br_i32_word(m, m_bitlen - 32);
+
+	/*
+	 * We estimate a divisor q. If the quotient returned by br_div()
+	 * is g:
+	 * -- If a0 == b0 then g == 0; we want q = 0xFFFFFFFF.
+	 * -- Otherwise:
+	 *    -- if g == 0 then we set q = 0;
+	 *    -- otherwise, we set q = g - 1.
+	 * The properties described above then ensure that the true
+	 * quotient is q-1, q or q+1.
+	 */
+	g = br_div(a0, a1, b0);
+	q = MUX(EQ(a0, b0), 0xFFFFFFFF, MUX(EQ(g, 0), 0, g - 1));
+
+	/*
+	 * We subtract q*m from x (with the extra high word of value 'hi').
+	 * Since q may be off by 1 (in either direction), we may have to
+	 * add or subtract m afterwards.
+	 *
+	 * The 'tb' flag will be true (1)@the end of the loop if the
+	 * result is greater than or equal to the modulus (not counting
+	 * 'hi' or the carry).
+	 */
+	cc = 0;
+	tb = 1;
+	for (u = 1; u <= mlen; u ++) {
+		uint32_t mw, zw, xw, nxw;
+		uint64_t zl;
+
+		mw = m[u];
+		zl = MUL(mw, q) + cc;
+		cc = (uint32_t)(zl >> 32);
+		zw = (uint32_t)zl;
+		xw = x[u];
+		nxw = xw - zw;
+		cc += (uint64_t)GT(nxw, xw);
+		x[u] = nxw;
+		tb = MUX(EQ(nxw, mw), tb, GT(nxw, mw));
+	}
+
+	/*
+	 * If we underestimated q, then either cc < hi (one extra bit
+	 * beyond the top array word), or cc == hi and tb is true (no
+	 * extra bit, but the result is not lower than the modulus). In
+	 * these cases we must subtract m once.
+	 *
+	 * Otherwise, we may have overestimated, which will show as
+	 * cc > hi (thus a negative result). Correction is adding m once.
+	 */
+	chf = (uint32_t)(cc >> 32);
+	clow = (uint32_t)cc;
+	over = chf | GT(clow, hi);
+	under = ~over & (tb | (~chf & LT(clow, hi)));
+	br_i32_add(x, m, over);
+	br_i32_sub(x, m, under);
+}
+
+/* src/int/i32_reduce.c */
+
+static void
+br_i32_reduce(uint32_t *x, const uint32_t *a, const uint32_t *m)
+{
+	uint32_t m_bitlen, a_bitlen;
+	size_t mlen, alen, u;
+
+	m_bitlen = m[0];
+	mlen = (m_bitlen + 31) >> 5;
+
+	x[0] = m_bitlen;
+	if (m_bitlen == 0) {
+		return;
+	}
+
+	/*
+	 * If the source is shorter, then simply copy all words from a[]
+	 * and zero out the upper words.
+	 */
+	a_bitlen = a[0];
+	alen = (a_bitlen + 31) >> 5;
+	if (a_bitlen < m_bitlen) {
+		memcpy(x + 1, a + 1, alen * sizeof *a);
+		for (u = alen; u < mlen; u ++) {
+			x[u + 1] = 0;
+		}
+		return;
+	}
+
+	/*
+	 * The source length is@least equal to that of the modulus.
+	 * We must thus copy N-1 words, and input the remaining words
+	 * one by one.
+	 */
+	memcpy(x + 1, a + 2 + (alen - mlen), (mlen - 1) * sizeof *a);
+	x[mlen] = 0;
+	for (u = 1 + alen - mlen; u > 0; u --) {
+		br_i32_muladd_small(x, a[u], m);
+	}
+}
+
+void rsa_free_key_prop(struct key_prop *prop)
+{
+	if (!prop)
+		return;
+
+	free((void *)prop->modulus);
+	free((void *)prop->public_exponent);
+	free((void *)prop->rr);
+
+	free(prop);
+}
+
+struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen)
+{
+	struct key_prop *prop;
+	struct rsa_key rsa_key;
+#define BR_MAX_RSA_SIZE 4096
+	uint32_t *n, *rr, *rrtmp;
+	int rlen, i, ret;
+
+	prop = calloc(sizeof(*prop), 1);
+	if (!prop)
+		return NULL;
+	n = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
+	rr = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
+	rrtmp = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
+	if (!n || !rr || !rrtmp)
+		return NULL;
+
+	ret = rsa_parse_pub_key(&rsa_key, key, keylen);
+	if (ret)
+		goto err;
+
+	/* modulus */
+	/* removing leading 0's */
+	for (i = 0; i < rsa_key.n_sz && !rsa_key.n[i]; i++)
+		;
+	prop->num_bits = (rsa_key.n_sz - i) * 8;
+	prop->modulus = malloc(rsa_key.n_sz - i);
+	if (!prop->modulus)
+		goto err;
+	memcpy((void *)prop->modulus, &rsa_key.n[i], rsa_key.n_sz - i);
+
+	/* exponent */
+	prop->public_exponent = calloc(1, sizeof(uint64_t));
+	if (!prop->public_exponent)
+		goto err;
+	memcpy((void *)prop->public_exponent + sizeof(uint64_t) - rsa_key.e_sz,
+	       rsa_key.e, rsa_key.e_sz);
+	prop->exp_len = rsa_key.e_sz;
+
+	/* n0 inverse */
+	br_i32_decode(n, &rsa_key.n[i], rsa_key.n_sz - i);
+	prop->n0inv = br_i32_ninv32(n[1]);
+
+	/* R^2 mod n; R = 2^(num_bits) */
+	rlen = prop->num_bits * 2; /* #bits of R^2 = (2^num_bits)^2 */
+	rr[0] = 0;
+	*(uint8_t *)&rr[0] = (1 << (rlen % 8));
+	for (i = 1; i < (((rlen + 31) >> 5) + 1); i++)
+		rr[i] = 0;
+	br_i32_decode(rrtmp, rr, ((rlen + 7) >> 3) + 1);
+	br_i32_reduce(rr, rrtmp, n);
+
+	rlen = (prop->num_bits + 7) >> 3; /* #bytes of R^2 mod n */
+	prop->rr = malloc(rlen);
+	if (!prop->rr)
+		goto err;
+	br_i32_encode((void *)prop->rr, rlen, rr);
+
+	return prop;
+
+err:
+	free(n);
+	free(rr);
+	free(rrtmp);
+	rsa_free_key_prop(prop);
+	return NULL;
+}
-- 
2.21.0

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[AKASHI Takahiro]

This function, and hence rsa_verify(), will perform RSA verification
with two essential parameters for a RSA public key in contract of
rsa_verify_with_keynode(), which requires additional three parameters
stored in FIT image.

It will be used in implementing UEFI secure boot, i.e. image authentication
and variable authentication.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/rsa/Kconfig      |  7 -----
 lib/rsa/Makefile     |  1 -
 lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
index d1743d7a4c47..62b7ab9c5e5c 100644
--- a/lib/rsa/Kconfig
+++ b/lib/rsa/Kconfig
@@ -30,13 +30,6 @@ config RSA_VERIFY
 	help
 	  Add RSA signature verification support.
 
-config RSA_VERIFY_WITH_PKEY
-	bool "Execute RSA verification without key parameters from FDT"
-	depends on RSA
-	help
-	  This options enables RSA signature verification without
-	  using public key parameters which is embedded control FDT.
-
 config RSA_SOFTWARE_EXP
 	bool "Enable driver for RSA Modular Exponentiation in software"
 	depends on DM
diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
index 14ed3cb4012b..c07305188e0c 100644
--- a/lib/rsa/Makefile
+++ b/lib/rsa/Makefile
@@ -6,5 +6,4 @@
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
 obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
-obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
 obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index 1df42f28c64a..ce79984b30f9 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -17,9 +17,14 @@
 #include "mkimage.h"
 #include <fdt_support.h>
 #endif
+#include <linux/kconfig.h>
 #include <u-boot/rsa-mod-exp.h>
 #include <u-boot/rsa.h>
 
+#ifndef __UBOOT__ /* for host tools */
+#undef CONFIG_RSA_VERIFY_WITH_PKEY
+#endif
+
 /* Default public exponent for backward compatibility */
 #define RSA_DEFAULT_PUBEXP	65537
 
@@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
 }
 #endif
 
+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
+/**
+ * rsa_verify_with_pkey()
+ *
+ */
+static int rsa_verify_with_pkey(struct image_sign_info *info,
+				const void *hash, uint8_t *sig, uint sig_len)
+{
+	struct key_prop *prop;
+	int ret;
+
+	/* Public key is self-described to fill key_prop */
+	prop = rsa_gen_key_prop(info->key, info->keylen);
+	if (!prop) {
+		debug("Generating necessary parameter for decoding failed\n");
+		return -EACCES;
+	}
+
+	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
+			     info->crypto->key_len);
+
+	rsa_free_key_prop(prop);
+
+	return ret;
+}
+#endif
+
+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
 /**
  * rsa_verify_with_keynode() - Verify a signature against some data using
  * information in node with prperties of RSA Key like modulus, exponent etc.
@@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
 
 	return ret;
 }
+#endif
 
 int rsa_verify(struct image_sign_info *info,
 	       const struct image_region region[], int region_count,
 	       uint8_t *sig, uint sig_len)
 {
-	const void *blob = info->fdt_blob;
 	/* Reserve memory for maximum checksum-length */
 	uint8_t hash[info->crypto->key_len];
+	int ret = -EACCES;
+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
+	const void *blob = info->fdt_blob;
 	int ndepth, noffset;
 	int sig_node, node;
 	char name[100];
-	int ret;
+#endif
 
 	/*
 	 * Verify that the checksum-length does not exceed the
@@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
 		return -EINVAL;
 	}
 
-	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
-	if (sig_node < 0) {
-		debug("%s: No signature node found\n", __func__);
-		return -ENOENT;
-	}
-
 	/* Calculate checksum with checksum-algorithm */
 	ret = info->checksum->calculate(info->checksum->name,
 					region, region_count, hash);
@@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
 		return -EINVAL;
 	}
 
+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
+	if (!info->fdt_blob) {
+		/* don't rely on fdt properties */
+		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
+
+		return ret;
+	}
+#endif
+
+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
+	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
+	if (sig_node < 0) {
+		debug("%s: No signature node found\n", __func__);
+		return -ENOENT;
+	}
+
 	/* See if we must use a particular key */
 	if (info->required_keynode != -1) {
 		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
@@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
 				break;
 		}
 	}
+#endif
 
 	return ret;
 }
-- 
2.21.0

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[Heinrich Schuchardt]

On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
> This function, and hence rsa_verify(), will perform RSA verification
> with two essential parameters for a RSA public key in contract of
> rsa_verify_with_keynode(), which requires additional three parameters
> stored in FIT image.
>
> It will be used in implementing UEFI secure boot, i.e. image authentication
> and variable authentication.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Is this code tested by test/py/tests/test_vboot.py? Or is there another
unit test?

> ---
>   lib/rsa/Kconfig      |  7 -----
>   lib/rsa/Makefile     |  1 -
>   lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
>   3 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> index d1743d7a4c47..62b7ab9c5e5c 100644
> --- a/lib/rsa/Kconfig
> +++ b/lib/rsa/Kconfig
> @@ -30,13 +30,6 @@ config RSA_VERIFY
>   	help
>   	  Add RSA signature verification support.
>
> -config RSA_VERIFY_WITH_PKEY
> -	bool "Execute RSA verification without key parameters from FDT"
> -	depends on RSA
> -	help
> -	  This options enables RSA signature verification without
> -	  using public key parameters which is embedded control FDT.
> -
>   config RSA_SOFTWARE_EXP
>   	bool "Enable driver for RSA Modular Exponentiation in software"
>   	depends on DM
> diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> index 14ed3cb4012b..c07305188e0c 100644
> --- a/lib/rsa/Makefile
> +++ b/lib/rsa/Makefile
> @@ -6,5 +6,4 @@
>   # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>
>   obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> -obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
>   obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> index 1df42f28c64a..ce79984b30f9 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -17,9 +17,14 @@
>   #include "mkimage.h"
>   #include <fdt_support.h>
>   #endif
> +#include <linux/kconfig.h>
>   #include <u-boot/rsa-mod-exp.h>
>   #include <u-boot/rsa.h>
>
> +#ifndef __UBOOT__ /* for host tools */
> +#undef CONFIG_RSA_VERIFY_WITH_PKEY

Where should it have been defined?

> +#endif
> +
>   /* Default public exponent for backward compatibility */
>   #define RSA_DEFAULT_PUBEXP	65537
>
> @@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
>   }
>   #endif
>
> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY

Where would CONFIG_RSA_VERIFY_WITH_PKEY be defined? You just a removed
it from Kconfig.

> +/**
> + * rsa_verify_with_pkey()

The short text for the function is missing.

> + *

Please, describe the parameters.

> + */
> +static int rsa_verify_with_pkey(struct image_sign_info *info,
> +				const void *hash, uint8_t *sig, uint sig_len)
> +{
> +	struct key_prop *prop;
> +	int ret;
> +
> +	/* Public key is self-described to fill key_prop */
> +	prop = rsa_gen_key_prop(info->key, info->keylen);
> +	if (!prop) {
> +		debug("Generating necessary parameter for decoding failed\n");
> +		return -EACCES;
> +	}
> +
> +	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> +			     info->crypto->key_len);
> +
> +	rsa_free_key_prop(prop);
> +
> +	return ret;
> +}
> +#endif
> +
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>   /**
>    * rsa_verify_with_keynode() - Verify a signature against some data using
>    * information in node with prperties of RSA Key like modulus, exponent etc.
> @@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
>
>   	return ret;
>   }
> +#endif
>
>   int rsa_verify(struct image_sign_info *info,
>   	       const struct image_region region[], int region_count,
>   	       uint8_t *sig, uint sig_len)
>   {
> -	const void *blob = info->fdt_blob;
>   	/* Reserve memory for maximum checksum-length */
>   	uint8_t hash[info->crypto->key_len];
> +	int ret = -EACCES;
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +	const void *blob = info->fdt_blob;
>   	int ndepth, noffset;
>   	int sig_node, node;
>   	char name[100];
> -	int ret;
> +#endif
>
>   	/*
>   	 * Verify that the checksum-length does not exceed the
> @@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
>   		return -EINVAL;
>   	}
>
> -	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> -	if (sig_node < 0) {
> -		debug("%s: No signature node found\n", __func__);
> -		return -ENOENT;
> -	}
> -
>   	/* Calculate checksum with checksum-algorithm */
>   	ret = info->checksum->calculate(info->checksum->name,
>   					region, region_count, hash);
> @@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
>   		return -EINVAL;
>   	}
>
> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY

Where should this have been defined?

Best regards

Heinrich

> +	if (!info->fdt_blob) {
> +		/* don't rely on fdt properties */
> +		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> +
> +		return ret;
> +	}
> +#endif
> +
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> +	if (sig_node < 0) {
> +		debug("%s: No signature node found\n", __func__);
> +		return -ENOENT;
> +	}
> +
>   	/* See if we must use a particular key */
>   	if (info->required_keynode != -1) {
>   		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> @@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
>   				break;
>   		}
>   	}
> +#endif
>
>   	return ret;
>   }
>

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[AKASHI Takahiro]

On Wed, Oct 09, 2019 at 07:56:04PM +0200, Heinrich Schuchardt wrote:
> On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
> >This function, and hence rsa_verify(), will perform RSA verification
> >with two essential parameters for a RSA public key in contract of
> >rsa_verify_with_keynode(), which requires additional three parameters
> >stored in FIT image.
> >
> >It will be used in implementing UEFI secure boot, i.e. image authentication
> >and variable authentication.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Is this code tested by test/py/tests/test_vboot.py? Or is there another
> unit test?

I haven't run vboot test yet.
For rsa_verify_with_pkey(), as with vtest for FIT signature,
we can test it with my "UEFI secure boot" pytest.

> >---
> >  lib/rsa/Kconfig      |  7 -----
> >  lib/rsa/Makefile     |  1 -
> >  lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 55 insertions(+), 16 deletions(-)
> >
> >diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> >index d1743d7a4c47..62b7ab9c5e5c 100644
> >--- a/lib/rsa/Kconfig
> >+++ b/lib/rsa/Kconfig
> >@@ -30,13 +30,6 @@ config RSA_VERIFY
> >  	help
> >  	  Add RSA signature verification support.
> >
> >-config RSA_VERIFY_WITH_PKEY
> >-	bool "Execute RSA verification without key parameters from FDT"
> >-	depends on RSA
> >-	help
> >-	  This options enables RSA signature verification without
> >-	  using public key parameters which is embedded control FDT.
> >-
> >  config RSA_SOFTWARE_EXP
> >  	bool "Enable driver for RSA Modular Exponentiation in software"
> >  	depends on DM
> >diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> >index 14ed3cb4012b..c07305188e0c 100644
> >--- a/lib/rsa/Makefile
> >+++ b/lib/rsa/Makefile
> >@@ -6,5 +6,4 @@
> >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >
> >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> >-obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> >diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> >index 1df42f28c64a..ce79984b30f9 100644
> >--- a/lib/rsa/rsa-verify.c
> >+++ b/lib/rsa/rsa-verify.c
> >@@ -17,9 +17,14 @@
> >  #include "mkimage.h"
> >  #include <fdt_support.h>
> >  #endif
> >+#include <linux/kconfig.h>
> >  #include <u-boot/rsa-mod-exp.h>
> >  #include <u-boot/rsa.h>
> >
> >+#ifndef __UBOOT__ /* for host tools */
> >+#undef CONFIG_RSA_VERIFY_WITH_PKEY
> 
> Where should it have been defined?

defined in patch#2

> >+#endif
> >+
> >  /* Default public exponent for backward compatibility */
> >  #define RSA_DEFAULT_PUBEXP	65537
> >
> >@@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
> >  }
> >  #endif
> >
> >+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> 
> Where would CONFIG_RSA_VERIFY_WITH_PKEY be defined? You just a removed
> it from Kconfig.

Because rsa-keyprop.c be compiled only under CONFIG_RSA_VERIFY_WITH_PKEY
per Simon's comment.

> >+/**
> >+ * rsa_verify_with_pkey()
> 
> The short text for the function is missing.
> 
> >+ *
> 
> Please, describe the parameters.

Sure.

-Takahiro Akashi

> >+ */
> >+static int rsa_verify_with_pkey(struct image_sign_info *info,
> >+				const void *hash, uint8_t *sig, uint sig_len)
> >+{
> >+	struct key_prop *prop;
> >+	int ret;
> >+
> >+	/* Public key is self-described to fill key_prop */
> >+	prop = rsa_gen_key_prop(info->key, info->keylen);
> >+	if (!prop) {
> >+		debug("Generating necessary parameter for decoding failed\n");
> >+		return -EACCES;
> >+	}
> >+
> >+	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> >+			     info->crypto->key_len);
> >+
> >+	rsa_free_key_prop(prop);
> >+
> >+	return ret;
> >+}
> >+#endif
> >+
> >+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >  /**
> >   * rsa_verify_with_keynode() - Verify a signature against some data using
> >   * information in node with prperties of RSA Key like modulus, exponent etc.
> >@@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
> >
> >  	return ret;
> >  }
> >+#endif
> >
> >  int rsa_verify(struct image_sign_info *info,
> >  	       const struct image_region region[], int region_count,
> >  	       uint8_t *sig, uint sig_len)
> >  {
> >-	const void *blob = info->fdt_blob;
> >  	/* Reserve memory for maximum checksum-length */
> >  	uint8_t hash[info->crypto->key_len];
> >+	int ret = -EACCES;
> >+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >+	const void *blob = info->fdt_blob;
> >  	int ndepth, noffset;
> >  	int sig_node, node;
> >  	char name[100];
> >-	int ret;
> >+#endif
> >
> >  	/*
> >  	 * Verify that the checksum-length does not exceed the
> >@@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
> >  		return -EINVAL;
> >  	}
> >
> >-	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> >-	if (sig_node < 0) {
> >-		debug("%s: No signature node found\n", __func__);
> >-		return -ENOENT;
> >-	}
> >-
> >  	/* Calculate checksum with checksum-algorithm */
> >  	ret = info->checksum->calculate(info->checksum->name,
> >  					region, region_count, hash);
> >@@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
> >  		return -EINVAL;
> >  	}
> >
> >+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> 
> Where should this have been defined?
> 
> Best regards
> 
> Heinrich
> 
> >+	if (!info->fdt_blob) {
> >+		/* don't rely on fdt properties */
> >+		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> >+
> >+		return ret;
> >+	}
> >+#endif
> >+
> >+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >+	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> >+	if (sig_node < 0) {
> >+		debug("%s: No signature node found\n", __func__);
> >+		return -ENOENT;
> >+	}
> >+
> >  	/* See if we must use a particular key */
> >  	if (info->required_keynode != -1) {
> >  		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> >@@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
> >  				break;
> >  		}
> >  	}
> >+#endif
> >
> >  	return ret;
> >  }
> >
> 

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[AKASHI Takahiro]

On Wed, Oct 09, 2019 at 02:30:44PM +0900, AKASHI Takahiro wrote:
> This function, and hence rsa_verify(), will perform RSA verification
> with two essential parameters for a RSA public key in contract of
> rsa_verify_with_keynode(), which requires additional three parameters
> stored in FIT image.
> 
> It will be used in implementing UEFI secure boot, i.e. image authentication
> and variable authentication.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/rsa/Kconfig      |  7 -----
>  lib/rsa/Makefile     |  1 -
>  lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> index d1743d7a4c47..62b7ab9c5e5c 100644
> --- a/lib/rsa/Kconfig
> +++ b/lib/rsa/Kconfig
> @@ -30,13 +30,6 @@ config RSA_VERIFY
>  	help
>  	  Add RSA signature verification support.
>  
> -config RSA_VERIFY_WITH_PKEY
> -	bool "Execute RSA verification without key parameters from FDT"
> -	depends on RSA
> -	help
> -	  This options enables RSA signature verification without
> -	  using public key parameters which is embedded control FDT.
> -
>  config RSA_SOFTWARE_EXP
>  	bool "Enable driver for RSA Modular Exponentiation in software"
>  	depends on DM
> diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> index 14ed3cb4012b..c07305188e0c 100644
> --- a/lib/rsa/Makefile
> +++ b/lib/rsa/Makefile
> @@ -6,5 +6,4 @@
>  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>  
>  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> -obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
>  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o

Oops, those changes above against Kconfig and Makefile are wrong.
They should not be included in this commit. By removing them,
everything will work well.
# Those hunks are remnants from last-minute cleanup.

Thanks,
-Takahiro Akashi

> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> index 1df42f28c64a..ce79984b30f9 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -17,9 +17,14 @@
>  #include "mkimage.h"
>  #include <fdt_support.h>
>  #endif
> +#include <linux/kconfig.h>
>  #include <u-boot/rsa-mod-exp.h>
>  #include <u-boot/rsa.h>
>  
> +#ifndef __UBOOT__ /* for host tools */
> +#undef CONFIG_RSA_VERIFY_WITH_PKEY
> +#endif
> +
>  /* Default public exponent for backward compatibility */
>  #define RSA_DEFAULT_PUBEXP	65537
>  
> @@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
>  }
>  #endif
>  
> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> +/**
> + * rsa_verify_with_pkey()
> + *
> + */
> +static int rsa_verify_with_pkey(struct image_sign_info *info,
> +				const void *hash, uint8_t *sig, uint sig_len)
> +{
> +	struct key_prop *prop;
> +	int ret;
> +
> +	/* Public key is self-described to fill key_prop */
> +	prop = rsa_gen_key_prop(info->key, info->keylen);
> +	if (!prop) {
> +		debug("Generating necessary parameter for decoding failed\n");
> +		return -EACCES;
> +	}
> +
> +	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> +			     info->crypto->key_len);
> +
> +	rsa_free_key_prop(prop);
> +
> +	return ret;
> +}
> +#endif
> +
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>  /**
>   * rsa_verify_with_keynode() - Verify a signature against some data using
>   * information in node with prperties of RSA Key like modulus, exponent etc.
> @@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
>  
>  	return ret;
>  }
> +#endif
>  
>  int rsa_verify(struct image_sign_info *info,
>  	       const struct image_region region[], int region_count,
>  	       uint8_t *sig, uint sig_len)
>  {
> -	const void *blob = info->fdt_blob;
>  	/* Reserve memory for maximum checksum-length */
>  	uint8_t hash[info->crypto->key_len];
> +	int ret = -EACCES;
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +	const void *blob = info->fdt_blob;
>  	int ndepth, noffset;
>  	int sig_node, node;
>  	char name[100];
> -	int ret;
> +#endif
>  
>  	/*
>  	 * Verify that the checksum-length does not exceed the
> @@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
>  		return -EINVAL;
>  	}
>  
> -	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> -	if (sig_node < 0) {
> -		debug("%s: No signature node found\n", __func__);
> -		return -ENOENT;
> -	}
> -
>  	/* Calculate checksum with checksum-algorithm */
>  	ret = info->checksum->calculate(info->checksum->name,
>  					region, region_count, hash);
> @@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
>  		return -EINVAL;
>  	}
>  
> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> +	if (!info->fdt_blob) {
> +		/* don't rely on fdt properties */
> +		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> +
> +		return ret;
> +	}
> +#endif
> +
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> +	if (sig_node < 0) {
> +		debug("%s: No signature node found\n", __func__);
> +		return -ENOENT;
> +	}
> +
>  	/* See if we must use a particular key */
>  	if (info->required_keynode != -1) {
>  		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> @@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
>  				break;
>  		}
>  	}
> +#endif
>  
>  	return ret;
>  }
> -- 
> 2.21.0
> 

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[Heinrich Schuchardt]

On 10/10/19 3:04 AM, AKASHI Takahiro wrote:
> On Wed, Oct 09, 2019 at 07:56:04PM +0200, Heinrich Schuchardt wrote:
>> On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
>>> This function, and hence rsa_verify(), will perform RSA verification
>>> with two essential parameters for a RSA public key in contract of
>>> rsa_verify_with_keynode(), which requires additional three parameters
>>> stored in FIT image.
>>>
>>> It will be used in implementing UEFI secure boot, i.e. image authentication
>>> and variable authentication.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>
>> Is this code tested by test/py/tests/test_vboot.py? Or is there another
>> unit test?
>
> I haven't run vboot test yet.

I would have assumed that you to through Travis CI before submitting.

> For rsa_verify_with_pkey(), as with vtest for FIT signature,
> we can test it with my "UEFI secure boot" pytest.

I can't see such a test in this patch series. So how I can test the
changes before merging?

Best regards

Heinrich

>
>>> ---
>>>   lib/rsa/Kconfig      |  7 -----
>>>   lib/rsa/Makefile     |  1 -
>>>   lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
>>>   3 files changed, 55 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
>>> index d1743d7a4c47..62b7ab9c5e5c 100644
>>> --- a/lib/rsa/Kconfig
>>> +++ b/lib/rsa/Kconfig
>>> @@ -30,13 +30,6 @@ config RSA_VERIFY
>>>   	help
>>>   	  Add RSA signature verification support.
>>>
>>> -config RSA_VERIFY_WITH_PKEY
>>> -	bool "Execute RSA verification without key parameters from FDT"
>>> -	depends on RSA
>>> -	help
>>> -	  This options enables RSA signature verification without
>>> -	  using public key parameters which is embedded control FDT.
>>> -
>>>   config RSA_SOFTWARE_EXP
>>>   	bool "Enable driver for RSA Modular Exponentiation in software"
>>>   	depends on DM
>>> diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
>>> index 14ed3cb4012b..c07305188e0c 100644
>>> --- a/lib/rsa/Makefile
>>> +++ b/lib/rsa/Makefile
>>> @@ -6,5 +6,4 @@
>>>   # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>>>
>>>   obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
>>> -obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
>>>   obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
>>> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
>>> index 1df42f28c64a..ce79984b30f9 100644
>>> --- a/lib/rsa/rsa-verify.c
>>> +++ b/lib/rsa/rsa-verify.c
>>> @@ -17,9 +17,14 @@
>>>   #include "mkimage.h"
>>>   #include <fdt_support.h>
>>>   #endif
>>> +#include <linux/kconfig.h>
>>>   #include <u-boot/rsa-mod-exp.h>
>>>   #include <u-boot/rsa.h>
>>>
>>> +#ifndef __UBOOT__ /* for host tools */
>>> +#undef CONFIG_RSA_VERIFY_WITH_PKEY
>>
>> Where should it have been defined?
>
> defined in patch#2
>
>>> +#endif
>>> +
>>>   /* Default public exponent for backward compatibility */
>>>   #define RSA_DEFAULT_PUBEXP	65537
>>>
>>> @@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
>>>   }
>>>   #endif
>>>
>>> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
>>
>> Where would CONFIG_RSA_VERIFY_WITH_PKEY be defined? You just a removed
>> it from Kconfig.
>
> Because rsa-keyprop.c be compiled only under CONFIG_RSA_VERIFY_WITH_PKEY
> per Simon's comment.
>
>>> +/**
>>> + * rsa_verify_with_pkey()
>>
>> The short text for the function is missing.
>>
>>> + *
>>
>> Please, describe the parameters.
>
> Sure.
>
> -Takahiro Akashi
>
>>> + */
>>> +static int rsa_verify_with_pkey(struct image_sign_info *info,
>>> +				const void *hash, uint8_t *sig, uint sig_len)
>>> +{
>>> +	struct key_prop *prop;
>>> +	int ret;
>>> +
>>> +	/* Public key is self-described to fill key_prop */
>>> +	prop = rsa_gen_key_prop(info->key, info->keylen);
>>> +	if (!prop) {
>>> +		debug("Generating necessary parameter for decoding failed\n");
>>> +		return -EACCES;
>>> +	}
>>> +
>>> +	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
>>> +			     info->crypto->key_len);
>>> +
>>> +	rsa_free_key_prop(prop);
>>> +
>>> +	return ret;
>>> +}
>>> +#endif
>>> +
>>> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>>>   /**
>>>    * rsa_verify_with_keynode() - Verify a signature against some data using
>>>    * information in node with prperties of RSA Key like modulus, exponent etc.
>>> @@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
>>>
>>>   	return ret;
>>>   }
>>> +#endif
>>>
>>>   int rsa_verify(struct image_sign_info *info,
>>>   	       const struct image_region region[], int region_count,
>>>   	       uint8_t *sig, uint sig_len)
>>>   {
>>> -	const void *blob = info->fdt_blob;
>>>   	/* Reserve memory for maximum checksum-length */
>>>   	uint8_t hash[info->crypto->key_len];
>>> +	int ret = -EACCES;
>>> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>>> +	const void *blob = info->fdt_blob;
>>>   	int ndepth, noffset;
>>>   	int sig_node, node;
>>>   	char name[100];
>>> -	int ret;
>>> +#endif
>>>
>>>   	/*
>>>   	 * Verify that the checksum-length does not exceed the
>>> @@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
>>>   		return -EINVAL;
>>>   	}
>>>
>>> -	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
>>> -	if (sig_node < 0) {
>>> -		debug("%s: No signature node found\n", __func__);
>>> -		return -ENOENT;
>>> -	}
>>> -
>>>   	/* Calculate checksum with checksum-algorithm */
>>>   	ret = info->checksum->calculate(info->checksum->name,
>>>   					region, region_count, hash);
>>> @@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
>>>   		return -EINVAL;
>>>   	}
>>>
>>> +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
>>
>> Where should this have been defined?
>>
>> Best regards
>>
>> Heinrich
>>
>>> +	if (!info->fdt_blob) {
>>> +		/* don't rely on fdt properties */
>>> +		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
>>> +
>>> +		return ret;
>>> +	}
>>> +#endif
>>> +
>>> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>>> +	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
>>> +	if (sig_node < 0) {
>>> +		debug("%s: No signature node found\n", __func__);
>>> +		return -ENOENT;
>>> +	}
>>> +
>>>   	/* See if we must use a particular key */
>>>   	if (info->required_keynode != -1) {
>>>   		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
>>> @@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
>>>   				break;
>>>   		}
>>>   	}
>>> +#endif
>>>
>>>   	return ret;
>>>   }
>>>
>>
>

[U-Boot] [PATCH v1 0/3] rsa: extend rsa_verify() for UEFI secure boot

[Heinrich Schuchardt]

On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
> The current rsa_verify() requires five parameters for a RSA public key
> for efficiency while RSA, in theory, requires only two. In addition,
> those parameters are expected to come from FIT image.
>
> So this function won't fit very well when we want to use it for the purpose
> of implementing UEFI secure boot, in particular, image authentication
> as well as variable authentication, where the essential two parameters
> are set to be retrieved from one of X509 certificates in signature
> database.
>
> So, in this patch, additional three parameters will be calculated
> on the fly when rsa_verify() is called without fdt which should contain
> parameters above.
>
> This calculation heavily relies on "big-number (or multi-precision)
> library." Therefore some routines from BearSSL[1] under MIT license are
> imported in this implementation. See Patch#2.
> # Please let me know if this is not appropriate.
>
> # Checkpatch will complain with lots of warnings/errors, but
> # I intentionally don't fix them for maximum maintainability.

This patch series does not even compile:
https://travis-ci.org/xypron2/u-boot/builds/596983699

+common/image-sig.c:20:22: error: array type has incomplete element type
'struct checksum_algo'
+ struct checksum_algo checksum_algos[] = {
+                      ^~~~~~~~~~~~~~
+common/image-sig.c:22:3: error: field name not in record or union
initializer
+   .name = "sha1",
+   ^

Before resubmitting, please, run the whole series through Travis CI or
Gitlab CI.

Best regards

Heinrich

>
>   [1] https://bearssl.org/
>
> Changes in v1 (Oct 9, 2019)
> * fix a build error on pine64-lts_defconfig (reported by Heinrich)
>   by defining FIT_IMAGE_ENABLE_VERIFY flag and adding
>   SPL_RSA_VERIFY config (patch#1)
> * remove FIT-specific code from image-sig.c and put them to new
>   image-fit-sig.c to allow us to disable CONFIG_FIT_SIGNATURE (patch#1)
> * compile rsa-keyprop.c only if necessary (i.e. if
>   CONFIG_RSA_VERIFY_WITH_PKEY) (patch#2)
> * add SPDX license identifier in rsa-keyprop.c (patch#2)
> * include <common.h> instead of <stdio.h> (patch#2)
> * use U-Boot's byteorder helper functions instead of BearSSL's (patch#2)
>
> AKASHI Takahiro (3):
>   lib: rsa: decouple rsa from FIT image verification
>   lib: rsa: generate additional parameters for public key
>   lib: rsa: add rsa_verify_with_pkey()
>
>  Kconfig                      |   1 +
>  common/Makefile              |   3 +-
>  common/image-fit-sig.c       | 417 +++++++++++++++++++++++++
>  common/image-fit.c           |   6 +-
>  common/image-sig.c           | 396 ------------------------
>  include/image.h              |  14 +-
>  include/u-boot/rsa-mod-exp.h |   3 +
>  lib/rsa/Kconfig              |  12 +
>  lib/rsa/Makefile             |   2 +-
>  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
>  lib/rsa/rsa-verify.c         |  65 +++-
>  tools/Makefile               |   2 +-
>  12 files changed, 1095 insertions(+), 411 deletions(-)
>  create mode 100644 common/image-fit-sig.c
>  create mode 100644 lib/rsa/rsa-keyprop.c
>

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[AKASHI Takahiro]

On Sat, Oct 12, 2019 at 02:47:33PM +0200, Heinrich Schuchardt wrote:
> On 10/10/19 3:04 AM, AKASHI Takahiro wrote:
> >On Wed, Oct 09, 2019 at 07:56:04PM +0200, Heinrich Schuchardt wrote:
> >>On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
> >>>This function, and hence rsa_verify(), will perform RSA verification
> >>>with two essential parameters for a RSA public key in contract of
> >>>rsa_verify_with_keynode(), which requires additional three parameters
> >>>stored in FIT image.
> >>>
> >>>It will be used in implementing UEFI secure boot, i.e. image authentication
> >>>and variable authentication.
> >>>
> >>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>
> >>Is this code tested by test/py/tests/test_vboot.py? Or is there another
> >>unit test?
> >
> >I haven't run vboot test yet.
> 
> I would have assumed that you to through Travis CI before submitting.
> 
> >For rsa_verify_with_pkey(), as with vtest for FIT signature,
> >we can test it with my "UEFI secure boot" pytest.
> 
> I can't see such a test in this patch series. So how I can test the
> changes before merging?

The only solution that I can give you is that I would submit
rsa patch with my "UEFI secure boot" patch.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >>>---
> >>>  lib/rsa/Kconfig      |  7 -----
> >>>  lib/rsa/Makefile     |  1 -
> >>>  lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
> >>>  3 files changed, 55 insertions(+), 16 deletions(-)
> >>>
> >>>diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> >>>index d1743d7a4c47..62b7ab9c5e5c 100644
> >>>--- a/lib/rsa/Kconfig
> >>>+++ b/lib/rsa/Kconfig
> >>>@@ -30,13 +30,6 @@ config RSA_VERIFY
> >>>  	help
> >>>  	  Add RSA signature verification support.
> >>>
> >>>-config RSA_VERIFY_WITH_PKEY
> >>>-	bool "Execute RSA verification without key parameters from FDT"
> >>>-	depends on RSA
> >>>-	help
> >>>-	  This options enables RSA signature verification without
> >>>-	  using public key parameters which is embedded control FDT.
> >>>-
> >>>  config RSA_SOFTWARE_EXP
> >>>  	bool "Enable driver for RSA Modular Exponentiation in software"
> >>>  	depends on DM
> >>>diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> >>>index 14ed3cb4012b..c07305188e0c 100644
> >>>--- a/lib/rsa/Makefile
> >>>+++ b/lib/rsa/Makefile
> >>>@@ -6,5 +6,4 @@
> >>>  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >>>
> >>>  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> >>>-obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> >>>  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> >>>diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> >>>index 1df42f28c64a..ce79984b30f9 100644
> >>>--- a/lib/rsa/rsa-verify.c
> >>>+++ b/lib/rsa/rsa-verify.c
> >>>@@ -17,9 +17,14 @@
> >>>  #include "mkimage.h"
> >>>  #include <fdt_support.h>
> >>>  #endif
> >>>+#include <linux/kconfig.h>
> >>>  #include <u-boot/rsa-mod-exp.h>
> >>>  #include <u-boot/rsa.h>
> >>>
> >>>+#ifndef __UBOOT__ /* for host tools */
> >>>+#undef CONFIG_RSA_VERIFY_WITH_PKEY
> >>
> >>Where should it have been defined?
> >
> >defined in patch#2
> >
> >>>+#endif
> >>>+
> >>>  /* Default public exponent for backward compatibility */
> >>>  #define RSA_DEFAULT_PUBEXP	65537
> >>>
> >>>@@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
> >>>  }
> >>>  #endif
> >>>
> >>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> >>
> >>Where would CONFIG_RSA_VERIFY_WITH_PKEY be defined? You just a removed
> >>it from Kconfig.
> >
> >Because rsa-keyprop.c be compiled only under CONFIG_RSA_VERIFY_WITH_PKEY
> >per Simon's comment.
> >
> >>>+/**
> >>>+ * rsa_verify_with_pkey()
> >>
> >>The short text for the function is missing.
> >>
> >>>+ *
> >>
> >>Please, describe the parameters.
> >
> >Sure.
> >
> >-Takahiro Akashi
> >
> >>>+ */
> >>>+static int rsa_verify_with_pkey(struct image_sign_info *info,
> >>>+				const void *hash, uint8_t *sig, uint sig_len)
> >>>+{
> >>>+	struct key_prop *prop;
> >>>+	int ret;
> >>>+
> >>>+	/* Public key is self-described to fill key_prop */
> >>>+	prop = rsa_gen_key_prop(info->key, info->keylen);
> >>>+	if (!prop) {
> >>>+		debug("Generating necessary parameter for decoding failed\n");
> >>>+		return -EACCES;
> >>>+	}
> >>>+
> >>>+	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> >>>+			     info->crypto->key_len);
> >>>+
> >>>+	rsa_free_key_prop(prop);
> >>>+
> >>>+	return ret;
> >>>+}
> >>>+#endif
> >>>+
> >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >>>  /**
> >>>   * rsa_verify_with_keynode() - Verify a signature against some data using
> >>>   * information in node with prperties of RSA Key like modulus, exponent etc.
> >>>@@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
> >>>
> >>>  	return ret;
> >>>  }
> >>>+#endif
> >>>
> >>>  int rsa_verify(struct image_sign_info *info,
> >>>  	       const struct image_region region[], int region_count,
> >>>  	       uint8_t *sig, uint sig_len)
> >>>  {
> >>>-	const void *blob = info->fdt_blob;
> >>>  	/* Reserve memory for maximum checksum-length */
> >>>  	uint8_t hash[info->crypto->key_len];
> >>>+	int ret = -EACCES;
> >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >>>+	const void *blob = info->fdt_blob;
> >>>  	int ndepth, noffset;
> >>>  	int sig_node, node;
> >>>  	char name[100];
> >>>-	int ret;
> >>>+#endif
> >>>
> >>>  	/*
> >>>  	 * Verify that the checksum-length does not exceed the
> >>>@@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
> >>>  		return -EINVAL;
> >>>  	}
> >>>
> >>>-	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> >>>-	if (sig_node < 0) {
> >>>-		debug("%s: No signature node found\n", __func__);
> >>>-		return -ENOENT;
> >>>-	}
> >>>-
> >>>  	/* Calculate checksum with checksum-algorithm */
> >>>  	ret = info->checksum->calculate(info->checksum->name,
> >>>  					region, region_count, hash);
> >>>@@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
> >>>  		return -EINVAL;
> >>>  	}
> >>>
> >>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> >>
> >>Where should this have been defined?
> >>
> >>Best regards
> >>
> >>Heinrich
> >>
> >>>+	if (!info->fdt_blob) {
> >>>+		/* don't rely on fdt properties */
> >>>+		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> >>>+
> >>>+		return ret;
> >>>+	}
> >>>+#endif
> >>>+
> >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> >>>+	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> >>>+	if (sig_node < 0) {
> >>>+		debug("%s: No signature node found\n", __func__);
> >>>+		return -ENOENT;
> >>>+	}
> >>>+
> >>>  	/* See if we must use a particular key */
> >>>  	if (info->required_keynode != -1) {
> >>>  		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> >>>@@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
> >>>  				break;
> >>>  		}
> >>>  	}
> >>>+#endif
> >>>
> >>>  	return ret;
> >>>  }
> >>>
> >>
> >
> 

[U-Boot] [PATCH v1 3/3] lib: rsa: add rsa_verify_with_pkey()

[AKASHI Takahiro]

On Tue, Oct 15, 2019 at 04:17:04PM +0900, AKASHI Takahiro wrote:
> On Sat, Oct 12, 2019 at 02:47:33PM +0200, Heinrich Schuchardt wrote:
> > On 10/10/19 3:04 AM, AKASHI Takahiro wrote:
> > >On Wed, Oct 09, 2019 at 07:56:04PM +0200, Heinrich Schuchardt wrote:
> > >>On 10/9/19 7:30 AM, AKASHI Takahiro wrote:
> > >>>This function, and hence rsa_verify(), will perform RSA verification
> > >>>with two essential parameters for a RSA public key in contract of
> > >>>rsa_verify_with_keynode(), which requires additional three parameters
> > >>>stored in FIT image.
> > >>>
> > >>>It will be used in implementing UEFI secure boot, i.e. image authentication
> > >>>and variable authentication.
> > >>>
> > >>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >>
> > >>Is this code tested by test/py/tests/test_vboot.py? Or is there another
> > >>unit test?
> > >
> > >I haven't run vboot test yet.
> > 
> > I would have assumed that you to through Travis CI before submitting.
> > 
> > >For rsa_verify_with_pkey(), as with vtest for FIT signature,
> > >we can test it with my "UEFI secure boot" pytest.
> > 
> > I can't see such a test in this patch series. So how I can test the
> > changes before merging?
> 
> The only solution that I can give you is that I would submit
> rsa patch with my "UEFI secure boot" patch.

I added a very simple function test in "ut" command but will defer
sending a new version until after v2 of "x509 parser" patch
as I found there is some dependency on public key parser in rsa_keyprop.c.

-Takahiro Akashi

> 
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >>>---
> > >>>  lib/rsa/Kconfig      |  7 -----
> > >>>  lib/rsa/Makefile     |  1 -
> > >>>  lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
> > >>>  3 files changed, 55 insertions(+), 16 deletions(-)
> > >>>
> > >>>diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > >>>index d1743d7a4c47..62b7ab9c5e5c 100644
> > >>>--- a/lib/rsa/Kconfig
> > >>>+++ b/lib/rsa/Kconfig
> > >>>@@ -30,13 +30,6 @@ config RSA_VERIFY
> > >>>  	help
> > >>>  	  Add RSA signature verification support.
> > >>>
> > >>>-config RSA_VERIFY_WITH_PKEY
> > >>>-	bool "Execute RSA verification without key parameters from FDT"
> > >>>-	depends on RSA
> > >>>-	help
> > >>>-	  This options enables RSA signature verification without
> > >>>-	  using public key parameters which is embedded control FDT.
> > >>>-
> > >>>  config RSA_SOFTWARE_EXP
> > >>>  	bool "Enable driver for RSA Modular Exponentiation in software"
> > >>>  	depends on DM
> > >>>diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > >>>index 14ed3cb4012b..c07305188e0c 100644
> > >>>--- a/lib/rsa/Makefile
> > >>>+++ b/lib/rsa/Makefile
> > >>>@@ -6,5 +6,4 @@
> > >>>  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > >>>
> > >>>  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > >>>-obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > >>>  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > >>>diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> > >>>index 1df42f28c64a..ce79984b30f9 100644
> > >>>--- a/lib/rsa/rsa-verify.c
> > >>>+++ b/lib/rsa/rsa-verify.c
> > >>>@@ -17,9 +17,14 @@
> > >>>  #include "mkimage.h"
> > >>>  #include <fdt_support.h>
> > >>>  #endif
> > >>>+#include <linux/kconfig.h>
> > >>>  #include <u-boot/rsa-mod-exp.h>
> > >>>  #include <u-boot/rsa.h>
> > >>>
> > >>>+#ifndef __UBOOT__ /* for host tools */
> > >>>+#undef CONFIG_RSA_VERIFY_WITH_PKEY
> > >>
> > >>Where should it have been defined?
> > >
> > >defined in patch#2
> > >
> > >>>+#endif
> > >>>+
> > >>>  /* Default public exponent for backward compatibility */
> > >>>  #define RSA_DEFAULT_PUBEXP	65537
> > >>>
> > >>>@@ -344,6 +349,34 @@ static int rsa_verify_key(struct image_sign_info *info,
> > >>>  }
> > >>>  #endif
> > >>>
> > >>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > >>
> > >>Where would CONFIG_RSA_VERIFY_WITH_PKEY be defined? You just a removed
> > >>it from Kconfig.
> > >
> > >Because rsa-keyprop.c be compiled only under CONFIG_RSA_VERIFY_WITH_PKEY
> > >per Simon's comment.
> > >
> > >>>+/**
> > >>>+ * rsa_verify_with_pkey()
> > >>
> > >>The short text for the function is missing.
> > >>
> > >>>+ *
> > >>
> > >>Please, describe the parameters.
> > >
> > >Sure.
> > >
> > >-Takahiro Akashi
> > >
> > >>>+ */
> > >>>+static int rsa_verify_with_pkey(struct image_sign_info *info,
> > >>>+				const void *hash, uint8_t *sig, uint sig_len)
> > >>>+{
> > >>>+	struct key_prop *prop;
> > >>>+	int ret;
> > >>>+
> > >>>+	/* Public key is self-described to fill key_prop */
> > >>>+	prop = rsa_gen_key_prop(info->key, info->keylen);
> > >>>+	if (!prop) {
> > >>>+		debug("Generating necessary parameter for decoding failed\n");
> > >>>+		return -EACCES;
> > >>>+	}
> > >>>+
> > >>>+	ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> > >>>+			     info->crypto->key_len);
> > >>>+
> > >>>+	rsa_free_key_prop(prop);
> > >>>+
> > >>>+	return ret;
> > >>>+}
> > >>>+#endif
> > >>>+
> > >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > >>>  /**
> > >>>   * rsa_verify_with_keynode() - Verify a signature against some data using
> > >>>   * information in node with prperties of RSA Key like modulus, exponent etc.
> > >>>@@ -397,18 +430,21 @@ static int rsa_verify_with_keynode(struct image_sign_info *info,
> > >>>
> > >>>  	return ret;
> > >>>  }
> > >>>+#endif
> > >>>
> > >>>  int rsa_verify(struct image_sign_info *info,
> > >>>  	       const struct image_region region[], int region_count,
> > >>>  	       uint8_t *sig, uint sig_len)
> > >>>  {
> > >>>-	const void *blob = info->fdt_blob;
> > >>>  	/* Reserve memory for maximum checksum-length */
> > >>>  	uint8_t hash[info->crypto->key_len];
> > >>>+	int ret = -EACCES;
> > >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > >>>+	const void *blob = info->fdt_blob;
> > >>>  	int ndepth, noffset;
> > >>>  	int sig_node, node;
> > >>>  	char name[100];
> > >>>-	int ret;
> > >>>+#endif
> > >>>
> > >>>  	/*
> > >>>  	 * Verify that the checksum-length does not exceed the
> > >>>@@ -421,12 +457,6 @@ int rsa_verify(struct image_sign_info *info,
> > >>>  		return -EINVAL;
> > >>>  	}
> > >>>
> > >>>-	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> > >>>-	if (sig_node < 0) {
> > >>>-		debug("%s: No signature node found\n", __func__);
> > >>>-		return -ENOENT;
> > >>>-	}
> > >>>-
> > >>>  	/* Calculate checksum with checksum-algorithm */
> > >>>  	ret = info->checksum->calculate(info->checksum->name,
> > >>>  					region, region_count, hash);
> > >>>@@ -435,6 +465,22 @@ int rsa_verify(struct image_sign_info *info,
> > >>>  		return -EINVAL;
> > >>>  	}
> > >>>
> > >>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > >>
> > >>Where should this have been defined?
> > >>
> > >>Best regards
> > >>
> > >>Heinrich
> > >>
> > >>>+	if (!info->fdt_blob) {
> > >>>+		/* don't rely on fdt properties */
> > >>>+		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> > >>>+
> > >>>+		return ret;
> > >>>+	}
> > >>>+#endif
> > >>>+
> > >>>+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > >>>+	sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> > >>>+	if (sig_node < 0) {
> > >>>+		debug("%s: No signature node found\n", __func__);
> > >>>+		return -ENOENT;
> > >>>+	}
> > >>>+
> > >>>  	/* See if we must use a particular key */
> > >>>  	if (info->required_keynode != -1) {
> > >>>  		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> > >>>@@ -461,6 +507,7 @@ int rsa_verify(struct image_sign_info *info,
> > >>>  				break;
> > >>>  		}
> > >>>  	}
> > >>>+#endif
> > >>>
> > >>>  	return ret;
> > >>>  }
> > >>>
> > >>
> > >
> > 

[U-Boot] [PATCH v1 1/3] lib: rsa: decouple rsa from FIT image verification

[Simon Glass]

Hi Takahiro,

On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Introduce new configuration, CONFIG_RSA_VERIFY which will decouple building
> RSA functions from FIT verification and allow for adding a RSA-based
> signature verification for other file formats, in particular PE file
> for UEFI secure boot.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  Kconfig                |   1 +
>  common/Makefile        |   3 +-
>  common/image-fit-sig.c | 417 +++++++++++++++++++++++++++++++++++++++++
>  common/image-fit.c     |   6 +-
>  common/image-sig.c     | 396 --------------------------------------
>  include/image.h        |  14 +-
>  lib/rsa/Kconfig        |  12 ++
>  lib/rsa/Makefile       |   2 +-
>  lib/rsa/rsa-verify.c   |   2 +
>  tools/Makefile         |   2 +-
>  10 files changed, 452 insertions(+), 403 deletions(-)
>  create mode 100644 common/image-fit-sig.c

[..]

> diff --git a/include/image.h b/include/image.h
> index c1065c06f9bd..e37011b7e463 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -18,6 +18,15 @@
>  #include "compiler.h"
>  #include <asm/byteorder.h>
>  #include <stdbool.h>
> +#if 1
> +#ifndef USE_HOSTCC
> +/*
> + * We should not include kconfig.h> on host, otherwise it may cause
> + * build errors due to unexpected CONFIG_* defined.
> + */
> +#include <linux/kconfig.h>
> +#endif
> +#endif

Can we remove all this somehow? We seem to have got this far without
including kconfig.h

Other than that it looks good.


- Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[Simon Glass]

Hi Takahiro,

On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> In the current implementation of FIT_SIGNATURE, five parameters for
> a RSA public key are required while only two of them are essential.
> (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> This is a result of considering relatively limited computer power
> and resources on embedded systems, while such a assumption may not
> be quite practical for other use cases.
>
> In this patch, added is a function, rsa_gen_key_prop(), which will
> generate additional parameters for other uses, in particular
> UEFI secure boot, on the fly.
>
> Note: the current code uses some "big number" routines from BearSSL
> for the calculation.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/u-boot/rsa-mod-exp.h |   3 +
>  lib/rsa/Kconfig              |   7 +
>  lib/rsa/Makefile             |   1 +
>  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
>  4 files changed, 596 insertions(+)
>  create mode 100644 lib/rsa/rsa-keyprop.c
>
> diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> index 8a428c4b6a1a..ca189292d869 100644
> --- a/include/u-boot/rsa-mod-exp.h
> +++ b/include/u-boot/rsa-mod-exp.h
> @@ -26,6 +26,9 @@ struct key_prop {
>         uint32_t exp_len;       /* Exponent length in number of uint8_t */
>  };
>
> +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> +void rsa_free_key_prop(struct key_prop *prop);

Please add full function comments.

> +
>  /**
>   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
>   *
> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> index 62b7ab9c5e5c..d1743d7a4c47 100644
> --- a/lib/rsa/Kconfig
> +++ b/lib/rsa/Kconfig
> @@ -30,6 +30,13 @@ config RSA_VERIFY
>         help
>           Add RSA signature verification support.
>
> +config RSA_VERIFY_WITH_PKEY
> +       bool "Execute RSA verification without key parameters from FDT"
> +       depends on RSA
> +       help
> +         This options enables RSA signature verification without
> +         using public key parameters which is embedded control FDT.

How about something like...

The standard RSA-signature verification code uses ....

This does not suit the use case where ...

This option enables ...

> +
>  config RSA_SOFTWARE_EXP
>         bool "Enable driver for RSA Modular Exponentiation in software"
>         depends on DM
> diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> index c07305188e0c..14ed3cb4012b 100644
> --- a/lib/rsa/Makefile
> +++ b/lib/rsa/Makefile
> @@ -6,4 +6,5 @@
>  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>
>  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
>  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> new file mode 100644
> index 000000000000..d7d222e9bed9
> --- /dev/null
> +++ b/lib/rsa/rsa-keyprop.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0+ and MIT
> +/*
> + * RSA library - generate parameters for a public key
> + *
> + * Copyright (c) 2019 Linaro Limited
> + * Author: AKASHI Takahiro
> + *
> + * Big number routines in this file come from BearSSL:
> + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> + */
> +
> +#include <common.h>
> +#include <image.h>
> +#include <malloc.h>
> +#include <asm/byteorder.h>
> +#include <crypto/internal/rsa.h>
> +#include <u-boot/rsa-mod-exp.h>
> +
> +static inline unsigned

Please drop the inlines unless needed. Would prefer to leave this to
the compiler.

Also please keep the function name on the same line as the 'static'.

> +br_dec16be(const void *src)
> +{
> +       return be16_to_cpup(src);
> +}
> +
> +static inline uint32_t
> +br_dec32be(const void *src)
> +{
> +       return be32_to_cpup(src);
> +}
> +
> +static inline void
> +br_enc32be(void *dst, uint32_t x)
> +{
> +       __be32 tmp;
> +
> +       tmp = cpu_to_be32(x);
> +       memcpy(dst, &tmp, sizeof(tmp));
> +}
> +
> +/* stripped version of src/inner.h */

??

> +
> +static inline uint32_t
> +NOT(uint32_t ctl)
> +{
> +       return ctl ^ 1;
> +}
> +

All of these functions need full comments please.

> +static inline uint32_t
> +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> +{
> +       return y ^ (-ctl & (x ^ y));
> +}
> +
> +static inline uint32_t
> +EQ(uint32_t x, uint32_t y)

Should use lower case. Please run patman.

> +{
> +       uint32_t q;
> +
> +       q = x ^ y;
> +       return NOT((q | -q) >> 31);
> +}
> +

[...]

Regards,
Simon

[U-Boot] [PATCH v1 1/3] lib: rsa: decouple rsa from FIT image verification

[AKASHI Takahiro]

On Mon, Oct 21, 2019 at 06:17:02PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Introduce new configuration, CONFIG_RSA_VERIFY which will decouple building
> > RSA functions from FIT verification and allow for adding a RSA-based
> > signature verification for other file formats, in particular PE file
> > for UEFI secure boot.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  Kconfig                |   1 +
> >  common/Makefile        |   3 +-
> >  common/image-fit-sig.c | 417 +++++++++++++++++++++++++++++++++++++++++
> >  common/image-fit.c     |   6 +-
> >  common/image-sig.c     | 396 --------------------------------------
> >  include/image.h        |  14 +-
> >  lib/rsa/Kconfig        |  12 ++
> >  lib/rsa/Makefile       |   2 +-
> >  lib/rsa/rsa-verify.c   |   2 +
> >  tools/Makefile         |   2 +-
> >  10 files changed, 452 insertions(+), 403 deletions(-)
> >  create mode 100644 common/image-fit-sig.c
> 
> [..]
> 
> > diff --git a/include/image.h b/include/image.h
> > index c1065c06f9bd..e37011b7e463 100644
> > --- a/include/image.h
> > +++ b/include/image.h
> > @@ -18,6 +18,15 @@
> >  #include "compiler.h"
> >  #include <asm/byteorder.h>
> >  #include <stdbool.h>
> > +#if 1
> > +#ifndef USE_HOSTCC
> > +/*
> > + * We should not include kconfig.h> on host, otherwise it may cause
> > + * build errors due to unexpected CONFIG_* defined.
> > + */
> > +#include <linux/kconfig.h>
> > +#endif
> > +#endif
> 
> Can we remove all this somehow? We seem to have got this far without
> including kconfig.h

I don't remember why I added this #ifndef here. As I can't reproduce
an issue, I will remove it.

-Takahiro Akashi

> Other than that it looks good.
> 
> 
> - Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[AKASHI Takahiro]

On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > In the current implementation of FIT_SIGNATURE, five parameters for
> > a RSA public key are required while only two of them are essential.
> > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > This is a result of considering relatively limited computer power
> > and resources on embedded systems, while such a assumption may not
> > be quite practical for other use cases.
> >
> > In this patch, added is a function, rsa_gen_key_prop(), which will
> > generate additional parameters for other uses, in particular
> > UEFI secure boot, on the fly.
> >
> > Note: the current code uses some "big number" routines from BearSSL
> > for the calculation.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/u-boot/rsa-mod-exp.h |   3 +
> >  lib/rsa/Kconfig              |   7 +
> >  lib/rsa/Makefile             |   1 +
> >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 596 insertions(+)
> >  create mode 100644 lib/rsa/rsa-keyprop.c
> >
> > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > index 8a428c4b6a1a..ca189292d869 100644
> > --- a/include/u-boot/rsa-mod-exp.h
> > +++ b/include/u-boot/rsa-mod-exp.h
> > @@ -26,6 +26,9 @@ struct key_prop {
> >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> >  };
> >
> > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > +void rsa_free_key_prop(struct key_prop *prop);
> 
> Please add full function comments.

Sure.

> > +
> >  /**
> >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> >   *
> > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > --- a/lib/rsa/Kconfig
> > +++ b/lib/rsa/Kconfig
> > @@ -30,6 +30,13 @@ config RSA_VERIFY
> >         help
> >           Add RSA signature verification support.
> >
> > +config RSA_VERIFY_WITH_PKEY
> > +       bool "Execute RSA verification without key parameters from FDT"
> > +       depends on RSA
> > +       help
> > +         This options enables RSA signature verification without
> > +         using public key parameters which is embedded control FDT.
> 
> How about something like...
> 
> The standard RSA-signature verification code uses ....
> 
> This does not suit the use case where ...
> 
> This option enables ...

I will try to improve ...

> > +
> >  config RSA_SOFTWARE_EXP
> >         bool "Enable driver for RSA Modular Exponentiation in software"
> >         depends on DM
> > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > index c07305188e0c..14ed3cb4012b 100644
> > --- a/lib/rsa/Makefile
> > +++ b/lib/rsa/Makefile
> > @@ -6,4 +6,5 @@
> >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >
> >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > new file mode 100644
> > index 000000000000..d7d222e9bed9
> > --- /dev/null
> > +++ b/lib/rsa/rsa-keyprop.c
> > @@ -0,0 +1,585 @@
> > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > +/*
> > + * RSA library - generate parameters for a public key
> > + *
> > + * Copyright (c) 2019 Linaro Limited
> > + * Author: AKASHI Takahiro
> > + *
> > + * Big number routines in this file come from BearSSL:
> > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > + */
> > +
> > +#include <common.h>
> > +#include <image.h>
> > +#include <malloc.h>
> > +#include <asm/byteorder.h>
> > +#include <crypto/internal/rsa.h>
> > +#include <u-boot/rsa-mod-exp.h>
> > +
> > +static inline unsigned
> 
> Please drop the inlines unless needed. Would prefer to leave this to
> the compiler.

Okay.

> Also please keep the function name on the same line as the 'static'.

Okay.

> > +br_dec16be(const void *src)
> > +{
> > +       return be16_to_cpup(src);
> > +}
> > +
> > +static inline uint32_t
> > +br_dec32be(const void *src)
> > +{
> > +       return be32_to_cpup(src);
> > +}
> > +
> > +static inline void
> > +br_enc32be(void *dst, uint32_t x)
> > +{
> > +       __be32 tmp;
> > +
> > +       tmp = cpu_to_be32(x);
> > +       memcpy(dst, &tmp, sizeof(tmp));
> > +}
> > +
> > +/* stripped version of src/inner.h */
> 
> ??

I mean that only a small subset of BearSSL's src/inner.h was imported here.

> > +
> > +static inline uint32_t
> > +NOT(uint32_t ctl)
> > +{
> > +       return ctl ^ 1;
> > +}
> > +
> 
> All of these functions need full comments please.

All the functions imported from BearSSL?
Please note that all the functions, except rsa_gen_key_prop()
rsa_free_key_prop(), are just imported without changes and
all are declared as static.

> > +static inline uint32_t
> > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > +{
> > +       return y ^ (-ctl & (x ^ y));
> > +}
> > +
> > +static inline uint32_t
> > +EQ(uint32_t x, uint32_t y)
> 
> Should use lower case. Please run patman.

I would like to honer the original code(BearSSL)'s notation including
upper/lower cases for minimizing differences.


> > +{
> > +       uint32_t q;
> > +
> > +       q = x ^ y;
> > +       return NOT((q | -q) >> 31);
> > +}
> > +
> 
> [...]
> 
> Regards,
> Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[Simon Glass]

Hi,

On Tue, 22 Oct 2019 at 23:23, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > In the current implementation of FIT_SIGNATURE, five parameters for
> > > a RSA public key are required while only two of them are essential.
> > > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > > This is a result of considering relatively limited computer power
> > > and resources on embedded systems, while such a assumption may not
> > > be quite practical for other use cases.
> > >
> > > In this patch, added is a function, rsa_gen_key_prop(), which will
> > > generate additional parameters for other uses, in particular
> > > UEFI secure boot, on the fly.
> > >
> > > Note: the current code uses some "big number" routines from BearSSL
> > > for the calculation.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  include/u-boot/rsa-mod-exp.h |   3 +
> > >  lib/rsa/Kconfig              |   7 +
> > >  lib/rsa/Makefile             |   1 +
> > >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> > >  4 files changed, 596 insertions(+)
> > >  create mode 100644 lib/rsa/rsa-keyprop.c
> > >
> > > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > > index 8a428c4b6a1a..ca189292d869 100644
> > > --- a/include/u-boot/rsa-mod-exp.h
> > > +++ b/include/u-boot/rsa-mod-exp.h
> > > @@ -26,6 +26,9 @@ struct key_prop {
> > >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> > >  };
> > >
> > > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > > +void rsa_free_key_prop(struct key_prop *prop);
> >
> > Please add full function comments.
>
> Sure.
>
> > > +
> > >  /**
> > >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > >   *
> > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > > --- a/lib/rsa/Kconfig
> > > +++ b/lib/rsa/Kconfig
> > > @@ -30,6 +30,13 @@ config RSA_VERIFY
> > >         help
> > >           Add RSA signature verification support.
> > >
> > > +config RSA_VERIFY_WITH_PKEY
> > > +       bool "Execute RSA verification without key parameters from FDT"
> > > +       depends on RSA
> > > +       help
> > > +         This options enables RSA signature verification without
> > > +         using public key parameters which is embedded control FDT.
> >
> > How about something like...
> >
> > The standard RSA-signature verification code uses ....
> >
> > This does not suit the use case where ...
> >
> > This option enables ...
>
> I will try to improve ...
>
> > > +
> > >  config RSA_SOFTWARE_EXP
> > >         bool "Enable driver for RSA Modular Exponentiation in software"
> > >         depends on DM
> > > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > > index c07305188e0c..14ed3cb4012b 100644
> > > --- a/lib/rsa/Makefile
> > > +++ b/lib/rsa/Makefile
> > > @@ -6,4 +6,5 @@
> > >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > >
> > >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > > new file mode 100644
> > > index 000000000000..d7d222e9bed9
> > > --- /dev/null
> > > +++ b/lib/rsa/rsa-keyprop.c
> > > @@ -0,0 +1,585 @@
> > > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > > +/*
> > > + * RSA library - generate parameters for a public key
> > > + *
> > > + * Copyright (c) 2019 Linaro Limited
> > > + * Author: AKASHI Takahiro
> > > + *
> > > + * Big number routines in this file come from BearSSL:
> > > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <image.h>
> > > +#include <malloc.h>
> > > +#include <asm/byteorder.h>
> > > +#include <crypto/internal/rsa.h>
> > > +#include <u-boot/rsa-mod-exp.h>
> > > +
> > > +static inline unsigned
> >
> > Please drop the inlines unless needed. Would prefer to leave this to
> > the compiler.
>
> Okay.
>
> > Also please keep the function name on the same line as the 'static'.
>
> Okay.
>
> > > +br_dec16be(const void *src)
> > > +{
> > > +       return be16_to_cpup(src);
> > > +}
> > > +
> > > +static inline uint32_t
> > > +br_dec32be(const void *src)
> > > +{
> > > +       return be32_to_cpup(src);
> > > +}
> > > +
> > > +static inline void
> > > +br_enc32be(void *dst, uint32_t x)
> > > +{
> > > +       __be32 tmp;
> > > +
> > > +       tmp = cpu_to_be32(x);
> > > +       memcpy(dst, &tmp, sizeof(tmp));
> > > +}
> > > +
> > > +/* stripped version of src/inner.h */
> >
> > ??
>
> I mean that only a small subset of BearSSL's src/inner.h was imported here.
>
> > > +
> > > +static inline uint32_t
> > > +NOT(uint32_t ctl)
> > > +{
> > > +       return ctl ^ 1;
> > > +}
> > > +
> >
> > All of these functions need full comments please.
>
> All the functions imported from BearSSL?
> Please note that all the functions, except rsa_gen_key_prop()
> rsa_free_key_prop(), are just imported without changes and
> all are declared as static.
>
> > > +static inline uint32_t
> > > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > > +{
> > > +       return y ^ (-ctl & (x ^ y));
> > > +}
> > > +
> > > +static inline uint32_t
> > > +EQ(uint32_t x, uint32_t y)
> >
> > Should use lower case. Please run patman.
>
> I would like to honer the original code(BearSSL)'s notation including
> upper/lower cases for minimizing differences.

There is precedent for this. It's just that I'm not sure the code is
complicated enough to worry about keeping the comment-free
single-char-variable style?

+Tom Rini I assume you are OK with this?

Regards,
Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[Tom Rini]

On Thu, Oct 24, 2019 at 03:36:07PM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 22 Oct 2019 at 23:23, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > In the current implementation of FIT_SIGNATURE, five parameters for
> > > > a RSA public key are required while only two of them are essential.
> > > > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > > > This is a result of considering relatively limited computer power
> > > > and resources on embedded systems, while such a assumption may not
> > > > be quite practical for other use cases.
> > > >
> > > > In this patch, added is a function, rsa_gen_key_prop(), which will
> > > > generate additional parameters for other uses, in particular
> > > > UEFI secure boot, on the fly.
> > > >
> > > > Note: the current code uses some "big number" routines from BearSSL
> > > > for the calculation.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  include/u-boot/rsa-mod-exp.h |   3 +
> > > >  lib/rsa/Kconfig              |   7 +
> > > >  lib/rsa/Makefile             |   1 +
> > > >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 596 insertions(+)
> > > >  create mode 100644 lib/rsa/rsa-keyprop.c
> > > >
> > > > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > > > index 8a428c4b6a1a..ca189292d869 100644
> > > > --- a/include/u-boot/rsa-mod-exp.h
> > > > +++ b/include/u-boot/rsa-mod-exp.h
> > > > @@ -26,6 +26,9 @@ struct key_prop {
> > > >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> > > >  };
> > > >
> > > > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > > > +void rsa_free_key_prop(struct key_prop *prop);
> > >
> > > Please add full function comments.
> >
> > Sure.
> >
> > > > +
> > > >  /**
> > > >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > > >   *
> > > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > > > --- a/lib/rsa/Kconfig
> > > > +++ b/lib/rsa/Kconfig
> > > > @@ -30,6 +30,13 @@ config RSA_VERIFY
> > > >         help
> > > >           Add RSA signature verification support.
> > > >
> > > > +config RSA_VERIFY_WITH_PKEY
> > > > +       bool "Execute RSA verification without key parameters from FDT"
> > > > +       depends on RSA
> > > > +       help
> > > > +         This options enables RSA signature verification without
> > > > +         using public key parameters which is embedded control FDT.
> > >
> > > How about something like...
> > >
> > > The standard RSA-signature verification code uses ....
> > >
> > > This does not suit the use case where ...
> > >
> > > This option enables ...
> >
> > I will try to improve ...
> >
> > > > +
> > > >  config RSA_SOFTWARE_EXP
> > > >         bool "Enable driver for RSA Modular Exponentiation in software"
> > > >         depends on DM
> > > > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > > > index c07305188e0c..14ed3cb4012b 100644
> > > > --- a/lib/rsa/Makefile
> > > > +++ b/lib/rsa/Makefile
> > > > @@ -6,4 +6,5 @@
> > > >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > > >
> > > >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > > > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > > >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > > > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > > > new file mode 100644
> > > > index 000000000000..d7d222e9bed9
> > > > --- /dev/null
> > > > +++ b/lib/rsa/rsa-keyprop.c
> > > > @@ -0,0 +1,585 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > > > +/*
> > > > + * RSA library - generate parameters for a public key
> > > > + *
> > > > + * Copyright (c) 2019 Linaro Limited
> > > > + * Author: AKASHI Takahiro
> > > > + *
> > > > + * Big number routines in this file come from BearSSL:
> > > > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <image.h>
> > > > +#include <malloc.h>
> > > > +#include <asm/byteorder.h>
> > > > +#include <crypto/internal/rsa.h>
> > > > +#include <u-boot/rsa-mod-exp.h>
> > > > +
> > > > +static inline unsigned
> > >
> > > Please drop the inlines unless needed. Would prefer to leave this to
> > > the compiler.
> >
> > Okay.
> >
> > > Also please keep the function name on the same line as the 'static'.
> >
> > Okay.
> >
> > > > +br_dec16be(const void *src)
> > > > +{
> > > > +       return be16_to_cpup(src);
> > > > +}
> > > > +
> > > > +static inline uint32_t
> > > > +br_dec32be(const void *src)
> > > > +{
> > > > +       return be32_to_cpup(src);
> > > > +}
> > > > +
> > > > +static inline void
> > > > +br_enc32be(void *dst, uint32_t x)
> > > > +{
> > > > +       __be32 tmp;
> > > > +
> > > > +       tmp = cpu_to_be32(x);
> > > > +       memcpy(dst, &tmp, sizeof(tmp));
> > > > +}
> > > > +
> > > > +/* stripped version of src/inner.h */
> > >
> > > ??
> >
> > I mean that only a small subset of BearSSL's src/inner.h was imported here.
> >
> > > > +
> > > > +static inline uint32_t
> > > > +NOT(uint32_t ctl)
> > > > +{
> > > > +       return ctl ^ 1;
> > > > +}
> > > > +
> > >
> > > All of these functions need full comments please.
> >
> > All the functions imported from BearSSL?
> > Please note that all the functions, except rsa_gen_key_prop()
> > rsa_free_key_prop(), are just imported without changes and
> > all are declared as static.
> >
> > > > +static inline uint32_t
> > > > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > > > +{
> > > > +       return y ^ (-ctl & (x ^ y));
> > > > +}
> > > > +
> > > > +static inline uint32_t
> > > > +EQ(uint32_t x, uint32_t y)
> > >
> > > Should use lower case. Please run patman.
> >
> > I would like to honer the original code(BearSSL)'s notation including
> > upper/lower cases for minimizing differences.
> 
> There is precedent for this. It's just that I'm not sure the code is
> complicated enough to worry about keeping the comment-free
> single-char-variable style?
> 
> +Tom Rini I assume you are OK with this?

When we import code from another project that we expect to re-sync we do
what we need to do to keep the overall re-sync pain down, even we we
otherwise may disagree on style points.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191025/fa6473df/attachment.sig>

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[Simon Glass]

Hi,

On Fri, 25 Oct 2019 at 12:27, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Oct 24, 2019 at 03:36:07PM -0600, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 22 Oct 2019 at 23:23, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> > > > Hi Takahiro,
> > > >
> > > > On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > In the current implementation of FIT_SIGNATURE, five parameters for
> > > > > a RSA public key are required while only two of them are essential.
> > > > > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > > > > This is a result of considering relatively limited computer power
> > > > > and resources on embedded systems, while such a assumption may not
> > > > > be quite practical for other use cases.
> > > > >
> > > > > In this patch, added is a function, rsa_gen_key_prop(), which will
> > > > > generate additional parameters for other uses, in particular
> > > > > UEFI secure boot, on the fly.
> > > > >
> > > > > Note: the current code uses some "big number" routines from BearSSL
> > > > > for the calculation.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  include/u-boot/rsa-mod-exp.h |   3 +
> > > > >  lib/rsa/Kconfig              |   7 +
> > > > >  lib/rsa/Makefile             |   1 +
> > > > >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 596 insertions(+)
> > > > >  create mode 100644 lib/rsa/rsa-keyprop.c
> > > > >
> > > > > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > > > > index 8a428c4b6a1a..ca189292d869 100644
> > > > > --- a/include/u-boot/rsa-mod-exp.h
> > > > > +++ b/include/u-boot/rsa-mod-exp.h
> > > > > @@ -26,6 +26,9 @@ struct key_prop {
> > > > >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> > > > >  };
> > > > >
> > > > > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > > > > +void rsa_free_key_prop(struct key_prop *prop);
> > > >
> > > > Please add full function comments.
> > >
> > > Sure.
> > >
> > > > > +
> > > > >  /**
> > > > >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > > > >   *
> > > > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > > > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > > > > --- a/lib/rsa/Kconfig
> > > > > +++ b/lib/rsa/Kconfig
> > > > > @@ -30,6 +30,13 @@ config RSA_VERIFY
> > > > >         help
> > > > >           Add RSA signature verification support.
> > > > >
> > > > > +config RSA_VERIFY_WITH_PKEY
> > > > > +       bool "Execute RSA verification without key parameters from FDT"
> > > > > +       depends on RSA
> > > > > +       help
> > > > > +         This options enables RSA signature verification without
> > > > > +         using public key parameters which is embedded control FDT.
> > > >
> > > > How about something like...
> > > >
> > > > The standard RSA-signature verification code uses ....
> > > >
> > > > This does not suit the use case where ...
> > > >
> > > > This option enables ...
> > >
> > > I will try to improve ...
> > >
> > > > > +
> > > > >  config RSA_SOFTWARE_EXP
> > > > >         bool "Enable driver for RSA Modular Exponentiation in software"
> > > > >         depends on DM
> > > > > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > > > > index c07305188e0c..14ed3cb4012b 100644
> > > > > --- a/lib/rsa/Makefile
> > > > > +++ b/lib/rsa/Makefile
> > > > > @@ -6,4 +6,5 @@
> > > > >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > > > >
> > > > >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > > > > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > > > >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > > > > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > > > > new file mode 100644
> > > > > index 000000000000..d7d222e9bed9
> > > > > --- /dev/null
> > > > > +++ b/lib/rsa/rsa-keyprop.c
> > > > > @@ -0,0 +1,585 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > > > > +/*
> > > > > + * RSA library - generate parameters for a public key
> > > > > + *
> > > > > + * Copyright (c) 2019 Linaro Limited
> > > > > + * Author: AKASHI Takahiro
> > > > > + *
> > > > > + * Big number routines in this file come from BearSSL:
> > > > > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <image.h>
> > > > > +#include <malloc.h>
> > > > > +#include <asm/byteorder.h>
> > > > > +#include <crypto/internal/rsa.h>
> > > > > +#include <u-boot/rsa-mod-exp.h>
> > > > > +
> > > > > +static inline unsigned
> > > >
> > > > Please drop the inlines unless needed. Would prefer to leave this to
> > > > the compiler.
> > >
> > > Okay.
> > >
> > > > Also please keep the function name on the same line as the 'static'.
> > >
> > > Okay.
> > >
> > > > > +br_dec16be(const void *src)
> > > > > +{
> > > > > +       return be16_to_cpup(src);
> > > > > +}
> > > > > +
> > > > > +static inline uint32_t
> > > > > +br_dec32be(const void *src)
> > > > > +{
> > > > > +       return be32_to_cpup(src);
> > > > > +}
> > > > > +
> > > > > +static inline void
> > > > > +br_enc32be(void *dst, uint32_t x)
> > > > > +{
> > > > > +       __be32 tmp;
> > > > > +
> > > > > +       tmp = cpu_to_be32(x);
> > > > > +       memcpy(dst, &tmp, sizeof(tmp));
> > > > > +}
> > > > > +
> > > > > +/* stripped version of src/inner.h */
> > > >
> > > > ??
> > >
> > > I mean that only a small subset of BearSSL's src/inner.h was imported here.
> > >
> > > > > +
> > > > > +static inline uint32_t
> > > > > +NOT(uint32_t ctl)
> > > > > +{
> > > > > +       return ctl ^ 1;
> > > > > +}
> > > > > +
> > > >
> > > > All of these functions need full comments please.
> > >
> > > All the functions imported from BearSSL?
> > > Please note that all the functions, except rsa_gen_key_prop()
> > > rsa_free_key_prop(), are just imported without changes and
> > > all are declared as static.
> > >
> > > > > +static inline uint32_t
> > > > > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > > > > +{
> > > > > +       return y ^ (-ctl & (x ^ y));
> > > > > +}
> > > > > +
> > > > > +static inline uint32_t
> > > > > +EQ(uint32_t x, uint32_t y)
> > > >
> > > > Should use lower case. Please run patman.
> > >
> > > I would like to honer the original code(BearSSL)'s notation including
> > > upper/lower cases for minimizing differences.
> >
> > There is precedent for this. It's just that I'm not sure the code is
> > complicated enough to worry about keeping the comment-free
> > single-char-variable style?
> >
> > +Tom Rini I assume you are OK with this?
>
> When we import code from another project that we expect to re-sync we do
> what we need to do to keep the overall re-sync pain down, even we we
> otherwise may disagree on style points.

OK ta.

Regards,
Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[AKASHI Takahiro]

Simon, Tom,

On Fri, Oct 25, 2019 at 12:29:08PM -0600, Simon Glass wrote:
> Hi,
> 
> On Fri, 25 Oct 2019 at 12:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 24, 2019 at 03:36:07PM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 22 Oct 2019 at 23:23, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> > > > > Hi Takahiro,
> > > > >
> > > > > On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > In the current implementation of FIT_SIGNATURE, five parameters for
> > > > > > a RSA public key are required while only two of them are essential.
> > > > > > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > > > > > This is a result of considering relatively limited computer power
> > > > > > and resources on embedded systems, while such a assumption may not
> > > > > > be quite practical for other use cases.
> > > > > >
> > > > > > In this patch, added is a function, rsa_gen_key_prop(), which will
> > > > > > generate additional parameters for other uses, in particular
> > > > > > UEFI secure boot, on the fly.
> > > > > >
> > > > > > Note: the current code uses some "big number" routines from BearSSL
> > > > > > for the calculation.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > ---
> > > > > >  include/u-boot/rsa-mod-exp.h |   3 +
> > > > > >  lib/rsa/Kconfig              |   7 +
> > > > > >  lib/rsa/Makefile             |   1 +
> > > > > >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> > > > > >  4 files changed, 596 insertions(+)
> > > > > >  create mode 100644 lib/rsa/rsa-keyprop.c
> > > > > >
> > > > > > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > > > > > index 8a428c4b6a1a..ca189292d869 100644
> > > > > > --- a/include/u-boot/rsa-mod-exp.h
> > > > > > +++ b/include/u-boot/rsa-mod-exp.h
> > > > > > @@ -26,6 +26,9 @@ struct key_prop {
> > > > > >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> > > > > >  };
> > > > > >
> > > > > > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > > > > > +void rsa_free_key_prop(struct key_prop *prop);
> > > > >
> > > > > Please add full function comments.
> > > >
> > > > Sure.
> > > >
> > > > > > +
> > > > > >  /**
> > > > > >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > > > > >   *
> > > > > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > > > > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > > > > > --- a/lib/rsa/Kconfig
> > > > > > +++ b/lib/rsa/Kconfig
> > > > > > @@ -30,6 +30,13 @@ config RSA_VERIFY
> > > > > >         help
> > > > > >           Add RSA signature verification support.
> > > > > >
> > > > > > +config RSA_VERIFY_WITH_PKEY
> > > > > > +       bool "Execute RSA verification without key parameters from FDT"
> > > > > > +       depends on RSA
> > > > > > +       help
> > > > > > +         This options enables RSA signature verification without
> > > > > > +         using public key parameters which is embedded control FDT.
> > > > >
> > > > > How about something like...
> > > > >
> > > > > The standard RSA-signature verification code uses ....
> > > > >
> > > > > This does not suit the use case where ...
> > > > >
> > > > > This option enables ...
> > > >
> > > > I will try to improve ...
> > > >
> > > > > > +
> > > > > >  config RSA_SOFTWARE_EXP
> > > > > >         bool "Enable driver for RSA Modular Exponentiation in software"
> > > > > >         depends on DM
> > > > > > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > > > > > index c07305188e0c..14ed3cb4012b 100644
> > > > > > --- a/lib/rsa/Makefile
> > > > > > +++ b/lib/rsa/Makefile
> > > > > > @@ -6,4 +6,5 @@
> > > > > >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > > > > >
> > > > > >  obj-$(CONFIG_$(SPL_)RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > > > > > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > > > > >  obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > > > > > diff --git a/lib/rsa/rsa-keyprop.c b/lib/rsa/rsa-keyprop.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..d7d222e9bed9
> > > > > > --- /dev/null
> > > > > > +++ b/lib/rsa/rsa-keyprop.c
> > > > > > @@ -0,0 +1,585 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > > > > > +/*
> > > > > > + * RSA library - generate parameters for a public key
> > > > > > + *
> > > > > > + * Copyright (c) 2019 Linaro Limited
> > > > > > + * Author: AKASHI Takahiro
> > > > > > + *
> > > > > > + * Big number routines in this file come from BearSSL:
> > > > > > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <image.h>
> > > > > > +#include <malloc.h>
> > > > > > +#include <asm/byteorder.h>
> > > > > > +#include <crypto/internal/rsa.h>
> > > > > > +#include <u-boot/rsa-mod-exp.h>
> > > > > > +
> > > > > > +static inline unsigned
> > > > >
> > > > > Please drop the inlines unless needed. Would prefer to leave this to
> > > > > the compiler.
> > > >
> > > > Okay.
> > > >
> > > > > Also please keep the function name on the same line as the 'static'.
> > > >
> > > > Okay.
> > > >
> > > > > > +br_dec16be(const void *src)
> > > > > > +{
> > > > > > +       return be16_to_cpup(src);
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +br_dec32be(const void *src)
> > > > > > +{
> > > > > > +       return be32_to_cpup(src);
> > > > > > +}
> > > > > > +
> > > > > > +static inline void
> > > > > > +br_enc32be(void *dst, uint32_t x)
> > > > > > +{
> > > > > > +       __be32 tmp;
> > > > > > +
> > > > > > +       tmp = cpu_to_be32(x);
> > > > > > +       memcpy(dst, &tmp, sizeof(tmp));
> > > > > > +}
> > > > > > +
> > > > > > +/* stripped version of src/inner.h */
> > > > >
> > > > > ??
> > > >
> > > > I mean that only a small subset of BearSSL's src/inner.h was imported here.
> > > >
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +NOT(uint32_t ctl)
> > > > > > +{
> > > > > > +       return ctl ^ 1;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > All of these functions need full comments please.
> > > >
> > > > All the functions imported from BearSSL?
> > > > Please note that all the functions, except rsa_gen_key_prop()
> > > > rsa_free_key_prop(), are just imported without changes and
> > > > all are declared as static.
> > > >
> > > > > > +static inline uint32_t
> > > > > > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > > > > > +{
> > > > > > +       return y ^ (-ctl & (x ^ y));
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +EQ(uint32_t x, uint32_t y)
> > > > >
> > > > > Should use lower case. Please run patman.
> > > >
> > > > I would like to honer the original code(BearSSL)'s notation including
> > > > upper/lower cases for minimizing differences.
> > >
> > > There is precedent for this. It's just that I'm not sure the code is
> > > complicated enough to worry about keeping the comment-free
> > > single-char-variable style?
> > >
> > > +Tom Rini I assume you are OK with this?
> >
> > When we import code from another project that we expect to re-sync we do
> > what we need to do to keep the overall re-sync pain down, even we we
> > otherwise may disagree on style points.
> 
> OK ta.

Thank you for your understanding.
Yes, this patch is small but if I had to modify my "import x509/pkcs7
parsers from linux" patch, it would be quite painful.

-Takahiro Akashi


> Regards,
> Simon

[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[Simon Glass]

Hi AKASHI,

> > > >
> > > > There is precedent for this. It's just that I'm not sure the code is
> > > > complicated enough to worry about keeping the comment-free
> > > > single-char-variable style?
> > > >
> > > > +Tom Rini I assume you are OK with this?
> > >
> > > When we import code from another project that we expect to re-sync we do
> > > what we need to do to keep the overall re-sync pain down, even we we
> > > otherwise may disagree on style points.
> >
> > OK ta.
>
> Thank you for your understanding.
> Yes, this patch is small but if I had to modify my "import x509/pkcs7
> parsers from linux" patch, it would be quite painful.

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> -Takahiro Akashi
>
>
> > Regards,
> > Simon