[PATCH v2 0/3] efi: Minimal revert to rodata change

[PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

The changes to move from devicetree to rodata take things in the wrong
direction for various reasons:

- devicetree is where config should be stored
- it provides no memory production in any case, particularly when U-Boot
  is relocated
- testing becomes harder, with the suggestion of adding an entire new
  sandbox build just for this

Revert this until a new direction can be established.

Changes in v2:
- Also revert two other patches, based on comment from Takahiro

Simon Glass (3):
  Revert "doc: Update CapsuleUpdate READMEs"
  Revert "mkeficapsule: Remove dtb related options"
  Revert "efi_capsule: Move signature from DTB to .rodata"

 board/emulation/common/Makefile             |   1 +
 board/emulation/common/qemu_capsule.c       |  43 ++++
 doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
 doc/develop/uefi/uefi.rst                   | 125 -----------
 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 --
 tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
 10 files changed, 472 insertions(+), 181 deletions(-)
 create mode 100644 board/emulation/common/qemu_capsule.c
 create mode 100644 doc/board/emulation/qemu_capsule_update.rst
 delete mode 100644 lib/efi_loader/efi_capsule_key.S

-- 
2.32.0.554.ge1b32706d8-goog

[PATCH v2 1/3] Revert "doc: Update CapsuleUpdate READMEs"

[Simon Glass]

This reverts commit 316ab801c0d91c02b21b8be1e3db7e69555364e9.

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

(no changes since v1)

 doc/board/emulation/qemu_capsule_update.rst | 203 ++++++++++++++++++++
 doc/develop/uefi/uefi.rst                   | 125 ------------
 2 files changed, 203 insertions(+), 125 deletions(-)
 create 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
new file mode 100644
index 00000000000..0a2286d039d
--- /dev/null
+++ b/doc/board/emulation/qemu_capsule_update.rst
@@ -0,0 +1,203 @@
+.. 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 64fe9346c7f..4f2b8b036db 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -277,131 +277,6 @@ 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.32.0.554.ge1b32706d8-goog

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

[Simon Glass]

This reverts commit f86caab058ff062ce72b24cd1ab9ec1253cc1352.

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

(no changes since v1)

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

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index 4995ba4e0c2..de0a6289888 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -4,17 +4,22 @@
  *		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;
@@ -24,6 +29,9 @@ typedef __s32 s32;
 
 #define aligned_u64 __aligned_u64
 
+#define SIGNATURE_NODENAME	"signature"
+#define OVERLAY_NODENAME	"__overlay__"
+
 #ifndef __packed
 #define __packed __attribute__((packed))
 #endif
@@ -44,6 +52,9 @@ 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},
 };
@@ -57,10 +68,187 @@ 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)
 {
@@ -178,16 +366,22 @@ 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:h", options, &idx);
+		c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);
 		if (c == -1)
 			break;
 
@@ -214,22 +408,43 @@ 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 an output file */
-	if (argc != optind + 1) {
+	/* need a fit image file or raw image file */
+	if (!file && !pkey_file && !dtb_file) {
 		print_usage();
 		exit(EXIT_FAILURE);
 	}
 
-	/* need a fit image file or raw image file */
-	if (!file) {
-		print_usage();
-		exit(EXIT_SUCCESS);
+	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);
+		}
 	}
 
 	if (create_fwbin(argv[optind], file, guid, index, instance)
-- 
2.32.0.554.ge1b32706d8-goog

Re: [PATCH v2 2/3] Revert "mkeficapsule: Remove dtb related options"

[KASHI Takahiro]

Simon,

On Mon, Aug 02, 2021 at 08:44:30AM -0600, Simon Glass wrote:
> This reverts commit f86caab058ff062ce72b24cd1ab9ec1253cc1352.

Whether we choose a devicetree approach or not, we don't have
to revert this patch because we can do the same thing with
standard commands (i.e. fdtoverlay).

See my patches and discussions[1].
(We all agreed to removing this feature from mkeficapsule.)

[1] https://lists.denx.de/pipermail/u-boot/2021-May/449573.html
    https://lists.denx.de/pipermail/u-boot/2021-May/449574.html

-Takahiro Akashi


> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  tools/mkeficapsule.c | 229 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 222 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index 4995ba4e0c2..de0a6289888 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -4,17 +4,22 @@
>   *		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;
> @@ -24,6 +29,9 @@ typedef __s32 s32;
>  
>  #define aligned_u64 __aligned_u64
>  
> +#define SIGNATURE_NODENAME	"signature"
> +#define OVERLAY_NODENAME	"__overlay__"
> +
>  #ifndef __packed
>  #define __packed __attribute__((packed))
>  #endif
> @@ -44,6 +52,9 @@ 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},
>  };
> @@ -57,10 +68,187 @@ 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)
>  {
> @@ -178,16 +366,22 @@ 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:h", options, &idx);
> +		c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);
>  		if (c == -1)
>  			break;
>  
> @@ -214,22 +408,43 @@ 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 an output file */
> -	if (argc != optind + 1) {
> +	/* need a fit image file or raw image file */
> +	if (!file && !pkey_file && !dtb_file) {
>  		print_usage();
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	/* need a fit image file or raw image file */
> -	if (!file) {
> -		print_usage();
> -		exit(EXIT_SUCCESS);
> +	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);
> +		}
>  	}
>  
>  	if (create_fwbin(argv[optind], file, guid, index, instance)
> -- 
> 2.32.0.554.ge1b32706d8-goog
> 

Re: [PATCH v2 2/3] Revert "mkeficapsule: Remove dtb related options"

[Simon Glass]

Hi Takahiro,

On Mon, 2 Aug 2021 at 23:30, KASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Simon,
>
> On Mon, Aug 02, 2021 at 08:44:30AM -0600, Simon Glass wrote:
> > This reverts commit f86caab058ff062ce72b24cd1ab9ec1253cc1352.
>
> Whether we choose a devicetree approach or not, we don't have
> to revert this patch because we can do the same thing with
> standard commands (i.e. fdtoverlay).
>
> See my patches and discussions[1].
> (We all agreed to removing this feature from mkeficapsule.)

Well this just shows that I am not the right person to be sorting out
this mess. Can someone in the team working on come up with the right
fix?

Regards,
Simon

>
> [1] https://lists.denx.de/pipermail/u-boot/2021-May/449573.html
>     https://lists.denx.de/pipermail/u-boot/2021-May/449574.html
>
> -Takahiro Akashi
>
>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  tools/mkeficapsule.c | 229 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 222 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> > index 4995ba4e0c2..de0a6289888 100644
> > --- a/tools/mkeficapsule.c
> > +++ b/tools/mkeficapsule.c
> > @@ -4,17 +4,22 @@
> >   *           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;
> > @@ -24,6 +29,9 @@ typedef __s32 s32;
> >
> >  #define aligned_u64 __aligned_u64
> >
> > +#define SIGNATURE_NODENAME   "signature"
> > +#define OVERLAY_NODENAME     "__overlay__"
> > +
> >  #ifndef __packed
> >  #define __packed __attribute__((packed))
> >  #endif
> > @@ -44,6 +52,9 @@ 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},
> >  };
> > @@ -57,10 +68,187 @@ 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)
> >  {
> > @@ -178,16 +366,22 @@ 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:h", options, &idx);
> > +             c = getopt_long(argc, argv, "f:r:i:I:v:D:K:Oh", options, &idx);
> >               if (c == -1)
> >                       break;
> >
> > @@ -214,22 +408,43 @@ 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 an output file */
> > -     if (argc != optind + 1) {
> > +     /* need a fit image file or raw image file */
> > +     if (!file && !pkey_file && !dtb_file) {
> >               print_usage();
> >               exit(EXIT_FAILURE);
> >       }
> >
> > -     /* need a fit image file or raw image file */
> > -     if (!file) {
> > -             print_usage();
> > -             exit(EXIT_SUCCESS);
> > +     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);
> > +             }
> >       }
> >
> >       if (create_fwbin(argv[optind], file, guid, index, instance)
> > --
> > 2.32.0.554.ge1b32706d8-goog
> >

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

[Simon Glass]

This was unfortunately applied despite much discussion about it beiong
the wrong way to implement this feature.

Revert it before too many other things are built on top of it.

This reverts commit ddf67daac39de76d2697d587148f4c2cb768f492.
Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Also revert two other patches, based on comment from Takahiro

 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, 47 insertions(+), 49 deletions(-)
 create mode 100644 board/emulation/common/qemu_capsule.c
 delete mode 100644 lib/efi_loader/efi_capsule_key.S

diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile
index c5b452e7e34..7ed447a69dc 100644
--- a/board/emulation/common/Makefile
+++ b/board/emulation/common/Makefile
@@ -2,3 +2,4 @@
 
 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
new file mode 100644
index 00000000000..6b8a87022a4
--- /dev/null
+++ b/board/emulation/common/qemu_capsule.c
@@ -0,0 +1,43 @@
+// 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 ec992b0c2e3..267f1db73f2 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -27,8 +27,6 @@ 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 dacc3b58810..7a469f22721 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -214,13 +214,6 @@ 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 9b369430e25..fd344cea29b 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -20,19 +20,11 @@ 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 26990bc2df4..b75e4bcba1a 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -16,7 +16,6 @@
 #include <mapmem.h>
 #include <sort.h>
 
-#include <asm/sections.h>
 #include <crypto/pkcs7.h>
 #include <crypto/pkcs7_parser.h>
 #include <linux/err.h>
@@ -253,23 +252,12 @@ out:
 
 #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 
-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 *stored_pkey, *pkey;
+	void *fdt_pkey, *pkey;
 	efi_uintn_t pkey_len;
 	uint64_t monotonic_count;
 	struct efi_signature_store *truststore;
@@ -322,7 +310,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 		goto out;
 	}
 
-	ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
+	ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
 	if (ret < 0)
 		goto out;
 
@@ -330,7 +318,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 	if (!pkey)
 		goto out;
 
-	memcpy(pkey, stored_pkey, pkey_len);
+	memcpy(pkey, fdt_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
deleted file mode 100644
index 58f00b8e4bc..00000000000
--- a/lib/efi_loader/efi_capsule_key.S
+++ /dev/null
@@ -1,17 +0,0 @@
-/* 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.32.0.554.ge1b32706d8-goog

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Ilias Apalodimas]

Hi Simon,

On Mon, Aug 02, 2021 at 08:44:28AM -0600, Simon Glass wrote:
> The changes to move from devicetree to rodata take things in the wrong
> direction for various reasons:
> 

As I said on the previous thread, I think this should remain as is for a
number of reasons (and mainly because it only works with 1/3 valid
CONFIG_OF_XXX U-Boot provides), but I'll let Heinrich decide.

> - devicetree is where config should be stored
> - it provides no memory production in any case, particularly when U-Boot
>   is relocated
> - testing becomes harder, with the suggestion of adding an entire new
>   sandbox build just for this
> 
> Revert this until a new direction can be established.
> 

Regards
/Ilias
> Changes in v2:
> - Also revert two other patches, based on comment from Takahiro
> 
> Simon Glass (3):
>   Revert "doc: Update CapsuleUpdate READMEs"
>   Revert "mkeficapsule: Remove dtb related options"
>   Revert "efi_capsule: Move signature from DTB to .rodata"
> 
>  board/emulation/common/Makefile             |   1 +
>  board/emulation/common/qemu_capsule.c       |  43 ++++
>  doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
>  doc/develop/uefi/uefi.rst                   | 125 -----------
>  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 --
>  tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
>  10 files changed, 472 insertions(+), 181 deletions(-)
>  create mode 100644 board/emulation/common/qemu_capsule.c
>  create mode 100644 doc/board/emulation/qemu_capsule_update.rst
>  delete mode 100644 lib/efi_loader/efi_capsule_key.S
> 
> -- 
> 2.32.0.554.ge1b32706d8-goog
> 

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

Hi Ilias,

On Mon, 2 Aug 2021 at 09:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, Aug 02, 2021 at 08:44:28AM -0600, Simon Glass wrote:
> > The changes to move from devicetree to rodata take things in the wrong
> > direction for various reasons:
> >
>
> As I said on the previous thread, I think this should remain as is for a
> number of reasons (and mainly because it only works with 1/3 valid
> CONFIG_OF_XXX U-Boot provides), but I'll let Heinrich decide.

Do you mean OF_EMBED and OF_HOSTFILE?

We happily use OF_HOSTFILE in the sandbox vboot tests. I don't see any
issue there.

OF_EMBED should not be used in production code. It is for debugging only.


>
> > - devicetree is where config should be stored
> > - it provides no memory production in any case, particularly when U-Boot
> >   is relocated
> > - testing becomes harder, with the suggestion of adding an entire new
> >   sandbox build just for this
> >
> > Revert this until a new direction can be established.
> >
>
> Regards
> /Ilias
> > Changes in v2:
> > - Also revert two other patches, based on comment from Takahiro
> >
> > Simon Glass (3):
> >   Revert "doc: Update CapsuleUpdate READMEs"
> >   Revert "mkeficapsule: Remove dtb related options"
> >   Revert "efi_capsule: Move signature from DTB to .rodata"
> >
> >  board/emulation/common/Makefile             |   1 +
> >  board/emulation/common/qemu_capsule.c       |  43 ++++
> >  doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
> >  doc/develop/uefi/uefi.rst                   | 125 -----------
> >  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 --
> >  tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
> >  10 files changed, 472 insertions(+), 181 deletions(-)
> >  create mode 100644 board/emulation/common/qemu_capsule.c
> >  create mode 100644 doc/board/emulation/qemu_capsule_update.rst
> >  delete mode 100644 lib/efi_loader/efi_capsule_key.S
> >
> > --
> > 2.32.0.554.ge1b32706d8-goog
> >

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Ilias Apalodimas]

On Mon, Aug 02, 2021 at 02:02:56PM -0600, Simon Glass wrote:
> Hi Ilias,
> 
> On Mon, 2 Aug 2021 at 09:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Aug 02, 2021 at 08:44:28AM -0600, Simon Glass wrote:
> > > The changes to move from devicetree to rodata take things in the wrong
> > > direction for various reasons:
> > >
> >
> > As I said on the previous thread, I think this should remain as is for a
> > number of reasons (and mainly because it only works with 1/3 valid
> > CONFIG_OF_XXX U-Boot provides), but I'll let Heinrich decide.
> 
> Do you mean OF_EMBED and OF_HOSTFILE?
> 
> We happily use OF_HOSTFILE in the sandbox vboot tests. I don't see any
> issue there.
> 
> OF_EMBED should not be used in production code. It is for debugging only.

No I mean CONFIG_OF_SEPARATE and CONFIG_OF_PRIOR_STAGE (apart from
CONFIG_OF_BOARD)

Thanks
/Ilias
> 
> 
> >
> > > - devicetree is where config should be stored
> > > - it provides no memory production in any case, particularly when U-Boot
> > >   is relocated
> > > - testing becomes harder, with the suggestion of adding an entire new
> > >   sandbox build just for this
> > >
> > > Revert this until a new direction can be established.
> > >
> >
> > Regards
> > /Ilias
> > > Changes in v2:
> > > - Also revert two other patches, based on comment from Takahiro
> > >
> > > Simon Glass (3):
> > >   Revert "doc: Update CapsuleUpdate READMEs"
> > >   Revert "mkeficapsule: Remove dtb related options"
> > >   Revert "efi_capsule: Move signature from DTB to .rodata"
> > >
> > >  board/emulation/common/Makefile             |   1 +
> > >  board/emulation/common/qemu_capsule.c       |  43 ++++
> > >  doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
> > >  doc/develop/uefi/uefi.rst                   | 125 -----------
> > >  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 --
> > >  tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
> > >  10 files changed, 472 insertions(+), 181 deletions(-)
> > >  create mode 100644 board/emulation/common/qemu_capsule.c
> > >  create mode 100644 doc/board/emulation/qemu_capsule_update.rst
> > >  delete mode 100644 lib/efi_loader/efi_capsule_key.S
> > >
> > > --
> > > 2.32.0.554.ge1b32706d8-goog
> > >

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

HI Ilias,

On Mon, 2 Aug 2021 at 23:43, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, Aug 02, 2021 at 02:02:56PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 2 Aug 2021 at 09:37, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Aug 02, 2021 at 08:44:28AM -0600, Simon Glass wrote:
> > > > The changes to move from devicetree to rodata take things in the wrong
> > > > direction for various reasons:
> > > >
> > >
> > > As I said on the previous thread, I think this should remain as is for a
> > > number of reasons (and mainly because it only works with 1/3 valid
> > > CONFIG_OF_XXX U-Boot provides), but I'll let Heinrich decide.
> >
> > Do you mean OF_EMBED and OF_HOSTFILE?
> >
> > We happily use OF_HOSTFILE in the sandbox vboot tests. I don't see any
> > issue there.
> >
> > OF_EMBED should not be used in production code. It is for debugging only.
>
> No I mean CONFIG_OF_SEPARATE and CONFIG_OF_PRIOR_STAGE (apart from
> CONFIG_OF_BOARD)

Well OF_SEPARATE works fine.

OF_PRIOR_STAGE is no different, as I understand it. I just means that
the prior stage (whatever that is) needs to have the public key.

Regards,
Simon

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Heinrich Schuchardt]

On 8/2/21 4:44 PM, Simon Glass wrote:
> The changes to move from devicetree to rodata take things in the wrong
> direction for various reasons:
>
> - devicetree is where config should be stored

We are not talking about configuration here but about bundling a file.

> - it provides no memory production in any case, particularly when U-Boot

What do you mean by "production"?

Should you mean memory protection: I cannot see that the memory pages
containing the devicetree are set to readonly. Furthermore setenv can
completely replace the devicetree.

>    is relocated
> - testing becomes harder, with the suggestion of adding an entire new
>    sandbox build just for this
>
> Revert this until a new direction can be established.

We can change the current solution *after* anything better has been
designed.

Best regards

Heinrich

>
> Changes in v2:
> - Also revert two other patches, based on comment from Takahiro
>
> Simon Glass (3):
>    Revert "doc: Update CapsuleUpdate READMEs"
>    Revert "mkeficapsule: Remove dtb related options"
>    Revert "efi_capsule: Move signature from DTB to .rodata"
>
>   board/emulation/common/Makefile             |   1 +
>   board/emulation/common/qemu_capsule.c       |  43 ++++
>   doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
>   doc/develop/uefi/uefi.rst                   | 125 -----------
>   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 --
>   tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
>   10 files changed, 472 insertions(+), 181 deletions(-)
>   create mode 100644 board/emulation/common/qemu_capsule.c
>   create mode 100644 doc/board/emulation/qemu_capsule_update.rst
>   delete mode 100644 lib/efi_loader/efi_capsule_key.S
>

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

Hi Heinrich,

On Mon, 2 Aug 2021 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 8/2/21 4:44 PM, Simon Glass wrote:
> > The changes to move from devicetree to rodata take things in the wrong
> > direction for various reasons:
> >
> > - devicetree is where config should be stored
>
> We are not talking about configuration here but about bundling a file.
>
> > - it provides no memory production in any case, particularly when U-Boot
>
> What do you mean by "production"?
>
> Should you mean memory protection: I cannot see that the memory pages
> containing the devicetree are set to readonly. Furthermore setenv can

Did you read the discussion? Neither can rodata, so this is a pointless change.

> completely replace the devicetree.

Yes and 'mw' can overwrite memory...so...?

>
> >    is relocated
> > - testing becomes harder, with the suggestion of adding an entire new
> >    sandbox build just for this
> >
> > Revert this until a new direction can be established.
>
> We can change the current solution *after* anything better has been
> designed.

The original solution was fine IMO and the new one is much worse. Now
I see a patch to create a new sandbox build. All of this is yet
another parallel implementation within U-Boot for EFI. I have yet to
see any effort to address the parallel driver model.

We should just use devicetree for run-time configuration.

Regards,
SImon

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change\

[Ilias Apalodimas]

On Mon, Aug 02, 2021 at 01:22:18PM -0600, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 2 Aug 2021 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 8/2/21 4:44 PM, Simon Glass wrote:
> > > The changes to move from devicetree to rodata take things in the wrong
> > > direction for various reasons:
> > >
> > > - devicetree is where config should be stored
> >
> > We are not talking about configuration here but about bundling a file.
> >
> > > - it provides no memory production in any case, particularly when U-Boot
> >
> > What do you mean by "production"?
> >
> > Should you mean memory protection: I cannot see that the memory pages
> > containing the devicetree are set to readonly. Furthermore setenv can
> 
> Did you read the discussion? Neither can rodata, so this is a pointless change.
> 

It's far from pointless imho. In that same discussion I pointed out that the
DTB might need to remain r/w for it's entire lifetime, while .rodata is
just a matter of missing code to switch pages to RO-.

Thanks
/Ilias

> > completely replace the devicetree.
> 
> Yes and 'mw' can overwrite memory...so...?
> 
> >
> > >    is relocated
> > > - testing becomes harder, with the suggestion of adding an entire new
> > >    sandbox build just for this
> > >
> > > Revert this until a new direction can be established.
> >
> > We can change the current solution *after* anything better has been
> > designed.
> 
> The original solution was fine IMO and the new one is much worse. Now
> I see a patch to create a new sandbox build. All of this is yet
> another parallel implementation within U-Boot for EFI. I have yet to
> see any effort to address the parallel driver model.
> 
> We should just use devicetree for run-time configuration.
> 
> Regards,
> SImon

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change\

[Simon Glass]

Hi Ilias,

On Mon, 2 Aug 2021 at 23:46, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Mon, Aug 02, 2021 at 01:22:18PM -0600, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 2 Aug 2021 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > On 8/2/21 4:44 PM, Simon Glass wrote:
> > > > The changes to move from devicetree to rodata take things in the wrong
> > > > direction for various reasons:
> > > >
> > > > - devicetree is where config should be stored
> > >
> > > We are not talking about configuration here but about bundling a file.
> > >
> > > > - it provides no memory production in any case, particularly when U-Boot
> > >
> > > What do you mean by "production"?
> > >
> > > Should you mean memory protection: I cannot see that the memory pages
> > > containing the devicetree are set to readonly. Furthermore setenv can
> >
> > Did you read the discussion? Neither can rodata, so this is a pointless change.
> >
>
> It's far from pointless imho. In that same discussion I pointed out that the
> DTB might need to remain r/w for it's entire lifetime, while .rodata is
> just a matter of missing code to switch pages to RO-.

We don't support a r/w control DTB in U-Boot. At present any attempt
to update the DTB will cause devices to fail to probe since the
offsets they point to will be incorrect. If r/w is desired, I think
OF_LIVE is the only reasonable option.

So I think that point is moot also.

[..]

Regards,
SImon

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Heinrich Schuchardt]

On 02.08.21 16:44, Simon Glass wrote:
> The changes to move from devicetree to rodata take things in the wrong
> direction for various reasons:
>
> - devicetree is where config should be stored

We are not talking about configuration here at all.

> - it provides no memory production in any case, particularly when U-Boot

No clue what you mean by "memory production".

>    is relocated
> - testing becomes harder, with the suggestion of adding an entire new
>    sandbox build just for this

Having an extra config is not required when putting the certificate into
.rodata.

Best regards

Heinrich

>
> Revert this until a new direction can be established.
>
> Changes in v2:
> - Also revert two other patches, based on comment from Takahiro
>
> Simon Glass (3):
>    Revert "doc: Update CapsuleUpdate READMEs"
>    Revert "mkeficapsule: Remove dtb related options"
>    Revert "efi_capsule: Move signature from DTB to .rodata"
>
>   board/emulation/common/Makefile             |   1 +
>   board/emulation/common/qemu_capsule.c       |  43 ++++
>   doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
>   doc/develop/uefi/uefi.rst                   | 125 -----------
>   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 --
>   tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
>   10 files changed, 472 insertions(+), 181 deletions(-)
>   create mode 100644 board/emulation/common/qemu_capsule.c
>   create mode 100644 doc/board/emulation/qemu_capsule_update.rst
>   delete mode 100644 lib/efi_loader/efi_capsule_key.S
>

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

Hi Heinrich,

On Thu, 5 Aug 2021 at 09:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 02.08.21 16:44, Simon Glass wrote:
> > The changes to move from devicetree to rodata take things in the wrong
> > direction for various reasons:
> >
> > - devicetree is where config should be stored
>
> We are not talking about configuration here at all.

I thought we were talking about the public key. That is run-time
config in my book, just like the devicetree itself, which controls all
the devices.

>
> > - it provides no memory production in any case, particularly when U-Boot
>
> No clue what you mean by "memory production".

memory protection. But it turns out this is pointless anyway. We
discussed it at length in the contributor call. We came down to one
issue with the way the firmware is packaged by users (with U-Boot
coming from one place and TF-A another). I think Ilias is going to
write something up to help get to the bottom of it.

>
> >    is relocated
> > - testing becomes harder, with the suggestion of adding an entire new
> >    sandbox build just for this
>
> Having an extra config is not required when putting the certificate into
> .rodata.

The certificate should not go in rodata, period. Please just fix it.
It use to be fine a few weeks ago so it should not be hard.

Regards,
Simon

>
> Best regards
>
> Heinrich
>
> >
> > Revert this until a new direction can be established.
> >
> > Changes in v2:
> > - Also revert two other patches, based on comment from Takahiro
> >
> > Simon Glass (3):
> >    Revert "doc: Update CapsuleUpdate READMEs"
> >    Revert "mkeficapsule: Remove dtb related options"
> >    Revert "efi_capsule: Move signature from DTB to .rodata"
> >
> >   board/emulation/common/Makefile             |   1 +
> >   board/emulation/common/qemu_capsule.c       |  43 ++++
> >   doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
> >   doc/develop/uefi/uefi.rst                   | 125 -----------
> >   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 --
> >   tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
> >   10 files changed, 472 insertions(+), 181 deletions(-)
> >   create mode 100644 board/emulation/common/qemu_capsule.c
> >   create mode 100644 doc/board/emulation/qemu_capsule_update.rst
> >   delete mode 100644 lib/efi_loader/efi_capsule_key.S
> >

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[KASHI Takahiro]

On Thu, Aug 05, 2021 at 09:46:07AM -0600, Simon Glass wrote:
> Hi Heinrich,
> 
> On Thu, 5 Aug 2021 at 09:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 02.08.21 16:44, Simon Glass wrote:
> > > The changes to move from devicetree to rodata take things in the wrong
> > > direction for various reasons:
> > >
> > > - devicetree is where config should be stored
> >
> > We are not talking about configuration here at all.
> 
> I thought we were talking about the public key. That is run-time
> config in my book, just like the devicetree itself, which controls all
> the devices.
> 
> >
> > > - it provides no memory production in any case, particularly when U-Boot
> >
> > No clue what you mean by "memory production".
> 
> memory protection. But it turns out this is pointless anyway. We
> discussed it at length in the contributor call. We came down to one

What was clarified and decided in that meeting?
I know you have a meeting note, but it was not very clear for me
which direction the discussion is heading now.

# Yes, I should have been there, but ...
# Simon, if possible, please announce the agenda a bit earlier
# so that I can notice that. I'm usually in the bed at that time :)

I don't think that memory protection is really a matter if there is
no assumption that the storage where the firmware resides are
securely protected.

-Takahiro Akashi

> issue with the way the firmware is packaged by users (with U-Boot
> coming from one place and TF-A another). I think Ilias is going to
> write something up to help get to the bottom of it.
> 
> >
> > >    is relocated
> > > - testing becomes harder, with the suggestion of adding an entire new
> > >    sandbox build just for this
> >
> > Having an extra config is not required when putting the certificate into
> > .rodata.
> 
> The certificate should not go in rodata, period. Please just fix it.
> It use to be fine a few weeks ago so it should not be hard.
> 
> Regards,
> Simon
> 
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > Revert this until a new direction can be established.
> > >
> > > Changes in v2:
> > > - Also revert two other patches, based on comment from Takahiro
> > >
> > > Simon Glass (3):
> > >    Revert "doc: Update CapsuleUpdate READMEs"
> > >    Revert "mkeficapsule: Remove dtb related options"
> > >    Revert "efi_capsule: Move signature from DTB to .rodata"
> > >
> > >   board/emulation/common/Makefile             |   1 +
> > >   board/emulation/common/qemu_capsule.c       |  43 ++++
> > >   doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
> > >   doc/develop/uefi/uefi.rst                   | 125 -----------
> > >   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 --
> > >   tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
> > >   10 files changed, 472 insertions(+), 181 deletions(-)
> > >   create mode 100644 board/emulation/common/qemu_capsule.c
> > >   create mode 100644 doc/board/emulation/qemu_capsule_update.rst
> > >   delete mode 100644 lib/efi_loader/efi_capsule_key.S
> > >

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Simon Glass]

Hi Takahiro,

On Thu, 5 Aug 2021 at 18:13, KASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Aug 05, 2021 at 09:46:07AM -0600, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Thu, 5 Aug 2021 at 09:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > On 02.08.21 16:44, Simon Glass wrote:
> > > > The changes to move from devicetree to rodata take things in the wrong
> > > > direction for various reasons:
> > > >
> > > > - devicetree is where config should be stored
> > >
> > > We are not talking about configuration here at all.
> >
> > I thought we were talking about the public key. That is run-time
> > config in my book, just like the devicetree itself, which controls all
> > the devices.
> >
> > >
> > > > - it provides no memory production in any case, particularly when U-Boot
> > >
> > > No clue what you mean by "memory production".
> >
> > memory protection. But it turns out this is pointless anyway. We
> > discussed it at length in the contributor call. We came down to one
>
> What was clarified and decided in that meeting?
> I know you have a meeting note, but it was not very clear for me
> which direction the discussion is heading now.

https://bit.ly/3bFvwA1

I don't think anything was decided, despite the time taken, but we did
talk through a lot of the issues.

>
> # Yes, I should have been there, but ...
> # Simon, if possible, please announce the agenda a bit earlier
> # so that I can notice that. I'm usually in the bed at that time :)

The agenda in this case was added some days in advance but as one
participant was a bit late we moved to the 'last-minute' topic of this
thread.

Also note that I don't set the agenda, although I might add a topic if
there is nothing there.

If you are in Asia, we used to have an Asia call but it was not well
attended so we dropped it.

>
> I don't think that memory protection is really a matter if there is
> no assumption that the storage where the firmware resides are
> securely protected.

OK. If it does matter, we can solve it.

Regards,
SImon

>
> -Takahiro Akashi
>
> > issue with the way the firmware is packaged by users (with U-Boot
> > coming from one place and TF-A another). I think Ilias is going to
> > write something up to help get to the bottom of it.
> >
> > >
> > > >    is relocated
> > > > - testing becomes harder, with the suggestion of adding an entire new
> > > >    sandbox build just for this
> > >
> > > Having an extra config is not required when putting the certificate into
> > > .rodata.
> >
> > The certificate should not go in rodata, period. Please just fix it.
> > It use to be fine a few weeks ago so it should not be hard.
> >
> > Regards,
> > Simon
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >
> > > > Revert this until a new direction can be established.
> > > >
> > > > Changes in v2:
> > > > - Also revert two other patches, based on comment from Takahiro
> > > >
> > > > Simon Glass (3):
> > > >    Revert "doc: Update CapsuleUpdate READMEs"
> > > >    Revert "mkeficapsule: Remove dtb related options"
> > > >    Revert "efi_capsule: Move signature from DTB to .rodata"
> > > >
> > > >   board/emulation/common/Makefile             |   1 +
> > > >   board/emulation/common/qemu_capsule.c       |  43 ++++
> > > >   doc/board/emulation/qemu_capsule_update.rst | 203 +++++++++++++++++
> > > >   doc/develop/uefi/uefi.rst                   | 125 -----------
> > > >   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 --
> > > >   tools/mkeficapsule.c                        | 229 +++++++++++++++++++-
> > > >   10 files changed, 472 insertions(+), 181 deletions(-)
> > > >   create mode 100644 board/emulation/common/qemu_capsule.c
> > > >   create mode 100644 doc/board/emulation/qemu_capsule_update.rst
> > > >   delete mode 100644 lib/efi_loader/efi_capsule_key.S
> > > >

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Thu, Aug 05, 2021 at 09:46:07AM -0600, Simon Glass wrote:

> Hi Heinrich,
> 
> On Thu, 5 Aug 2021 at 09:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 02.08.21 16:44, Simon Glass wrote:
> > > The changes to move from devicetree to rodata take things in the wrong
> > > direction for various reasons:
> > >
> > > - devicetree is where config should be stored
> >
> > We are not talking about configuration here at all.
> 
> I thought we were talking about the public key. That is run-time
> config in my book, just like the devicetree itself, which controls all
> the devices.
> 
> >
> > > - it provides no memory production in any case, particularly when U-Boot
> >
> > No clue what you mean by "memory production".
> 
> memory protection. But it turns out this is pointless anyway. We
> discussed it at length in the contributor call. We came down to one
> issue with the way the firmware is packaged by users (with U-Boot
> coming from one place and TF-A another). I think Ilias is going to
> write something up to help get to the bottom of it.
> 
> >
> > >    is relocated
> > > - testing becomes harder, with the suggestion of adding an entire new
> > >    sandbox build just for this
> >
> > Having an extra config is not required when putting the certificate into
> > .rodata.
> 
> The certificate should not go in rodata, period. Please just fix it.
> It use to be fine a few weeks ago so it should not be hard.

Where are we at here, Heinrich?  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 0/3] efi: Minimal revert to rodata change

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On Mon, Aug 09, 2021 at 12:01:20PM -0400, Tom Rini wrote:
> On Thu, Aug 05, 2021 at 09:46:07AM -0600, Simon Glass wrote:
> 
> > Hi Heinrich,
> > 
> > On Thu, 5 Aug 2021 at 09:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > On 02.08.21 16:44, Simon Glass wrote:
> > > > The changes to move from devicetree to rodata take things in the wrong
> > > > direction for various reasons:
> > > >
> > > > - devicetree is where config should be stored
> > >
> > > We are not talking about configuration here at all.
> > 
> > I thought we were talking about the public key. That is run-time
> > config in my book, just like the devicetree itself, which controls all
> > the devices.
> > 
> > >
> > > > - it provides no memory production in any case, particularly when U-Boot
> > >
> > > No clue what you mean by "memory production".
> > 
> > memory protection. But it turns out this is pointless anyway. We
> > discussed it at length in the contributor call. We came down to one
> > issue with the way the firmware is packaged by users (with U-Boot
> > coming from one place and TF-A another). I think Ilias is going to
> > write something up to help get to the bottom of it.
> > 
> > >
> > > >    is relocated
> > > > - testing becomes harder, with the suggestion of adding an entire new
> > > >    sandbox build just for this
> > >
> > > Having an extra config is not required when putting the certificate into
> > > .rodata.
> > 
> > The certificate should not go in rodata, period. Please just fix it.
> > It use to be fine a few weeks ago so it should not be hard.
> 
> Where are we at here, Heinrich?  Thanks.

Heinrich?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]