[PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

The capsule signature is now part of our DTB.  This is problematic when a
user is allowed to change/fixup that DTB from U-Boots command line since he
can overwrite the signature as well.
So Instead of adding the key on the DTB, embed it in the u-boot binary it
self as part of it's .rodata.  This assumes that the U-Boot binary we load
is authenticated by a previous boot stage loader.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
changes since v1:
- added static keyword on efi_get_public_key_data()
- added missing config.h on efi_capsule_key.S 

 board/emulation/common/Makefile       |  1 -
 board/emulation/common/qemu_capsule.c | 43 ---------------------------
 include/asm-generic/sections.h        |  2 ++
 lib/efi_loader/Kconfig                |  7 +++++
 lib/efi_loader/Makefile               |  8 +++++
 lib/efi_loader/efi_capsule.c          | 18 +++++++++--
 lib/efi_loader/efi_capsule_key.S      | 17 +++++++++++
 7 files changed, 49 insertions(+), 47 deletions(-)
 delete mode 100644 board/emulation/common/qemu_capsule.c
 create mode 100644 lib/efi_loader/efi_capsule_key.S

diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile
index 7ed447a69dce..c5b452e7e341 100644
--- a/board/emulation/common/Makefile
+++ b/board/emulation/common/Makefile
@@ -2,4 +2,3 @@
 
 obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o
 obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o
-obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o
diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
deleted file mode 100644
index 6b8a87022a4c..000000000000
--- a/board/emulation/common/qemu_capsule.c
+++ /dev/null
@@ -1,43 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (c) 2020 Linaro Limited
- */
-
-#include <common.h>
-#include <efi_api.h>
-#include <efi_loader.h>
-#include <env.h>
-#include <fdtdec.h>
-#include <asm/global_data.h>
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
-{
-	const void *fdt_blob = gd->fdt_blob;
-	const void *blob;
-	const char *cnode_name = "capsule-key";
-	const char *snode_name = "signature";
-	int sig_node;
-	int len;
-
-	sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
-	if (sig_node < 0) {
-		EFI_PRINT("Unable to get signature node offset\n");
-		return -FDT_ERR_NOTFOUND;
-	}
-
-	blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
-
-	if (!blob || len < 0) {
-		EFI_PRINT("Unable to get capsule-key value\n");
-		*pkey = NULL;
-		*pkey_len = 0;
-		return -FDT_ERR_NOTFOUND;
-	}
-
-	*pkey = (void *)blob;
-	*pkey_len = len;
-
-	return 0;
-}
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 267f1db73f23..ec992b0c2e3f 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -27,6 +27,8 @@ extern char __efi_helloworld_begin[];
 extern char __efi_helloworld_end[];
 extern char __efi_var_file_begin[];
 extern char __efi_var_file_end[];
+extern char __efi_capsule_sig_begin[];
+extern char __efi_capsule_sig_end[];
 
 /* Private data used by of-platdata devices/uclasses */
 extern char __priv_data_start[], __priv_data_end[];
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 156b39152112..cf6ff2d537f4 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -213,6 +213,13 @@ config EFI_CAPSULE_AUTHENTICATE
 	  Select this option if you want to enable capsule
 	  authentication
 
+config EFI_CAPSULE_KEY_PATH
+	string "Path to .esl cert for capsule authentication"
+	depends on EFI_CAPSULE_AUTHENTICATE
+	help
+	  Provide the EFI signature list (esl) certificate used for capsule
+	  authentication
+
 config EFI_DEVICE_PATH_TO_TEXT
 	bool "Device path to text protocol"
 	default y
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index fd344cea29b0..9b369430e258 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -20,11 +20,19 @@ always += helloworld.efi
 targets += helloworld.o
 endif
 
+ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y)
+EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH))
+ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","")
+$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH)
+endif
+endif
+
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
 obj-y += efi_boottime.o
 obj-y += efi_helper.o
 obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
+obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o
 obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
 obj-y += efi_console.o
 obj-y += efi_device_path.o
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index b878e71438b8..1900a938c140 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -16,6 +16,7 @@
 #include <mapmem.h>
 #include <sort.h>
 
+#include <asm/sections.h>
 #include <crypto/pkcs7.h>
 #include <crypto/pkcs7_parser.h>
 #include <linux/err.h>
@@ -222,12 +223,23 @@ skip:
 const efi_guid_t efi_guid_capsule_root_cert_guid =
 	EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
 
+static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
+{
+	const void *blob = __efi_capsule_sig_begin;
+	const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin;
+
+	*pkey = (void *)blob;
+	*pkey_len = len;
+
+	return 0;
+}
+
 efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
 				      void **image, efi_uintn_t *image_size)
 {
 	u8 *buf;
 	int ret;
-	void *fdt_pkey, *pkey;
+	void *stored_pkey, *pkey;
 	efi_uintn_t pkey_len;
 	uint64_t monotonic_count;
 	struct efi_signature_store *truststore;
@@ -286,7 +298,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 		goto out;
 	}
 
-	ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
+	ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
 	if (ret < 0)
 		goto out;
 
@@ -294,7 +306,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 	if (!pkey)
 		goto out;
 
-	memcpy(pkey, fdt_pkey, pkey_len);
+	memcpy(pkey, stored_pkey, pkey_len);
 	truststore = efi_build_signature_store(pkey, pkey_len);
 	if (!truststore)
 		goto out;
diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S
new file mode 100644
index 000000000000..58f00b8e4bcb
--- /dev/null
+++ b/lib/efi_loader/efi_capsule_key.S
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * .esl cert for capsule authentication
+ *
+ * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org>
+ */
+
+#include <config.h>
+
+.section .rodata.capsule_key.init,"a"
+.balign 16
+.global __efi_capsule_sig_begin
+__efi_capsule_sig_begin:
+.incbin CONFIG_EFI_CAPSULE_KEY_PATH
+__efi_capsule_sig_end:
+.global __efi_capsule_sig_end
+.balign 16
-- 
2.31.1

[PATCH 2/3] mkeficapsule: Remove dtb related options

[Ilias Apalodimas]

commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb")
added a bunch of options enabling the addition of the capsule public key
in a dtb.  Since now we embedded the key in U-Boot's .rodata we don't this
this functionality anymore

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
changes since v1:
- removed 2 unused includes

 tools/mkeficapsule.c | 229 ++-----------------------------------------
 1 file changed, 7 insertions(+), 222 deletions(-)

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index de0a62898886..1c0d597a4b07 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -4,22 +4,17 @@
  *		Author: AKASHI Takahiro
  */
 
-#include <errno.h>
 #include <getopt.h>
 #include <malloc.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <unistd.h>
 #include <linux/types.h>
 
-#include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
-#include "fdt_host.h"
-
 typedef __u8 u8;
 typedef __u16 u16;
 typedef __u32 u32;
@@ -29,9 +24,6 @@ typedef __s32 s32;
 
 #define aligned_u64 __aligned_u64
 
-#define SIGNATURE_NODENAME	"signature"
-#define OVERLAY_NODENAME	"__overlay__"
-
 #ifndef __packed
 #define __packed __attribute__((packed))
 #endif
@@ -52,9 +44,6 @@ static struct option options[] = {
 	{"raw", required_argument, NULL, 'r'},
 	{"index", required_argument, NULL, 'i'},
 	{"instance", required_argument, NULL, 'I'},
-	{"dtb", required_argument, NULL, 'D'},
-	{"public key", required_argument, NULL, 'K'},
-	{"overlay", no_argument, NULL, 'O'},
 	{"help", no_argument, NULL, 'h'},
 	{NULL, 0, NULL, 0},
 };
@@ -68,187 +57,10 @@ static void print_usage(void)
 	       "\t-r, --raw <raw image>       new raw image file\n"
 	       "\t-i, --index <index>         update image index\n"
 	       "\t-I, --instance <instance>   update hardware instance\n"
-	       "\t-K, --public-key <key file> public key esl file\n"
-	       "\t-D, --dtb <dtb file>        dtb file\n"
-	       "\t-O, --overlay               the dtb file is an overlay\n"
 	       "\t-h, --help                  print a help message\n",
 	       tool_name);
 }
 
-static int fdt_add_pub_key_data(void *sptr, void *dptr, size_t key_size,
-				bool overlay)
-{
-	int parent;
-	int ov_node;
-	int frag_node;
-	int ret = 0;
-
-	if (overlay) {
-		/*
-		 * The signature would be stored in the
-		 * first fragment node of the overlay
-		 */
-		frag_node = fdt_first_subnode(dptr, 0);
-		if (frag_node == -FDT_ERR_NOTFOUND) {
-			fprintf(stderr,
-				"Couldn't find the fragment node: %s\n",
-				fdt_strerror(frag_node));
-			goto done;
-		}
-
-		ov_node = fdt_subnode_offset(dptr, frag_node, OVERLAY_NODENAME);
-		if (ov_node == -FDT_ERR_NOTFOUND) {
-			fprintf(stderr,
-				"Couldn't find the __overlay__ node: %s\n",
-				fdt_strerror(ov_node));
-			goto done;
-		}
-	} else {
-		ov_node = 0;
-	}
-
-	parent = fdt_subnode_offset(dptr, ov_node, SIGNATURE_NODENAME);
-	if (parent == -FDT_ERR_NOTFOUND) {
-		parent = fdt_add_subnode(dptr, ov_node, SIGNATURE_NODENAME);
-		if (parent < 0) {
-			ret = parent;
-			if (ret != -FDT_ERR_NOSPACE) {
-				fprintf(stderr,
-					"Couldn't create signature node: %s\n",
-					fdt_strerror(parent));
-			}
-		}
-	}
-	if (ret)
-		goto done;
-
-	/* Write the key to the FDT node */
-	ret = fdt_setprop(dptr, parent, "capsule-key",
-			  sptr, key_size);
-
-done:
-	if (ret)
-		ret = ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;
-
-	return ret;
-}
-
-static int add_public_key(const char *pkey_file, const char *dtb_file,
-			  bool overlay)
-{
-	int ret;
-	int srcfd = -1;
-	int destfd = -1;
-	void *sptr = NULL;
-	void *dptr = NULL;
-	off_t src_size;
-	struct stat pub_key;
-	struct stat dtb;
-
-	/* Find out the size of the public key */
-	srcfd = open(pkey_file, O_RDONLY);
-	if (srcfd == -1) {
-		fprintf(stderr, "%s: Can't open %s: %s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	ret = fstat(srcfd, &pub_key);
-	if (ret == -1) {
-		fprintf(stderr, "%s: Can't stat %s: %s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	src_size = pub_key.st_size;
-
-	/* mmap the public key esl file */
-	sptr = mmap(0, src_size, PROT_READ, MAP_SHARED, srcfd, 0);
-	if (sptr == MAP_FAILED) {
-		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
-			__func__, pkey_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	/* Open the dest FDT */
-	destfd = open(dtb_file, O_RDWR);
-	if (destfd == -1) {
-		fprintf(stderr, "%s: Can't open %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	ret = fstat(destfd, &dtb);
-	if (ret == -1) {
-		fprintf(stderr, "%s: Can't stat %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		goto err;
-	}
-
-	dtb.st_size += src_size + 0x30;
-	if (ftruncate(destfd, dtb.st_size)) {
-		fprintf(stderr, "%s: Can't expand %s: %s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	errno = 0;
-	/* mmap the dtb file */
-	dptr = mmap(0, dtb.st_size, PROT_READ | PROT_WRITE, MAP_SHARED,
-		    destfd, 0);
-	if (dptr == MAP_FAILED) {
-		fprintf(stderr, "%s: Failed to mmap %s:%s\n",
-			__func__, dtb_file, strerror(errno));
-		ret = -1;
-		goto err;
-	}
-
-	if (fdt_check_header(dptr)) {
-		fprintf(stderr, "%s: Invalid FDT header\n", __func__);
-		ret = -1;
-		goto err;
-	}
-
-	ret = fdt_open_into(dptr, dptr, dtb.st_size);
-	if (ret) {
-		fprintf(stderr, "%s: Cannot expand FDT: %s\n",
-			__func__, fdt_strerror(ret));
-		ret = -1;
-		goto err;
-	}
-
-	/* Copy the esl file to the expanded FDT */
-	ret = fdt_add_pub_key_data(sptr, dptr, src_size, overlay);
-	if (ret < 0) {
-		fprintf(stderr, "%s: Unable to add public key to the FDT\n",
-			__func__);
-		ret = -1;
-		goto err;
-	}
-
-	ret = 0;
-
-err:
-	if (sptr)
-		munmap(sptr, src_size);
-
-	if (dptr)
-		munmap(dptr, dtb.st_size);
-
-	if (srcfd != -1)
-		close(srcfd);
-
-	if (destfd != -1)
-		close(destfd);
-
-	return ret;
-}
-
 static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 			unsigned long index, unsigned long instance)
 {
@@ -366,22 +178,16 @@ err_1:
 int main(int argc, char **argv)
 {
 	char *file;
-	char *pkey_file;
-	char *dtb_file;
 	efi_guid_t *guid;
 	unsigned long index, instance;
 	int c, idx;
-	int ret;
-	bool overlay = false;
 
 	file = NULL;
-	pkey_file = NULL;
-	dtb_file = NULL;
 	guid = NULL;
 	index = 0;
 	instance = 0;
 	for (;;) {
-		c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);
+		c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
 		if (c == -1)
 			break;
 
@@ -408,43 +214,22 @@ int main(int argc, char **argv)
 		case 'I':
 			instance = strtoul(optarg, NULL, 0);
 			break;
-		case 'K':
-			if (pkey_file) {
-				printf("Public Key already specified\n");
-				return -1;
-			}
-			pkey_file = optarg;
-			break;
-		case 'D':
-			if (dtb_file) {
-				printf("DTB file already specified\n");
-				return -1;
-			}
-			dtb_file = optarg;
-			break;
-		case 'O':
-			overlay = true;
-			break;
 		case 'h':
 			print_usage();
 			return 0;
 		}
 	}
 
-	/* need a fit image file or raw image file */
-	if (!file && !pkey_file && !dtb_file) {
+	/* need a output file */
+	if (argc != optind + 1) {
 		print_usage();
 		exit(EXIT_FAILURE);
 	}
 
-	if (pkey_file && dtb_file) {
-		ret = add_public_key(pkey_file, dtb_file, overlay);
-		if (ret == -1) {
-			printf("Adding public key to the dtb failed\n");
-			exit(EXIT_FAILURE);
-		} else {
-			exit(EXIT_SUCCESS);
-		}
+	/* need a fit image file or raw image file */
+	if (!file) {
+		print_usage();
+		exit(EXIT_SUCCESS);
 	}
 
 	if (create_fwbin(argv[optind], file, guid, index, instance)
-- 
2.31.1

[PATCH 3/3] doc: Update CapsuleUpdate READMEs

[Ilias Apalodimas]

Since we removed embeddingg the capsule key into a .dtb and fixed
authenticated capsule updates for all boards, move the relevant
documentation in the efi file and update it accordingly

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
changes since v1:
- fixed a typo

 doc/board/emulation/qemu_capsule_update.rst | 203 --------------------
 doc/develop/uefi/uefi.rst                   | 125 ++++++++++++
 2 files changed, 125 insertions(+), 203 deletions(-)
 delete mode 100644 doc/board/emulation/qemu_capsule_update.rst

diff --git a/doc/board/emulation/qemu_capsule_update.rst b/doc/board/emulation/qemu_capsule_update.rst
deleted file mode 100644
index 0a2286d039d9..000000000000
--- a/doc/board/emulation/qemu_capsule_update.rst
+++ /dev/null
@@ -1,203 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0+
-.. Copyright (C) 2020, Linaro Limited
-
-Enabling UEFI Capsule Update feature
-------------------------------------
-
-Support has been added for the UEFI capsule update feature which
-enables updating the U-Boot image using the UEFI firmware management
-protocol (fmp). The capsules are not passed to the firmware through
-the UpdateCapsule runtime service. Instead, capsule-on-disk
-functionality is used for fetching the capsule from the EFI System
-Partition (ESP) by placing the capsule file under the
-\EFI\UpdateCapsule directory.
-
-Currently, support has been added on the QEMU ARM64 virt platform for
-updating the U-Boot binary as a raw image when the platform is booted
-in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this
-configuration, the QEMU platform needs to be booted with
-'secure=off'. The U-Boot binary placed on the first bank of the NOR
-flash at offset 0x0. The U-Boot environment is placed on the second
-NOR flash bank at offset 0x4000000.
-
-The capsule update feature is enabled with the following configuration
-settings::
-
-    CONFIG_MTD=y
-    CONFIG_FLASH_CFI_MTD=y
-    CONFIG_CMD_MTDPARTS=y
-    CONFIG_CMD_DFU=y
-    CONFIG_DFU_MTD=y
-    CONFIG_PCI_INIT_R=y
-    CONFIG_EFI_CAPSULE_ON_DISK=y
-    CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
-    CONFIG_EFI_CAPSULE_FIRMWARE=y
-    CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
-    CONFIG_EFI_CAPSULE_FMP_HEADER=y
-
-In addition, the following config needs to be disabled(QEMU ARM specific)::
-
-    CONFIG_TFABOOT
-
-The capsule file can be generated by using the tools/mkeficapsule::
-
-    $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
-
-As per the UEFI specification, the capsule file needs to be placed on
-the EFI System Partition, under the \EFI\UpdateCapsule directory. The
-EFI System Partition can be a virtio-blk-device.
-
-Before initiating the firmware update, the efi variables BootNext,
-BootXXXX and OsIndications need to be set. The BootXXXX variable needs
-to be pointing to the EFI System Partition which contains the capsule
-file. The BootNext, BootXXXX and OsIndications variables can be set
-using the following commands::
-
-    => efidebug boot add -b 0 Boot0000 virtio 0:1 <capsule_file_name>
-    => efidebug boot next 0
-    => setenv -e -nv -bs -rt -v OsIndications =0x04
-    => saveenv
-
-Finally, the capsule update can be initiated with the following
-command::
-
-    => efidebug capsule disk-update
-
-The updated U-Boot image will be booted on subsequent boot.
-
-Enabling Capsule Authentication
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-The UEFI specification defines a way of authenticating the capsule to
-be updated by verifying the capsule signature. The capsule signature
-is computed and prepended to the capsule payload at the time of
-capsule generation. This signature is then verified by using the
-public key stored as part of the X509 certificate. This certificate is
-in the form of an efi signature list (esl) file, which is embedded as
-part of the platform's device tree blob using the mkeficapsule
-utility.
-
-On the QEMU virt platforms, the device-tree is generated on the fly
-based on the devices configured. This device tree is then passed on to
-the various software components booting on the platform, including
-U-Boot. Therefore, on the QEMU virt platform, the signatute is
-embedded on an overlay. This overlay is then applied at runtime to the
-base platform device-tree. Steps needed for embedding the esl file in
-the overlay are highlighted below.
-
-The capsule authentication feature can be enabled through the
-following config, in addition to the configs listed above for capsule
-update::
-
-    CONFIG_EFI_CAPSULE_AUTHENTICATE=y
-
-The public and private keys used for the signing process are generated
-and used by the steps highlighted below::
-
-    1. Install utility commands on your host
-       * OPENSSL
-       * efitools
-
-    2. Create signing keys and certificate files on your host
-
-        $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \
-            -keyout CRT.key -out CRT.crt -nodes -days 365
-        $ cert-to-efi-sig-list CRT.crt CRT.esl
-
-        $ openssl x509 -in CRT.crt -out CRT.cer -outform DER
-        $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem
-
-        $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt
-        $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem
-
-The capsule file can be generated by using the GenerateCapsule.py
-script in EDKII::
-
-    $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
-      <capsule_file_name> --monotonic-count <val> --fw-version \
-      <val> --lsv <val> --guid \
-      e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \
-      --update-image-index <val> --signer-private-cert \
-      /path/to/CRT.pem --trusted-public-cert \
-      /path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \
-      <u-boot.bin>
-
-Place the capsule generated in the above step on the EFI System
-Partition under the EFI/UpdateCapsule directory
-
-For embedding the public key certificate, the following steps need to
-be followed::
-
-    1. Generate a skeleton overlay dts file, with a single fragment
-       node and an empty __overlay__ node
-
-       A typical skeleton overlay file will look like this
-
-       /dts-v1/;
-       /plugin/;
-
-       / {
-               fragment@0 {
-                       target-path = "/";
-                       __overlay__ {
-                       };
-               };
-       };
-
-
-    2. Convert the dts to a corresponding dtb with the following
-       command
-        ./scripts/dtc/dtc -@ -I dts -O dtb -o <ov_dtb_file_name> \
-        <dts_file>
-
-    3. Run the dtb file generated above through the mkeficapsule tool
-       in U-Boot
-        ./tools/mkeficapsule -O <pub_key.esl> -D <ov_dtb>
-
-Running the above command results in the creation of a 'signature'
-node in the dtb, under which the public key is stored as a
-'capsule-key' property. The '-O' option is to be used since the
-public key certificate(esl) file is being embedded in an overlay.
-
-The dtb file embedded with the certificate is now to be placed on an
-EFI System Partition. This would then be loaded and "merged" with the
-base platform flattened device-tree(dtb) at runtime.
-
-Build U-Boot with the following steps(QEMU ARM64)::
-
-    $ make qemu_arm64_defconfig
-    $ make menuconfig
-        Disable CONFIG_TFABOOT
-        Enable CONFIG_EFI_CAPSULE_AUTHENTICATE
-        Enable all configs needed for capsule update(listed above)
-    $ make all
-
-Boot the platform and perform the following steps on the U-Boot
-command line::
-
-    1. Enable capsule authentication by setting the following env
-       variable
-
-        => setenv capsule_authentication_enabled 1
-        => saveenv
-
-    2. Load the overlay dtb to memory and merge it with the base fdt
-
-        => fatload virtio 0:1 <$fdtovaddr> EFI/<ov_dtb_file>
-        => fdt addr $fdtcontroladdr
-        => fdt resize <size_of_ov_dtb_file>
-        => fdt apply <$fdtovaddr>
-
-    3. Set the following environment and UEFI boot variables
-
-        => setenv -e -nv -bs -rt -v OsIndications =0x04
-        => efidebug boot add -b 0 Boot0000 virtio 0:1 <capsule_file_name>
-        => efidebug boot next 0
-        => saveenv
-
-    4. Finally, the capsule update can be initiated with the following
-       command
-
-        => efidebug capsule disk-update
-
-On subsequent reboot, the platform should boot the updated U-Boot binary.
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index 4f2b8b036db8..64fe9346c7f2 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -277,6 +277,131 @@ Enable ``CONFIG_OPTEE``, ``CONFIG_CMD_OPTEE_RPMB`` and ``CONFIG_EFI_MM_COMM_TEE`
 
 [1] https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html
 
+Enabling UEFI Capsule Update feature
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Support has been added for the UEFI capsule update feature which
+enables updating the U-Boot image using the UEFI firmware management
+protocol (FMP). The capsules are not passed to the firmware through
+the UpdateCapsule runtime service. Instead, capsule-on-disk
+functionality is used for fetching the capsule from the EFI System
+Partition (ESP) by placing the capsule file under the
+\EFI\UpdateCapsule directory.
+
+The directory \EFI\UpdateCapsule is checked for capsules only within the
+EFI system partition on the device specified in the active boot option
+determined by reference to BootNext variable or BootOrder variable processing.
+The active Boot Variable is the variable with highest priority BootNext or
+within BootOrder that refers to a device found to be present. Boot variables
+in BootOrder but referring to devices not present are ignored when determining
+active boot variable.
+Before starting a capsule update make sure your capsules are installed in the
+correct ESP partition or set BootNext.
+
+Performing the update
+*********************
+
+Since U-boot doesn't currently support SetVariable at runtime there's a Kconfig
+option (CONFIG_EFI_IGNORE_OSINDICATIONS) to disable the OsIndications variable
+check. If that option is enabled just copy your capsule to \EFI\UpdateCapsule.
+
+If that option is disabled, you'll need to set the OsIndications variable with::
+
+    => setenv -e -nv -bs -rt -v OsIndications =0x04
+
+Finally, the capsule update can be initiated either by rebooting the board,
+which is the preferred method, or by issuing the following command::
+
+    => efidebug capsule disk-update
+
+**The efidebug command is should only be used during debugging/development.**
+
+Enabling Capsule Authentication
+*******************************
+
+The UEFI specification defines a way of authenticating the capsule to
+be updated by verifying the capsule signature. The capsule signature
+is computed and prepended to the capsule payload at the time of
+capsule generation. This signature is then verified by using the
+public key stored as part of the X509 certificate. This certificate is
+in the form of an efi signature list (esl) file, which is embedded as
+part of U-Boot.
+
+The capsule authentication feature can be enabled through the
+following config, in addition to the configs listed above for capsule
+update::
+
+    CONFIG_EFI_CAPSULE_AUTHENTICATE=y
+    CONFIG_EFI_CAPSULE_KEY_PATH=<path to .esl cert>
+
+The public and private keys used for the signing process are generated
+and used by the steps highlighted below::
+
+    1. Install utility commands on your host
+       * OPENSSL
+       * efitools
+
+    2. Create signing keys and certificate files on your host
+
+        $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \
+            -keyout CRT.key -out CRT.crt -nodes -days 365
+        $ cert-to-efi-sig-list CRT.crt CRT.esl
+
+        $ openssl x509 -in CRT.crt -out CRT.cer -outform DER
+        $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem
+
+        $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt
+        $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem
+
+The capsule file can be generated by using the GenerateCapsule.py
+script in EDKII::
+
+    $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
+      <capsule_file_name> --monotonic-count <val> --fw-version \
+      <val> --lsv <val> --guid \
+      e2bb9c06-70e9-4b14-97a3-5a7913176e3f --verbose \
+      --update-image-index <val> --signer-private-cert \
+      /path/to/CRT.pem --trusted-public-cert \
+      /path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \
+      <u-boot.bin>
+
+Place the capsule generated in the above step on the EFI System
+Partition under the EFI/UpdateCapsule directory
+
+Testing on QEMU
+***************
+
+Currently, support has been added on the QEMU ARM64 virt platform for
+updating the U-Boot binary as a raw image when the platform is booted
+in non-secure mode, i.e. with CONFIG_TFABOOT disabled. For this
+configuration, the QEMU platform needs to be booted with
+'secure=off'. The U-Boot binary placed on the first bank of the NOR
+flash at offset 0x0. The U-Boot environment is placed on the second
+NOR flash bank at offset 0x4000000.
+
+The capsule update feature is enabled with the following configuration
+settings::
+
+    CONFIG_MTD=y
+    CONFIG_FLASH_CFI_MTD=y
+    CONFIG_CMD_MTDPARTS=y
+    CONFIG_CMD_DFU=y
+    CONFIG_DFU_MTD=y
+    CONFIG_PCI_INIT_R=y
+    CONFIG_EFI_CAPSULE_ON_DISK=y
+    CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
+    CONFIG_EFI_CAPSULE_FIRMWARE=y
+    CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
+    CONFIG_EFI_CAPSULE_FMP_HEADER=y
+
+In addition, the following config needs to be disabled(QEMU ARM specific)::
+
+    CONFIG_TFABOOT
+
+The capsule file can be generated by using the tools/mkeficapsule::
+
+    $ mkeficapsule --raw <u-boot.bin> --index 1 <capsule_file_name>
+
 Executing the boot manager
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.31.1

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi Ilias,

On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> The capsule signature is now part of our DTB.  This is problematic when a
> user is allowed to change/fixup that DTB from U-Boots command line since he
> can overwrite the signature as well.

Just to repeat my question since it looks like I didn't get a response
on the last patch:

Do you mean with the 'fdt' command?
>
If you mean the FDT fixups, they happen to a different DT, the one
being passed to Linux.

> So Instead of adding the key on the DTB, embed it in the u-boot binary it
> self as part of it's .rodata.  This assumes that the U-Boot binary we load
> is authenticated by a previous boot stage loader.

As I mentioned, this means you need to build U-Boot from source and
include the key. I don't think that is a good idea at all. Signing
should be a separate step from building.


- Simon

>
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> changes since v1:
> - added static keyword on efi_get_public_key_data()
> - added missing config.h on efi_capsule_key.S
>
>  board/emulation/common/Makefile       |  1 -
>  board/emulation/common/qemu_capsule.c | 43 ---------------------------
>  include/asm-generic/sections.h        |  2 ++
>  lib/efi_loader/Kconfig                |  7 +++++
>  lib/efi_loader/Makefile               |  8 +++++
>  lib/efi_loader/efi_capsule.c          | 18 +++++++++--
>  lib/efi_loader/efi_capsule_key.S      | 17 +++++++++++
>  7 files changed, 49 insertions(+), 47 deletions(-)
>  delete mode 100644 board/emulation/common/qemu_capsule.c
>  create mode 100644 lib/efi_loader/efi_capsule_key.S
>
> diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile
> index 7ed447a69dce..c5b452e7e341 100644
> --- a/board/emulation/common/Makefile
> +++ b/board/emulation/common/Makefile
> @@ -2,4 +2,3 @@
>
>  obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o
>  obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o
> -obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o
> diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
> deleted file mode 100644
> index 6b8a87022a4c..000000000000
> --- a/board/emulation/common/qemu_capsule.c
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - * Copyright (c) 2020 Linaro Limited
> - */
> -
> -#include <common.h>
> -#include <efi_api.h>
> -#include <efi_loader.h>
> -#include <env.h>
> -#include <fdtdec.h>
> -#include <asm/global_data.h>
> -
> -DECLARE_GLOBAL_DATA_PTR;
> -
> -int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> -{
> -       const void *fdt_blob = gd->fdt_blob;
> -       const void *blob;
> -       const char *cnode_name = "capsule-key";
> -       const char *snode_name = "signature";
> -       int sig_node;
> -       int len;
> -
> -       sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
> -       if (sig_node < 0) {
> -               EFI_PRINT("Unable to get signature node offset\n");
> -               return -FDT_ERR_NOTFOUND;
> -       }
> -
> -       blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
> -
> -       if (!blob || len < 0) {
> -               EFI_PRINT("Unable to get capsule-key value\n");
> -               *pkey = NULL;
> -               *pkey_len = 0;
> -               return -FDT_ERR_NOTFOUND;
> -       }
> -
> -       *pkey = (void *)blob;
> -       *pkey_len = len;
> -
> -       return 0;
> -}
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 267f1db73f23..ec992b0c2e3f 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -27,6 +27,8 @@ extern char __efi_helloworld_begin[];
>  extern char __efi_helloworld_end[];
>  extern char __efi_var_file_begin[];
>  extern char __efi_var_file_end[];
> +extern char __efi_capsule_sig_begin[];
> +extern char __efi_capsule_sig_end[];
>
>  /* Private data used by of-platdata devices/uclasses */
>  extern char __priv_data_start[], __priv_data_end[];
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 156b39152112..cf6ff2d537f4 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -213,6 +213,13 @@ config EFI_CAPSULE_AUTHENTICATE
>           Select this option if you want to enable capsule
>           authentication
>
> +config EFI_CAPSULE_KEY_PATH
> +       string "Path to .esl cert for capsule authentication"
> +       depends on EFI_CAPSULE_AUTHENTICATE
> +       help
> +         Provide the EFI signature list (esl) certificate used for capsule
> +         authentication
> +
>  config EFI_DEVICE_PATH_TO_TEXT
>         bool "Device path to text protocol"
>         default y
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index fd344cea29b0..9b369430e258 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -20,11 +20,19 @@ always += helloworld.efi
>  targets += helloworld.o
>  endif
>
> +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y)
> +EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH))
> +ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","")
> +$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH)
> +endif
> +endif
> +
>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>  obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
>  obj-y += efi_boottime.o
>  obj-y += efi_helper.o
>  obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> +obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o
>  obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
>  obj-y += efi_console.o
>  obj-y += efi_device_path.o
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index b878e71438b8..1900a938c140 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -16,6 +16,7 @@
>  #include <mapmem.h>
>  #include <sort.h>
>
> +#include <asm/sections.h>
>  #include <crypto/pkcs7.h>
>  #include <crypto/pkcs7_parser.h>
>  #include <linux/err.h>
> @@ -222,12 +223,23 @@ skip:
>  const efi_guid_t efi_guid_capsule_root_cert_guid =
>         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>
> +static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> +{
> +       const void *blob = __efi_capsule_sig_begin;
> +       const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin;
> +
> +       *pkey = (void *)blob;
> +       *pkey_len = len;
> +
> +       return 0;
> +}
> +
>  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
>                                       void **image, efi_uintn_t *image_size)
>  {
>         u8 *buf;
>         int ret;
> -       void *fdt_pkey, *pkey;
> +       void *stored_pkey, *pkey;
>         efi_uintn_t pkey_len;
>         uint64_t monotonic_count;
>         struct efi_signature_store *truststore;
> @@ -286,7 +298,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>                 goto out;
>         }
>
> -       ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
> +       ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
>         if (ret < 0)
>                 goto out;
>
> @@ -294,7 +306,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>         if (!pkey)
>                 goto out;
>
> -       memcpy(pkey, fdt_pkey, pkey_len);
> +       memcpy(pkey, stored_pkey, pkey_len);
>         truststore = efi_build_signature_store(pkey, pkey_len);
>         if (!truststore)
>                 goto out;
> diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S
> new file mode 100644
> index 000000000000..58f00b8e4bcb
> --- /dev/null
> +++ b/lib/efi_loader/efi_capsule_key.S
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * .esl cert for capsule authentication
> + *
> + * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org>
> + */
> +
> +#include <config.h>
> +
> +.section .rodata.capsule_key.init,"a"
> +.balign 16
> +.global __efi_capsule_sig_begin
> +__efi_capsule_sig_begin:
> +.incbin CONFIG_EFI_CAPSULE_KEY_PATH
> +__efi_capsule_sig_end:
> +.global __efi_capsule_sig_end
> +.balign 16
> --
> 2.31.1
>

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

Hi Simon,
On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > The capsule signature is now part of our DTB.  This is problematic when a
> > user is allowed to change/fixup that DTB from U-Boots command line since he
> > can overwrite the signature as well.
>
> Just to repeat my question since it looks like I didn't get a response
> on the last patch:
>
> Do you mean with the 'fdt' command?
> >
> If you mean the FDT fixups, they happen to a different DT, the one
> being passed to Linux.

In some platforms the key is derived from the relocated DTB, which we
can overwrite. But I'll let Sughosh who figured it out explain the
details.

>
> > So Instead of adding the key on the DTB, embed it in the u-boot binary it
> > self as part of it's .rodata.  This assumes that the U-Boot binary we load
> > is authenticated by a previous boot stage loader.
>
> As I mentioned, this means you need to build U-Boot from source and
> include the key. I don't think that is a good idea at all. Signing
> should be a separate step from building.

You need this key (which is an .esl cert) to authenticate the capsule
you are going to apply.  You *sign* the capsules with an external
application (like GenerateCapsule provided by edk2 and we can also
extend uboot's mkeficapsule for that).  So we aren't signing anything
here

Thanks
/Ilias

/Ilias
>
>
> - Simon
>
> >
> > Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > changes since v1:
> > - added static keyword on efi_get_public_key_data()
> > - added missing config.h on efi_capsule_key.S
> >
> >  board/emulation/common/Makefile       |  1 -
> >  board/emulation/common/qemu_capsule.c | 43 ---------------------------
> >  include/asm-generic/sections.h        |  2 ++
> >  lib/efi_loader/Kconfig                |  7 +++++
> >  lib/efi_loader/Makefile               |  8 +++++
> >  lib/efi_loader/efi_capsule.c          | 18 +++++++++--
> >  lib/efi_loader/efi_capsule_key.S      | 17 +++++++++++
> >  7 files changed, 49 insertions(+), 47 deletions(-)
> >  delete mode 100644 board/emulation/common/qemu_capsule.c
> >  create mode 100644 lib/efi_loader/efi_capsule_key.S
> >
> > diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile
> > index 7ed447a69dce..c5b452e7e341 100644
> > --- a/board/emulation/common/Makefile
> > +++ b/board/emulation/common/Makefile
> > @@ -2,4 +2,3 @@
> >
> >  obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o
> >  obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o
> > -obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o
> > diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
> > deleted file mode 100644
> > index 6b8a87022a4c..000000000000
> > --- a/board/emulation/common/qemu_capsule.c
> > +++ /dev/null
> > @@ -1,43 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0+
> > -/*
> > - * Copyright (c) 2020 Linaro Limited
> > - */
> > -
> > -#include <common.h>
> > -#include <efi_api.h>
> > -#include <efi_loader.h>
> > -#include <env.h>
> > -#include <fdtdec.h>
> > -#include <asm/global_data.h>
> > -
> > -DECLARE_GLOBAL_DATA_PTR;
> > -
> > -int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > -{
> > -       const void *fdt_blob = gd->fdt_blob;
> > -       const void *blob;
> > -       const char *cnode_name = "capsule-key";
> > -       const char *snode_name = "signature";
> > -       int sig_node;
> > -       int len;
> > -
> > -       sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
> > -       if (sig_node < 0) {
> > -               EFI_PRINT("Unable to get signature node offset\n");
> > -               return -FDT_ERR_NOTFOUND;
> > -       }
> > -
> > -       blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
> > -
> > -       if (!blob || len < 0) {
> > -               EFI_PRINT("Unable to get capsule-key value\n");
> > -               *pkey = NULL;
> > -               *pkey_len = 0;
> > -               return -FDT_ERR_NOTFOUND;
> > -       }
> > -
> > -       *pkey = (void *)blob;
> > -       *pkey_len = len;
> > -
> > -       return 0;
> > -}
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > index 267f1db73f23..ec992b0c2e3f 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -27,6 +27,8 @@ extern char __efi_helloworld_begin[];
> >  extern char __efi_helloworld_end[];
> >  extern char __efi_var_file_begin[];
> >  extern char __efi_var_file_end[];
> > +extern char __efi_capsule_sig_begin[];
> > +extern char __efi_capsule_sig_end[];
> >
> >  /* Private data used by of-platdata devices/uclasses */
> >  extern char __priv_data_start[], __priv_data_end[];
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 156b39152112..cf6ff2d537f4 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -213,6 +213,13 @@ config EFI_CAPSULE_AUTHENTICATE
> >           Select this option if you want to enable capsule
> >           authentication
> >
> > +config EFI_CAPSULE_KEY_PATH
> > +       string "Path to .esl cert for capsule authentication"
> > +       depends on EFI_CAPSULE_AUTHENTICATE
> > +       help
> > +         Provide the EFI signature list (esl) certificate used for capsule
> > +         authentication
> > +
> >  config EFI_DEVICE_PATH_TO_TEXT
> >         bool "Device path to text protocol"
> >         default y
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index fd344cea29b0..9b369430e258 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -20,11 +20,19 @@ always += helloworld.efi
> >  targets += helloworld.o
> >  endif
> >
> > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y)
> > +EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH))
> > +ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","")
> > +$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH)
> > +endif
> > +endif
> > +
> >  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> >  obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> >  obj-y += efi_boottime.o
> >  obj-y += efi_helper.o
> >  obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > +obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o
> >  obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
> >  obj-y += efi_console.o
> >  obj-y += efi_device_path.o
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index b878e71438b8..1900a938c140 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -16,6 +16,7 @@
> >  #include <mapmem.h>
> >  #include <sort.h>
> >
> > +#include <asm/sections.h>
> >  #include <crypto/pkcs7.h>
> >  #include <crypto/pkcs7_parser.h>
> >  #include <linux/err.h>
> > @@ -222,12 +223,23 @@ skip:
> >  const efi_guid_t efi_guid_capsule_root_cert_guid =
> >         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> >
> > +static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > +{
> > +       const void *blob = __efi_capsule_sig_begin;
> > +       const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin;
> > +
> > +       *pkey = (void *)blob;
> > +       *pkey_len = len;
> > +
> > +       return 0;
> > +}
> > +
> >  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
> >                                       void **image, efi_uintn_t *image_size)
> >  {
> >         u8 *buf;
> >         int ret;
> > -       void *fdt_pkey, *pkey;
> > +       void *stored_pkey, *pkey;
> >         efi_uintn_t pkey_len;
> >         uint64_t monotonic_count;
> >         struct efi_signature_store *truststore;
> > @@ -286,7 +298,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> >                 goto out;
> >         }
> >
> > -       ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
> > +       ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
> >         if (ret < 0)
> >                 goto out;
> >
> > @@ -294,7 +306,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> >         if (!pkey)
> >                 goto out;
> >
> > -       memcpy(pkey, fdt_pkey, pkey_len);
> > +       memcpy(pkey, stored_pkey, pkey_len);
> >         truststore = efi_build_signature_store(pkey, pkey_len);
> >         if (!truststore)
> >                 goto out;
> > diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S
> > new file mode 100644
> > index 000000000000..58f00b8e4bcb
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_capsule_key.S
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * .esl cert for capsule authentication
> > + *
> > + * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > + */
> > +
> > +#include <config.h>
> > +
> > +.section .rodata.capsule_key.init,"a"
> > +.balign 16
> > +.global __efi_capsule_sig_begin
> > +__efi_capsule_sig_begin:
> > +.incbin CONFIG_EFI_CAPSULE_KEY_PATH
> > +__efi_capsule_sig_end:
> > +.global __efi_capsule_sig_end
> > +.balign 16
> > --
> > 2.31.1
> >

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Sughosh Ganu]

hi Simon,

On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:

> Hi Simon,
> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > The capsule signature is now part of our DTB.  This is problematic
> when a
> > > user is allowed to change/fixup that DTB from U-Boots command line
> since he
> > > can overwrite the signature as well.
> >
> > Just to repeat my question since it looks like I didn't get a response
> > on the last patch:
> >
> > Do you mean with the 'fdt' command?
> > >
> > If you mean the FDT fixups, they happen to a different DT, the one
> > being passed to Linux.
>
> In some platforms the key is derived from the relocated DTB, which we
> can overwrite. But I'll let Sughosh who figured it out explain the
> details.
>

On platforms where the dtb is concatenated with the u-boot image, using
CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory.
We retrieve the public key from this dtb. By default, the fdtcontroladdr
env variable is getting set to this relocated dtb address -- this address
can also be accessed using the bdinfo command. Thus the public key can be
modified before attempting the capsule update. Which is the reason why
Ilias is moving the public key to the embedded rodata section.

-sughosh


> >
> > > So Instead of adding the key on the DTB, embed it in the u-boot binary
> it
> > > self as part of it's .rodata.  This assumes that the U-Boot binary we
> load
> > > is authenticated by a previous boot stage loader.
> >
> > As I mentioned, this means you need to build U-Boot from source and
> > include the key. I don't think that is a good idea at all. Signing
> > should be a separate step from building.
>
> You need this key (which is an .esl cert) to authenticate the capsule
> you are going to apply.  You *sign* the capsules with an external
> application (like GenerateCapsule provided by edk2 and we can also
> extend uboot's mkeficapsule for that).  So we aren't signing anything
> here
>
> Thanks
> /Ilias
>
> /Ilias
> >
> >
> > - Simon
> >
> > >
> > > Reviewed-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > Tested-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > changes since v1:
> > > - added static keyword on efi_get_public_key_data()
> > > - added missing config.h on efi_capsule_key.S
> > >
> > >  board/emulation/common/Makefile       |  1 -
> > >  board/emulation/common/qemu_capsule.c | 43 ---------------------------
> > >  include/asm-generic/sections.h        |  2 ++
> > >  lib/efi_loader/Kconfig                |  7 +++++
> > >  lib/efi_loader/Makefile               |  8 +++++
> > >  lib/efi_loader/efi_capsule.c          | 18 +++++++++--
> > >  lib/efi_loader/efi_capsule_key.S      | 17 +++++++++++
> > >  7 files changed, 49 insertions(+), 47 deletions(-)
> > >  delete mode 100644 board/emulation/common/qemu_capsule.c
> > >  create mode 100644 lib/efi_loader/efi_capsule_key.S
> > >
> > > diff --git a/board/emulation/common/Makefile
> b/board/emulation/common/Makefile
> > > index 7ed447a69dce..c5b452e7e341 100644
> > > --- a/board/emulation/common/Makefile
> > > +++ b/board/emulation/common/Makefile
> > > @@ -2,4 +2,3 @@
> > >
> > >  obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o
> > >  obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o
> > > -obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o
> > > diff --git a/board/emulation/common/qemu_capsule.c
> b/board/emulation/common/qemu_capsule.c
> > > deleted file mode 100644
> > > index 6b8a87022a4c..000000000000
> > > --- a/board/emulation/common/qemu_capsule.c
> > > +++ /dev/null
> > > @@ -1,43 +0,0 @@
> > > -// SPDX-License-Identifier: GPL-2.0+
> > > -/*
> > > - * Copyright (c) 2020 Linaro Limited
> > > - */
> > > -
> > > -#include <common.h>
> > > -#include <efi_api.h>
> > > -#include <efi_loader.h>
> > > -#include <env.h>
> > > -#include <fdtdec.h>
> > > -#include <asm/global_data.h>
> > > -
> > > -DECLARE_GLOBAL_DATA_PTR;
> > > -
> > > -int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > > -{
> > > -       const void *fdt_blob = gd->fdt_blob;
> > > -       const void *blob;
> > > -       const char *cnode_name = "capsule-key";
> > > -       const char *snode_name = "signature";
> > > -       int sig_node;
> > > -       int len;
> > > -
> > > -       sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
> > > -       if (sig_node < 0) {
> > > -               EFI_PRINT("Unable to get signature node offset\n");
> > > -               return -FDT_ERR_NOTFOUND;
> > > -       }
> > > -
> > > -       blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
> > > -
> > > -       if (!blob || len < 0) {
> > > -               EFI_PRINT("Unable to get capsule-key value\n");
> > > -               *pkey = NULL;
> > > -               *pkey_len = 0;
> > > -               return -FDT_ERR_NOTFOUND;
> > > -       }
> > > -
> > > -       *pkey = (void *)blob;
> > > -       *pkey_len = len;
> > > -
> > > -       return 0;
> > > -}
> > > diff --git a/include/asm-generic/sections.h
> b/include/asm-generic/sections.h
> > > index 267f1db73f23..ec992b0c2e3f 100644
> > > --- a/include/asm-generic/sections.h
> > > +++ b/include/asm-generic/sections.h
> > > @@ -27,6 +27,8 @@ extern char __efi_helloworld_begin[];
> > >  extern char __efi_helloworld_end[];
> > >  extern char __efi_var_file_begin[];
> > >  extern char __efi_var_file_end[];
> > > +extern char __efi_capsule_sig_begin[];
> > > +extern char __efi_capsule_sig_end[];
> > >
> > >  /* Private data used by of-platdata devices/uclasses */
> > >  extern char __priv_data_start[], __priv_data_end[];
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 156b39152112..cf6ff2d537f4 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -213,6 +213,13 @@ config EFI_CAPSULE_AUTHENTICATE
> > >           Select this option if you want to enable capsule
> > >           authentication
> > >
> > > +config EFI_CAPSULE_KEY_PATH
> > > +       string "Path to .esl cert for capsule authentication"
> > > +       depends on EFI_CAPSULE_AUTHENTICATE
> > > +       help
> > > +         Provide the EFI signature list (esl) certificate used for
> capsule
> > > +         authentication
> > > +
> > >  config EFI_DEVICE_PATH_TO_TEXT
> > >         bool "Device path to text protocol"
> > >         default y
> > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > index fd344cea29b0..9b369430e258 100644
> > > --- a/lib/efi_loader/Makefile
> > > +++ b/lib/efi_loader/Makefile
> > > @@ -20,11 +20,19 @@ always += helloworld.efi
> > >  targets += helloworld.o
> > >  endif
> > >
> > > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y)
> > > +EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH))
> > > +ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","")
> > > +$(error .esl cerificate not found. Configure your
> CONFIG_EFI_CAPSULE_KEY_PATH)
> > > +endif
> > > +endif
> > > +
> > >  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > >  obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > >  obj-y += efi_boottime.o
> > >  obj-y += efi_helper.o
> > >  obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > > +obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o
> > >  obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
> > >  obj-y += efi_console.o
> > >  obj-y += efi_device_path.o
> > > diff --git a/lib/efi_loader/efi_capsule.c
> b/lib/efi_loader/efi_capsule.c
> > > index b878e71438b8..1900a938c140 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -16,6 +16,7 @@
> > >  #include <mapmem.h>
> > >  #include <sort.h>
> > >
> > > +#include <asm/sections.h>
> > >  #include <crypto/pkcs7.h>
> > >  #include <crypto/pkcs7_parser.h>
> > >  #include <linux/err.h>
> > > @@ -222,12 +223,23 @@ skip:
> > >  const efi_guid_t efi_guid_capsule_root_cert_guid =
> > >         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > >
> > > +static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > > +{
> > > +       const void *blob = __efi_capsule_sig_begin;
> > > +       const int len = __efi_capsule_sig_end -
> __efi_capsule_sig_begin;
> > > +
> > > +       *pkey = (void *)blob;
> > > +       *pkey_len = len;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  efi_status_t efi_capsule_authenticate(const void *capsule,
> efi_uintn_t capsule_size,
> > >                                       void **image, efi_uintn_t
> *image_size)
> > >  {
> > >         u8 *buf;
> > >         int ret;
> > > -       void *fdt_pkey, *pkey;
> > > +       void *stored_pkey, *pkey;
> > >         efi_uintn_t pkey_len;
> > >         uint64_t monotonic_count;
> > >         struct efi_signature_store *truststore;
> > > @@ -286,7 +298,7 @@ efi_status_t efi_capsule_authenticate(const void
> *capsule, efi_uintn_t capsule_s
> > >                 goto out;
> > >         }
> > >
> > > -       ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
> > > +       ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
> > >         if (ret < 0)
> > >                 goto out;
> > >
> > > @@ -294,7 +306,7 @@ efi_status_t efi_capsule_authenticate(const void
> *capsule, efi_uintn_t capsule_s
> > >         if (!pkey)
> > >                 goto out;
> > >
> > > -       memcpy(pkey, fdt_pkey, pkey_len);
> > > +       memcpy(pkey, stored_pkey, pkey_len);
> > >         truststore = efi_build_signature_store(pkey, pkey_len);
> > >         if (!truststore)
> > >                 goto out;
> > > diff --git a/lib/efi_loader/efi_capsule_key.S
> b/lib/efi_loader/efi_capsule_key.S
> > > new file mode 100644
> > > index 000000000000..58f00b8e4bcb
> > > --- /dev/null
> > > +++ b/lib/efi_loader/efi_capsule_key.S
> > > @@ -0,0 +1,17 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * .esl cert for capsule authentication
> > > + *
> > > + * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > + */
> > > +
> > > +#include <config.h>
> > > +
> > > +.section .rodata.capsule_key.init,"a"
> > > +.balign 16
> > > +.global __efi_capsule_sig_begin
> > > +__efi_capsule_sig_begin:
> > > +.incbin CONFIG_EFI_CAPSULE_KEY_PATH
> > > +__efi_capsule_sig_end:
> > > +.global __efi_capsule_sig_end
> > > +.balign 16
> > > --
> > > 2.31.1
> > >
>

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi Sughosh,

On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Simon,
>> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
>> >
>> > Hi Ilias,
>> >
>> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
>> > <ilias.apalodimas@linaro.org> wrote:
>> > >
>> > > The capsule signature is now part of our DTB.  This is problematic when a
>> > > user is allowed to change/fixup that DTB from U-Boots command line since he
>> > > can overwrite the signature as well.
>> >
>> > Just to repeat my question since it looks like I didn't get a response
>> > on the last patch:
>> >
>> > Do you mean with the 'fdt' command?
>> > >
>> > If you mean the FDT fixups, they happen to a different DT, the one
>> > being passed to Linux.
>>
>> In some platforms the key is derived from the relocated DTB, which we
>> can overwrite. But I'll let Sughosh who figured it out explain the
>> details.
>
>
> On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.

You should be clearer about what problem you are trying to solve. Are
you worried about a script changing the DT? Or just it being writable
in general?

U-Boot itself is relocated also, including the rodata. So are you
using the public key from the original location? What if that is not
accessible after relocation?

There is also the 'fdt addr -c' command to find the control DT. It is
not expected to be written to though. So just protect the memory to
which it is relocated, or relocate it to a place that you can protect.

If the DT is writable it will affect U-Boot's operation, since that is
where all the config is stored. There is no point in pretending that
pulling one thing out of it and protecting it will result in any sort
of improvement. This needs to be done properly.

Regards,
Simon

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

Hi Simon,

On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Simon,
> >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> > Hi Ilias,
> >> >
> >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> >> > <ilias.apalodimas@linaro.org> wrote:
> >> > >
> >> > > The capsule signature is now part of our DTB.  This is problematic when a
> >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> >> > > can overwrite the signature as well.
> >> >
> >> > Just to repeat my question since it looks like I didn't get a response
> >> > on the last patch:
> >> >
> >> > Do you mean with the 'fdt' command?
> >> > >
> >> > If you mean the FDT fixups, they happen to a different DT, the one
> >> > being passed to Linux.
> >>
> >> In some platforms the key is derived from the relocated DTB, which we
> >> can overwrite. But I'll let Sughosh who figured it out explain the
> >> details.
> >
> >
> > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
>
> You should be clearer about what problem you are trying to solve. Are
> you worried about a script changing the DT? Or just it being writable
> in general?

Being writable in general is my main concern. Doing fixup internally
from U-Boot might be something we'll always need but the ability to
completely change it doesn't play well security.

>
> U-Boot itself is relocated also, including the rodata. So are you
> using the public key from the original location? What if that is not
> accessible after relocation?

We are accessing he key from the relocated address.

>
> There is also the 'fdt addr -c' command to find the control DT. It is
> not expected to be written to though. So just protect the memory to
> which it is relocated, or relocate it to a place that you can protect.

Can you define 'protect'? The mmu support in U-Boot is kind of limited
from what I can see.  In order to protect anything we'd have to switch
the pages ro R-- or RX-.  Someone please shout if I  am wrong, but I
couldn't find code doing that in U-Boot.

>
> If the DT is writable it will affect U-Boot's operation, since that is
> where all the config is stored. There is no point in pretending that
> pulling one thing out of it and protecting it will result in any sort
> of improvement. This needs to be done properly.

Tbh I thought the relocation was done properly and the .rodata section
was either merged with .text (and was RX-) or R--.
If we fix the relocation properly, the .rodata will be read only,
while the DTB should still be on RW memory.   I understand that you
are using the DTB for some configuration on U-Boot ,but arguably the
public key of the EFI firmware you need to authenticate hardly
classifies as configuration.
Keep in mind that even if we fix the page tables permissions, on a
secure platform the command line should be disabled, since anyone
could load an arbitrary application and modify them.

Thanks
/Ilias
>
> Regards,
> Simon

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi Ilias,

On Wed, 21 Jul 2021 at 00:42, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > >>
> > >> Hi Simon,
> > >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> > >> >
> > >> > Hi Ilias,
> > >> >
> > >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > >> > <ilias.apalodimas@linaro.org> wrote:
> > >> > >
> > >> > > The capsule signature is now part of our DTB.  This is problematic when a
> > >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> > >> > > can overwrite the signature as well.
> > >> >
> > >> > Just to repeat my question since it looks like I didn't get a response
> > >> > on the last patch:
> > >> >
> > >> > Do you mean with the 'fdt' command?
> > >> > >
> > >> > If you mean the FDT fixups, they happen to a different DT, the one
> > >> > being passed to Linux.
> > >>
> > >> In some platforms the key is derived from the relocated DTB, which we
> > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > >> details.
> > >
> > >
> > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> >
> > You should be clearer about what problem you are trying to solve. Are
> > you worried about a script changing the DT? Or just it being writable
> > in general?
>
> Being writable in general is my main concern. Doing fixup internally
> from U-Boot might be something we'll always need but the ability to
> completely change it doesn't play well security.
>
> >
> > U-Boot itself is relocated also, including the rodata. So are you
> > using the public key from the original location? What if that is not
> > accessible after relocation?
>
> We are accessing he key from the relocated address.

Then in what way are you protecting it? This is so confusing. Are you
saying that you are protecting the relocated address? If so, protect
the relocated devicetree too!

>
> >
> > There is also the 'fdt addr -c' command to find the control DT. It is
> > not expected to be written to though. So just protect the memory to
> > which it is relocated, or relocate it to a place that you can protect.
>
> Can you define 'protect'? The mmu support in U-Boot is kind of limited
> from what I can see.  In order to protect anything we'd have to switch
> the pages ro R-- or RX-.  Someone please shout if I  am wrong, but I
> couldn't find code doing that in U-Boot.
>
> >
> > If the DT is writable it will affect U-Boot's operation, since that is
> > where all the config is stored. There is no point in pretending that
> > pulling one thing out of it and protecting it will result in any sort
> > of improvement. This needs to be done properly.
>
> Tbh I thought the relocation was done properly and the .rodata section
> was either merged with .text (and was RX-) or R--.
> If we fix the relocation properly, the .rodata will be read only,
> while the DTB should still be on RW memory.   I understand that you
> are using the DTB for some configuration on U-Boot ,but arguably the
> public key of the EFI firmware you need to authenticate hardly
> classifies as configuration.

What is it then?

> Keep in mind that even if we fix the page tables permissions, on a
> secure platform the command line should be disabled, since anyone
> could load an arbitrary application and modify them.

Yes we brought in CONFIG_CMDLINE for that reason about five years ago :-)

Regards,
Simon

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi Ilias,

On Thu, 22 Jul 2021 at 07:28, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 21 Jul 2021 at 00:42, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > > >>
> > > >> Hi Simon,
> > > >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> > > >> >
> > > >> > Hi Ilias,
> > > >> >
> > > >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > > >> > <ilias.apalodimas@linaro.org> wrote:
> > > >> > >
> > > >> > > The capsule signature is now part of our DTB.  This is problematic when a
> > > >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> > > >> > > can overwrite the signature as well.
> > > >> >
> > > >> > Just to repeat my question since it looks like I didn't get a response
> > > >> > on the last patch:
> > > >> >
> > > >> > Do you mean with the 'fdt' command?
> > > >> > >
> > > >> > If you mean the FDT fixups, they happen to a different DT, the one
> > > >> > being passed to Linux.
> > > >>
> > > >> In some platforms the key is derived from the relocated DTB, which we
> > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > >> details.
> > > >
> > > >
> > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > >
> > > You should be clearer about what problem you are trying to solve. Are
> > > you worried about a script changing the DT? Or just it being writable
> > > in general?
> >
> > Being writable in general is my main concern. Doing fixup internally
> > from U-Boot might be something we'll always need but the ability to
> > completely change it doesn't play well security.
> >
> > >
> > > U-Boot itself is relocated also, including the rodata. So are you
> > > using the public key from the original location? What if that is not
> > > accessible after relocation?
> >
> > We are accessing he key from the relocated address.
>
> Then in what way are you protecting it? This is so confusing. Are you
> saying that you are protecting the relocated address? If so, protect
> the relocated devicetree too!
>
> >
> > >
> > > There is also the 'fdt addr -c' command to find the control DT. It is
> > > not expected to be written to though. So just protect the memory to
> > > which it is relocated, or relocate it to a place that you can protect.
> >
> > Can you define 'protect'? The mmu support in U-Boot is kind of limited
> > from what I can see.  In order to protect anything we'd have to switch
> > the pages ro R-- or RX-.  Someone please shout if I  am wrong, but I
> > couldn't find code doing that in U-Boot.
> >
> > >
> > > If the DT is writable it will affect U-Boot's operation, since that is
> > > where all the config is stored. There is no point in pretending that
> > > pulling one thing out of it and protecting it will result in any sort
> > > of improvement. This needs to be done properly.
> >
> > Tbh I thought the relocation was done properly and the .rodata section
> > was either merged with .text (and was RX-) or R--.
> > If we fix the relocation properly, the .rodata will be read only,
> > while the DTB should still be on RW memory.   I understand that you
> > are using the DTB for some configuration on U-Boot ,but arguably the
> > public key of the EFI firmware you need to authenticate hardly
> > classifies as configuration.
>
> What is it then?

Please just do this properly. Add an API to protect memory, implement
it for your chosen platform and then we will actually fix the problem
you are worried about. Until then, work-arounds and hacks are
pointless and just confuse the issue. Most haste, less speed.

>
> > Keep in mind that even if we fix the page tables permissions, on a
> > secure platform the command line should be disabled, since anyone
> > could load an arbitrary application and modify them.
>
> Yes we brought in CONFIG_CMDLINE for that reason about five years ago :-)

Regards,
SImon

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

On Thu, 22 Jul 2021 at 16:30, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 22 Jul 2021 at 07:28, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 21 Jul 2021 at 00:42, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > > > >>
> > > > >> Hi Simon,
> > > > >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> > > > >> >
> > > > >> > Hi Ilias,
> > > > >> >
> > > > >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > > > >> > <ilias.apalodimas@linaro.org> wrote:
> > > > >> > >
> > > > >> > > The capsule signature is now part of our DTB.  This is problematic when a
> > > > >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> > > > >> > > can overwrite the signature as well.
> > > > >> >
> > > > >> > Just to repeat my question since it looks like I didn't get a response
> > > > >> > on the last patch:
> > > > >> >
> > > > >> > Do you mean with the 'fdt' command?
> > > > >> > >
> > > > >> > If you mean the FDT fixups, they happen to a different DT, the one
> > > > >> > being passed to Linux.
> > > > >>
> > > > >> In some platforms the key is derived from the relocated DTB, which we
> > > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > > >> details.
> > > > >
> > > > >
> > > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > > >
> > > > You should be clearer about what problem you are trying to solve. Are
> > > > you worried about a script changing the DT? Or just it being writable
> > > > in general?
> > >
> > > Being writable in general is my main concern. Doing fixup internally
> > > from U-Boot might be something we'll always need but the ability to
> > > completely change it doesn't play well security.
> > >
> > > >
> > > > U-Boot itself is relocated also, including the rodata. So are you
> > > > using the public key from the original location? What if that is not
> > > > accessible after relocation?
> > >
> > > We are accessing he key from the relocated address.
> >
> > Then in what way are you protecting it? This is so confusing. Are you
> > saying that you are protecting the relocated address? If so, protect
> > the relocated devicetree too!
> >

How? DTBs if fixed up and there's a protocol proposal from Heinrich,
which allows fixups from GRUB2.  So how exactly are you going to put
it in r/o memory (which is what .rodata is supposed to achieve).
A big portion of the DTBs we build today are horribly outdated
compared to the current upstream.  Since nowdays there's a spec
describing what can and can't go in a DTB, I'd much rather prefer we
stick to that and make a potential update easier.

Regards
/Ilias

> > >
> > > >
> > > > There is also the 'fdt addr -c' command to find the control DT. It is
> > > > not expected to be written to though. So just protect the memory to
> > > > which it is relocated, or relocate it to a place that you can protect.
> > >
> > > Can you define 'protect'? The mmu support in U-Boot is kind of limited
> > > from what I can see.  In order to protect anything we'd have to switch
> > > the pages ro R-- or RX-.  Someone please shout if I  am wrong, but I
> > > couldn't find code doing that in U-Boot.
> > >
> > > >
> > > > If the DT is writable it will affect U-Boot's operation, since that is
> > > > where all the config is stored. There is no point in pretending that
> > > > pulling one thing out of it and protecting it will result in any sort
> > > > of improvement. This needs to be done properly.
> > >
> > > Tbh I thought the relocation was done properly and the .rodata section
> > > was either merged with .text (and was RX-) or R--.
> > > If we fix the relocation properly, the .rodata will be read only,
> > > while the DTB should still be on RW memory.   I understand that you
> > > are using the DTB for some configuration on U-Boot ,but arguably the
> > > public key of the EFI firmware you need to authenticate hardly
> > > classifies as configuration.
> >
> > What is it then?
>
> Please just do this properly. Add an API to protect memory, implement
> it for your chosen platform and then we will actually fix the problem
> you are worried about. Until then, work-arounds and hacks are
> pointless and just confuse the issue. Most haste, less speed.
>
> >
> > > Keep in mind that even if we fix the page tables permissions, on a
> > > secure platform the command line should be disabled, since anyone
> > > could load an arbitrary application and modify them.
> >
> > Yes we brought in CONFIG_CMDLINE for that reason about five years ago :-)
>
> Regards,
> SImon

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi Ilias,

On Thu, 22 Jul 2021 at 07:56, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 22 Jul 2021 at 16:30, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 22 Jul 2021 at 07:28, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 21 Jul 2021 at 00:42, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > > > > >>
> > > > > >> Hi Simon,
> > > > > >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg@chromium.org> wrote:
> > > > > >> >
> > > > > >> > Hi Ilias,
> > > > > >> >
> > > > > >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > > > > >> > <ilias.apalodimas@linaro.org> wrote:
> > > > > >> > >
> > > > > >> > > The capsule signature is now part of our DTB.  This is problematic when a
> > > > > >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> > > > > >> > > can overwrite the signature as well.
> > > > > >> >
> > > > > >> > Just to repeat my question since it looks like I didn't get a response
> > > > > >> > on the last patch:
> > > > > >> >
> > > > > >> > Do you mean with the 'fdt' command?
> > > > > >> > >
> > > > > >> > If you mean the FDT fixups, they happen to a different DT, the one
> > > > > >> > being passed to Linux.
> > > > > >>
> > > > > >> In some platforms the key is derived from the relocated DTB, which we
> > > > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > > > >> details.
> > > > > >
> > > > > >
> > > > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > > > >
> > > > > You should be clearer about what problem you are trying to solve. Are
> > > > > you worried about a script changing the DT? Or just it being writable
> > > > > in general?
> > > >
> > > > Being writable in general is my main concern. Doing fixup internally
> > > > from U-Boot might be something we'll always need but the ability to
> > > > completely change it doesn't play well security.
> > > >
> > > > >
> > > > > U-Boot itself is relocated also, including the rodata. So are you
> > > > > using the public key from the original location? What if that is not
> > > > > accessible after relocation?
> > > >
> > > > We are accessing he key from the relocated address.
> > >
> > > Then in what way are you protecting it? This is so confusing. Are you
> > > saying that you are protecting the relocated address? If so, protect
> > > the relocated devicetree too!
> > >
>
> How? DTBs if fixed up and there's a protocol proposal from Heinrich,
> which allows fixups from GRUB2.  So how exactly are you going to put
> it in r/o memory (which is what .rodata is supposed to achieve).

Because they are different DTBs, right? Either it can be read-only or
can't be read-only. At first you said it could not be read-only. Now
you are saying it needs to be changed. Where is all this coming from?

> A big portion of the DTBs we build today are horribly outdated
> compared to the current upstream.  Since nowdays there's a spec

That may be true on some boards but it is not my experience, at least
on ARM. Anyway that is an issue for the board maintainers. I don't
think this has any bearing on the points we are discussing here.

> describing what can and can't go in a DTB, I'd much rather prefer we
> stick to that and make a potential update easier.

There is just so much confusion in all of this and we are going around
in circles. Let me try to state what I think are points of confusion.

1. The U-Boot DT needs to be protected against change for lots of
reasons (drivers misbehaving, etc.). The signature is only one of
them.

2. The U-Boot DT is separate from the one passed to Linux. So
discussion about where U-Boot config should go in the Linux DT is not
germain.

3. U-Boot uses DT for its configuration and that is that. It has done
that for about 7-8 years. U-Boot does not have a user space to provide
policy and configuration . It cannot do what Linux does and run
programs and look up filesystems to figure out how to boot. So
configuration / runtime info go in the DT in U-Boot. I have not seen
any proposal to do it any other way. I hope you can understand how
frustrating to have someone come from the Linux world and say, Oh it's
all wrong...we should put it user space, etc. The alternative to DT is
a mishmash of random places and ideas with no schema and no
discoverability, etc, or a forest of CONFIG options, like it used to
me.

4. The DT is relocated anyway so is not actually read-only just
because you put it in the rodata area.

5. You can make the DT read-only if you want to. You can make any part
of memory read-only. You need to create an API for that if that's what
you want. It would be nice to have a command to look at what is
protected and change it. See for example the 'mtrr' command on x86.

Also I would add that there has always been a DT spec. I think the
latest version is here:
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
So far as I can tell it does not talk about what can and cannot go in a DT

Perhaps the spec you are referring to is a Linux spec. Do you have a
link? So far as I can tell, U-Boot, Zephyr, etc. have had very little
input into that. I know for a fact that no one has asked what I think.
For example, even the u-boot,xxx tags are kept in separate files in
U-Boot because of resistance to putting that in Linux. Zephyr
completely does its own thing with DT. U-Boot very much follows Binux,
BUT it has its own things as well, just as Linux does. I would LOVE to
see that change and if you would like to help with that, or have ideas
on how, please go ahead.

Regards,
Simon

[..]

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

Hi Simon,

On Thu, Jul 22, 2021 at 10:46:40AM -0600, Simon Glass wrote:
> > > > > > >> In some platforms the key is derived from the relocated DTB, which we

[...]

> > > > > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > > > > >> details.
> > > > > > >
> > > > > > >
> > > > > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > > > > >
> > > > > > You should be clearer about what problem you are trying to solve. Are
> > > > > > you worried about a script changing the DT? Or just it being writable
> > > > > > in general?
> > > > >
> > > > > Being writable in general is my main concern. Doing fixup internally
> > > > > from U-Boot might be something we'll always need but the ability to
> > > > > completely change it doesn't play well security.
> > > > >
> > > > > >
> > > > > > U-Boot itself is relocated also, including the rodata. So are you
> > > > > > using the public key from the original location? What if that is not
> > > > > > accessible after relocation?
> > > > >
> > > > > We are accessing he key from the relocated address.
> > > >
> > > > Then in what way are you protecting it? This is so confusing. Are you
> > > > saying that you are protecting the relocated address? If so, protect
> > > > the relocated devicetree too!
> > > >
> >
> > How? DTBs if fixed up and there's a protocol proposal from Heinrich,
> > which allows fixups from GRUB2.  So how exactly are you going to put
> > it in r/o memory (which is what .rodata is supposed to achieve).
> 
> Because they are different DTBs, right? Either it can be read-only or
> can't be read-only. At first you said it could not be read-only. Now
> you are saying it needs to be changed. Where is all this coming from?
> 

Not exactly, I said it can be read only, assuming we can switch pages to
RO, as long CONFIG_OF_EMBED is enabled.  What happens if you choose
CONFIG_OF_PRIOR_STAGE or CONFIG_OF_SEPARATE? That means the *prior* stage
boot loader needs to know your public key and inject in the dtb it hands
over?

> > A big portion of the DTBs we build today are horribly outdated
> > compared to the current upstream.  Since nowdays there's a spec
> 
> That may be true on some boards but it is not my experience, at least
> on ARM. Anyway that is an issue for the board maintainers. I don't
> think this has any bearing on the points we are discussing here.
> 

There's a discussion for DTs and it's evolution. Some are indeed a bit
outdated and I can find details on that.  The point I am trying to make
here is that the closer we keep the DTBs to what linux hosts, the easier 
it's going to be to keep them up to date.

> > describing what can and can't go in a DTB, I'd much rather prefer we
> > stick to that and make a potential update easier.
> 
> There is just so much confusion in all of this and we are going around
> in circles. Let me try to state what I think are points of confusion.
> 
> 1. The U-Boot DT needs to be protected against change for lots of
> reasons (drivers misbehaving, etc.). The signature is only one of
> them.
> 

We agree on that 

> 2. The U-Boot DT is separate from the one passed to Linux. So
> discussion about where U-Boot config should go in the Linux DT is not
> germain.

Not always, there's cases were you can use the same DTB. The reason we don't
is due to the diversion we have.  Ideally you should have a single DTB,
which can be embedded into your firmware and you can authenticate it by
just authentication the firmware, but I've abandoned that dream long
ago.

> 
> 3. U-Boot uses DT for its configuration and that is that. It has done
> that for about 7-8 years. U-Boot does not have a user space to provide
> policy and configuration . It cannot do what Linux does and run
> programs and look up filesystems to figure out how to boot. So
> configuration / runtime info go in the DT in U-Boot. I have not seen
> any proposal to do it any other way. I hope you can understand how
> frustrating to have someone come from the Linux world and say, Oh it's
> all wrong...

Apologies if it sounded like that. I am not trying to point any fingers or 
judge any code that's been there for a couple of years.
I never said it's all wrong. I can understand some hardware specific config
going into a DT that's been used for a couple of years.  What I don't
understand is how a signature fits that profile.

> we should put it user space, etc. The alternative to DT is
> a mishmash of random places and ideas with no schema and no
> discoverability, etc, or a forest of CONFIG options, like it used to
> me.
> 
> 4. The DT is relocated anyway so is not actually read-only just
> because you put it in the rodata area.

You mean the signature here right (not the DT).  The point is if we fix the
.rodata section and it ends up on RO memory, then you do get what we want
and it's always available not matter who provides the DTB, or what
expectations you have for fixing it up (hence put it in R/W).

> 
> 5. You can make the DT read-only if you want to. You can make any part
> of memory read-only. You need to create an API for that if that's what
> you want. It would be nice to have a command to look at what is
> protected and change it. See for example the 'mtrr' command on x86.
> 

I'll repeat myself but isn't that the case for CONFIG_OF_EMBED only?

> Also I would add that there has always been a DT spec. I think the
> latest version is here:
> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
> So far as I can tell it does not talk about what can and cannot go in a DT
> 
> Perhaps the spec you are referring to is a Linux spec. Do you have a
> link? 

I don't think there's a 'linux spec'. The DTS ended up being maintained in
the kernel. I am not gonna argue if this is a good or bad thing, but that
codebase is trying to follow the spec you pasted

> So far as I can tell, U-Boot, Zephyr, etc. have had very little
> input into that. I know for a fact that no one has asked what I think.
> For example, even the u-boot,xxx tags are kept in separate files in
> U-Boot because of resistance to putting that in Linux. Zephyr
> completely does its own thing with DT. U-Boot very much follows Binux,
> BUT it has its own things as well, just as Linux does. I would LOVE to
> see that change and if you would like to help with that, or have ideas
> on how, please go ahead.

I am joining the calls for that exact reason. So we can discuss that there
I guess?

Regards
/Ilias
> 
> Regards,
> Simon
> 
> [..]

Re: [PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Simon Glass]

Hi,

On Thu, 22 Jul 2021 at 14:54, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, Jul 22, 2021 at 10:46:40AM -0600, Simon Glass wrote:
> > > > > > > >> In some platforms the key is derived from the relocated DTB, which we
>
> [...]
>
> > > > > > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > > > > > >> details.
> > > > > > > >
> > > > > > > >
> > > > > > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > > > > > >
> > > > > > > You should be clearer about what problem you are trying to solve. Are
> > > > > > > you worried about a script changing the DT? Or just it being writable
> > > > > > > in general?
> > > > > >
> > > > > > Being writable in general is my main concern. Doing fixup internally
> > > > > > from U-Boot might be something we'll always need but the ability to
> > > > > > completely change it doesn't play well security.
> > > > > >
> > > > > > >
> > > > > > > U-Boot itself is relocated also, including the rodata. So are you
> > > > > > > using the public key from the original location? What if that is not
> > > > > > > accessible after relocation?
> > > > > >
> > > > > > We are accessing he key from the relocated address.
> > > > >
> > > > > Then in what way are you protecting it? This is so confusing. Are you
> > > > > saying that you are protecting the relocated address? If so, protect
> > > > > the relocated devicetree too!
> > > > >
> > >
> > > How? DTBs if fixed up and there's a protocol proposal from Heinrich,
> > > which allows fixups from GRUB2.  So how exactly are you going to put
> > > it in r/o memory (which is what .rodata is supposed to achieve).
> >
> > Because they are different DTBs, right? Either it can be read-only or
> > can't be read-only. At first you said it could not be read-only. Now
> > you are saying it needs to be changed. Where is all this coming from?
> >
>
> Not exactly, I said it can be read only, assuming we can switch pages to
> RO, as long CONFIG_OF_EMBED is enabled.  What happens if you choose
> CONFIG_OF_PRIOR_STAGE or CONFIG_OF_SEPARATE? That means the *prior* stage
> boot loader needs to know your public key and inject in the dtb it hands
> over?
>
> > > A big portion of the DTBs we build today are horribly outdated
> > > compared to the current upstream.  Since nowdays there's a spec
> >
> > That may be true on some boards but it is not my experience, at least
> > on ARM. Anyway that is an issue for the board maintainers. I don't
> > think this has any bearing on the points we are discussing here.
> >
>
> There's a discussion for DTs and it's evolution. Some are indeed a bit
> outdated and I can find details on that.  The point I am trying to make
> here is that the closer we keep the DTBs to what linux hosts, the easier
> it's going to be to keep them up to date.
>
> > > describing what can and can't go in a DTB, I'd much rather prefer we
> > > stick to that and make a potential update easier.
> >
> > There is just so much confusion in all of this and we are going around
> > in circles. Let me try to state what I think are points of confusion.
> >
> > 1. The U-Boot DT needs to be protected against change for lots of
> > reasons (drivers misbehaving, etc.). The signature is only one of
> > them.
> >
>
> We agree on that
>
> > 2. The U-Boot DT is separate from the one passed to Linux. So
> > discussion about where U-Boot config should go in the Linux DT is not
> > germain.
>
> Not always, there's cases were you can use the same DTB. The reason we don't
> is due to the diversion we have.  Ideally you should have a single DTB,
> which can be embedded into your firmware and you can authenticate it by
> just authentication the firmware, but I've abandoned that dream long
> ago.
>
> >
> > 3. U-Boot uses DT for its configuration and that is that. It has done
> > that for about 7-8 years. U-Boot does not have a user space to provide
> > policy and configuration . It cannot do what Linux does and run
> > programs and look up filesystems to figure out how to boot. So
> > configuration / runtime info go in the DT in U-Boot. I have not seen
> > any proposal to do it any other way. I hope you can understand how
> > frustrating to have someone come from the Linux world and say, Oh it's
> > all wrong...
>
> Apologies if it sounded like that. I am not trying to point any fingers or
> judge any code that's been there for a couple of years.
> I never said it's all wrong. I can understand some hardware specific config
> going into a DT that's been used for a couple of years.  What I don't
> understand is how a signature fits that profile.
>
> > we should put it user space, etc. The alternative to DT is
> > a mishmash of random places and ideas with no schema and no
> > discoverability, etc, or a forest of CONFIG options, like it used to
> > me.
> >
> > 4. The DT is relocated anyway so is not actually read-only just
> > because you put it in the rodata area.
>
> You mean the signature here right (not the DT).  The point is if we fix the
> .rodata section and it ends up on RO memory, then you do get what we want
> and it's always available not matter who provides the DTB, or what
> expectations you have for fixing it up (hence put it in R/W).
>
> >
> > 5. You can make the DT read-only if you want to. You can make any part
> > of memory read-only. You need to create an API for that if that's what
> > you want. It would be nice to have a command to look at what is
> > protected and change it. See for example the 'mtrr' command on x86.
> >
>
> I'll repeat myself but isn't that the case for CONFIG_OF_EMBED only?
>
> > Also I would add that there has always been a DT spec. I think the
> > latest version is here:
> > https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3
> > So far as I can tell it does not talk about what can and cannot go in a DT
> >
> > Perhaps the spec you are referring to is a Linux spec. Do you have a
> > link?
>
> I don't think there's a 'linux spec'. The DTS ended up being maintained in
> the kernel. I am not gonna argue if this is a good or bad thing, but that
> codebase is trying to follow the spec you pasted
>
> > So far as I can tell, U-Boot, Zephyr, etc. have had very little
> > input into that. I know for a fact that no one has asked what I think.
> > For example, even the u-boot,xxx tags are kept in separate files in
> > U-Boot because of resistance to putting that in Linux. Zephyr
> > completely does its own thing with DT. U-Boot very much follows Binux,
> > BUT it has its own things as well, just as Linux does. I would LOVE to
> > see that change and if you would like to help with that, or have ideas
> > on how, please go ahead.
>
> I am joining the calls for that exact reason. So we can discuss that there
> I guess?

Very unfortunately this patch was silently applied to a tree and is
now in mainline, despite this discussion not being resolved.

I will send a revert until this can be resolved.

Regards,
Simon