RE: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Roman Kopytin <Roman.Kopytin@kaspersky.com>]

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

Hi, all
I prepared 3 patches for fdt_add_pubkey adding.
But in our company infrastructure I can't use git send-email. Our IT can't help me to resolve issue.

-----Original Message-----
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> 
Sent: Tuesday, September 28, 2021 11:57 AM
To: u-boot@lists.denx.de
Cc: Alex Kiernan <alex.kiernan@gmail.com>; Simon Glass <sjg@chromium.org>; Roman Kopytin <Roman.Kopytin@kaspersky.com>; Masahiro Yamada <masahiroy@kernel.org>; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: [PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

The build system already automatically looks for and includes an in-tree *-u-boot.dtsi when building the control .dtb. However, there are some things that are awkward to maintain in such an in-tree file, most notably the metadata associated to public keys used for verified boot.

The only "official" API to get that metadata into the .dtb is via mkimage, as a side effect of building an actual signed image. But there are multiple problems with that. First of all, the final U-Boot (be it U-Boot proper or an SPL) image is built based on a binary image, the .dtb, and possibly some other binary artifacts. So modifying the .dtb after the build requires the meta-buildsystem (Yocto, buildroot, whatnot) to know about and repeat some of the steps that are already known to and handled by U-Boot's build system, resulting in needless duplication of code. It's also somewhat annoying and inconsistent to have a .dtb file in the build folder which is not generated by the command listed in the corresponding .cmd file (that of course applies to any generated file).

So the contents of the /signature node really needs to be baked into the .dtb file when it is first created, which means providing the relevant data in the form of a .dtsi file. One could in theory put that data into the *-u-boot.dtsi file, but it's more convenient to be able to provide it externally: For example, when developing for a customer, it's common to use a set of dummy keys for development, while the consultants do not (and should not) have access to the actual keys used in production. For such a setup, it's easier if the keys used are chosen via the meta-buildsystem and the path(s) patched in during the configure step. And of course, nothing prevents anybody from having DEVICE_TREE_INCLUDES point at files maintained in git, or for that matter from including the public key metadata in the *-u-boot.dtsi directly and ignore this feature.

There are other uses for this, e.g. in combination with ENV_IMPORT_FDT it can be used for providing the contents of the /config/environment node, so I don't want to tie this exclusively to use for verified boot.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

Getting the public key metadata into .dtsi form can be done with a little scripting (roughly 'mkimage -K' of a dummy image followed by 'dtc -I dtb -O dts'). I have a script that does that, along with options to set 'required' and 'required-mode' properties, include u-boot,dm-spl properties in case one wants to check U-Boot proper from SPL with the verified boot mechanism, etc. I'm happy to share that script if this gets accepted, but it's moot if this is rejected.

I have previously tried to get an fdt_add_pubkey tool accepted [1,2] to disentangle the kernel and u-boot builds (or u-boot and SPL builds for that matter!), but as I've since realized, that isn't quite enough
- the above points re modifying the .dtb after it is created but before that is used to create further build artifacts still stand. However, such a tool could still be useful for creating the .dtsi info without the private keys being present, and my key2dtsi.sh script could easily be modified to use a tool like that should it ever appear.

[1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
[2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/

 dts/Kconfig          | 9 +++++++++
 scripts/Makefile.lib | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/dts/Kconfig b/dts/Kconfig
index dabe0080c1..593dddbaf0 100644
--- a/dts/Kconfig
+++ b/dts/Kconfig
@@ -139,6 +139,15 @@ config DEFAULT_DEVICE_TREE
 	  It can be overridden from the command line:
 	  $ make DEVICE_TREE=<device-tree-name>
 
+config DEVICE_TREE_INCLUDES
+       string "Extra .dtsi files to include when building DT control"
+	depends on OF_CONTROL
+	help
+	  U-Boot's control .dtb is usually built from an in-tree .dts
+	  file, plus (if available) an in-tree U-Boot-specific .dtsi
+	  file. This option specifies a space-separated list of extra
+	  .dtsi files that will also be used.
+
 config OF_LIST
 	string "List of device tree files to include for DT control"
 	depends on SPL_LOAD_FIT || MULTI_DTB_FIT diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 78bbebe7e9..a2accba940 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -320,6 +320,8 @@ quiet_cmd_dtc = DTC     $@
 # Bring in any U-Boot-specific include at the end of the file  cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
+	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
+	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
 	$(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
 	$(DTC) -O dtb -o $@ -b 0 \
 		-i $(dir $<) $(DTC_FLAGS) \
--
2.31.1


[-- Attachment #2: 0000-cover-letter.patch --]
[-- Type: application/octet-stream, Size: 597 bytes --]

From c9cec63586fbce1a5f665ff22b9c3b01a43cfbc4 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 18:15:35 +0300
Subject: [PATCH 0/2] *** SUBJECT HERE ***

*** BLURB HERE ***

Roman Kopytin (2):
  tools: add fdt_add_pubkey
  test_vboot.py: include test of fdt_add_pubkey tool

 test/py/tests/test_vboot.py |  8 +++
 tools/.gitignore            |  1 +
 tools/Makefile              |  3 ++
 tools/fdt_add_pubkey.c      | 97 +++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+)
 create mode 100755 tools/fdt_add_pubkey.c

-- 
2.25.1


[-- Attachment #3: 0001-tools-add-fdt_add_pubkey.patch --]
[-- Type: application/octet-stream, Size: 5376 bytes --]

From 8c7eab9d5e5a1428c84114e04c21a5d0940f0966 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 15:21:50 +0300
Subject: [PATCH 1/2] tools: add fdt_add_pubkey

Having to use the -K option to mkimage to populate U-Boot's .dtb with the
public key while signing the kernel FIT image is often a little
awkward. In particular, when using a meta-build system such as
bitbake/Yocto, having the tasks of the kernel and U-Boot recipes
intertwined, modifying deployed artifacts and rebuilding U-Boot with
an updated .dtb is quite cumbersome. Also, in some scenarios one may
wish to build U-Boot complete with the public key(s) embedded in the
.dtb without the corresponding private keys being present on the same
build host.

So this adds a simple tool that allows one to disentangle the kernel
and U-Boot builds, by simply copy-pasting just enough of the mkimage
code to allow one to add a public key to a .dtb. When using mkimage,
some of the information is taken from the .its used to build the
kernel (algorithm and key name), so that of course needs to be
supplied on the command line.

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/.gitignore       |  1 +
 tools/Makefile         |  3 ++
 tools/fdt_add_pubkey.c | 97 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100755 tools/fdt_add_pubkey.c

diff --git a/tools/.gitignore b/tools/.gitignore
index a88453f64d..f312b760e4 100644
--- a/tools/.gitignore
+++ b/tools/.gitignore
@@ -6,6 +6,7 @@
 /dumpimage
 /easylogo/easylogo
 /envcrc
+/fdt_add_pubkey
 /fdtgrep
 /file2include
 /fit_check_sign
diff --git a/tools/Makefile b/tools/Makefile
index 4a86321f64..44f25dda18 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -73,6 +73,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
 
 hostprogs-y += dumpimage mkimage
 hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fit_info fit_check_sign
+hostprogs-$(CONFIG_TOOLS_LIBCRYPTO) += fdt_add_pubkey
 
 hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
 
@@ -153,6 +154,7 @@ dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
 mkimage-objs   := $(dumpimage-mkimage-objs) mkimage.o
 fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
 fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
+fdt_add_pubkey-objs   := $(dumpimage-mkimage-objs) fdt_add_pubkey.o
 file2include-objs := file2include.o
 
 ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
@@ -190,6 +192,7 @@ HOSTCFLAGS_fit_image.o += -DMKIMAGE_DTC=\"$(CONFIG_MKIMAGE_DTC_PATH)\"
 HOSTLDLIBS_dumpimage := $(HOSTLDLIBS_mkimage)
 HOSTLDLIBS_fit_info := $(HOSTLDLIBS_mkimage)
 HOSTLDLIBS_fit_check_sign := $(HOSTLDLIBS_mkimage)
+HOSTLDLIBS_fdt_add_pubkey := $(HOSTLDLIBS_mkimage)
 
 hostprogs-$(CONFIG_EXYNOS5250) += mkexynosspl
 hostprogs-$(CONFIG_EXYNOS5420) += mkexynosspl
diff --git a/tools/fdt_add_pubkey.c b/tools/fdt_add_pubkey.c
new file mode 100755
index 0000000000..9306ecedd1
--- /dev/null
+++ b/tools/fdt_add_pubkey.c
@@ -0,0 +1,97 @@
+#include <image.h>
+#include "fit_common.h"
+
+static const char *cmdname;
+
+static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */
+static const char *keydir = "."; /* -k <keydir> */
+static const char *keyname = "key"; /* -n <keyname> */
+static const char *require_keys; /* -r <conf|image> */
+static const char *keydest; /* argv[n] */
+
+static void usage(const char *msg)
+{
+	fprintf(stderr, "Error: %s\n", msg);
+	fprintf(stderr, "Usage: %s [-a <algo>] [-k <keydir>] [-n <keyname>] [-r <conf|image>] <fdt blob>\n",
+		cmdname);
+	exit(EXIT_FAILURE);
+}
+
+static void process_args(int argc, char *argv[])
+{
+	int opt;
+
+	while((opt = getopt(argc, argv, "a:k:n:r:")) != -1) {
+		switch (opt) {
+		case 'k':
+			keydir = optarg;
+			break;
+		case 'a':
+			algo_name = optarg;
+			break;
+		case 'n':
+			keyname = optarg;
+			break;
+		case 'r':
+			require_keys = optarg;
+			break;
+		default:
+			usage("Invalid option");
+		}
+	}
+	/* The last parameter is expected to be the .dtb to add the public key to */
+	if (optind < argc)
+		keydest = argv[optind];
+
+	if (!keydest)
+		usage("Missing dtb file to update");
+}
+
+int main(int argc, char *argv[])
+{
+	struct image_sign_info info;
+	int destfd, ret;
+	void *dest_blob = NULL;
+	struct stat dest_sbuf;
+	size_t size_inc = 0;
+
+	cmdname = argv[0];
+
+	process_args(argc, argv);
+
+	memset(&info, 0, sizeof(info));
+
+	info.keydir = keydir;
+	info.keyname = keyname;
+	info.name = algo_name;
+	info.require_keys = require_keys;
+	info.crypto = image_get_crypto_algo(algo_name);
+	if (!info.crypto) {
+                fprintf(stderr, "Unsupported signature algorithm '%s'\n", algo_name);
+		exit(EXIT_FAILURE);
+	}
+
+	while (1) {
+		destfd = mmap_fdt(cmdname, keydest, size_inc, &dest_blob, &dest_sbuf, false, false);
+		if (destfd < 0)
+			exit(EXIT_FAILURE);
+
+		ret = info.crypto->add_verify_data(&info, dest_blob);
+
+		munmap(dest_blob, dest_sbuf.st_size);
+		close(destfd);
+		if (!ret || ret != -ENOSPC)
+			break;
+		fprintf(stderr, ".dtb too small, increasing size by 1024 bytes\n");
+		size_inc = 1024;
+	}
+
+	if (ret) {
+		fprintf(stderr, "%s: Cannot add public key to FIT blob: %s\n",
+			cmdname, strerror(-ret));
+		exit(EXIT_FAILURE);
+	}
+
+	exit(EXIT_SUCCESS);
+}
+
-- 
2.25.1


[-- Attachment #4: 0002-test_vboot.py-include-test-of-fdt_add_pubkey-tool.patch --]
[-- Type: application/octet-stream, Size: 1810 bytes --]

From c9cec63586fbce1a5f665ff22b9c3b01a43cfbc4 Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Fri, 6 Aug 2021 15:23:16 +0300
Subject: [PATCH 2/2] test_vboot.py: include test of fdt_add_pubkey tool

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/py/tests/test_vboot.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 6dff6779d1..cf7416b39a 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -230,6 +230,13 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
         cons.log.action('%s: Check signed config on the host' % sha_algo)
 
         util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
+        
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+        # Then add the dev key via the fdt_add_pubkey tool
+        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048' % sha_algo,
+                                '-k', tmpdir, '-n', 'dev', '-r', 'conf', dtb])
+        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
 
         if full_test:
             # Make sure that U-Boot checks that the config is in the list of
@@ -370,6 +377,7 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
     fit = '%stest.fit' % tmpdir
     mkimage = cons.config.build_dir + '/tools/mkimage'
     fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
+    fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey'
     dtc_args = '-I dts -O dtb -i %s' % tmpdir
     dtb = '%ssandbox-u-boot.dtb' % tmpdir
     sig_node = '/configurations/conf-1/signature'
-- 
2.25.1