[RFC PATCH v3 0/8] image: add a stage pre-load

[RFC PATCH v3 0/8] image: add a stage pre-load

[Philippe Reynes]

This serie adds a stage pre-load before launching an image.
This stage is used to read a header before the image and
this header contains the signature of the full image.
So u-boot may check the full image before using any
data of the image.

Changelog:
v3:
- move image-pre-load.c to /boot
- update mkimage to add public key in u-boot device tree
- add script gen_pre_load_header.sh
v2:
- move the code to image-pre-load
- add support of stage pre-load for spl
- add support of stage pre-load on spl_ram

Philippe Reynes (8):
  lib: allow to build asn1 decoder and oid registry in SPL
  lib: crypto: allow to build crypyo in SPL
  lib: rsa: allow rsa verify with pkey in SPL
  boot: image: add a stage pre-load
  cmd: bootm: add a stage pre-load
  common: spl: fit_ram: allow to use image pre load
  mkimage: add public key for image pre-load stage
  tools: gen_pre_load_header.sh: initial import

 boot/Kconfig                 |  33 ++++
 boot/Makefile                |   1 +
 boot/bootm.c                 |  33 ++++
 boot/image-pre-load.c        | 291 +++++++++++++++++++++++++++++++++++
 cmd/Kconfig                  |  10 ++
 cmd/bootm.c                  |   2 +-
 common/spl/spl_ram.c         |  21 ++-
 include/image.h              |  25 +++
 lib/Kconfig                  |   6 +
 lib/Makefile                 |   9 +-
 lib/crypto/Kconfig           |  15 ++
 lib/crypto/Makefile          |  19 ++-
 lib/rsa/Kconfig              |   8 +
 tools/fit_image.c            |   3 +
 tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++
 tools/image-host.c           | 116 ++++++++++++++
 16 files changed, 755 insertions(+), 11 deletions(-)
 create mode 100644 boot/image-pre-load.c
 create mode 100755 tools/gen_pre_load_header.sh

-- 
2.17.1

[RFC PATCH v3 1/8] lib: allow to build asn1 decoder and oid registry in SPL

[Philippe Reynes]

This commit adds the options:
- SPL_ASN1_DECODER
- SPL_OID_REGISTRY

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 lib/Kconfig  | 6 ++++++
 lib/Makefile | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index 70bf8e7a46..ebff84f113 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -758,11 +758,17 @@ config ASN1_DECODER
 	help
 	  Enable asn1 decoder library.
 
+config SPL_ASN1_DECODER
+	bool
+
 config OID_REGISTRY
 	bool
 	help
 	  Enable fast lookup object identifier registry.
 
+config SPL_OID_REGISTRY
+	bool
+
 config SMBIOS_PARSER
 	bool "SMBIOS parser"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 5ddbc77ed6..900e684d62 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -17,7 +17,6 @@ obj-$(CONFIG_OF_LIVE) += of_live.o
 obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
 obj-$(CONFIG_ARCH_AT91) += at91/
 obj-$(CONFIG_OPTEE_LIB) += optee/
-obj-$(CONFIG_ASN1_DECODER) += asn1_decoder.o
 obj-y += crypto/
 
 obj-$(CONFIG_AES) += aes.o
@@ -67,6 +66,7 @@ obj-$(CONFIG_SHA1) += sha1.o
 obj-$(CONFIG_SHA256) += sha256.o
 obj-$(CONFIG_SHA512) += sha512.o
 obj-$(CONFIG_CRYPT_PW) += crypt/
+obj-$(CONFIG_$(SPL_)ASN1_DECODER) += asn1_decoder.o
 
 obj-$(CONFIG_$(SPL_)ZLIB) += zlib/
 obj-$(CONFIG_$(SPL_)ZSTD) += zstd/
@@ -128,9 +128,9 @@ obj-$(CONFIG_$(SPL_TPL_)STRTO) += strto.o
 else
 # Main U-Boot always uses the full printf support
 obj-y += vsprintf.o strto.o
-obj-$(CONFIG_OID_REGISTRY) += oid_registry.o
 obj-$(CONFIG_SSCANF) += sscanf.o
 endif
+obj-$(CONFIG_$(SPL_)OID_REGISTRY) += oid_registry.o
 
 obj-y += abuf.o
 obj-y += date.o
@@ -141,6 +141,9 @@ obj-$(CONFIG_LIB_ELF) += elf.o
 # Build a fast OID lookup registry from include/linux/oid_registry.h
 #
 $(obj)/oid_registry.o: $(obj)/oid_registry_data.c
+ifdef CONFIG_SPL_BUILD
+CFLAGS_oid_registry.o += -I$(obj)
+endif
 
 $(obj)/oid_registry_data.c: $(srctree)/include/linux/oid_registry.h \
 			    $(srctree)/scripts/build_OID_registry
-- 
2.17.1

[RFC PATCH v3 2/8] lib: crypto: allow to build crypyo in SPL

[Philippe Reynes]

This commit adds the options:
- SPL_ASYMMETRIC_KEY_TYPE
- SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
- SPL_RSA_PUBLIC_KEY_PARSER

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 lib/Makefile        |  2 +-
 lib/crypto/Kconfig  | 15 +++++++++++++++
 lib/crypto/Makefile | 19 +++++++++++++------
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 900e684d62..df70917b49 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -17,7 +17,6 @@ obj-$(CONFIG_OF_LIVE) += of_live.o
 obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
 obj-$(CONFIG_ARCH_AT91) += at91/
 obj-$(CONFIG_OPTEE_LIB) += optee/
-obj-y += crypto/
 
 obj-$(CONFIG_AES) += aes.o
 obj-$(CONFIG_AES) += aes/
@@ -57,6 +56,7 @@ obj-$(CONFIG_TPM_V1) += tpm-v1.o
 obj-$(CONFIG_TPM_V2) += tpm-v2.o
 endif
 
+obj-y += crypto/
 obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
 obj-$(CONFIG_$(SPL_)MD5) += md5.o
 obj-$(CONFIG_ECDSA) += ecdsa/
diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
index 6369bafac0..9351865f2c 100644
--- a/lib/crypto/Kconfig
+++ b/lib/crypto/Kconfig
@@ -8,6 +8,10 @@ menuconfig ASYMMETRIC_KEY_TYPE
 
 if ASYMMETRIC_KEY_TYPE
 
+config SPL_ASYMMETRIC_KEY_TYPE
+	bool "Asymmetric (public-key cryptographic) key Support within SPL"
+	depends on SPL
+
 config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
 	bool "Asymmetric public-key crypto algorithm subtype"
 	help
@@ -16,6 +20,10 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
 	  appropriate hash algorithms (such as SHA-1) must be available.
 	  ENOPKG will be reported if the requisite algorithm is unavailable.
 
+config SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+	bool "Asymmetric public-key crypto algorithm subtype within SPL"
+	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+
 config RSA_PUBLIC_KEY_PARSER
 	bool "RSA public key parser"
 	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
@@ -27,6 +35,13 @@ config RSA_PUBLIC_KEY_PARSER
 	  public key data and provides the ability to instantiate a public
 	  key.
 
+config SPL_RSA_PUBLIC_KEY_PARSER
+	bool "RSA public key parser within SPL"
+	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+	select SPL_ASN1_DECODER
+	select ASN1_COMPILER
+	select SPL_OID_REGISTRY
+
 config X509_CERTIFICATE_PARSER
 	bool "X.509 certificate parser"
 	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
index f3a414525d..6792b1d4f0 100644
--- a/lib/crypto/Makefile
+++ b/lib/crypto/Makefile
@@ -3,27 +3,34 @@
 # Makefile for asymmetric cryptographic keys
 #
 
-obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o
+obj-$(CONFIG_$(SPL_)ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o
 
 asymmetric_keys-y := asymmetric_type.o
 
-obj-$(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
+obj-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
 
 #
 # RSA public key parser
 #
-obj-$(CONFIG_RSA_PUBLIC_KEY_PARSER) += rsa_public_key.o
+obj-$(CONFIG_$(SPL_)RSA_PUBLIC_KEY_PARSER) += rsa_public_key.o
 rsa_public_key-y := \
 	rsapubkey.asn1.o \
 	rsa_helper.o
 
 $(obj)/rsapubkey.asn1.o: $(obj)/rsapubkey.asn1.c $(obj)/rsapubkey.asn1.h
+ifdef CONFIG_SPL_BUILD
+CFLAGS_rsapubkey.asn1.o += -I$(obj)
+endif
+
 $(obj)/rsa_helper.o: $(obj)/rsapubkey.asn1.h
+ifdef CONFIG_SPL_BUILD
+CFLAGS_rsa_helper.o += -I$(obj)
+endif
 
 #
 # X.509 Certificate handling
 #
-obj-$(CONFIG_X509_CERTIFICATE_PARSER) += x509_key_parser.o
+obj-$(CONFIG_$(SPL_)X509_CERTIFICATE_PARSER) += x509_key_parser.o
 x509_key_parser-y := \
 	x509.asn1.o \
 	x509_akid.asn1.o \
@@ -40,11 +47,11 @@ $(obj)/x509_akid.asn1.o: $(obj)/x509_akid.asn1.c $(obj)/x509_akid.asn1.h
 #
 # PKCS#7 message handling
 #
-obj-$(CONFIG_PKCS7_MESSAGE_PARSER) += pkcs7_message.o
+obj-$(CONFIG_$(SPL_)PKCS7_MESSAGE_PARSER) += pkcs7_message.o
 pkcs7_message-y := \
 	pkcs7.asn1.o \
 	pkcs7_parser.o
-obj-$(CONFIG_PKCS7_VERIFY) += pkcs7_verify.o
+obj-$(CONFIG_$(SPL_)PKCS7_VERIFY) += pkcs7_verify.o
 
 $(obj)/pkcs7_parser.o: $(obj)/pkcs7.asn1.h
 $(obj)/pkcs7.asn1.o: $(obj)/pkcs7.asn1.c $(obj)/pkcs7.asn1.h
-- 
2.17.1

[RFC PATCH v3 3/8] lib: rsa: allow rsa verify with pkey in SPL

[Philippe Reynes]

This commit adds the option SPL_RSA_VERIFY_WITH_PKEY.

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 lib/rsa/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
index 469596abe7..608d51c428 100644
--- a/lib/rsa/Kconfig
+++ b/lib/rsa/Kconfig
@@ -46,6 +46,14 @@ config RSA_VERIFY_WITH_PKEY
 	  directly specified in image_sign_info, where all the necessary
 	  key properties will be calculated on the fly in verification code.
 
+config SPL_RSA_VERIFY_WITH_PKEY
+	bool "Execute RSA verification without key parameters from FDT within SPL"
+	depends on SPL
+	select SPL_RSA_VERIFY
+	select SPL_ASYMMETRIC_KEY_TYPE
+	select SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+	select SPL_RSA_PUBLIC_KEY_PARSER
+
 config RSA_SOFTWARE_EXP
 	bool "Enable driver for RSA Modular Exponentiation in software"
 	depends on DM
-- 
2.17.1

[RFC PATCH v3 4/8] boot: image: add a stage pre-load

[Philippe Reynes]

This commit adds a stage pre-load that could
check or modify an image.

For the moment, only a header with a signature is
supported. This header has this format:
- magic : 4 bytes
- image size : 4 bytes
- signature : n bytes
- padding : up to header size

The stage use a node /image/pre-load/sig to
get some information:
- header-size (mandatory) : size of the header
- algo-name (mandatory) : name of the algo used to sign
- padding-name : name of padding used to sign
- signature-size : size of the signature (in the header)
- mandatory : set to yes if this sig is mandatory
- public-key (madatory) : value of the public key

Before running the image, the stage pre-load check
the signature provided in the header.

This is an initial support, later we could add the
support of:
- ciphering
- uncompressing
- ...

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 boot/Kconfig          |  33 +++++
 boot/Makefile         |   1 +
 boot/image-pre-load.c | 291 ++++++++++++++++++++++++++++++++++++++++++
 include/image.h       |   9 ++
 4 files changed, 334 insertions(+)
 create mode 100644 boot/image-pre-load.c

diff --git a/boot/Kconfig b/boot/Kconfig
index d3a12be228..3856580af6 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -958,6 +958,39 @@ config AUTOBOOT_MENU_SHOW
 
 endmenu
 
+menu "Image support"
+
+config IMAGE_PRE_LOAD
+	bool "Image pre-load support"
+	help
+	  Enable image pre-load support
+
+config SPL_IMAGE_PRE_LOAD
+	bool "Image pre-load support within SPL"
+	depends on SPL && IMAGE_PRE_LOAD
+	help
+	  Enable image pre-load support in SPL
+
+config IMAGE_PRE_LOAD_SIG
+	bool "Image pre-load signature support"
+	depends on IMAGE_PRE_LOAD
+	select FIT_SIGNATURE
+	select RSA
+	select RSA_VERIFY_WITH_PKEY
+	help
+	  Enable image pre-load signature support
+
+config SPL_IMAGE_PRE_LOAD_SIG
+	bool "Image pre-load signature support witin SPL"
+	depends on SPL_IMAGE_PRE_LOAD && IMAGE_PRE_LOAD_SIG
+	select SPL_FIT_SIGNATURE
+	select SPL_RSA
+	select SPL_RSA_VERIFY_WITH_PKEY
+	help
+	  Enable image pre-load signature support in SPL
+
+endmenu
+
 config USE_BOOTARGS
 	bool "Enable boot arguments"
 	help
diff --git a/boot/Makefile b/boot/Makefile
index 2938c3f145..59752c65ca 100644
--- a/boot/Makefile
+++ b/boot/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
 obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.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_)IMAGE_PRE_LOAD) += image-pre-load.o
 obj-$(CONFIG_$(SPL_TPL_)IMAGE_SIGN_INFO) += image-sig.o
 obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-fit-sig.o
 obj-$(CONFIG_$(SPL_TPL_)FIT_CIPHER) += image-cipher.o
diff --git a/boot/image-pre-load.c b/boot/image-pre-load.c
new file mode 100644
index 0000000000..6ed21c3f51
--- /dev/null
+++ b/boot/image-pre-load.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Philippe Reynes <philippe.reynes@softathome.com>
+ */
+
+#include <common.h>
+#include <asm/global_data.h>
+DECLARE_GLOBAL_DATA_PTR;
+#include <image.h>
+#include <mapmem.h>
+
+#define IMAGE_PRE_LOAD_SIG_MAGIC		0x55425348
+#define IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC		0
+#define IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN	4
+#define IMAGE_PRE_LOAD_SIG_OFFSET_SIG		8
+
+#define IMAGE_PRE_LOAD_PATH			"/image/pre-load/sig"
+#define IMAGE_PRE_LOAD_PROP_HEADER_SIZE		"header-size"
+#define IMAGE_PRE_LOAD_PROP_ALGO_NAME		"algo-name"
+#define IMAGE_PRE_LOAD_PROP_PADDING_NAME	"padding-name"
+#define IMAGE_PRE_LOAD_PROP_SIG_SIZE		"signature-size"
+#define IMAGE_PRE_LOAD_PROP_PUBLIC_KEY		"public-key"
+#define IMAGE_PRE_LOAD_PROP_MANDATORY		"mandatory"
+
+#ifndef CONFIG_SYS_BOOTM_LEN
+/* use 8MByte as default max gunzip size */
+#define CONFIG_SYS_BOOTM_LEN	0x800000
+#endif
+
+struct image_sig_header {
+	u32 magic;
+	u32 size;
+	u8 *sig;
+};
+
+struct image_sig_info {
+	ulong header_size;
+	char *algo_name;
+	char *padding_name;
+	u8 *key;
+	int key_len;
+	u32 sig_size;
+	int mandatory;
+};
+
+ulong image_load_offset;
+
+/*
+ * This function gathers information about the signature check
+ * that could be done before launching the image.
+ *
+ * return:
+ * -1 => an error has occurred
+ *  0 => OK
+ *  1 => no setup
+ */
+static int image_pre_load_sig_setup(struct image_sig_info *info)
+{
+	const void *algo_name, *padding_name, *key, *mandatory;
+	const u32 *header_size, *sig_size;
+	int key_len;
+	int node, ret = 0;
+
+	if (!info) {
+		printf("ERROR: info is NULL for image pre-load sig check\n");
+		ret = -1;
+		goto out;
+	}
+
+	memset(info, 0, sizeof(*info));
+
+	node = fdt_path_offset(gd_fdt_blob(), IMAGE_PRE_LOAD_PATH);
+	if (node < 0) {
+		printf("INFO: no info for image pre-load sig check\n");
+		ret = 1;
+		goto out;
+	}
+
+	header_size = fdt_getprop(gd_fdt_blob(), node,
+				  IMAGE_PRE_LOAD_PROP_HEADER_SIZE, NULL);
+	if (!header_size) {
+		printf("ERROR: no header-size for image pre-load sig check\n");
+		ret = -1;
+		goto out;
+	}
+
+	algo_name = fdt_getprop(gd_fdt_blob(), node,
+				IMAGE_PRE_LOAD_PROP_ALGO_NAME, NULL);
+	if (!algo_name) {
+		printf("ERROR: no algo_name for image pre-load sig check\n");
+		ret = -1;
+		goto out;
+	}
+
+	padding_name = fdt_getprop(gd_fdt_blob(), node,
+				   IMAGE_PRE_LOAD_PROP_PADDING_NAME, NULL);
+	if (!padding_name) {
+		printf("INFO: no padding_name provided, so using pkcs-1.5\n");
+		padding_name = "pkcs-1.5";
+	}
+
+	sig_size = fdt_getprop(gd_fdt_blob(), node,
+			       IMAGE_PRE_LOAD_PROP_SIG_SIZE, NULL);
+	if (!sig_size) {
+		printf("ERROR: no signature-size for image pre-load sig check\n");
+		ret = -1;
+		goto out;
+	}
+
+	key = fdt_getprop(gd_fdt_blob(), node,
+			  IMAGE_PRE_LOAD_PROP_PUBLIC_KEY, &key_len);
+	if (!key) {
+		printf("ERROR: no key for image pre-load sig check\n");
+		ret = -1;
+		goto out;
+	}
+
+	info->header_size	= fdt32_to_cpu(*header_size);
+	info->algo_name		= (char *)algo_name;
+	info->padding_name	= (char *)padding_name;
+	info->key		= (uint8_t *)key;
+	info->key_len		= key_len;
+	info->sig_size		= fdt32_to_cpu(*sig_size);
+
+	mandatory = fdt_getprop(gd_fdt_blob(), node,
+				IMAGE_PRE_LOAD_PROP_MANDATORY, NULL);
+	if (mandatory && !strcmp((char *)mandatory, "yes"))
+		info->mandatory = 1;
+
+ out:
+	return ret;
+}
+
+static int image_pre_load_sig_get_header_u32(struct image_sig_info *info,
+					     ulong addr,  u32 offset,
+					     u32 *value)
+{
+	void *header;
+	u32 *tmp;
+	int ret = 0;
+
+	header = map_sysmem(addr, info->header_size);
+	if (!header) {
+		printf("ERROR: can't map header image pre-load sig\n");
+		ret = -1;
+		goto out;
+	}
+
+	tmp = header + offset;
+	*value = be32_to_cpu(*tmp);
+
+	unmap_sysmem(header);
+
+ out:
+	return ret;
+}
+
+static int image_pre_load_sig_get_magic(struct image_sig_info *info,
+					ulong addr, u32 *magic)
+{
+	int ret;
+
+	ret = image_pre_load_sig_get_header_u32(info, addr,
+						IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC, magic);
+
+	return ret;
+}
+
+static int image_pre_load_sig_get_img_len(struct image_sig_info *info,
+					  ulong addr, u32 *len)
+{
+	int ret;
+
+	ret = image_pre_load_sig_get_header_u32(info, addr,
+						IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN, len);
+	if (ret < 0)
+		goto out;
+
+	if (*len > CONFIG_SYS_BOOTM_LEN) {
+		printf("ERROR: size of image (%u) bigger than CONFIG_SYS_BOOTM_LEN (%u)\n",
+		       *len, CONFIG_SYS_BOOTM_LEN);
+		ret = -1;
+		goto out;
+	}
+
+	if (*len == 0) {
+		printf("ERROR: size of image (%u) is zero\n", *len);
+		ret = -1;
+		goto out;
+	}
+
+ out:
+	return ret;
+}
+
+static int image_pre_load_sig_check(struct image_sig_info *info, ulong addr, int img_len)
+{
+	void *image;
+	struct image_sign_info sig_info;
+	struct image_region reg;
+	u32 sig_len;
+	u8 *sig;
+	int ret = 0;
+
+	image = (void *)map_sysmem(addr, info->header_size + img_len);
+	if (!image) {
+		printf("ERROR: can't map full image\n");
+		ret = -1;
+		goto out;
+	}
+
+	memset(&sig_info, 0, sizeof(sig_info));
+	sig_info.name = info->algo_name;
+	sig_info.padding = image_get_padding_algo(info->padding_name);
+	sig_info.checksum = image_get_checksum_algo(sig_info.name);
+	sig_info.crypto = image_get_crypto_algo(sig_info.name);
+	sig_info.key = info->key;
+	sig_info.keylen = info->key_len;
+
+	reg.data = image + info->header_size;
+	reg.size = img_len;
+
+	sig = (uint8_t *)image + IMAGE_PRE_LOAD_SIG_OFFSET_SIG;
+	sig_len = info->sig_size;
+
+	ret = sig_info.crypto->verify(&sig_info, &reg, 1, sig, sig_len);
+	if (ret) {
+		printf("ERROR: signature check has failed (err=%d)\n", ret);
+		ret = -1;
+		goto out_unmap;
+	}
+
+	printf("INFO: signature check has succeed\n");
+
+out_unmap:
+	unmap_sysmem(image);
+
+ out:
+	return ret;
+}
+
+int image_pre_load_sig(ulong addr)
+{
+	struct image_sig_info info;
+	u32 magic, img_len;
+	int ret;
+
+	ret = image_pre_load_sig_setup(&info);
+	if (ret < 0)
+		goto out;
+	if (ret > 0) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = image_pre_load_sig_get_magic(&info, addr, &magic);
+	if (ret < 0)
+		goto out;
+
+	if (magic != IMAGE_PRE_LOAD_SIG_MAGIC) {
+		if (info.mandatory) {
+			printf("ERROR: signature is mandatory\n");
+			ret = -1;
+		}
+		goto out;
+	}
+
+	ret = image_pre_load_sig_get_img_len(&info, addr, &img_len);
+	if (ret < 0)
+		goto out;
+
+	ret = image_pre_load_sig_check(&info, addr, img_len);
+
+	if (!ret)
+		image_load_offset += info.header_size;
+
+ out:
+	return ret;
+}
+
+int image_pre_load(ulong addr)
+{
+	int ret = 0;
+
+	image_load_offset = 0;
+
+	if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD_SIG))
+		ret = image_pre_load_sig(addr);
+
+	return ret;
+}
diff --git a/include/image.h b/include/image.h
index fd662e74b4..5f83e4c747 100644
--- a/include/image.h
+++ b/include/image.h
@@ -48,6 +48,7 @@ struct fdt_region;
 extern ulong image_load_addr;		/* Default Load Address */
 extern ulong image_save_addr;		/* Default Save Address */
 extern ulong image_save_size;		/* Default Save Size */
+extern ulong image_load_offset;	/* Default Load Address Offset */
 
 /* An invalid size, meaning that the image size is not known */
 #define IMAGE_SIZE_INVAL	(-1UL)
@@ -1289,6 +1290,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name);
  */
 struct padding_algo *image_get_padding_algo(const char *name);
 
+/**
+ * image_pre_load() - Manage pre load header
+ *
+ * @param addr		Address of the image
+ * @return: 0 on success, -ve on error
+ */
+int image_pre_load(ulong addr);
+
 /**
  * fit_image_verify_required_sigs() - Verify signatures marked as 'required'
  *
-- 
2.17.1

[RFC PATCH v3 5/8] cmd: bootm: add a stage pre-load

[Philippe Reynes]

This commit adds a stage pre-load to the command
bootm. Right now, this stage may be used to read
a header and check the signature of the full
image.

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 boot/bootm.c    | 33 +++++++++++++++++++++++++++++++++
 cmd/Kconfig     | 10 ++++++++++
 cmd/bootm.c     |  2 +-
 include/image.h |  1 +
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 4482f84b40..4803c577cc 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -87,6 +87,33 @@ static int bootm_start(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
+static ulong bootm_data_addr(int argc, char *const argv[])
+{
+	ulong addr;
+
+	if (argc > 0)
+		addr = simple_strtoul(argv[0], NULL, 16);
+	else
+		addr = image_load_addr;
+
+	return addr;
+}
+
+static int bootm_pre_load(struct cmd_tbl *cmdtp, int flag, int argc,
+			  char *const argv[])
+{
+	ulong data_addr = bootm_data_addr(argc, argv);
+	int ret = 0;
+
+	if (CONFIG_IS_ENABLED(CMD_BOOTM_PRE_LOAD))
+		ret = image_pre_load(data_addr);
+
+	if (ret)
+		ret = CMD_RET_FAILURE;
+
+	return ret;
+}
+
 static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc,
 			 char *const argv[])
 {
@@ -677,6 +704,9 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (states & BOOTM_STATE_START)
 		ret = bootm_start(cmdtp, flag, argc, argv);
 
+	if (!ret && (states & BOOTM_STATE_PRE_LOAD))
+		ret = bootm_pre_load(cmdtp, flag, argc, argv);
+
 	if (!ret && (states & BOOTM_STATE_FINDOS))
 		ret = bootm_find_os(cmdtp, flag, argc, argv);
 
@@ -866,6 +896,9 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc,
 					      &fit_uname_config,
 					      &fit_uname_kernel);
 
+	if (CONFIG_IS_ENABLED(CMD_BOOTM_PRE_LOAD))
+		img_addr += image_load_offset;
+
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
 
 	/* check image type, for FIT images get FIT kernel node */
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5b30b13e43..cad2cda0bf 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -194,6 +194,16 @@ config CMD_BOOTM
 	help
 	  Boot an application image from the memory.
 
+config CMD_BOOTM_PRE_LOAD
+       bool "enable pre-load on bootm"
+       depends on CMD_BOOTM
+       depends on IMAGE_PRE_LOAD
+       default n
+       help
+         Enable support of stage pre-load for the bootm command.
+	 This stage allow to check of modifty the image provided
+	 to the bootm command.
+
 config BOOTM_EFI
 	bool "Support booting UEFI FIT images"
 	depends on CMD_BOOTEFI && CMD_BOOTM && FIT
diff --git a/cmd/bootm.c b/cmd/bootm.c
index 92468d09a1..acfb8eedde 100644
--- a/cmd/bootm.c
+++ b/cmd/bootm.c
@@ -126,7 +126,7 @@ int do_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	return do_bootm_states(cmdtp, flag, argc, argv, BOOTM_STATE_START |
-		BOOTM_STATE_FINDOS | BOOTM_STATE_FINDOTHER |
+		BOOTM_STATE_FINDOS | BOOTM_STATE_PRE_LOAD | BOOTM_STATE_FINDOTHER |
 		BOOTM_STATE_LOADOS |
 #ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
 		BOOTM_STATE_RAMDISK |
diff --git a/include/image.h b/include/image.h
index 5f83e4c747..42fb01ab07 100644
--- a/include/image.h
+++ b/include/image.h
@@ -351,6 +351,7 @@ typedef struct bootm_headers {
 #define	BOOTM_STATE_OS_PREP	(0x00000100)
 #define	BOOTM_STATE_OS_FAKE_GO	(0x00000200)	/* 'Almost' run the OS */
 #define	BOOTM_STATE_OS_GO	(0x00000400)
+#define	BOOTM_STATE_PRE_LOAD	(0x00000800)
 	int		state;
 
 #if defined(CONFIG_LMB) && !defined(USE_HOSTCC)
-- 
2.17.1

[RFC PATCH v3 6/8] common: spl: fit_ram: allow to use image pre load

[Philippe Reynes]

This commit add the support of image pre load in spl or tpl
when loading an image from ram.

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 common/spl/spl_ram.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
index df9f3a4d00..c8c7155a93 100644
--- a/common/spl/spl_ram.c
+++ b/common/spl/spl_ram.c
@@ -24,9 +24,17 @@
 static ulong spl_ram_load_read(struct spl_load_info *load, ulong sector,
 			       ulong count, void *buf)
 {
+	ulong addr;
+
 	debug("%s: sector %lx, count %lx, buf %lx\n",
 	      __func__, sector, count, (ulong)buf);
-	memcpy(buf, (void *)(CONFIG_SPL_LOAD_FIT_ADDRESS + sector), count);
+
+	addr = (ulong)CONFIG_SPL_LOAD_FIT_ADDRESS + sector;
+	if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD))
+		addr += image_load_offset;
+
+	memcpy(buf, (void *)addr, count);
+
 	return count;
 }
 
@@ -37,6 +45,17 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
 
 	header = (struct image_header *)CONFIG_SPL_LOAD_FIT_ADDRESS;
 
+	if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD)) {
+		unsigned long addr = (unsigned long)header;
+		int ret = image_pre_load(addr);
+
+		if (ret)
+			return ret;
+
+		addr += image_load_offset;
+		header = (struct image_header *)addr;
+	}
+
 #if CONFIG_IS_ENABLED(DFU)
 	if (bootdev->boot_device == BOOT_DEVICE_DFU)
 		spl_dfu_cmd(0, "dfu_alt_info_ram", "ram", "0");
-- 
2.17.1

[RFC PATCH v3 7/8] mkimage: add public key for image pre-load stage

[Philippe Reynes]

This commit enhances mkimage to update the node
/image/pre-load/sig with the public key.

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 include/image.h    |  15 ++++++
 tools/fit_image.c  |   3 ++
 tools/image-host.c | 116 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)

diff --git a/include/image.h b/include/image.h
index 42fb01ab07..ac27e7acb2 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1019,6 +1019,21 @@ int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
 
 int fit_set_timestamp(void *fit, int noffset, time_t timestamp);
 
+/**
+ * fit_pre_load_data() - add public key to fdt blob
+ *
+ * @keydir:	Directory containing keys
+ * @keydest:	FDT blob to write public key
+ * @fit:	Pointer to the FIT format image header
+ *
+ * Adds public key to the node pre load.
+ *
+ * returns:
+ *	0, on success
+ *	< 0, on failure
+ */
+int fit_pre_load_data(const char *keydir, void *keydest, void *fit);
+
 int fit_cipher_data(const char *keydir, void *keydest, void *fit,
 		    const char *comment, int require_keys,
 		    const char *engine_id, const char *cmdname);
diff --git a/tools/fit_image.c b/tools/fit_image.c
index f4f372ba62..43ce41efbe 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -59,6 +59,9 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 		ret = fit_set_timestamp(ptr, 0, time);
 	}
 
+	if (!ret)
+		ret = fit_pre_load_data(params->keydir, dest_blob, ptr);
+
 	if (!ret) {
 		ret = fit_cipher_data(params->keydir, dest_blob, ptr,
 				      params->comment,
diff --git a/tools/image-host.c b/tools/image-host.c
index a6b0a94420..20e59c14a9 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -14,6 +14,11 @@
 #include <image.h>
 #include <version.h>
 
+#include <openssl/pem.h>
+#include <openssl/evp.h>
+
+#define IMAGE_PRE_LOAD_PATH                             "/image/pre-load/sig"
+
 /**
  * fit_set_hash_value - set hash value in requested has node
  * @fit: pointer to the FIT format image header
@@ -1020,6 +1025,117 @@ static int fit_config_add_verification_data(const char *keydir,
 	return 0;
 }
 
+/*
+ * 0) open file (open)
+ * 1) read certificate (PEM_read_X509)
+ * 2) get public key (X509_get_pubkey)
+ * 3) provide der format (d2i_RSAPublicKey)
+ */
+static int read_pub_key(const char *keydir, const void *name,
+			unsigned char **pubkey, int *pubkey_len)
+{
+	char path[1024];
+	EVP_PKEY *key = NULL;
+	X509 *cert;
+	FILE *f;
+	int ret;
+
+	memset(path, 0, 1024);
+	snprintf(path, sizeof(path), "%s/%s.crt", keydir, (char *)name);
+
+	/* Open certificate file */
+	f = fopen(path, "r");
+	if (!f) {
+		fprintf(stderr, "Couldn't open RSA certificate: '%s': %s\n",
+			path, strerror(errno));
+		return -EACCES;
+	}
+
+	/* Read the certificate */
+	cert = NULL;
+	if (!PEM_read_X509(f, &cert, NULL, NULL)) {
+		printf("Couldn't read certificate");
+		ret = -EINVAL;
+		goto err_cert;
+	}
+
+	/* Get the public key from the certificate. */
+	key = X509_get_pubkey(cert);
+	if (!key) {
+		printf("Couldn't read public key\n");
+		ret = -EINVAL;
+		goto err_pubkey;
+	}
+
+	/* Get DER form */
+	ret = i2d_PublicKey(key, pubkey);
+	if (ret < 0) {
+		printf("Couldn't get DER form\n");
+		ret = -EINVAL;
+		goto err_pubkey;
+	}
+
+	*pubkey_len = ret;
+	ret = 0;
+
+err_pubkey:
+	X509_free(cert);
+err_cert:
+	fclose(f);
+	return ret;
+}
+
+int fit_pre_load_data(const char *keydir, void *keydest, void *fit)
+{
+	int pre_load_noffset;
+	const void *header_size, *algo_name;
+	const void *key_name;
+	unsigned char *pubkey = NULL;
+	int ret, pubkey_len;
+
+	if (!keydir || !keydest || !fit)
+		return 0;
+
+	/* Search node pre-load sig */
+	pre_load_noffset = fdt_path_offset(keydest, IMAGE_PRE_LOAD_PATH);
+	if (pre_load_noffset < 0) {
+		ret = 0;
+		goto out;
+	}
+
+	header_size  = fdt_getprop(keydest, pre_load_noffset, "header-size", NULL);
+	algo_name    = fdt_getprop(keydest, pre_load_noffset, "algo-name", NULL);
+	key_name     = fdt_getprop(keydest, pre_load_noffset, "key-name", NULL);
+
+	/* Check that all mandatory properties are present */
+	if (!header_size || !algo_name || !key_name) {
+		if (!header_size)
+			printf("The property header-size is missing in the node %s\n",
+			       IMAGE_PRE_LOAD_PATH);
+		if (!algo_name)
+			printf("The property algo-name is missing in the node %s\n",
+			       IMAGE_PRE_LOAD_PATH);
+		if (!key_name)
+			printf("The property key-name is missing in the node %s\n",
+			       IMAGE_PRE_LOAD_PATH);
+		ret = -ENODATA;
+		goto out;
+	}
+
+	/* Read public key */
+	ret = read_pub_key(keydir, key_name, &pubkey, &pubkey_len);
+	if (ret < 0)
+		goto out;
+
+	/* Add the public key to the device tree */
+	ret = fdt_setprop(keydest, pre_load_noffset, "public-key", pubkey, pubkey_len);
+	if (ret)
+		printf("Can't set public-key in node %s\n", IMAGE_PRE_LOAD_PATH);
+
+ out:
+	return ret;
+}
+
 int fit_cipher_data(const char *keydir, void *keydest, void *fit,
 		    const char *comment, int require_keys,
 		    const char *engine_id, const char *cmdname)
-- 
2.17.1

[RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Philippe Reynes]

This commit adds a script gen_pre_load_header.sh
that generate the header used by the image pre-load
stage.

Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)
 create mode 100755 tools/gen_pre_load_header.sh

diff --git a/tools/gen_pre_load_header.sh b/tools/gen_pre_load_header.sh
new file mode 100755
index 0000000000..8256fa80ee
--- /dev/null
+++ b/tools/gen_pre_load_header.sh
@@ -0,0 +1,174 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+
+#
+# default value
+#
+size='4096'
+algo='sha256,rsa2048'
+padding='pkcs-1.5'
+key=''
+verbose='false'
+input=''
+output=''
+
+usage() {
+	printf "Usage: $0 -a <algo> -k <key> [-p <padding>] [-s <size>] [-v] -i <input> -o <output>\n"
+}
+
+#
+# parse arguments
+#
+while getopts 'a:hi:k:o:p:s:v' flag; do
+	case "${flag}" in
+		a) algo="${OPTARG}" ;;
+		h) usage
+		   exit 0 ;;
+		i) input="${OPTARG}" ;;
+		k) key="${OPTARG}" ;;
+		o) output="${OPTARG}" ;;
+		p) padding="${OPTARG}" ;;
+		s) size="${OPTARG}" ;;
+		v) verbose='true' ;;
+		*) usage
+		   exit 1 ;;
+	esac
+done
+
+#
+# check that mandatory arguments are provided
+#
+if [ -z "$key" -o -z "$input" -o -z "$output" ]
+then
+	usage
+	exit 0
+fi
+
+hash=$(echo $algo | cut -d',' -f1)
+sign=$(echo $algo | cut -d',' -f2)
+
+echo "status:"
+echo "size    = $size"
+echo "algo    = $algo"
+echo "hash    = $hash"
+echo "sign    = $sign"
+echo "padding = $padding"
+echo "key     = $key"
+echo "verbose = $verbose"
+
+#
+# check if input file exist
+#
+if [ ! -f "$input" ]
+then
+	echo "Error: file '$input' doesn't exist"
+	exit 1
+fi
+
+#
+# check if output is not empty
+#
+if [ -z "$output" ]
+then
+	echo "Error: output is empty"
+	exit 1
+fi
+
+#
+# check that size is bigger than 0
+#
+if [ $size -le 0 ]
+then
+	echo "Error: $size lower than 0"
+	exit 1
+fi
+
+#
+# check if the key file exist
+#
+if [ ! -f "$key" ]
+then
+	echo "Error: file $key doesn't exist\n"
+	exit 1
+fi
+
+#
+# check if the hash is valid and supported
+#
+print_supported_hash() {
+	echo "Supported hash:"
+	echo "- sha1"
+	echo "- sha256"
+	echo "- sha384"
+	echo "- sha512"
+}
+
+case "$hash" in
+	"sha1") hashOption="-sha1" ;;
+	"sha256") hashOption="-sha256" ;;
+	"sha384") hashOption="-sha384" ;;
+	"sha512") hashOption="-sha512" ;;
+	*) echo "Error: $hash is an invalid hash"
+	   print_supported_hash
+	   exit 1;;
+esac
+
+#
+# check if the sign is valid and supported
+#
+print_supported_sign() {
+	echo "Supported sign:"
+	echo "- rsa1024"
+	echo "- rsa2048"
+	echo "- rsa4096"
+}
+
+case "$sign" in
+	"rsa1024") ;;
+	"rsa2048") ;;
+	"rsa4096") ;;
+	*) echo "Error: $sign is an invalid signature type"
+	   print_supported_sign
+	   exit 1;;
+esac
+
+#
+# check if the padding is valid and supported
+#
+print_supported_padding() {
+	echo "Supported padding:"
+	echo "- pkcs-1.5"
+	echo "- pss"
+}
+
+case "$padding" in
+	"pkcs-1.5") optionPadding='' ;;
+	"pss") optionPadding='-sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-2' ;;
+	*) echo "Error: $padding is an invalid padding"
+	   print_supported_padding
+	   exit 1;;
+esac
+
+
+#
+# generate the sigature
+#
+sig=$(openssl dgst $optionHash -sign $key $optionPadding $input | xxd -p)
+
+#
+# generate the header
+#
+# 0 = magic
+# 4 = image size
+# 8 = signature
+#
+h=$(printf "%08x" 0x55425348)
+i=$(stat --printf="%s" $input)
+i=$(printf "%08x" $i)
+
+echo "$h$i$sig" | xxd -r -p > $output
+
+#
+# fill the header with '\0'  to reach the expected size
+#
+truncate -s $size $output
-- 
2.17.1

Re: [RFC PATCH v3 1/8] lib: allow to build asn1 decoder and oid registry in SPL

[Simon Glass]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds the options:
> - SPL_ASN1_DECODER
> - SPL_OID_REGISTRY
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  lib/Kconfig  | 6 ++++++
>  lib/Makefile | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 70bf8e7a46..ebff84f113 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -758,11 +758,17 @@ config ASN1_DECODER
>         help
>           Enable asn1 decoder library.
>
> +config SPL_ASN1_DECODER
> +       bool

Please add help.

While you are here could you fix up the help above? It is pretty
useless. It should mention what ASN stands for, what the library
allows and a link to some information.

> +
>  config OID_REGISTRY
>         bool
>         help
>           Enable fast lookup object identifier registry.
>
> +config SPL_OID_REGISTRY
> +       bool

help

Again that help is not very useful, please expand.

[..]

Regards,
SImon

Re: [RFC PATCH v3 2/8] lib: crypto: allow to build crypyo in SPL

[Simon Glass]

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds the options:
> - SPL_ASYMMETRIC_KEY_TYPE
> - SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> - SPL_RSA_PUBLIC_KEY_PARSER
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  lib/Makefile        |  2 +-
>  lib/crypto/Kconfig  | 15 +++++++++++++++
>  lib/crypto/Makefile | 19 +++++++++++++------
>  3 files changed, 29 insertions(+), 7 deletions(-)
>

Please add in the help.


> diff --git a/lib/Makefile b/lib/Makefile
> index 900e684d62..df70917b49 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -17,7 +17,6 @@ obj-$(CONFIG_OF_LIVE) += of_live.o
>  obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
>  obj-$(CONFIG_ARCH_AT91) += at91/
>  obj-$(CONFIG_OPTEE_LIB) += optee/
> -obj-y += crypto/
>
>  obj-$(CONFIG_AES) += aes.o
>  obj-$(CONFIG_AES) += aes/
> @@ -57,6 +56,7 @@ obj-$(CONFIG_TPM_V1) += tpm-v1.o
>  obj-$(CONFIG_TPM_V2) += tpm-v2.o
>  endif
>
> +obj-y += crypto/
>  obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
>  obj-$(CONFIG_$(SPL_)MD5) += md5.o
>  obj-$(CONFIG_ECDSA) += ecdsa/
> diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
> index 6369bafac0..9351865f2c 100644
> --- a/lib/crypto/Kconfig
> +++ b/lib/crypto/Kconfig
> @@ -8,6 +8,10 @@ menuconfig ASYMMETRIC_KEY_TYPE
>
>  if ASYMMETRIC_KEY_TYPE
>
> +config SPL_ASYMMETRIC_KEY_TYPE
> +       bool "Asymmetric (public-key cryptographic) key Support within SPL"
> +       depends on SPL
> +
>  config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
>         bool "Asymmetric public-key crypto algorithm subtype"
>         help
> @@ -16,6 +20,10 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
>           appropriate hash algorithms (such as SHA-1) must be available.
>           ENOPKG will be reported if the requisite algorithm is unavailable.
>
> +config SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +       bool "Asymmetric public-key crypto algorithm subtype within SPL"
> +       depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +
>  config RSA_PUBLIC_KEY_PARSER
>         bool "RSA public key parser"
>         depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> @@ -27,6 +35,13 @@ config RSA_PUBLIC_KEY_PARSER
>           public key data and provides the ability to instantiate a public
>           key.
>
> +config SPL_RSA_PUBLIC_KEY_PARSER
> +       bool "RSA public key parser within SPL"
> +       depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +       select SPL_ASN1_DECODER
> +       select ASN1_COMPILER
> +       select SPL_OID_REGISTRY
> +
>  config X509_CERTIFICATE_PARSER
>         bool "X.509 certificate parser"
>         depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
> index f3a414525d..6792b1d4f0 100644
> --- a/lib/crypto/Makefile
> +++ b/lib/crypto/Makefile
> @@ -3,27 +3,34 @@
>  # Makefile for asymmetric cryptographic keys
>  #
>
> -obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o
> +obj-$(CONFIG_$(SPL_)ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o
>
>  asymmetric_keys-y := asymmetric_type.o
>
> -obj-$(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
> +obj-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
>
>  #
>  # RSA public key parser
>  #
> -obj-$(CONFIG_RSA_PUBLIC_KEY_PARSER) += rsa_public_key.o
> +obj-$(CONFIG_$(SPL_)RSA_PUBLIC_KEY_PARSER) += rsa_public_key.o
>  rsa_public_key-y := \
>         rsapubkey.asn1.o \
>         rsa_helper.o
>
>  $(obj)/rsapubkey.asn1.o: $(obj)/rsapubkey.asn1.c $(obj)/rsapubkey.asn1.h
> +ifdef CONFIG_SPL_BUILD
> +CFLAGS_rsapubkey.asn1.o += -I$(obj)
> +endif
> +
>  $(obj)/rsa_helper.o: $(obj)/rsapubkey.asn1.h
> +ifdef CONFIG_SPL_BUILD
> +CFLAGS_rsa_helper.o += -I$(obj)
> +endif
>
>  #
>  # X.509 Certificate handling
>  #
> -obj-$(CONFIG_X509_CERTIFICATE_PARSER) += x509_key_parser.o
> +obj-$(CONFIG_$(SPL_)X509_CERTIFICATE_PARSER) += x509_key_parser.o
>  x509_key_parser-y := \
>         x509.asn1.o \
>         x509_akid.asn1.o \
> @@ -40,11 +47,11 @@ $(obj)/x509_akid.asn1.o: $(obj)/x509_akid.asn1.c $(obj)/x509_akid.asn1.h
>  #
>  # PKCS#7 message handling
>  #
> -obj-$(CONFIG_PKCS7_MESSAGE_PARSER) += pkcs7_message.o
> +obj-$(CONFIG_$(SPL_)PKCS7_MESSAGE_PARSER) += pkcs7_message.o
>  pkcs7_message-y := \
>         pkcs7.asn1.o \
>         pkcs7_parser.o
> -obj-$(CONFIG_PKCS7_VERIFY) += pkcs7_verify.o
> +obj-$(CONFIG_$(SPL_)PKCS7_VERIFY) += pkcs7_verify.o
>
>  $(obj)/pkcs7_parser.o: $(obj)/pkcs7.asn1.h
>  $(obj)/pkcs7.asn1.o: $(obj)/pkcs7.asn1.c $(obj)/pkcs7.asn1.h
> --
> 2.17.1
>

Re: [RFC PATCH v3 3/8] lib: rsa: allow rsa verify with pkey in SPL

[Simon Glass]

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds the option SPL_RSA_VERIFY_WITH_PKEY.
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  lib/rsa/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> index 469596abe7..608d51c428 100644
> --- a/lib/rsa/Kconfig
> +++ b/lib/rsa/Kconfig
> @@ -46,6 +46,14 @@ config RSA_VERIFY_WITH_PKEY
>           directly specified in image_sign_info, where all the necessary
>           key properties will be calculated on the fly in verification code.
>
> +config SPL_RSA_VERIFY_WITH_PKEY
> +       bool "Execute RSA verification without key parameters from FDT within SPL"
> +       depends on SPL
> +       select SPL_RSA_VERIFY
> +       select SPL_ASYMMETRIC_KEY_TYPE
> +       select SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +       select SPL_RSA_PUBLIC_KEY_PARSER

missing help


> +
>  config RSA_SOFTWARE_EXP
>         bool "Enable driver for RSA Modular Exponentiation in software"
>         depends on DM
> --
> 2.17.1
>

Re: [RFC PATCH v3 4/8] boot: image: add a stage pre-load

[Simon Glass]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds a stage pre-load that could
> check or modify an image.
>
> For the moment, only a header with a signature is
> supported. This header has this format:
> - magic : 4 bytes
> - image size : 4 bytes
> - signature : n bytes
> - padding : up to header size
>
> The stage use a node /image/pre-load/sig to
> get some information:
> - header-size (mandatory) : size of the header
> - algo-name (mandatory) : name of the algo used to sign
> - padding-name : name of padding used to sign
> - signature-size : size of the signature (in the header)
> - mandatory : set to yes if this sig is mandatory
> - public-key (madatory) : value of the public key
>
> Before running the image, the stage pre-load check
> the signature provided in the header.
>
> This is an initial support, later we could add the
> support of:
> - ciphering
> - uncompressing
> - ...
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  boot/Kconfig          |  33 +++++
>  boot/Makefile         |   1 +
>  boot/image-pre-load.c | 291 ++++++++++++++++++++++++++++++++++++++++++
>  include/image.h       |   9 ++
>  4 files changed, 334 insertions(+)
>  create mode 100644 boot/image-pre-load.c

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

Please see below.

>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index d3a12be228..3856580af6 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -958,6 +958,39 @@ config AUTOBOOT_MENU_SHOW
>
>  endmenu
>
> +menu "Image support"
> +
> +config IMAGE_PRE_LOAD
> +       bool "Image pre-load support"
> +       help
> +         Enable image pre-load support
> +
> +config SPL_IMAGE_PRE_LOAD
> +       bool "Image pre-load support within SPL"
> +       depends on SPL && IMAGE_PRE_LOAD
> +       help
> +         Enable image pre-load support in SPL

Please expand all your help

> +
> +config IMAGE_PRE_LOAD_SIG
> +       bool "Image pre-load signature support"
> +       depends on IMAGE_PRE_LOAD
> +       select FIT_SIGNATURE
> +       select RSA
> +       select RSA_VERIFY_WITH_PKEY
> +       help
> +         Enable image pre-load signature support
> +
> +config SPL_IMAGE_PRE_LOAD_SIG
> +       bool "Image pre-load signature support witin SPL"
> +       depends on SPL_IMAGE_PRE_LOAD && IMAGE_PRE_LOAD_SIG
> +       select SPL_FIT_SIGNATURE
> +       select SPL_RSA
> +       select SPL_RSA_VERIFY_WITH_PKEY
> +       help
> +         Enable image pre-load signature support in SPL
> +
> +endmenu
> +
>  config USE_BOOTARGS
>         bool "Enable boot arguments"
>         help
> diff --git a/boot/Makefile b/boot/Makefile
> index 2938c3f145..59752c65ca 100644
> --- a/boot/Makefile
> +++ b/boot/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.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_)IMAGE_PRE_LOAD) += image-pre-load.o
>  obj-$(CONFIG_$(SPL_TPL_)IMAGE_SIGN_INFO) += image-sig.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += image-fit-sig.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT_CIPHER) += image-cipher.o
> diff --git a/boot/image-pre-load.c b/boot/image-pre-load.c
> new file mode 100644
> index 0000000000..6ed21c3f51
> --- /dev/null
> +++ b/boot/image-pre-load.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Philippe Reynes <philippe.reynes@softathome.com>
> + */
> +
> +#include <common.h>
> +#include <asm/global_data.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +#include <image.h>
> +#include <mapmem.h>
> +
> +#define IMAGE_PRE_LOAD_SIG_MAGIC               0x55425348
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC                0
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN      4
> +#define IMAGE_PRE_LOAD_SIG_OFFSET_SIG          8
> +
> +#define IMAGE_PRE_LOAD_PATH                    "/image/pre-load/sig"
> +#define IMAGE_PRE_LOAD_PROP_HEADER_SIZE                "header-size"
> +#define IMAGE_PRE_LOAD_PROP_ALGO_NAME          "algo-name"
> +#define IMAGE_PRE_LOAD_PROP_PADDING_NAME       "padding-name"
> +#define IMAGE_PRE_LOAD_PROP_SIG_SIZE           "signature-size"
> +#define IMAGE_PRE_LOAD_PROP_PUBLIC_KEY         "public-key"
> +#define IMAGE_PRE_LOAD_PROP_MANDATORY          "mandatory"
> +
> +#ifndef CONFIG_SYS_BOOTM_LEN
> +/* use 8MByte as default max gunzip size */
> +#define CONFIG_SYS_BOOTM_LEN   0x800000
> +#endif
> +
> +struct image_sig_header {
> +       u32 magic;
> +       u32 size;
> +       u8 *sig;
> +};
> +
> +struct image_sig_info {
> +       ulong header_size;
> +       char *algo_name;
> +       char *padding_name;
> +       u8 *key;
> +       int key_len;
> +       u32 sig_size;
> +       int mandatory;

comments

> +};
> +
> +ulong image_load_offset;
> +
> +/*
> + * This function gathers information about the signature check
> + * that could be done before launching the image.
> + *
> + * return:
> + * -1 => an error has occurred

Can you use -Exxx instead?

> + *  0 => OK
> + *  1 => no setup
> + */
> +static int image_pre_load_sig_setup(struct image_sig_info *info)
> +{
> +       const void *algo_name, *padding_name, *key, *mandatory;
> +       const u32 *header_size, *sig_size;
> +       int key_len;
> +       int node, ret = 0;
> +
> +       if (!info) {
> +               printf("ERROR: info is NULL for image pre-load sig check\n");

log_err() ?

> +               ret = -1;
> +               goto out;
> +       }
> +
> +       memset(info, 0, sizeof(*info));
> +
> +       node = fdt_path_offset(gd_fdt_blob(), IMAGE_PRE_LOAD_PATH);
> +       if (node < 0) {
> +               printf("INFO: no info for image pre-load sig check\n");
> +               ret = 1;
> +               goto out;
> +       }
> +
> +       header_size = fdt_getprop(gd_fdt_blob(), node,
> +                                 IMAGE_PRE_LOAD_PROP_HEADER_SIZE, NULL);
> +       if (!header_size) {
> +               printf("ERROR: no header-size for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       algo_name = fdt_getprop(gd_fdt_blob(), node,
> +                               IMAGE_PRE_LOAD_PROP_ALGO_NAME, NULL);
> +       if (!algo_name) {
> +               printf("ERROR: no algo_name for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       padding_name = fdt_getprop(gd_fdt_blob(), node,
> +                                  IMAGE_PRE_LOAD_PROP_PADDING_NAME, NULL);
> +       if (!padding_name) {
> +               printf("INFO: no padding_name provided, so using pkcs-1.5\n");
> +               padding_name = "pkcs-1.5";
> +       }
> +
> +       sig_size = fdt_getprop(gd_fdt_blob(), node,
> +                              IMAGE_PRE_LOAD_PROP_SIG_SIZE, NULL);
> +       if (!sig_size) {
> +               printf("ERROR: no signature-size for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       key = fdt_getprop(gd_fdt_blob(), node,
> +                         IMAGE_PRE_LOAD_PROP_PUBLIC_KEY, &key_len);
> +       if (!key) {
> +               printf("ERROR: no key for image pre-load sig check\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       info->header_size       = fdt32_to_cpu(*header_size);
> +       info->algo_name         = (char *)algo_name;
> +       info->padding_name      = (char *)padding_name;
> +       info->key               = (uint8_t *)key;
> +       info->key_len           = key_len;
> +       info->sig_size          = fdt32_to_cpu(*sig_size);
> +
> +       mandatory = fdt_getprop(gd_fdt_blob(), node,
> +                               IMAGE_PRE_LOAD_PROP_MANDATORY, NULL);
> +       if (mandatory && !strcmp((char *)mandatory, "yes"))
> +               info->mandatory = 1;
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_header_u32(struct image_sig_info *info,
> +                                            ulong addr,  u32 offset,
> +                                            u32 *value)
> +{
> +       void *header;
> +       u32 *tmp;
> +       int ret = 0;
> +
> +       header = map_sysmem(addr, info->header_size);
> +       if (!header) {
> +               printf("ERROR: can't map header image pre-load sig\n");
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       tmp = header + offset;
> +       *value = be32_to_cpu(*tmp);
> +
> +       unmap_sysmem(header);
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_magic(struct image_sig_info *info,
> +                                       ulong addr, u32 *magic)
> +{
> +       int ret;
> +
> +       ret = image_pre_load_sig_get_header_u32(info, addr,
> +                                               IMAGE_PRE_LOAD_SIG_OFFSET_MAGIC, magic);
> +
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_get_img_len(struct image_sig_info *info,
> +                                         ulong addr, u32 *len)
> +{
> +       int ret;
> +
> +       ret = image_pre_load_sig_get_header_u32(info, addr,
> +                                               IMAGE_PRE_LOAD_SIG_OFFSET_IMG_LEN, len);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (*len > CONFIG_SYS_BOOTM_LEN) {
> +               printf("ERROR: size of image (%u) bigger than CONFIG_SYS_BOOTM_LEN (%u)\n",
> +                      *len, CONFIG_SYS_BOOTM_LEN);
> +               ret = -1;
> +               goto out;
> +       }
> +
> +       if (*len == 0) {
> +               printf("ERROR: size of image (%u) is zero\n", *len);
> +               ret = -1;
> +               goto out;
> +       }
> +
> + out:
> +       return ret;
> +}
> +
> +static int image_pre_load_sig_check(struct image_sig_info *info, ulong addr, int img_len)
> +{
> +       void *image;
> +       struct image_sign_info sig_info;
> +       struct image_region reg;
> +       u32 sig_len;
> +       u8 *sig;
> +       int ret = 0;
> +
> +       image = (void *)map_sysmem(addr, info->header_size + img_len);
> +       if (!image) {
> +               printf("ERROR: can't map full image\n");
> +               ret = -1;

Please use real error codes throughout.

> +               goto out;
> +       }
> +
> +       memset(&sig_info, 0, sizeof(sig_info));

'\0'

> +       sig_info.name = info->algo_name;
> +       sig_info.padding = image_get_padding_algo(info->padding_name);
> +       sig_info.checksum = image_get_checksum_algo(sig_info.name);
> +       sig_info.crypto = image_get_crypto_algo(sig_info.name);
> +       sig_info.key = info->key;
> +       sig_info.keylen = info->key_len;
> +
> +       reg.data = image + info->header_size;
> +       reg.size = img_len;
> +
> +       sig = (uint8_t *)image + IMAGE_PRE_LOAD_SIG_OFFSET_SIG;
> +       sig_len = info->sig_size;
> +
> +       ret = sig_info.crypto->verify(&sig_info, &reg, 1, sig, sig_len);
> +       if (ret) {
> +               printf("ERROR: signature check has failed (err=%d)\n", ret);
> +               ret = -1;
> +               goto out_unmap;
> +       }
> +
> +       printf("INFO: signature check has succeed\n");
> +
> +out_unmap:
> +       unmap_sysmem(image);
> +
> + out:
> +       return ret;
> +}
> +
> +int image_pre_load_sig(ulong addr)
> +{
> +       struct image_sig_info info;
> +       u32 magic, img_len;
> +       int ret;
> +
> +       ret = image_pre_load_sig_setup(&info);
> +       if (ret < 0)
> +               goto out;
> +       if (ret > 0) {
> +               ret = 0;
> +               goto out;
> +       }
> +
> +       ret = image_pre_load_sig_get_magic(&info, addr, &magic);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (magic != IMAGE_PRE_LOAD_SIG_MAGIC) {
> +               if (info.mandatory) {
> +                       printf("ERROR: signature is mandatory\n");
> +                       ret = -1;
> +               }
> +               goto out;
> +       }
> +
> +       ret = image_pre_load_sig_get_img_len(&info, addr, &img_len);
> +       if (ret < 0)
> +               goto out;
> +
> +       ret = image_pre_load_sig_check(&info, addr, img_len);
> +
> +       if (!ret)
> +               image_load_offset += info.header_size;
> +
> + out:
> +       return ret;
> +}
> +
> +int image_pre_load(ulong addr)
> +{
> +       int ret = 0;
> +
> +       image_load_offset = 0;
> +
> +       if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD_SIG))
> +               ret = image_pre_load_sig(addr);
> +
> +       return ret;
> +}
> diff --git a/include/image.h b/include/image.h
> index fd662e74b4..5f83e4c747 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -48,6 +48,7 @@ struct fdt_region;
>  extern ulong image_load_addr;          /* Default Load Address */
>  extern ulong image_save_addr;          /* Default Save Address */
>  extern ulong image_save_size;          /* Default Save Size */
> +extern ulong image_load_offset;        /* Default Load Address Offset */
>
>  /* An invalid size, meaning that the image size is not known */
>  #define IMAGE_SIZE_INVAL       (-1UL)
> @@ -1289,6 +1290,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name);
>   */
>  struct padding_algo *image_get_padding_algo(const char *name);
>
> +/**
> + * image_pre_load() - Manage pre load header
> + *

??

Please add a description here.

> + * @param addr         Address of the image
> + * @return: 0 on success, -ve on error
> + */
> +int image_pre_load(ulong addr);
> +
>  /**
>   * fit_image_verify_required_sigs() - Verify signatures marked as 'required'
>   *
> --
> 2.17.1
>

Regards,
Simon

Re: [RFC PATCH v3 5/8] cmd: bootm: add a stage pre-load

[Simon Glass]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds a stage pre-load to the command

Add a stage...

> bootm. Right now, this stage may be used to read
> a header and check the signature of the full
> image.
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  boot/bootm.c    | 33 +++++++++++++++++++++++++++++++++
>  cmd/Kconfig     | 10 ++++++++++
>  cmd/bootm.c     |  2 +-
>  include/image.h |  1 +
>  4 files changed, 45 insertions(+), 1 deletion(-)
>

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

> diff --git a/boot/bootm.c b/boot/bootm.c
> index 4482f84b40..4803c577cc 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -87,6 +87,33 @@ static int bootm_start(struct cmd_tbl *cmdtp, int flag, int argc,
>         return 0;
>  }
>
> +static ulong bootm_data_addr(int argc, char *const argv[])
> +{
> +       ulong addr;
> +
> +       if (argc > 0)
> +               addr = simple_strtoul(argv[0], NULL, 16);
> +       else
> +               addr = image_load_addr;
> +
> +       return addr;
> +}
> +
> +static int bootm_pre_load(struct cmd_tbl *cmdtp, int flag, int argc,
> +                         char *const argv[])
> +{
> +       ulong data_addr = bootm_data_addr(argc, argv);
> +       int ret = 0;
> +
> +       if (CONFIG_IS_ENABLED(CMD_BOOTM_PRE_LOAD))
> +               ret = image_pre_load(data_addr);
> +
> +       if (ret)
> +               ret = CMD_RET_FAILURE;
> +
> +       return ret;
> +}
> +
>  static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc,
>                          char *const argv[])
>  {
> @@ -677,6 +704,9 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
>         if (states & BOOTM_STATE_START)
>                 ret = bootm_start(cmdtp, flag, argc, argv);
>
> +       if (!ret && (states & BOOTM_STATE_PRE_LOAD))
> +               ret = bootm_pre_load(cmdtp, flag, argc, argv);
> +
>         if (!ret && (states & BOOTM_STATE_FINDOS))
>                 ret = bootm_find_os(cmdtp, flag, argc, argv);
>
> @@ -866,6 +896,9 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc,
>                                               &fit_uname_config,
>                                               &fit_uname_kernel);
>
> +       if (CONFIG_IS_ENABLED(CMD_BOOTM_PRE_LOAD))
> +               img_addr += image_load_offset;
> +
>         bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
>
>         /* check image type, for FIT images get FIT kernel node */
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5b30b13e43..cad2cda0bf 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -194,6 +194,16 @@ config CMD_BOOTM
>         help
>           Boot an application image from the memory.
>
> +config CMD_BOOTM_PRE_LOAD
> +       bool "enable pre-load on bootm"
> +       depends on CMD_BOOTM
> +       depends on IMAGE_PRE_LOAD
> +       default n
> +       help
> +         Enable support of stage pre-load for the bootm command.
> +        This stage allow to check of modifty the image provided
> +        to the bootm command.

modify

> +
>  config BOOTM_EFI
>         bool "Support booting UEFI FIT images"
>         depends on CMD_BOOTEFI && CMD_BOOTM && FIT
> diff --git a/cmd/bootm.c b/cmd/bootm.c
> index 92468d09a1..acfb8eedde 100644
> --- a/cmd/bootm.c
> +++ b/cmd/bootm.c
> @@ -126,7 +126,7 @@ int do_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>         }
>
>         return do_bootm_states(cmdtp, flag, argc, argv, BOOTM_STATE_START |
> -               BOOTM_STATE_FINDOS | BOOTM_STATE_FINDOTHER |
> +               BOOTM_STATE_FINDOS | BOOTM_STATE_PRE_LOAD | BOOTM_STATE_FINDOTHER |
>                 BOOTM_STATE_LOADOS |
>  #ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
>                 BOOTM_STATE_RAMDISK |
> diff --git a/include/image.h b/include/image.h
> index 5f83e4c747..42fb01ab07 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -351,6 +351,7 @@ typedef struct bootm_headers {
>  #define        BOOTM_STATE_OS_PREP     (0x00000100)
>  #define        BOOTM_STATE_OS_FAKE_GO  (0x00000200)    /* 'Almost' run the OS */
>  #define        BOOTM_STATE_OS_GO       (0x00000400)
> +#define        BOOTM_STATE_PRE_LOAD    (0x00000800)

Drop ()

>         int             state;
>
>  #if defined(CONFIG_LMB) && !defined(USE_HOSTCC)
> --
> 2.17.1
>

Regards,
Simon

Re: [RFC PATCH v3 6/8] common: spl: fit_ram: allow to use image pre load

[Simon Glass]

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit add the support of image pre load in spl or tpl
> when loading an image from ram.

Add support for image pre-load ...

>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  common/spl/spl_ram.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)

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

Re: [RFC PATCH v3 7/8] mkimage: add public key for image pre-load stage

[Simon Glass]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit enhances mkimage to update the node
> /image/pre-load/sig with the public key.
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  include/image.h    |  15 ++++++
>  tools/fit_image.c  |   3 ++
>  tools/image-host.c | 116 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)

I'm a bit unsure about the format of the key here. Is it different
from the normal one used by U-Boot?

Regards,
Simon

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Simon Glass]

Hi,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This commit adds a script gen_pre_load_header.sh
> that generate the header used by the image pre-load
> stage.
>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100755 tools/gen_pre_load_header.sh

Please use binman to do this.

Regards,
Simon

Re: [RFC PATCH v3 0/8] image: add a stage pre-load

[Simon Glass]

Hi Philippe,

On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
<philippe.reynes@softathome.com> wrote:
>
> This serie adds a stage pre-load before launching an image.
> This stage is used to read a header before the image and
> this header contains the signature of the full image.
> So u-boot may check the full image before using any
> data of the image.
>
> Changelog:
> v3:
> - move image-pre-load.c to /boot
> - update mkimage to add public key in u-boot device tree
> - add script gen_pre_load_header.sh
> v2:
> - move the code to image-pre-load
> - add support of stage pre-load for spl
> - add support of stage pre-load on spl_ram
>
> Philippe Reynes (8):
>   lib: allow to build asn1 decoder and oid registry in SPL
>   lib: crypto: allow to build crypyo in SPL
>   lib: rsa: allow rsa verify with pkey in SPL
>   boot: image: add a stage pre-load
>   cmd: bootm: add a stage pre-load
>   common: spl: fit_ram: allow to use image pre load
>   mkimage: add public key for image pre-load stage
>   tools: gen_pre_load_header.sh: initial import
>
>  boot/Kconfig                 |  33 ++++
>  boot/Makefile                |   1 +
>  boot/bootm.c                 |  33 ++++
>  boot/image-pre-load.c        | 291 +++++++++++++++++++++++++++++++++++
>  cmd/Kconfig                  |  10 ++
>  cmd/bootm.c                  |   2 +-
>  common/spl/spl_ram.c         |  21 ++-
>  include/image.h              |  25 +++
>  lib/Kconfig                  |   6 +
>  lib/Makefile                 |   9 +-
>  lib/crypto/Kconfig           |  15 ++
>  lib/crypto/Makefile          |  19 ++-
>  lib/rsa/Kconfig              |   8 +
>  tools/fit_image.c            |   3 +
>  tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++
>  tools/image-host.c           | 116 ++++++++++++++
>  16 files changed, 755 insertions(+), 11 deletions(-)
>  create mode 100644 boot/image-pre-load.c
>  create mode 100755 tools/gen_pre_load_header.sh

Two main comments:

- Should use binman to add the header...or mkimage?
- Need to add a test, e.g. for sandbox_spl

Regards,
Simon

Re: [RFC PATCH v3 7/8] mkimage: add public key for image pre-load stage

[Philippe REYNES]

Hi Simon,

Le 25/11/2021 à 01:13, Simon Glass a écrit :
> Hi Philippe,
>
> On Wed, 17 Nov 2021 at 10:52, Philippe Reynes
> <philippe.reynes@softathome.com> wrote:
>> This commit enhances mkimage to update the node
>> /image/pre-load/sig with the public key.
>>
>> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
>> ---
>>   include/image.h    |  15 ++++++
>>   tools/fit_image.c  |   3 ++
>>   tools/image-host.c | 116 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 134 insertions(+)
> I'm a bit unsure about the format of the key here. Is it different
> from the normal one used by U-Boot?
The format used by pkey is the der format without the first 24 bytes.
For example, to create this key in a shell, I use the following commands :

openssl rsa -in private.pem -pubout -outform der -out public.der
dd if=public.der of=public.raw bs=24 skip=1

As described in the comment line 340 in the file test/lib/asn1.c.

> Regards,
> Simon
Regards,
Philippe


-- This message and any attachments herein are confidential, intended solely for the addressees and are SoftAtHome’s ownership. Any unauthorized use or dissemination is prohibited. If you are not the intended addressee of this message, please cancel it immediately and inform the sender.

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Rasmus Villemoes]

On 17/11/2021 18.52, Philippe Reynes wrote:
> This commit adds a script gen_pre_load_header.sh
> that generate the header used by the image pre-load
> stage.
> 
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100755 tools/gen_pre_load_header.sh
> 
> diff --git a/tools/gen_pre_load_header.sh b/tools/gen_pre_load_header.sh
> new file mode 100755
> index 0000000000..8256fa80ee
> --- /dev/null
> +++ b/tools/gen_pre_load_header.sh
> @@ -0,0 +1,174 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +#
> +# default value
> +#
> +size='4096'
> +algo='sha256,rsa2048'
> +padding='pkcs-1.5'
> +key=''
> +verbose='false'
> +input=''
> +output=''
> +
> +usage() {
> +	printf "Usage: $0 -a <algo> -k <key> [-p <padding>] [-s <size>] [-v] -i <input> -o <output>\n"
> +}
> +
> +#
> +# parse arguments
> +#
> +while getopts 'a:hi:k:o:p:s:v' flag; do
> +	case "${flag}" in
> +		a) algo="${OPTARG}" ;;
> +		h) usage
> +		   exit 0 ;;
> +		i) input="${OPTARG}" ;;
> +		k) key="${OPTARG}" ;;
> +		o) output="${OPTARG}" ;;
> +		p) padding="${OPTARG}" ;;
> +		s) size="${OPTARG}" ;;
> +		v) verbose='true' ;;
> +		*) usage
> +		   exit 1 ;;
> +	esac
> +done
> +
> +#
> +# check that mandatory arguments are provided
> +#
> +if [ -z "$key" -o -z "$input" -o -z "$output" ]
> +then
> +	usage
> +	exit 0
> +fi
> +
> +hash=$(echo $algo | cut -d',' -f1)
> +sign=$(echo $algo | cut -d',' -f2)
> +
> +echo "status:"
> +echo "size    = $size"
> +echo "algo    = $algo"
> +echo "hash    = $hash"
> +echo "sign    = $sign"
> +echo "padding = $padding"
> +echo "key     = $key"
> +echo "verbose = $verbose"
> +
> +#
> +# check if input file exist
> +#
> +if [ ! -f "$input" ]
> +then
> +	echo "Error: file '$input' doesn't exist"
> +	exit 1
> +fi
> +
> +#
> +# check if output is not empty
> +#
> +if [ -z "$output" ]
> +then
> +	echo "Error: output is empty"
> +	exit 1
> +fi
> +
> +#
> +# check that size is bigger than 0
> +#
> +if [ $size -le 0 ]
> +then
> +	echo "Error: $size lower than 0"
> +	exit 1
> +fi
> +
> +#
> +# check if the key file exist
> +#
> +if [ ! -f "$key" ]
> +then
> +	echo "Error: file $key doesn't exist\n"
> +	exit 1
> +fi
> +
> +#
> +# check if the hash is valid and supported
> +#
> +print_supported_hash() {
> +	echo "Supported hash:"
> +	echo "- sha1"
> +	echo "- sha256"
> +	echo "- sha384"
> +	echo "- sha512"
> +}
> +
> +case "$hash" in
> +	"sha1") hashOption="-sha1" ;;
> +	"sha256") hashOption="-sha256" ;;
> +	"sha384") hashOption="-sha384" ;;
> +	"sha512") hashOption="-sha512" ;;
> +	*) echo "Error: $hash is an invalid hash"
> +	   print_supported_hash
> +	   exit 1;;
> +esac
> +
> +#
> +# check if the sign is valid and supported
> +#
> +print_supported_sign() {
> +	echo "Supported sign:"
> +	echo "- rsa1024"
> +	echo "- rsa2048"
> +	echo "- rsa4096"
> +}
> +
> +case "$sign" in
> +	"rsa1024") ;;
> +	"rsa2048") ;;
> +	"rsa4096") ;;
> +	*) echo "Error: $sign is an invalid signature type"
> +	   print_supported_sign
> +	   exit 1;;
> +esac
> +
> +#
> +# check if the padding is valid and supported
> +#
> +print_supported_padding() {
> +	echo "Supported padding:"
> +	echo "- pkcs-1.5"
> +	echo "- pss"
> +}
> +
> +case "$padding" in
> +	"pkcs-1.5") optionPadding='' ;;
> +	"pss") optionPadding='-sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-2' ;;
> +	*) echo "Error: $padding is an invalid padding"
> +	   print_supported_padding
> +	   exit 1;;
> +esac
> +
> +
> +#
> +# generate the sigature
> +#
> +sig=$(openssl dgst $optionHash -sign $key $optionPadding $input | xxd -p)
> +
> +#
> +# generate the header
> +#
> +# 0 = magic
> +# 4 = image size
> +# 8 = signature
> +#
> +h=$(printf "%08x" 0x55425348)
> +i=$(stat --printf="%s" $input)
> +i=$(printf "%08x" $i)
> +
> +echo "$h$i$sig" | xxd -r -p > $output

So this sounds like a completely generic way of prepending a signature
to an arbitrary blob, whether that is a FIT image, a U-Boot script
wrapped in a FIT, some firmware blob or whatnot. So it sounds like it
could be generally useful, and a lot simpler than the complexity
inherent when trying to add signature data within the signed data
structure itself.

So, can we perhaps not tie it to bootm as such? It's not a problem if
bootm learns to recognize 0x55425348 as another image format and then
automatically knows how to locate the "real" image, and/or automatically
verifies it. But I'd really like to be able to

  fatload $loadaddr blabla && \
  verify $loadaddr && \
  source $loadaddr

where fatload can be any random command that gets a bunch of bytes into
memory at a specific location (tftpboot, mmc read, ubi...). Currently,
we simply don't have any sane way to verify a boot script, or random
blobs, AFAICT.

To that end, it would be nice if the header was a little more
self-describing. Something like

0 = magic
4 = header size (including padding)
8 = image size
12 = offset to image signature
16 = flags (currently enforced to 0)
20 = reserved (currently enforced to 0)
24 = signature of first 24 bytes

xx = signature of image

Why do I want the image size signed? Because I'd like to be able to
store the whole thing in a raw partition (or ubi volume, or...), where
there's no concept of "file size" available. So I'd like to be able to
read in some fixed size chunk (24+whatever I expect the signature could
be, so 4096 is certainly enough), and from that compute the whole size I
need to read. But I don't want to blindly trust the "image size" field.
So, for such a case, I'd also like a "verify header $loadaddr"
subcommand (and "verify image $loadaddr", with "verify $loadaddr" being
shorthand for doing both).

And continuing the wishlist, it could be even better if we had

  verify load $loadaddr 'mmc read %l% 0 %s512%'

i.e. we could pass a "parametrized shell command" to verify for it to
use to read in a bunch of bytes to a given address - with %l% being
substituted by the address and %s<optional unit>% by the size to load,
optionally specified in the given unit.

Rasmus

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Philippe REYNES]

Hi Rasmus,

First, thanks for the feedback.


Le 06/12/2021 à 09:23, Rasmus Villemoes a écrit :
> On 17/11/2021 18.52, Philippe Reynes wrote:
>> This commit adds a script gen_pre_load_header.sh
>> that generate the header used by the image pre-load
>> stage.
>>
>> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
>> ---
>>   tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 174 insertions(+)
>>   create mode 100755 tools/gen_pre_load_header.sh
>>
>> diff --git a/tools/gen_pre_load_header.sh b/tools/gen_pre_load_header.sh
>> new file mode 100755
>> index 0000000000..8256fa80ee
>> --- /dev/null
>> +++ b/tools/gen_pre_load_header.sh
>> @@ -0,0 +1,174 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0+
>> +
>> +#
>> +# default value
>> +#
>> +size='4096'
>> +algo='sha256,rsa2048'
>> +padding='pkcs-1.5'
>> +key=''
>> +verbose='false'
>> +input=''
>> +output=''
>> +
>> +usage() {
>> +    printf "Usage: $0 -a <algo> -k <key> [-p <padding>] [-s <size>] [-v] -i <input> -o <output>\n"
>> +}
>> +
>> +#
>> +# parse arguments
>> +#
>> +while getopts 'a:hi:k:o:p:s:v' flag; do
>> +    case "${flag}" in
>> +            a) algo="${OPTARG}" ;;
>> +            h) usage
>> +               exit 0 ;;
>> +            i) input="${OPTARG}" ;;
>> +            k) key="${OPTARG}" ;;
>> +            o) output="${OPTARG}" ;;
>> +            p) padding="${OPTARG}" ;;
>> +            s) size="${OPTARG}" ;;
>> +            v) verbose='true' ;;
>> +            *) usage
>> +               exit 1 ;;
>> +    esac
>> +done
>> +
>> +#
>> +# check that mandatory arguments are provided
>> +#
>> +if [ -z "$key" -o -z "$input" -o -z "$output" ]
>> +then
>> +    usage
>> +    exit 0
>> +fi
>> +
>> +hash=$(echo $algo | cut -d',' -f1)
>> +sign=$(echo $algo | cut -d',' -f2)
>> +
>> +echo "status:"
>> +echo "size    = $size"
>> +echo "algo    = $algo"
>> +echo "hash    = $hash"
>> +echo "sign    = $sign"
>> +echo "padding = $padding"
>> +echo "key     = $key"
>> +echo "verbose = $verbose"
>> +
>> +#
>> +# check if input file exist
>> +#
>> +if [ ! -f "$input" ]
>> +then
>> +    echo "Error: file '$input' doesn't exist"
>> +    exit 1
>> +fi
>> +
>> +#
>> +# check if output is not empty
>> +#
>> +if [ -z "$output" ]
>> +then
>> +    echo "Error: output is empty"
>> +    exit 1
>> +fi
>> +
>> +#
>> +# check that size is bigger than 0
>> +#
>> +if [ $size -le 0 ]
>> +then
>> +    echo "Error: $size lower than 0"
>> +    exit 1
>> +fi
>> +
>> +#
>> +# check if the key file exist
>> +#
>> +if [ ! -f "$key" ]
>> +then
>> +    echo "Error: file $key doesn't exist\n"
>> +    exit 1
>> +fi
>> +
>> +#
>> +# check if the hash is valid and supported
>> +#
>> +print_supported_hash() {
>> +    echo "Supported hash:"
>> +    echo "- sha1"
>> +    echo "- sha256"
>> +    echo "- sha384"
>> +    echo "- sha512"
>> +}
>> +
>> +case "$hash" in
>> +    "sha1") hashOption="-sha1" ;;
>> +    "sha256") hashOption="-sha256" ;;
>> +    "sha384") hashOption="-sha384" ;;
>> +    "sha512") hashOption="-sha512" ;;
>> +    *) echo "Error: $hash is an invalid hash"
>> +       print_supported_hash
>> +       exit 1;;
>> +esac
>> +
>> +#
>> +# check if the sign is valid and supported
>> +#
>> +print_supported_sign() {
>> +    echo "Supported sign:"
>> +    echo "- rsa1024"
>> +    echo "- rsa2048"
>> +    echo "- rsa4096"
>> +}
>> +
>> +case "$sign" in
>> +    "rsa1024") ;;
>> +    "rsa2048") ;;
>> +    "rsa4096") ;;
>> +    *) echo "Error: $sign is an invalid signature type"
>> +       print_supported_sign
>> +       exit 1;;
>> +esac
>> +
>> +#
>> +# check if the padding is valid and supported
>> +#
>> +print_supported_padding() {
>> +    echo "Supported padding:"
>> +    echo "- pkcs-1.5"
>> +    echo "- pss"
>> +}
>> +
>> +case "$padding" in
>> +    "pkcs-1.5") optionPadding='' ;;
>> +    "pss") optionPadding='-sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-2' ;;
>> +    *) echo "Error: $padding is an invalid padding"
>> +       print_supported_padding
>> +       exit 1;;
>> +esac
>> +
>> +
>> +#
>> +# generate the sigature
>> +#
>> +sig=$(openssl dgst $optionHash -sign $key $optionPadding $input | xxd -p)
>> +
>> +#
>> +# generate the header
>> +#
>> +# 0 = magic
>> +# 4 = image size
>> +# 8 = signature
>> +#
>> +h=$(printf "%08x" 0x55425348)
>> +i=$(stat --printf="%s" $input)
>> +i=$(printf "%08x" $i)
>> +
>> +echo "$h$i$sig" | xxd -r -p > $output
> So this sounds like a completely generic way of prepending a signature
> to an arbitrary blob, whether that is a FIT image, a U-Boot script
> wrapped in a FIT, some firmware blob or whatnot. So it sounds like it
> could be generally useful, and a lot simpler than the complexity
> inherent when trying to add signature data within the signed data
> structure itself.
We plan to use it with FIT, but it is very generic and may be used
with any firmware.
> So, can we perhaps not tie it to bootm as such? It's not a problem if
> bootm learns to recognize 0x55425348 as another image format and then
> automatically knows how to locate the "real" image, and/or automatically
> verifies it. But I'd really like to be able to
>
>    fatload $loadaddr blabla && \
>    verify $loadaddr && \
>    source $loadaddr
>
> where fatload can be any random command that gets a bunch of bytes into
> memory at a specific location (tftpboot, mmc read, ubi...). Currently,
> we simply don't have any sane way to verify a boot script, or random
> blobs, AFAICT.


Based on this header, it is quite easy to develop a command verify.
It wasn't planned but it is a very good idea. I will add it, in the next
version or in another series a bit after.


> To that end, it would be nice if the header was a little more
> self-describing. Something like
>
> 0 = magic
> 4 = header size (including padding)
> 8 = image size
> 12 = offset to image signature
> 16 = flags (currently enforced to 0)
> 20 = reserved (currently enforced to 0)
> 24 = signature of first 24 bytes
>
> xx = signature of image
>
> Why do I want the image size signed? Because I'd like to be able to
> store the whole thing in a raw partition (or ubi volume, or...), where
> there's no concept of "file size" available. So I'd like to be able to
> read in some fixed size chunk (24+whatever I expect the signature could
> be, so 4096 is certainly enough), and from that compute the whole size I
> need to read. But I don't want to blindly trust the "image size" field.
> So, for such a case, I'd also like a "verify header $loadaddr"
> subcommand (and "verify image $loadaddr", with "verify $loadaddr" being
> shorthand for doing both).
I understand why you want to add a signature for the header and I agree.

But if we add a signature for the header, I think that we should
sign everything (even the signature) or include a hash of the
image signature in the header.

So I would suggest something like:
0 = magic
4 = header size (including padding)
8 = image size
12 = offset to image signature
16 = version of the header
20 = flags (currently enforced to 0)
24 = reserved (currently enforced to 0)
28 = reserved (currently enforced to 0)
32 = sha256 of the signature
64 = signature of the first 64 bytes
..
xx = signature of the image

Another solution would be to keep the header size in the u-boot device
tree and
add the signature of the header at the end of the header.
It would become something like:
0 = magic
4 = image size
8 = offset to image signature
12 = version of the header
16 = flags (currently enforced to 0)
20 = reserved (currently enforced to 0)
24 = reserved (currently enforced to 0)
..
xx = signature of the image
..
header_size - sig_size = signature of the header (without the header
signature)

As you can see I also propose to add the header version.
I prefer the second solution.


Everybody agrees with this proposal ?


>
> And continuing the wishlist, it could be even better if we had
>
>    verify load $loadaddr 'mmc read %l% 0 %s512%'
>
> i.e. we could pass a "parametrized shell command" to verify for it to
> use to read in a bunch of bytes to a given address - with %l% being
> substituted by the address and %s<optional unit>% by the size to load,
> optionally specified in the given unit.

I agree, it would be nice. But I think it could be done in a second step.

>
> Rasmus


Regards,
Philippe


-- This message and any attachments herein are confidential, intended solely for the addressees and are SoftAtHome’s ownership. Any unauthorized use or dissemination is prohibited. If you are not the intended addressee of this message, please cancel it immediately and inform the sender.

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Rasmus Villemoes]

On 08/12/2021 19.10, Philippe REYNES wrote:
> Hi Rasmus,
> 
> First, thanks for the feedback.
> 
> 

>>> +echo "$h$i$sig" | xxd -r -p > $output
>> So this sounds like a completely generic way of prepending a signature
>> to an arbitrary blob, whether that is a FIT image, a U-Boot script
>> wrapped in a FIT, some firmware blob or whatnot. So it sounds like it
>> could be generally useful, and a lot simpler than the complexity
>> inherent when trying to add signature data within the signed data
>> structure itself.
> We plan to use it with FIT, but it is very generic and may be used
> with any firmware.

Excellent.

>> So, can we perhaps not tie it to bootm as such? It's not a problem if
>> bootm learns to recognize 0x55425348 as another image format and then
>> automatically knows how to locate the "real" image, and/or automatically
>> verifies it. But I'd really like to be able to
>>
>>    fatload $loadaddr blabla && \
>>    verify $loadaddr && \
>>    source $loadaddr
>>
>> where fatload can be any random command that gets a bunch of bytes into
>> memory at a specific location (tftpboot, mmc read, ubi...). Currently,
>> we simply don't have any sane way to verify a boot script, or random
>> blobs, AFAICT.
> 
> 
> Based on this header, it is quite easy to develop a command verify.
> It wasn't planned but it is a very good idea. I will add it, in the next
> version or in another series a bit after.

Thanks for being open to my suggestions/ideas.

> 
>> To that end, it would be nice if the header was a little more
>> self-describing. Something like
>>
[snip]
> I understand why you want to add a signature for the header and I agree.
> 
> But if we add a signature for the header, I think that we should
> sign everything (even the signature) or include a hash of the
> image signature in the header.
> 
> So I would suggest something like:
> 0 = magic
> 4 = header size (including padding)
> 8 = image size
> 12 = offset to image signature
> 16 = version of the header
> 20 = flags (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> 28 = reserved (currently enforced to 0)
> 32 = sha256 of the signature
> 64 = signature of the first 64 bytes
> ..
> xx = signature of the image

Yes, I like this.

> Another solution would be to keep the header size in the u-boot device
> tree and
> add the signature of the header at the end of the header.

Hm, no, I don't see any reason to hardcode the header size - though it
is of course not much of a loss as one must already hardcode the public
key and the method to use for verification. However, imagine at some
point we figure out a use for flags/reserved that means the header needs
to expand. Obviously U-Boot would need to be upgraded to understand
those flags, but the upgraded U-Boot should still be able to verify old
blobs with 0 flags and the old header size. Granted, it's a bit of a
thin argument.

> It would become something like:
> 0 = magic
> 4 = image size
> 8 = offset to image signature
> 12 = version of the header
> 16 = flags (currently enforced to 0)
> 20 = reserved (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> ..
> xx = signature of the image
> ..
> header_size - sig_size = signature of the header (without the header
> signature)

Is sig_size always a known constant, only depending on the algorithm used?

> As you can see I also propose to add the header version.
> I prefer the second solution.
> 
> Everybody agrees with this proposal ?

Well, I prefer the first, but I'll leave it to you.

>>
>>    verify load $loadaddr 'mmc read %l% 0 %s512%'
>>
>> i.e. we could pass a "parametrized shell command" to verify for it to
>> use to read in a bunch of bytes to a given address - with %l% being
>> substituted by the address and %s<optional unit>% by the size to load,
>> optionally specified in the given unit.
> 
> I agree, it would be nice. But I think it could be done in a second step.

Absolutely.

Rasmus

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Simon Glass]

Hi,

On Wed, 8 Dec 2021 at 11:10, Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> Hi Rasmus,
>
> First, thanks for the feedback.
>
>
> Le 06/12/2021 à 09:23, Rasmus Villemoes a écrit :
> > On 17/11/2021 18.52, Philippe Reynes wrote:
> >> This commit adds a script gen_pre_load_header.sh
> >> that generate the header used by the image pre-load
> >> stage.
> >>
> >> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> >> ---
> >>   tools/gen_pre_load_header.sh | 174 +++++++++++++++++++++++++++++++++++
> >>   1 file changed, 174 insertions(+)
> >>   create mode 100755 tools/gen_pre_load_header.sh
> >>
> >> diff --git a/tools/gen_pre_load_header.sh b/tools/gen_pre_load_header.sh
> >> new file mode 100755
> >> index 0000000000..8256fa80ee
> >> --- /dev/null
> >> +++ b/tools/gen_pre_load_header.sh
> >> @@ -0,0 +1,174 @@
> >> +#!/bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +
> >> +#
> >> +# default value
> >> +#
> >> +size='4096'
> >> +algo='sha256,rsa2048'
> >> +padding='pkcs-1.5'
> >> +key=''
> >> +verbose='false'
> >> +input=''
> >> +output=''
> >> +
> >> +usage() {
> >> +    printf "Usage: $0 -a <algo> -k <key> [-p <padding>] [-s <size>] [-v] -i <input> -o <output>\n"
> >> +}
> >> +
> >> +#
> >> +# parse arguments
> >> +#
> >> +while getopts 'a:hi:k:o:p:s:v' flag; do
> >> +    case "${flag}" in
> >> +            a) algo="${OPTARG}" ;;
> >> +            h) usage
> >> +               exit 0 ;;
> >> +            i) input="${OPTARG}" ;;
> >> +            k) key="${OPTARG}" ;;
> >> +            o) output="${OPTARG}" ;;
> >> +            p) padding="${OPTARG}" ;;
> >> +            s) size="${OPTARG}" ;;
> >> +            v) verbose='true' ;;
> >> +            *) usage
> >> +               exit 1 ;;
> >> +    esac
> >> +done
> >> +
> >> +#
> >> +# check that mandatory arguments are provided
> >> +#
> >> +if [ -z "$key" -o -z "$input" -o -z "$output" ]
> >> +then
> >> +    usage
> >> +    exit 0
> >> +fi
> >> +
> >> +hash=$(echo $algo | cut -d',' -f1)
> >> +sign=$(echo $algo | cut -d',' -f2)
> >> +
> >> +echo "status:"
> >> +echo "size    = $size"
> >> +echo "algo    = $algo"
> >> +echo "hash    = $hash"
> >> +echo "sign    = $sign"
> >> +echo "padding = $padding"
> >> +echo "key     = $key"
> >> +echo "verbose = $verbose"
> >> +
> >> +#
> >> +# check if input file exist
> >> +#
> >> +if [ ! -f "$input" ]
> >> +then
> >> +    echo "Error: file '$input' doesn't exist"
> >> +    exit 1
> >> +fi
> >> +
> >> +#
> >> +# check if output is not empty
> >> +#
> >> +if [ -z "$output" ]
> >> +then
> >> +    echo "Error: output is empty"
> >> +    exit 1
> >> +fi
> >> +
> >> +#
> >> +# check that size is bigger than 0
> >> +#
> >> +if [ $size -le 0 ]
> >> +then
> >> +    echo "Error: $size lower than 0"
> >> +    exit 1
> >> +fi
> >> +
> >> +#
> >> +# check if the key file exist
> >> +#
> >> +if [ ! -f "$key" ]
> >> +then
> >> +    echo "Error: file $key doesn't exist\n"
> >> +    exit 1
> >> +fi
> >> +
> >> +#
> >> +# check if the hash is valid and supported
> >> +#
> >> +print_supported_hash() {
> >> +    echo "Supported hash:"
> >> +    echo "- sha1"
> >> +    echo "- sha256"
> >> +    echo "- sha384"
> >> +    echo "- sha512"
> >> +}
> >> +
> >> +case "$hash" in
> >> +    "sha1") hashOption="-sha1" ;;
> >> +    "sha256") hashOption="-sha256" ;;
> >> +    "sha384") hashOption="-sha384" ;;
> >> +    "sha512") hashOption="-sha512" ;;
> >> +    *) echo "Error: $hash is an invalid hash"
> >> +       print_supported_hash
> >> +       exit 1;;
> >> +esac
> >> +
> >> +#
> >> +# check if the sign is valid and supported
> >> +#
> >> +print_supported_sign() {
> >> +    echo "Supported sign:"
> >> +    echo "- rsa1024"
> >> +    echo "- rsa2048"
> >> +    echo "- rsa4096"
> >> +}
> >> +
> >> +case "$sign" in
> >> +    "rsa1024") ;;
> >> +    "rsa2048") ;;
> >> +    "rsa4096") ;;
> >> +    *) echo "Error: $sign is an invalid signature type"
> >> +       print_supported_sign
> >> +       exit 1;;
> >> +esac
> >> +
> >> +#
> >> +# check if the padding is valid and supported
> >> +#
> >> +print_supported_padding() {
> >> +    echo "Supported padding:"
> >> +    echo "- pkcs-1.5"
> >> +    echo "- pss"
> >> +}
> >> +
> >> +case "$padding" in
> >> +    "pkcs-1.5") optionPadding='' ;;
> >> +    "pss") optionPadding='-sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-2' ;;
> >> +    *) echo "Error: $padding is an invalid padding"
> >> +       print_supported_padding
> >> +       exit 1;;
> >> +esac
> >> +
> >> +
> >> +#
> >> +# generate the sigature
> >> +#
> >> +sig=$(openssl dgst $optionHash -sign $key $optionPadding $input | xxd -p)
> >> +
> >> +#
> >> +# generate the header
> >> +#
> >> +# 0 = magic
> >> +# 4 = image size
> >> +# 8 = signature
> >> +#
> >> +h=$(printf "%08x" 0x55425348)
> >> +i=$(stat --printf="%s" $input)
> >> +i=$(printf "%08x" $i)
> >> +
> >> +echo "$h$i$sig" | xxd -r -p > $output
> > So this sounds like a completely generic way of prepending a signature
> > to an arbitrary blob, whether that is a FIT image, a U-Boot script
> > wrapped in a FIT, some firmware blob or whatnot. So it sounds like it
> > could be generally useful, and a lot simpler than the complexity
> > inherent when trying to add signature data within the signed data
> > structure itself.
> We plan to use it with FIT, but it is very generic and may be used
> with any firmware.
> > So, can we perhaps not tie it to bootm as such? It's not a problem if
> > bootm learns to recognize 0x55425348 as another image format and then
> > automatically knows how to locate the "real" image, and/or automatically
> > verifies it. But I'd really like to be able to
> >
> >    fatload $loadaddr blabla && \
> >    verify $loadaddr && \
> >    source $loadaddr
> >
> > where fatload can be any random command that gets a bunch of bytes into
> > memory at a specific location (tftpboot, mmc read, ubi...). Currently,
> > we simply don't have any sane way to verify a boot script, or random
> > blobs, AFAICT.
>
>
> Based on this header, it is quite easy to develop a command verify.
> It wasn't planned but it is a very good idea. I will add it, in the next
> version or in another series a bit after.
>
>
> > To that end, it would be nice if the header was a little more
> > self-describing. Something like
> >
> > 0 = magic
> > 4 = header size (including padding)
> > 8 = image size
> > 12 = offset to image signature
> > 16 = flags (currently enforced to 0)
> > 20 = reserved (currently enforced to 0)
> > 24 = signature of first 24 bytes
> >
> > xx = signature of image
> >
> > Why do I want the image size signed? Because I'd like to be able to
> > store the whole thing in a raw partition (or ubi volume, or...), where
> > there's no concept of "file size" available. So I'd like to be able to
> > read in some fixed size chunk (24+whatever I expect the signature could
> > be, so 4096 is certainly enough), and from that compute the whole size I
> > need to read. But I don't want to blindly trust the "image size" field.
> > So, for such a case, I'd also like a "verify header $loadaddr"
> > subcommand (and "verify image $loadaddr", with "verify $loadaddr" being
> > shorthand for doing both).
> I understand why you want to add a signature for the header and I agree.
>
> But if we add a signature for the header, I think that we should
> sign everything (even the signature) or include a hash of the
> image signature in the header.
>
> So I would suggest something like:
> 0 = magic
> 4 = header size (including padding)
> 8 = image size
> 12 = offset to image signature
> 16 = version of the header
> 20 = flags (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> 28 = reserved (currently enforced to 0)
> 32 = sha256 of the signature
> 64 = signature of the first 64 bytes
> ..
> xx = signature of the image
>
> Another solution would be to keep the header size in the u-boot device
> tree and
> add the signature of the header at the end of the header.
> It would become something like:
> 0 = magic
> 4 = image size
> 8 = offset to image signature
> 12 = version of the header
> 16 = flags (currently enforced to 0)
> 20 = reserved (currently enforced to 0)
> 24 = reserved (currently enforced to 0)
> ..
> xx = signature of the image
> ..
> header_size - sig_size = signature of the header (without the header
> signature)
>
> As you can see I also propose to add the header version.
> I prefer the second solution.
>
>
> Everybody agrees with this proposal ?

So long as this is not a shell script and is done with a binman entry, yes.

But why not use struct image_header, if we are going to have this as
an ad-hoc thing?

Regards,
Simon


>
>
> >
> > And continuing the wishlist, it could be even better if we had
> >
> >    verify load $loadaddr 'mmc read %l% 0 %s512%'
> >
> > i.e. we could pass a "parametrized shell command" to verify for it to
> > use to read in a bunch of bytes to a given address - with %l% being
> > substituted by the address and %s<optional unit>% by the size to load,
> > optionally specified in the given unit.
>
> I agree, it would be nice. But I think it could be done in a second step.
>
> >
> > Rasmus
>
>
> Regards,
> Philippe
>
>
> -- This message and any attachments herein are confidential, intended solely for the addressees and are SoftAtHome’s ownership. Any unauthorized use or dissemination is prohibited. If you are not the intended addressee of this message, please cancel it immediately and inform the sender.

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Rasmus Villemoes]

On 10/12/2021 01.14, Simon Glass wrote:
> Hi,
> 
> On Wed, 8 Dec 2021 at 11:10, Philippe REYNES
> <philippe.reynes@softathome.com> wrote:
>>

>>
>> Everybody agrees with this proposal ?
> 
> So long as this is not a shell script and is done with a binman entry, yes.

No, no and please no. Binman really isn't as wonderful and magic as you
think. Having a simple, well-defined format that can be generated _even
with a simple shell script_ is a good thing, because it means I can go
generate such a wrapper around any random binary artifact I may have
somewhere, completely outside the U-Boot repo or the context of the
U-Boot recipe. That will, in fact, be the most common case. I should not
have to add random noise to blabla-binman.dtsi to get the U-Boot build
to wrap my FIT image in the proper format, adding weird inter-recipe
dependencies between the kernel and U-Boot builds.

Rasmus

Re: [RFC PATCH v3 8/8] tools: gen_pre_load_header.sh: initial import

[Simon Glass]

Hi Rasmus,

On Fri, 10 Dec 2021 at 00:41, Rasmus Villemoes <rasmus.villemoes@prevas.dk>
wrote:
>
> On 10/12/2021 01.14, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 8 Dec 2021 at 11:10, Philippe REYNES
> > <philippe.reynes@softathome.com> wrote:
> >>
>
> >>
> >> Everybody agrees with this proposal ?
> >
> > So long as this is not a shell script and is done with a binman entry,
yes.
>
> No, no and please no. Binman really isn't as wonderful and magic as you
> think. Having a simple, well-defined format that can be generated _even
> with a simple shell script_ is a good thing, because it means I can go
> generate such a wrapper around any random binary artifact I may have
> somewhere, completely outside the U-Boot repo or the context of the
> U-Boot recipe. That will, in fact, be the most common case. I should not
> have to add random noise to blabla-binman.dtsi to get the U-Boot build
> to wrap my FIT image in the proper format, adding weird inter-recipe
> dependencies between the kernel and U-Boot builds.

That's fine if you want a separate tool - we have mkimage and various
others and we don't want to put all these things in Binman. But the shell
script makes no sense. You should write it in C and add a test. When people
do use Binman to bring things together, it can then still call the tool.

REgards,
Simon