[PATCH 0/2] RFC: add fdt_add_pubkey tool

[PATCH 0/2] RFC: add fdt_add_pubkey tool

[Roman Kopytin]

In order to reduce the coupling between building the kernel and
U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
without simultaneously signing a FIT image. That tool doesn't seem to
exist, so I stole the necessary pieces from mkimage et al and put it
in a single .c file.

I'm still working on the details of my proposed "require just k out
these n required keys" and how it should be implemented, but it will
probably involve teaching this tool a bunch of new options. These
patches are not necessarily ready for inclusion (unless someone else
finds fdt_add_pubkey useful as is), but I thought I might as well send
it out for early comments.

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

[PATCH 1/2] tools: add fdt_add_pubkey

[Roman Kopytin]

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

[PATCH 2/2] test_vboot.py: include test of fdt_add_pubkey tool

[Roman Kopytin]

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

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 08.11.21 16:28, Roman Kopytin wrote:
> In order to reduce the coupling between building the kernel and
> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> without simultaneously signing a FIT image. That tool doesn't seem to
> exist, so I stole the necessary pieces from mkimage et al and put it
> in a single .c file.
> 
> I'm still working on the details of my proposed "require just k out
> these n required keys" and how it should be implemented, but it will
> probably involve teaching this tool a bunch of new options. These
> patches are not necessarily ready for inclusion (unless someone else
> finds fdt_add_pubkey useful as is), but I thought I might as well send
> it out for early comments.

I'd also like to see the usage of this hooked into the build process.

And to my understanding of [1], that approach will provide a feature
that permits hooking with the build but would expect the key as dtsi
fragment. Can we consolidate the approaches?

My current vision of a user interface would be a Kconfig option that
takes a list of key files to be injected. Maybe make that three lists,
one for "required=image", one for "required=conf", and one for optional
keys (if that has a use case in practice, no idea).

Jan

[1]
https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

RE: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Roman Kopytin]

Can we have discussion with code lines? For me it is not very clear, because it isn't my code.

-----Original Message-----
From: Jan Kiszka <jan.kiszka@siemens.com> 
Sent: Tuesday, November 9, 2021 12:17 PM
To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

On 08.11.21 16:28, Roman Kopytin wrote:
> In order to reduce the coupling between building the kernel and 
> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb 
> without simultaneously signing a FIT image. That tool doesn't seem to 
> exist, so I stole the necessary pieces from mkimage et al and put it 
> in a single .c file.
> 
> I'm still working on the details of my proposed "require just k out 
> these n required keys" and how it should be implemented, but it will 
> probably involve teaching this tool a bunch of new options. These 
> patches are not necessarily ready for inclusion (unless someone else 
> finds fdt_add_pubkey useful as is), but I thought I might as well send 
> it out for early comments.

I'd also like to see the usage of this hooked into the build process.

And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?

My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).

Jan

[1]
https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 09.11.21 10:37, Roman Kopytin wrote:
> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
> 

Please do not top-post.

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com> 
> Sent: Tuesday, November 9, 2021 12:17 PM
> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> 
> On 08.11.21 16:28, Roman Kopytin wrote:
>> In order to reduce the coupling between building the kernel and 
>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb 
>> without simultaneously signing a FIT image. That tool doesn't seem to 
>> exist, so I stole the necessary pieces from mkimage et al and put it 
>> in a single .c file.
>>
>> I'm still working on the details of my proposed "require just k out 
>> these n required keys" and how it should be implemented, but it will 
>> probably involve teaching this tool a bunch of new options. These 
>> patches are not necessarily ready for inclusion (unless someone else 
>> finds fdt_add_pubkey useful as is), but I thought I might as well send 
>> it out for early comments.
> 
> I'd also like to see the usage of this hooked into the build process.
> 
> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
> 
> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
> 
> Jan
> 
> [1]
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> 
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
> 

For what would you like to have code? The kconfig addition?

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index d3a12be228..a9ed4d4ec4 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
 
 endif # SPL
 
+config FIT_SIGNATURE_PUB_KEYS
+	string "Public keys to use for FIT image verification"
+	depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
+	help
+	  Public keys, or certificate files to extract them from, that shall
+	  be used to verify signed FIT images. The keys will be embedded into
+	  the control device tree of U-Boot.
+
 endif # FIT
 
 config LEGACY_IMAGE_FORMAT


But note that we are in a design discussion here, and I'm at least 
reluctant to code up n-versions without having some common idea where 
things should move.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[François Ozog]

Hi

as we are in design discussions, I would promote the idea of not pushing
non hardware related things in the DTB that is passed to the kernel.
Is your use case to allow U-Boot to verify the kernel's signature ?
Why not putting it into an environment variable?

If your use case is on Arm or RISC-V, both environments are working heavily
to make https://arm-software.github.io/ebbr/ standard available on a large
number of boards.
This offers UEFI interface and SecureBoot (and later MeasuredBoot)
services. For Arm boards just check for SystemReady compliance.
In this context, traditional UEFI secure variables are used to deal with
certificates and hashes: PK, KEK, db...
You can obviously do differently but you will be on your own to extend the
chain of trust to IMA, secure containers (rooted down to hRoT) and other
security facilities in the Linux side.
Could you describe your use case in more details?

On Tue, 9 Nov 2021 at 11:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 09.11.21 10:37, Roman Kopytin wrote:
> > Can we have discussion with code lines? For me it is not very clear,
> because it isn't my code.
> >
>
> Please do not top-post.
>
> > -----Original Message-----
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > Sent: Tuesday, November 9, 2021 12:17 PM
> > To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de;
> Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >
> > On 08.11.21 16:28, Roman Kopytin wrote:
> >> In order to reduce the coupling between building the kernel and
> >> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >> without simultaneously signing a FIT image. That tool doesn't seem to
> >> exist, so I stole the necessary pieces from mkimage et al and put it
> >> in a single .c file.
> >>
> >> I'm still working on the details of my proposed "require just k out
> >> these n required keys" and how it should be implemented, but it will
> >> probably involve teaching this tool a bunch of new options. These
> >> patches are not necessarily ready for inclusion (unless someone else
> >> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >> it out for early comments.
> >
> > I'd also like to see the usage of this hooked into the build process.
> >
> > And to my understanding of [1], that approach will provide a feature
> that permits hooking with the build but would expect the key as dtsi
> fragment. Can we consolidate the approaches?
> >
> > My current vision of a user interface would be a Kconfig option that
> takes a list of key files to be injected. Maybe make that three lists, one
> for "required=image", one for "required=conf", and one for optional keys
> (if that has a use case in practice, no idea).
> >
> > Jan
> >
> > [1]
> >
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >
> > --
> > Siemens AG, T RDA IOT
> > Corporate Competence Center Embedded Linux
> >
>
> For what would you like to have code? The kconfig addition?
>
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index d3a12be228..a9ed4d4ec4 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>
>  endif # SPL
>
> +config FIT_SIGNATURE_PUB_KEYS
> +       string "Public keys to use for FIT image verification"
> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> +       help
> +         Public keys, or certificate files to extract them from, that
> shall
> +         be used to verify signed FIT images. The keys will be embedded
> into
> +         the control device tree of U-Boot.
> +
>  endif # FIT
>
>  config LEGACY_IMAGE_FORMAT
>
>
> But note that we are in a design discussion here, and I'm at least
> reluctant to code up n-versions without having some common idea where
> things should move.
>
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
>


-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 09.11.21 13:43, François Ozog wrote:
> Hi
> 
> as we are in design discussions, I would promote the idea of not pushing
> non hardware related things in the DTB that is passed to the kernel.

This was never proposed. The public keys go into the *control* FDTs.

> Is your use case to allow U-Boot to verify the kernel's signature ?
> Why not putting it into an environment variable?

This is both about validating OS FIT containers as well as SPL checking
U-Boot proper before continuing the boot there. The former case can be
replaced with UEFI logic and likely taking the key from TEE (we do that
as well in first prototypes), but the latter can't (yet).

> 
> If your use case is on Arm or RISC-V, both environments are working
> heavily to make https://arm-software.github.io/ebbr/
> <https://arm-software.github.io/ebbr/> standard available on a large
> number of boards.
> This offers UEFI interface and SecureBoot (and later MeasuredBoot)
> services. For Arm boards just check for SystemReady compliance.
> In this context, traditional UEFI secure variables are used to deal with
> certificates and hashes: PK, KEK, db...
> You can obviously do differently but you will be on your own to extend
> the chain of trust to IMA, secure containers (rooted down to hRoT) and
> other security facilities in the Linux side.
> Could you describe your use case in more details? 

doc/uImage.FIT/signature.txt ;)

More concrete: We are currently massaging board/siemens/iot2050 to close
its static chain of trust between SPL and U-Boot main, using software
means (vendor means do not work because FSBL key != TF-A/TEE/U-Boot key).

Jan

> 
> On Tue, 9 Nov 2021 at 11:07, Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 09.11.21 10:37, Roman Kopytin wrote:
>     > Can we have discussion with code lines? For me it is not very
>     clear, because it isn't my code.
>     >
> 
>     Please do not top-post.
> 
>     > -----Original Message-----
>     > From: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
>     > Sent: Tuesday, November 9, 2021 12:17 PM
>     > To: Roman Kopytin <Roman.Kopytin@kaspersky.com
>     <mailto:Roman.Kopytin@kaspersky.com>>; u-boot@lists.denx.de
>     <mailto:u-boot@lists.denx.de>; Rasmus Villemoes
>     <rasmus.villemoes@prevas.dk <mailto:rasmus.villemoes@prevas.dk>>
>     > Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
>     >
>     > On 08.11.21 16:28, Roman Kopytin wrote:
>     >> In order to reduce the coupling between building the kernel and
>     >> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>     >> without simultaneously signing a FIT image. That tool doesn't
>     seem to
>     >> exist, so I stole the necessary pieces from mkimage et al and put it
>     >> in a single .c file.
>     >>
>     >> I'm still working on the details of my proposed "require just k out
>     >> these n required keys" and how it should be implemented, but it will
>     >> probably involve teaching this tool a bunch of new options. These
>     >> patches are not necessarily ready for inclusion (unless someone else
>     >> finds fdt_add_pubkey useful as is), but I thought I might as well
>     send
>     >> it out for early comments.
>     >
>     > I'd also like to see the usage of this hooked into the build process.
>     >
>     > And to my understanding of [1], that approach will provide a
>     feature that permits hooking with the build but would expect the key
>     as dtsi fragment. Can we consolidate the approaches?
>     >
>     > My current vision of a user interface would be a Kconfig option
>     that takes a list of key files to be injected. Maybe make that three
>     lists, one for "required=image", one for "required=conf", and one
>     for optional keys (if that has a use case in practice, no idea).
>     >
>     > Jan
>     >
>     > [1]
>     >
>     https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>     <https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/>
>     >
>     > --
>     > Siemens AG, T RDA IOT
>     > Corporate Competence Center Embedded Linux
>     >
> 
>     For what would you like to have code? The kconfig addition?
> 
>     diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>     index d3a12be228..a9ed4d4ec4 100644
>     --- a/common/Kconfig.boot
>     +++ b/common/Kconfig.boot
>     @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> 
>      endif # SPL
> 
>     +config FIT_SIGNATURE_PUB_KEYS
>     +       string "Public keys to use for FIT image verification"
>     +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
>     +       help
>     +         Public keys, or certificate files to extract them from,
>     that shall
>     +         be used to verify signed FIT images. The keys will be
>     embedded into
>     +         the control device tree of U-Boot.
>     +
>      endif # FIT
> 
>      config LEGACY_IMAGE_FORMAT
> 
> 
>     But note that we are in a design discussion here, and I'm at least
>     reluctant to code up n-versions without having some common idea where
>     things should move.
> 
>     Jan
> 
>     -- 
>     Siemens AG, T RDA IOT
>     Corporate Competence Center Embedded Linux
> 
> 
> 
> -- 
> 	
> François-Frédéric Ozog | /Director Business Development/
> T: +33.67221.6485
> francois.ozog@linaro.org <mailto:francois.ozog@linaro.org> | Skype: ffozog
> 
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[François Ozog]

Hi Jan

On Tue, 9 Nov 2021 at 13:58, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 09.11.21 13:43, François Ozog wrote:
> > Hi
> >
> > as we are in design discussions, I would promote the idea of not pushing
> > non hardware related things in the DTB that is passed to the kernel.
>
> This was never proposed. The public keys go into the *control* FDTs.
>
> great ;-)

> > Is your use case to allow U-Boot to verify the kernel's signature ?
> > Why not putting it into an environment variable?
>
> This is both about validating OS FIT containers as well as SPL checking
> U-Boot proper before continuing the boot there. The former case can be
> replaced with UEFI logic and likely taking the key from TEE (we do that
> as well in first prototypes), but the latter can't (yet).
>
> >
> > If your use case is on Arm or RISC-V, both environments are working
> > heavily to make https://arm-software.github.io/ebbr/
> > <https://arm-software.github.io/ebbr/> standard available on a large
> > number of boards.
> > This offers UEFI interface and SecureBoot (and later MeasuredBoot)
> > services. For Arm boards just check for SystemReady compliance.
> > In this context, traditional UEFI secure variables are used to deal with
> > certificates and hashes: PK, KEK, db...
> > You can obviously do differently but you will be on your own to extend
> > the chain of trust to IMA, secure containers (rooted down to hRoT) and
> > other security facilities in the Linux side.
> > Could you describe your use case in more details?
>
> doc/uImage.FIT/signature.txt ;)
>
> More concrete: We are currently massaging board/siemens/iot2050 to close
> its static chain of trust between SPL and U-Boot main, using software
> means (vendor means do not work because FSBL key != TF-A/TEE/U-Boot key).
>
> And will the chain of trust be continuous or with a very brief breakage
with this approach?
I other words, can you enroll the  TF-A/TEE/U-Boot key so that it can be
trusted by FSBL key?

Jan
>
> >
> > On Tue, 9 Nov 2021 at 11:07, Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >     On 09.11.21 10:37, Roman Kopytin wrote:
> >     > Can we have discussion with code lines? For me it is not very
> >     clear, because it isn't my code.
> >     >
> >
> >     Please do not top-post.
> >
> >     > -----Original Message-----
> >     > From: Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>
> >     > Sent: Tuesday, November 9, 2021 12:17 PM
> >     > To: Roman Kopytin <Roman.Kopytin@kaspersky.com
> >     <mailto:Roman.Kopytin@kaspersky.com>>; u-boot@lists.denx.de
> >     <mailto:u-boot@lists.denx.de>; Rasmus Villemoes
> >     <rasmus.villemoes@prevas.dk <mailto:rasmus.villemoes@prevas.dk>>
> >     > Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >     >
> >     > On 08.11.21 16:28, Roman Kopytin wrote:
> >     >> In order to reduce the coupling between building the kernel and
> >     >> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >     >> without simultaneously signing a FIT image. That tool doesn't
> >     seem to
> >     >> exist, so I stole the necessary pieces from mkimage et al and put
> it
> >     >> in a single .c file.
> >     >>
> >     >> I'm still working on the details of my proposed "require just k
> out
> >     >> these n required keys" and how it should be implemented, but it
> will
> >     >> probably involve teaching this tool a bunch of new options. These
> >     >> patches are not necessarily ready for inclusion (unless someone
> else
> >     >> finds fdt_add_pubkey useful as is), but I thought I might as well
> >     send
> >     >> it out for early comments.
> >     >
> >     > I'd also like to see the usage of this hooked into the build
> process.
> >     >
> >     > And to my understanding of [1], that approach will provide a
> >     feature that permits hooking with the build but would expect the key
> >     as dtsi fragment. Can we consolidate the approaches?
> >     >
> >     > My current vision of a user interface would be a Kconfig option
> >     that takes a list of key files to be injected. Maybe make that three
> >     lists, one for "required=image", one for "required=conf", and one
> >     for optional keys (if that has a use case in practice, no idea).
> >     >
> >     > Jan
> >     >
> >     > [1]
> >     >
> >
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >     <
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >
> >     >
> >     > --
> >     > Siemens AG, T RDA IOT
> >     > Corporate Competence Center Embedded Linux
> >     >
> >
> >     For what would you like to have code? The kconfig addition?
> >
> >     diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >     index d3a12be228..a9ed4d4ec4 100644
> >     --- a/common/Kconfig.boot
> >     +++ b/common/Kconfig.boot
> >     @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >
> >      endif # SPL
> >
> >     +config FIT_SIGNATURE_PUB_KEYS
> >     +       string "Public keys to use for FIT image verification"
> >     +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> >     +       help
> >     +         Public keys, or certificate files to extract them from,
> >     that shall
> >     +         be used to verify signed FIT images. The keys will be
> >     embedded into
> >     +         the control device tree of U-Boot.
> >     +
> >      endif # FIT
> >
> >      config LEGACY_IMAGE_FORMAT
> >
> >
> >     But note that we are in a design discussion here, and I'm at least
> >     reluctant to code up n-versions without having some common idea where
> >     things should move.
> >
> >     Jan
> >
> >     --
> >     Siemens AG, T RDA IOT
> >     Corporate Competence Center Embedded Linux
> >
> >
> >
> > --
> >
> > François-Frédéric Ozog | /Director Business Development/
> > T: +33.67221.6485
> > francois.ozog@linaro.org <mailto:francois.ozog@linaro.org
> > | Skype: ffozog
> >
> >
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
>


-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 09.11.21 14:16, François Ozog wrote:
> Hi Jan
> 
> On Tue, 9 Nov 2021 at 13:58, Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 09.11.21 13:43, François Ozog wrote:
>     > Hi
>     >
>     > as we are in design discussions, I would promote the idea of not
>     pushing
>     > non hardware related things in the DTB that is passed to the kernel.
> 
>     This was never proposed. The public keys go into the *control* FDTs.
> 
> great ;-) 
> 
>     > Is your use case to allow U-Boot to verify the kernel's signature ?
>     > Why not putting it into an environment variable?
> 
>     This is both about validating OS FIT containers as well as SPL checking
>     U-Boot proper before continuing the boot there. The former case can be
>     replaced with UEFI logic and likely taking the key from TEE (we do that
>     as well in first prototypes), but the latter can't (yet).
> 
>     >
>     > If your use case is on Arm or RISC-V, both environments are working
>     > heavily to make https://arm-software.github.io/ebbr/
>     <https://arm-software.github.io/ebbr/>
>     > <https://arm-software.github.io/ebbr/
>     <https://arm-software.github.io/ebbr/>> standard available on a large
>     > number of boards.
>     > This offers UEFI interface and SecureBoot (and later MeasuredBoot)
>     > services. For Arm boards just check for SystemReady compliance.
>     > In this context, traditional UEFI secure variables are used to
>     deal with
>     > certificates and hashes: PK, KEK, db...
>     > You can obviously do differently but you will be on your own to extend
>     > the chain of trust to IMA, secure containers (rooted down to hRoT) and
>     > other security facilities in the Linux side.
>     > Could you describe your use case in more details?
> 
>     doc/uImage.FIT/signature.txt ;)
> 
>     More concrete: We are currently massaging board/siemens/iot2050 to close
>     its static chain of trust between SPL and U-Boot main, using software
>     means (vendor means do not work because FSBL key != TF-A/TEE/U-Boot
>     key).
> 
> And will the chain of trust be continuous or with a very brief breakage
> with this approach?
> I other words, can you enroll the  TF-A/TEE/U-Boot key so that it can be
> trusted by FSBL key?

The chain will be continuous. Both FSBL and TF-A/TEE/SPL will be
OTP-anchored (but deployed at different times / by different
authorities), FSBL will validate based on OTP, SPL should do that based
on control FDT key. Therefore, we need embedding into the SPL FDT in
this case.

If the user decides to continue "classically" by loading the OS from a
signed FIT image, also U-Boot proper needs the public key in its control
FDT.

That's just one (likely more special) scenario, but I bet there are
plenty others on other SOCs / systems.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[François Ozog]

Hi Jan,

On Tue, 9 Nov 2021 at 15:00, Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 09.11.21 14:16, François Ozog wrote:
> > Hi Jan
> >
> > On Tue, 9 Nov 2021 at 13:58, Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >     On 09.11.21 13:43, François Ozog wrote:
> >     > Hi
> >     >
> >     > as we are in design discussions, I would promote the idea of not
> >     pushing
> >     > non hardware related things in the DTB that is passed to the
> kernel.
> >
> >     This was never proposed. The public keys go into the *control* FDTs.
> >
> > great ;-)
> >
> >     > Is your use case to allow U-Boot to verify the kernel's signature ?
> >     > Why not putting it into an environment variable?
> >
> >     This is both about validating OS FIT containers as well as SPL
> checking
> >     U-Boot proper before continuing the boot there. The former case can
> be
> >     replaced with UEFI logic and likely taking the key from TEE (we do
> that
> >     as well in first prototypes), but the latter can't (yet).
> >
> >     >
> >     > If your use case is on Arm or RISC-V, both environments are working
> >     > heavily to make https://arm-software.github.io/ebbr/
> >     <https://arm-software.github.io/ebbr/>
> >     > <https://arm-software.github.io/ebbr/
> >     <https://arm-software.github.io/ebbr/>> standard available on a
> large
> >     > number of boards.
> >     > This offers UEFI interface and SecureBoot (and later MeasuredBoot)
> >     > services. For Arm boards just check for SystemReady compliance.
> >     > In this context, traditional UEFI secure variables are used to
> >     deal with
> >     > certificates and hashes: PK, KEK, db...
> >     > You can obviously do differently but you will be on your own to
> extend
> >     > the chain of trust to IMA, secure containers (rooted down to hRoT)
> and
> >     > other security facilities in the Linux side.
> >     > Could you describe your use case in more details?
> >
> >     doc/uImage.FIT/signature.txt ;)
> >
> >     More concrete: We are currently massaging board/siemens/iot2050 to
> close
> >     its static chain of trust between SPL and U-Boot main, using software
> >     means (vendor means do not work because FSBL key != TF-A/TEE/U-Boot
> >     key).
> >
> > And will the chain of trust be continuous or with a very brief breakage
> > with this approach?
> > I other words, can you enroll the  TF-A/TEE/U-Boot key so that it can be
> > trusted by FSBL key?
>
> The chain will be continuous. Both FSBL and TF-A/TEE/SPL will be
> OTP-anchored (but deployed at different times / by different
> authorities), FSBL will validate based on OTP, SPL should do that based
> on control FDT key. Therefore, we need embedding into the SPL FDT in
> this case.
>
> If the user decides to continue "classically" by loading the OS from a
> signed FIT image, also U-Boot proper needs the public key in its control
> FDT.
>
> That's just one (likely more special) scenario, but I bet there are
> plenty others on other SOCs / systems.
>
> Get it. Thanks for detailing the case, its very useful for me.

> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
>


-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 09.11.21 10:37, Roman Kopytin wrote:
> > Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
> >
>
> Please do not top-post.
>
> > -----Original Message-----
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > Sent: Tuesday, November 9, 2021 12:17 PM
> > To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >
> > On 08.11.21 16:28, Roman Kopytin wrote:
> >> In order to reduce the coupling between building the kernel and
> >> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >> without simultaneously signing a FIT image. That tool doesn't seem to
> >> exist, so I stole the necessary pieces from mkimage et al and put it
> >> in a single .c file.
> >>
> >> I'm still working on the details of my proposed "require just k out
> >> these n required keys" and how it should be implemented, but it will
> >> probably involve teaching this tool a bunch of new options. These
> >> patches are not necessarily ready for inclusion (unless someone else
> >> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >> it out for early comments.
> >
> > I'd also like to see the usage of this hooked into the build process.
> >
> > And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
> >
> > My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
> >
> > Jan
> >
> > [1]
> > https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >
> > --
> > Siemens AG, T RDA IOT
> > Corporate Competence Center Embedded Linux
> >
>
> For what would you like to have code? The kconfig addition?
>
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index d3a12be228..a9ed4d4ec4 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>
>  endif # SPL
>
> +config FIT_SIGNATURE_PUB_KEYS
> +       string "Public keys to use for FIT image verification"
> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> +       help
> +         Public keys, or certificate files to extract them from, that shall
> +         be used to verify signed FIT images. The keys will be embedded into
> +         the control device tree of U-Boot.
> +
>  endif # FIT
>
>  config LEGACY_IMAGE_FORMAT
>
>
> But note that we are in a design discussion here, and I'm at least
> reluctant to code up n-versions without having some common idea where
> things should move.

I'm not sure we want this built into U-Boot. I see signing of a
firmware image as a final step, with the keys being added then, e.g.
by binman.

Regards,
Simon

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi François,

On Tue, 9 Nov 2021 at 05:43, François Ozog <francois.ozog@linaro.org> wrote:
>
> Hi
>
> as we are in design discussions, I would promote the idea of not pushing
> non hardware related things in the DTB that is passed to the kernel.
> Is your use case to allow U-Boot to verify the kernel's signature ?
> Why not putting it into an environment variable?

We have been through this many times but I'll state it again. U-Boot
makes use of the devicetree for configuration information, including
public keys, etc.

It does not belong anywhere else.

http://patchwork.ozlabs.org/project/uboot/patch/20211026002344.405160-3-sjg@chromium.org/


>
> If your use case is on Arm or RISC-V, both environments are working heavily
> to make https://arm-software.github.io/ebbr/ standard available on a large
> number of boards.
> This offers UEFI interface and SecureBoot (and later MeasuredBoot)
> services. For Arm boards just check for SystemReady compliance.
> In this context, traditional UEFI secure variables are used to deal with
> certificates and hashes: PK, KEK, db...
> You can obviously do differently but you will be on your own to extend the
> chain of trust to IMA, secure containers (rooted down to hRoT) and other
> security facilities in the Linux side.
> Could you describe your use case in more details?
>

That feature does not use FIT images, nor U-Boot's verified boot.

Regards,
Simon


> On Tue, 9 Nov 2021 at 11:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> > On 09.11.21 10:37, Roman Kopytin wrote:
> > > Can we have discussion with code lines? For me it is not very clear,
> > because it isn't my code.
> > >
> >
> > Please do not top-post.
> >
> > > -----Original Message-----
> > > From: Jan Kiszka <jan.kiszka@siemens.com>
> > > Sent: Tuesday, November 9, 2021 12:17 PM
> > > To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de;
> > Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> > >
> > > On 08.11.21 16:28, Roman Kopytin wrote:
> > >> In order to reduce the coupling between building the kernel and
> > >> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> > >> without simultaneously signing a FIT image. That tool doesn't seem to
> > >> exist, so I stole the necessary pieces from mkimage et al and put it
> > >> in a single .c file.
> > >>
> > >> I'm still working on the details of my proposed "require just k out
> > >> these n required keys" and how it should be implemented, but it will
> > >> probably involve teaching this tool a bunch of new options. These
> > >> patches are not necessarily ready for inclusion (unless someone else
> > >> finds fdt_add_pubkey useful as is), but I thought I might as well send
> > >> it out for early comments.
> > >
> > > I'd also like to see the usage of this hooked into the build process.
> > >
> > > And to my understanding of [1], that approach will provide a feature
> > that permits hooking with the build but would expect the key as dtsi
> > fragment. Can we consolidate the approaches?
> > >
> > > My current vision of a user interface would be a Kconfig option that
> > takes a list of key files to be injected. Maybe make that three lists, one
> > for "required=image", one for "required=conf", and one for optional keys
> > (if that has a use case in practice, no idea).
> > >
> > > Jan
> > >
> > > [1]
> > >
> > https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> > >
> > > --
> > > Siemens AG, T RDA IOT
> > > Corporate Competence Center Embedded Linux
> > >
> >
> > For what would you like to have code? The kconfig addition?
> >
> > diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> > index d3a12be228..a9ed4d4ec4 100644
> > --- a/common/Kconfig.boot
> > +++ b/common/Kconfig.boot
> > @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >
> >  endif # SPL
> >
> > +config FIT_SIGNATURE_PUB_KEYS
> > +       string "Public keys to use for FIT image verification"
> > +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> > +       help
> > +         Public keys, or certificate files to extract them from, that
> > shall
> > +         be used to verify signed FIT images. The keys will be embedded
> > into
> > +         the control device tree of U-Boot.
> > +
> >  endif # FIT
> >
> >  config LEGACY_IMAGE_FORMAT
> >
> >
> > But note that we are in a design discussion here, and I'm at least
> > reluctant to code up n-versions without having some common idea where
> > things should move.
> >
> > Jan
> >
> > --
> > Siemens AG, T RDA IOT
> > Corporate Competence Center Embedded Linux
> >
>
>
> --
> François-Frédéric Ozog | *Director Business Development*
> T: +33.67221.6485
> francois.ozog@linaro.org | Skype: ffozog

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi,

On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 08.11.21 16:28, Roman Kopytin wrote:
> > In order to reduce the coupling between building the kernel and
> > U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> > without simultaneously signing a FIT image. That tool doesn't seem to
> > exist, so I stole the necessary pieces from mkimage et al and put it
> > in a single .c file.
> >
> > I'm still working on the details of my proposed "require just k out
> > these n required keys" and how it should be implemented, but it will
> > probably involve teaching this tool a bunch of new options. These
> > patches are not necessarily ready for inclusion (unless someone else
> > finds fdt_add_pubkey useful as is), but I thought I might as well send
> > it out for early comments.
>
> I'd also like to see the usage of this hooked into the build process.
>
> And to my understanding of [1], that approach will provide a feature
> that permits hooking with the build but would expect the key as dtsi
> fragment. Can we consolidate the approaches?
>
> My current vision of a user interface would be a Kconfig option that
> takes a list of key files to be injected. Maybe make that three lists,
> one for "required=image", one for "required=conf", and one for optional
> keys (if that has a use case in practice, no idea).

Also please take a look at binman which is designed to handle create
(or later updating from Yocto) the devicetree or firmware image.

Regards,
Simon


>
> Jan
>
> [1]
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Simon Glass]

Hi Roman,

On Mon, 8 Nov 2021 at 08:29, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>
> 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",

Some of these are not optional so should not have [] around them.

> +               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));

'\0'

> +
> +       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);
> +       }

Can you please put the block above and the loop below into a separate
function that returns an error code? Then you can print that out at
the bottom, with a single 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");

debug() I think. This isn't very important. BTW I found that 512 bytes
is enough, if you want to use that, but 1024 is fine too.

> +               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
>

Regards,
Simon

Re: [PATCH 2/2] test_vboot.py: include test of fdt_add_pubkey tool

[Simon Glass]

Hi Roman,

On Mon, 8 Nov 2021 at 08:29, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>

<please add a commit message here>

> 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])

Looks good!

>
>          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
>

Regards,
Simon

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

On 08.11.21 16:28, Roman Kopytin wrote:
> 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);
> +}
> +
> 

git says: "new blank line at EOF."

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 01:58, Simon Glass wrote:
> Hi Jan,
> 
> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 09.11.21 10:37, Roman Kopytin wrote:
>>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
>>>
>>
>> Please do not top-post.
>>
>>> -----Original Message-----
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>> Sent: Tuesday, November 9, 2021 12:17 PM
>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
>>>
>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>> In order to reduce the coupling between building the kernel and
>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>> in a single .c file.
>>>>
>>>> I'm still working on the details of my proposed "require just k out
>>>> these n required keys" and how it should be implemented, but it will
>>>> probably involve teaching this tool a bunch of new options. These
>>>> patches are not necessarily ready for inclusion (unless someone else
>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>> it out for early comments.
>>>
>>> I'd also like to see the usage of this hooked into the build process.
>>>
>>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
>>>
>>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
>>>
>>> Jan
>>>
>>> [1]
>>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>>>
>>> --
>>> Siemens AG, T RDA IOT
>>> Corporate Competence Center Embedded Linux
>>>
>>
>> For what would you like to have code? The kconfig addition?
>>
>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>> index d3a12be228..a9ed4d4ec4 100644
>> --- a/common/Kconfig.boot
>> +++ b/common/Kconfig.boot
>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>>
>>  endif # SPL
>>
>> +config FIT_SIGNATURE_PUB_KEYS
>> +       string "Public keys to use for FIT image verification"
>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
>> +       help
>> +         Public keys, or certificate files to extract them from, that shall
>> +         be used to verify signed FIT images. The keys will be embedded into
>> +         the control device tree of U-Boot.
>> +
>>  endif # FIT
>>
>>  config LEGACY_IMAGE_FORMAT
>>
>>
>> But note that we are in a design discussion here, and I'm at least
>> reluctant to code up n-versions without having some common idea where
>> things should move.
> 
> I'm not sure we want this built into U-Boot. I see signing of a
> firmware image as a final step, with the keys being added then, e.g.
> by binman.

This is not signing, this in embedding public key information into build
artifacts before they can be signed. As pointed out in my other thread,
not having an embedding feature is a major drawback of the current
workflow. It easily forces you to rebuild existing build flows in
out-of-tree scripts.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 01:58, Simon Glass wrote:
> Hi,
> 
> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 08.11.21 16:28, Roman Kopytin wrote:
>>> In order to reduce the coupling between building the kernel and
>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>> in a single .c file.
>>>
>>> I'm still working on the details of my proposed "require just k out
>>> these n required keys" and how it should be implemented, but it will
>>> probably involve teaching this tool a bunch of new options. These
>>> patches are not necessarily ready for inclusion (unless someone else
>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>> it out for early comments.
>>
>> I'd also like to see the usage of this hooked into the build process.
>>
>> And to my understanding of [1], that approach will provide a feature
>> that permits hooking with the build but would expect the key as dtsi
>> fragment. Can we consolidate the approaches?
>>
>> My current vision of a user interface would be a Kconfig option that
>> takes a list of key files to be injected. Maybe make that three lists,
>> one for "required=image", one for "required=conf", and one for optional
>> keys (if that has a use case in practice, no idea).
> 
> Also please take a look at binman which is designed to handle create
> (or later updating from Yocto) the devicetree or firmware image.
> 

Yes, binman is another problem area, but not for the public key
injection, rather for permitting to sign fit images that are described
for binman (rather than for mkimage). I'm currently back to dd for
signing the U-Boot container in
arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
that FIT image description from that file - both not optimal.

And another area: Trust centers that perform the signing (and only that)
usually do not support random formats and workflows but just few common
ones, e.g. x509. It would be nice to have a way to route out the payload
(hashes etc.) that mkimage would sign, ideally into a standard signing
request, and permit to inject the resulting signature at the right
places into the FIT image.

But one after the other.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

RE: [PATCH 1/2] tools: add fdt_add_pubkey

[Roman Kopytin]

Hi, Simon
I have question about:
Some of these are not optional so should not have [] around them.

As I see we have defaults for:
static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */
static const char *keydir = "."; /* -k <keydir> */
static const char *keyname = "key"; /* -n <keyname> */

It means that we can skip it in command line. Should I need to remove [] from code for those parameters?

-----Original Message-----
From: Simon Glass <sjg@chromium.org> 
Sent: Wednesday, November 10, 2021 3:58 AM
To: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH 1/2] tools: add fdt_add_pubkey

Hi Roman,

On Mon, 8 Nov 2021 at 08:29, Roman Kopytin <Roman.Kopytin@kaspersky.com> wrote:
>
> 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",

Some of these are not optional so should not have [] around them.

> +               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));

'\0'

> +
> +       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);
> +       }

Can you please put the block above and the loop below into a separate function that returns an error code? Then you can print that out at the bottom, with a single 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");

debug() I think. This isn't very important. BTW I found that 512 bytes is enough, if you want to use that, but 1024 is fine too.

> +               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
>

Regards,
Simon

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 07:55, Jan Kiszka wrote:
> On 10.11.21 01:58, Simon Glass wrote:
>> Hi,
>>
>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>> In order to reduce the coupling between building the kernel and
>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>> in a single .c file.
>>>>
>>>> I'm still working on the details of my proposed "require just k out
>>>> these n required keys" and how it should be implemented, but it will
>>>> probably involve teaching this tool a bunch of new options. These
>>>> patches are not necessarily ready for inclusion (unless someone else
>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>> it out for early comments.
>>>
>>> I'd also like to see the usage of this hooked into the build process.
>>>
>>> And to my understanding of [1], that approach will provide a feature
>>> that permits hooking with the build but would expect the key as dtsi
>>> fragment. Can we consolidate the approaches?
>>>
>>> My current vision of a user interface would be a Kconfig option that
>>> takes a list of key files to be injected. Maybe make that three lists,
>>> one for "required=image", one for "required=conf", and one for optional
>>> keys (if that has a use case in practice, no idea).
>>
>> Also please take a look at binman which is designed to handle create
>> (or later updating from Yocto) the devicetree or firmware image.
>>
> 
> Yes, binman is another problem area, but not for the public key
> injection, rather for permitting to sign fit images that are described
> for binman (rather than for mkimage). I'm currently back to dd for
> signing the U-Boot container in
> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
> that FIT image description from that file - both not optimal.

OK, this can already be optimized with "binman replace" - once I
understood where fdtmap can go and where not. Why no support for using
map files?

Jan

> 
> And another area: Trust centers that perform the signing (and only that)
> usually do not support random formats and workflows but just few common
> ones, e.g. x509. It would be nice to have a way to route out the payload
> (hashes etc.) that mkimage would sign, ideally into a standard signing
> request, and permit to inject the resulting signature at the right
> places into the FIT image.
> 
> But one after the other.
> 
> Jan
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

On 08.11.21 16:28, Roman Kopytin wrote:
> 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);

The tool should support --help and document the default of these
optional parameters that way.

Is there an easy way to derive algo from the key?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

[no top-posting please]

On 10.11.21 08:03, Roman Kopytin wrote:
> Hi, Simon
> I have question about:
> Some of these are not optional so should not have [] around them.
> 
> As I see we have defaults for:
> static const char *algo_name = "sha1,rsa2048"; /* -a <algo> */
> static const char *keydir = "."; /* -k <keydir> */
> static const char *keyname = "key"; /* -n <keyname> */
> 
> It means that we can skip it in command line. Should I need to remove [] from code for those parameters?
> 

Those have defaults, and if you place a rsa2048 key in ./key.key,
fdt_add_pubkey will be happy.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

RE: [PATCH 1/2] tools: add fdt_add_pubkey

[Roman Kopytin]

Could you please provide good example with needed style for helper?
In tools I saw a lot of programs w/o help.

-----Original Message-----
From: Jan Kiszka <jan.kiszka@siemens.com> 
Sent: Wednesday, November 10, 2021 10:39 AM
To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH 1/2] tools: add fdt_add_pubkey

On 08.11.21 16:28, Roman Kopytin wrote:
> 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);

The tool should support --help and document the default of these optional parameters that way.

Is there an easy way to derive algo from the key?

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 01:58, Simon Glass wrote:
> > Hi Jan,
> >
> > On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 09.11.21 10:37, Roman Kopytin wrote:
> >>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
> >>>
> >>
> >> Please do not top-post.
> >>
> >>> -----Original Message-----
> >>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Sent: Tuesday, November 9, 2021 12:17 PM
> >>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >>>
> >>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>> In order to reduce the coupling between building the kernel and
> >>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>> in a single .c file.
> >>>>
> >>>> I'm still working on the details of my proposed "require just k out
> >>>> these n required keys" and how it should be implemented, but it will
> >>>> probably involve teaching this tool a bunch of new options. These
> >>>> patches are not necessarily ready for inclusion (unless someone else
> >>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>> it out for early comments.
> >>>
> >>> I'd also like to see the usage of this hooked into the build process.
> >>>
> >>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
> >>>
> >>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
> >>>
> >>> Jan
> >>>
> >>> [1]
> >>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >>>
> >>> --
> >>> Siemens AG, T RDA IOT
> >>> Corporate Competence Center Embedded Linux
> >>>
> >>
> >> For what would you like to have code? The kconfig addition?
> >>
> >> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >> index d3a12be228..a9ed4d4ec4 100644
> >> --- a/common/Kconfig.boot
> >> +++ b/common/Kconfig.boot
> >> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >>
> >>  endif # SPL
> >>
> >> +config FIT_SIGNATURE_PUB_KEYS
> >> +       string "Public keys to use for FIT image verification"
> >> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> >> +       help
> >> +         Public keys, or certificate files to extract them from, that shall
> >> +         be used to verify signed FIT images. The keys will be embedded into
> >> +         the control device tree of U-Boot.
> >> +
> >>  endif # FIT
> >>
> >>  config LEGACY_IMAGE_FORMAT
> >>
> >>
> >> But note that we are in a design discussion here, and I'm at least
> >> reluctant to code up n-versions without having some common idea where
> >> things should move.
> >
> > I'm not sure we want this built into U-Boot. I see signing of a
> > firmware image as a final step, with the keys being added then, e.g.
> > by binman.
>
> This is not signing, this in embedding public key information into build
> artifacts before they can be signed. As pointed out in my other thread,
> not having an embedding feature is a major drawback of the current
> workflow. It easily forces you to rebuild existing build flows in
> out-of-tree scripts.

The public key is not needed for signing to work, right? I don't
understand what you are getting at here. If you want to add the public
key to the image before it is signed, that's fine. I just don't
understand why you want to do that. Why not have the signer do
everything?

Regards,
Simon

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 00:20, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 07:55, Jan Kiszka wrote:
> > On 10.11.21 01:58, Simon Glass wrote:
> >> Hi,
> >>
> >> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>
> >>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>> In order to reduce the coupling between building the kernel and
> >>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>> in a single .c file.
> >>>>
> >>>> I'm still working on the details of my proposed "require just k out
> >>>> these n required keys" and how it should be implemented, but it will
> >>>> probably involve teaching this tool a bunch of new options. These
> >>>> patches are not necessarily ready for inclusion (unless someone else
> >>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>> it out for early comments.
> >>>
> >>> I'd also like to see the usage of this hooked into the build process.
> >>>
> >>> And to my understanding of [1], that approach will provide a feature
> >>> that permits hooking with the build but would expect the key as dtsi
> >>> fragment. Can we consolidate the approaches?
> >>>
> >>> My current vision of a user interface would be a Kconfig option that
> >>> takes a list of key files to be injected. Maybe make that three lists,
> >>> one for "required=image", one for "required=conf", and one for optional
> >>> keys (if that has a use case in practice, no idea).
> >>
> >> Also please take a look at binman which is designed to handle create
> >> (or later updating from Yocto) the devicetree or firmware image.
> >>
> >
> > Yes, binman is another problem area, but not for the public key
> > injection, rather for permitting to sign fit images that are described
> > for binman (rather than for mkimage). I'm currently back to dd for
> > signing the U-Boot container in
> > arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
> > that FIT image description from that file - both not optimal.

Well I don't think binman supports that at present, or at least I'm
not sure what it would do. We don't have a test case for it. If you
have an idea for how it should work, please send some ideas and I can
look at it.

>
> OK, this can already be optimized with "binman replace" - once I
> understood where fdtmap can go and where not. Why no support for using
> map files?

The fdtmap provides enough information to extract anything from the
image and regenerate/replace things.

What is a map file?

>
> Jan
>
> >
> > And another area: Trust centers that perform the signing (and only that)
> > usually do not support random formats and workflows but just few common
> > ones, e.g. x509. It would be nice to have a way to route out the payload
> > (hashes etc.) that mkimage would sign, ideally into a standard signing
> > request, and permit to inject the resulting signature at the right
> > places into the FIT image.

Well that needs to be provided somewhere. It should be fairly easy to
get Binman to do this, so long as the image description has info about
what is being signed.

> >
> > But one after the other.

Possibly, but sometimes it is best to design things up-front.

Regards,
Simon

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 17:31, Simon Glass wrote:
> Hi Jan,
> 
> On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 10.11.21 01:58, Simon Glass wrote:
>>> Hi Jan,
>>>
>>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 09.11.21 10:37, Roman Kopytin wrote:
>>>>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
>>>>>
>>>>
>>>> Please do not top-post.
>>>>
>>>>> -----Original Message-----
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> Sent: Tuesday, November 9, 2021 12:17 PM
>>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
>>>>>
>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>>>> In order to reduce the coupling between building the kernel and
>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>>>> in a single .c file.
>>>>>>
>>>>>> I'm still working on the details of my proposed "require just k out
>>>>>> these n required keys" and how it should be implemented, but it will
>>>>>> probably involve teaching this tool a bunch of new options. These
>>>>>> patches are not necessarily ready for inclusion (unless someone else
>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>>>> it out for early comments.
>>>>>
>>>>> I'd also like to see the usage of this hooked into the build process.
>>>>>
>>>>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
>>>>>
>>>>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
>>>>>
>>>>> Jan
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>>>>>
>>>>> --
>>>>> Siemens AG, T RDA IOT
>>>>> Corporate Competence Center Embedded Linux
>>>>>
>>>>
>>>> For what would you like to have code? The kconfig addition?
>>>>
>>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>>>> index d3a12be228..a9ed4d4ec4 100644
>>>> --- a/common/Kconfig.boot
>>>> +++ b/common/Kconfig.boot
>>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>>>>
>>>>  endif # SPL
>>>>
>>>> +config FIT_SIGNATURE_PUB_KEYS
>>>> +       string "Public keys to use for FIT image verification"
>>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
>>>> +       help
>>>> +         Public keys, or certificate files to extract them from, that shall
>>>> +         be used to verify signed FIT images. The keys will be embedded into
>>>> +         the control device tree of U-Boot.
>>>> +
>>>>  endif # FIT
>>>>
>>>>  config LEGACY_IMAGE_FORMAT
>>>>
>>>>
>>>> But note that we are in a design discussion here, and I'm at least
>>>> reluctant to code up n-versions without having some common idea where
>>>> things should move.
>>>
>>> I'm not sure we want this built into U-Boot. I see signing of a
>>> firmware image as a final step, with the keys being added then, e.g.
>>> by binman.
>>
>> This is not signing, this in embedding public key information into build
>> artifacts before they can be signed. As pointed out in my other thread,
>> not having an embedding feature is a major drawback of the current
>> workflow. It easily forces you to rebuild existing build flows in
>> out-of-tree scripts.
> 
> The public key is not needed for signing to work, right? I don't
> understand what you are getting at here. If you want to add the public
> key to the image before it is signed, that's fine. I just don't
> understand why you want to do that. Why not have the signer do
> everything?

A) Because sensitive signing environments will not run arbitrary logic.
   They will hand out the public key, but they may not give you the
   chance to run mkimage with the private key, like you would do during
   development.

B) It avoids having to run the signing process in a specific order
   because it already embeds the public key during build, thus
   generates everything that shall be signed upfront.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 17:31, Simon Glass wrote:
> Hi Jan,
> 
> On Wed, 10 Nov 2021 at 00:20, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 10.11.21 07:55, Jan Kiszka wrote:
>>> On 10.11.21 01:58, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>
>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>>>> In order to reduce the coupling between building the kernel and
>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>>>> in a single .c file.
>>>>>>
>>>>>> I'm still working on the details of my proposed "require just k out
>>>>>> these n required keys" and how it should be implemented, but it will
>>>>>> probably involve teaching this tool a bunch of new options. These
>>>>>> patches are not necessarily ready for inclusion (unless someone else
>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>>>> it out for early comments.
>>>>>
>>>>> I'd also like to see the usage of this hooked into the build process.
>>>>>
>>>>> And to my understanding of [1], that approach will provide a feature
>>>>> that permits hooking with the build but would expect the key as dtsi
>>>>> fragment. Can we consolidate the approaches?
>>>>>
>>>>> My current vision of a user interface would be a Kconfig option that
>>>>> takes a list of key files to be injected. Maybe make that three lists,
>>>>> one for "required=image", one for "required=conf", and one for optional
>>>>> keys (if that has a use case in practice, no idea).
>>>>
>>>> Also please take a look at binman which is designed to handle create
>>>> (or later updating from Yocto) the devicetree or firmware image.
>>>>
>>>
>>> Yes, binman is another problem area, but not for the public key
>>> injection, rather for permitting to sign fit images that are described
>>> for binman (rather than for mkimage). I'm currently back to dd for
>>> signing the U-Boot container in
>>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
>>> that FIT image description from that file - both not optimal.
> 
> Well I don't think binman supports that at present, or at least I'm
> not sure what it would do. We don't have a test case for it. If you
> have an idea for how it should work, please send some ideas and I can
> look at it.
> 
>>
>> OK, this can already be optimized with "binman replace" - once I
>> understood where fdtmap can go and where not. Why no support for using
>> map files?
> 
> The fdtmap provides enough information to extract anything from the
> image and regenerate/replace things.
> 
> What is a map file?

*.map, e.g. image.map? Also generated by many binmap <cmd> -m?

> 
>>
>> Jan
>>
>>>
>>> And another area: Trust centers that perform the signing (and only that)
>>> usually do not support random formats and workflows but just few common
>>> ones, e.g. x509. It would be nice to have a way to route out the payload
>>> (hashes etc.) that mkimage would sign, ideally into a standard signing
>>> request, and permit to inject the resulting signature at the right
>>> places into the FIT image.
> 
> Well that needs to be provided somewhere. It should be fairly easy to
> get Binman to do this, so long as the image description has info about
> what is being signed.

I would assume that it has to have that information, already to use
mkimage on it or its parts.

> 
>>>
>>> But one after the other.
> 
> Possibly, but sometimes it is best to design things up-front.
> 

True as well.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[François Ozog]

Hi Jan

Le mer. 10 nov. 2021 à 17:48, Jan Kiszka <jan.kiszka@siemens.com> a écrit :

> On 10.11.21 17:31, Simon Glass wrote:
> > Hi Jan,
> >
> > On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 10.11.21 01:58, Simon Glass wrote:
> >>> Hi Jan,
> >>>
> >>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com>
> wrote:
> >>>>
> >>>> On 09.11.21 10:37, Roman Kopytin wrote:
> >>>>> Can we have discussion with code lines? For me it is not very clear,
> because it isn't my code.
> >>>>>
> >>>>
> >>>> Please do not top-post.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>> Sent: Tuesday, November 9, 2021 12:17 PM
> >>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>;
> u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >>>>>
> >>>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>>> In order to reduce the coupling between building the kernel and
> >>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>>> without simultaneously signing a FIT image. That tool doesn't seem
> to
> >>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>>> in a single .c file.
> >>>>>>
> >>>>>> I'm still working on the details of my proposed "require just k out
> >>>>>> these n required keys" and how it should be implemented, but it will
> >>>>>> probably involve teaching this tool a bunch of new options. These
> >>>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well
> send
> >>>>>> it out for early comments.
> >>>>>
> >>>>> I'd also like to see the usage of this hooked into the build process.
> >>>>>
> >>>>> And to my understanding of [1], that approach will provide a feature
> that permits hooking with the build but would expect the key as dtsi
> fragment. Can we consolidate the approaches?
> >>>>>
> >>>>> My current vision of a user interface would be a Kconfig option that
> takes a list of key files to be injected. Maybe make that three lists, one
> for "required=image", one for "required=conf", and one for optional keys
> (if that has a use case in practice, no idea).
> >>>>>
> >>>>> Jan
> >>>>>
> >>>>> [1]
> >>>>>
> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >>>>>
> >>>>> --
> >>>>> Siemens AG, T RDA IOT
> >>>>> Corporate Competence Center Embedded Linux
> >>>>>
> >>>>
> >>>> For what would you like to have code? The kconfig addition?
> >>>>
> >>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >>>> index d3a12be228..a9ed4d4ec4 100644
> >>>> --- a/common/Kconfig.boot
> >>>> +++ b/common/Kconfig.boot
> >>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >>>>
> >>>>  endif # SPL
> >>>>
> >>>> +config FIT_SIGNATURE_PUB_KEYS
> >>>> +       string "Public keys to use for FIT image verification"
> >>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> >>>> +       help
> >>>> +         Public keys, or certificate files to extract them from,
> that shall
> >>>> +         be used to verify signed FIT images. The keys will be
> embedded into
> >>>> +         the control device tree of U-Boot.
> >>>> +
> >>>>  endif # FIT
> >>>>
> >>>>  config LEGACY_IMAGE_FORMAT
> >>>>
> >>>>
> >>>> But note that we are in a design discussion here, and I'm at least
> >>>> reluctant to code up n-versions without having some common idea where
> >>>> things should move.
> >>>
> >>> I'm not sure we want this built into U-Boot. I see signing of a
> >>> firmware image as a final step, with the keys being added then, e.g.
> >>> by binman.
> >>
> >> This is not signing, this in embedding public key information into build
> >> artifacts before they can be signed. As pointed out in my other thread,
> >> not having an embedding feature is a major drawback of the current
> >> workflow. It easily forces you to rebuild existing build flows in
> >> out-of-tree scripts.
> >
> > The public key is not needed for signing to work, right? I don't
> > understand what you are getting at here. If you want to add the public
> > key to the image before it is signed, that's fine. I just don't
> > understand why you want to do that. Why not have the signer do
> > everything?
>
> A) Because sensitive signing environments will not run arbitrary logic.
>    They will hand out the public key, but they may not give you the
>    chance to run mkimage with the private key, like you would do during
>    development.
>
thanks for bringing this very important topic on the list. In this case
nobody may have the private key handy as it sits hidden inside an HSM:
correct ?

>
> B) It avoids having to run the signing process in a specific order
>    because it already embeds the public key during build, thus
>    generates everything that shall be signed upfront.
>
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
>
-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 18:29, François Ozog wrote:
> 
> Hi Jan
> 
> Le mer. 10 nov. 2021 à 17:48, Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> a écrit :
> 
>     On 10.11.21 17:31, Simon Glass wrote:
>     > Hi Jan,
>     >
>     > On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>> wrote:
>     >>
>     >> On 10.11.21 01:58, Simon Glass wrote:
>     >>> Hi Jan,
>     >>>
>     >>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>> wrote:
>     >>>>
>     >>>> On 09.11.21 10:37, Roman Kopytin wrote:
>     >>>>> Can we have discussion with code lines? For me it is not very
>     clear, because it isn't my code.
>     >>>>>
>     >>>>
>     >>>> Please do not top-post.
>     >>>>
>     >>>>> -----Original Message-----
>     >>>>> From: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
>     >>>>> Sent: Tuesday, November 9, 2021 12:17 PM
>     >>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com
>     <mailto:Roman.Kopytin@kaspersky.com>>; u-boot@lists.denx.de
>     <mailto:u-boot@lists.denx.de>; Rasmus Villemoes
>     <rasmus.villemoes@prevas.dk <mailto:rasmus.villemoes@prevas.dk>>
>     >>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
>     >>>>>
>     >>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>     >>>>>> In order to reduce the coupling between building the kernel and
>     >>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>     >>>>>> without simultaneously signing a FIT image. That tool doesn't
>     seem to
>     >>>>>> exist, so I stole the necessary pieces from mkimage et al and
>     put it
>     >>>>>> in a single .c file.
>     >>>>>>
>     >>>>>> I'm still working on the details of my proposed "require just
>     k out
>     >>>>>> these n required keys" and how it should be implemented, but
>     it will
>     >>>>>> probably involve teaching this tool a bunch of new options. These
>     >>>>>> patches are not necessarily ready for inclusion (unless
>     someone else
>     >>>>>> finds fdt_add_pubkey useful as is), but I thought I might as
>     well send
>     >>>>>> it out for early comments.
>     >>>>>
>     >>>>> I'd also like to see the usage of this hooked into the build
>     process.
>     >>>>>
>     >>>>> And to my understanding of [1], that approach will provide a
>     feature that permits hooking with the build but would expect the key
>     as dtsi fragment. Can we consolidate the approaches?
>     >>>>>
>     >>>>> My current vision of a user interface would be a Kconfig
>     option that takes a list of key files to be injected. Maybe make
>     that three lists, one for "required=image", one for "required=conf",
>     and one for optional keys (if that has a use case in practice, no idea).
>     >>>>>
>     >>>>> Jan
>     >>>>>
>     >>>>> [1]
>     >>>>>
>     https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>     <https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/>
>     >>>>>
>     >>>>> --
>     >>>>> Siemens AG, T RDA IOT
>     >>>>> Corporate Competence Center Embedded Linux
>     >>>>>
>     >>>>
>     >>>> For what would you like to have code? The kconfig addition?
>     >>>>
>     >>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>     >>>> index d3a12be228..a9ed4d4ec4 100644
>     >>>> --- a/common/Kconfig.boot
>     >>>> +++ b/common/Kconfig.boot
>     >>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>     >>>>
>     >>>>  endif # SPL
>     >>>>
>     >>>> +config FIT_SIGNATURE_PUB_KEYS
>     >>>> +       string "Public keys to use for FIT image verification"
>     >>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
>     >>>> +       help
>     >>>> +         Public keys, or certificate files to extract them
>     from, that shall
>     >>>> +         be used to verify signed FIT images. The keys will be
>     embedded into
>     >>>> +         the control device tree of U-Boot.
>     >>>> +
>     >>>>  endif # FIT
>     >>>>
>     >>>>  config LEGACY_IMAGE_FORMAT
>     >>>>
>     >>>>
>     >>>> But note that we are in a design discussion here, and I'm at least
>     >>>> reluctant to code up n-versions without having some common idea
>     where
>     >>>> things should move.
>     >>>
>     >>> I'm not sure we want this built into U-Boot. I see signing of a
>     >>> firmware image as a final step, with the keys being added then, e.g.
>     >>> by binman.
>     >>
>     >> This is not signing, this in embedding public key information
>     into build
>     >> artifacts before they can be signed. As pointed out in my other
>     thread,
>     >> not having an embedding feature is a major drawback of the current
>     >> workflow. It easily forces you to rebuild existing build flows in
>     >> out-of-tree scripts.
>     >
>     > The public key is not needed for signing to work, right? I don't
>     > understand what you are getting at here. If you want to add the public
>     > key to the image before it is signed, that's fine. I just don't
>     > understand why you want to do that. Why not have the signer do
>     > everything?
> 
>     A) Because sensitive signing environments will not run arbitrary logic.
>        They will hand out the public key, but they may not give you the
>        chance to run mkimage with the private key, like you would do during
>        development.
> 
> thanks for bringing this very important topic on the list. In this case
> nobody may have the private key handy as it sits hidden inside an HSM:
> correct ?

I have no insides where/how it is stored in our case, ie. our trust
center that is managing keys and issuing certificates. In any case,
private keys are definitely not in the hands of the build environments
or product engineers then - even if those have to be trusted as well,
just for different aspects.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

On 10.11.21 09:26, Roman Kopytin wrote:
> Could you please provide good example with needed style for helper?
> In tools I saw a lot of programs w/o help.
> 

Have a look at binman to see this full-blown - not a completely fair
comparison as it benefits from Python argparse.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 09:48, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 17:31, Simon Glass wrote:
> > Hi Jan,
> >
> > On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 10.11.21 01:58, Simon Glass wrote:
> >>> Hi Jan,
> >>>
> >>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 09.11.21 10:37, Roman Kopytin wrote:
> >>>>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
> >>>>>
> >>>>
> >>>> Please do not top-post.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>> Sent: Tuesday, November 9, 2021 12:17 PM
> >>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >>>>>
> >>>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>>> In order to reduce the coupling between building the kernel and
> >>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>>> in a single .c file.
> >>>>>>
> >>>>>> I'm still working on the details of my proposed "require just k out
> >>>>>> these n required keys" and how it should be implemented, but it will
> >>>>>> probably involve teaching this tool a bunch of new options. These
> >>>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>>>> it out for early comments.
> >>>>>
> >>>>> I'd also like to see the usage of this hooked into the build process.
> >>>>>
> >>>>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
> >>>>>
> >>>>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
> >>>>>
> >>>>> Jan
> >>>>>
> >>>>> [1]
> >>>>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >>>>>
> >>>>> --
> >>>>> Siemens AG, T RDA IOT
> >>>>> Corporate Competence Center Embedded Linux
> >>>>>
> >>>>
> >>>> For what would you like to have code? The kconfig addition?
> >>>>
> >>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >>>> index d3a12be228..a9ed4d4ec4 100644
> >>>> --- a/common/Kconfig.boot
> >>>> +++ b/common/Kconfig.boot
> >>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >>>>
> >>>>  endif # SPL
> >>>>
> >>>> +config FIT_SIGNATURE_PUB_KEYS
> >>>> +       string "Public keys to use for FIT image verification"
> >>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> >>>> +       help
> >>>> +         Public keys, or certificate files to extract them from, that shall
> >>>> +         be used to verify signed FIT images. The keys will be embedded into
> >>>> +         the control device tree of U-Boot.
> >>>> +
> >>>>  endif # FIT
> >>>>
> >>>>  config LEGACY_IMAGE_FORMAT
> >>>>
> >>>>
> >>>> But note that we are in a design discussion here, and I'm at least
> >>>> reluctant to code up n-versions without having some common idea where
> >>>> things should move.
> >>>
> >>> I'm not sure we want this built into U-Boot. I see signing of a
> >>> firmware image as a final step, with the keys being added then, e.g.
> >>> by binman.
> >>
> >> This is not signing, this in embedding public key information into build
> >> artifacts before they can be signed. As pointed out in my other thread,
> >> not having an embedding feature is a major drawback of the current
> >> workflow. It easily forces you to rebuild existing build flows in
> >> out-of-tree scripts.
> >
> > The public key is not needed for signing to work, right? I don't
> > understand what you are getting at here. If you want to add the public
> > key to the image before it is signed, that's fine. I just don't
> > understand why you want to do that. Why not have the signer do
> > everything?
>
> A) Because sensitive signing environments will not run arbitrary logic.
>    They will hand out the public key, but they may not give you the
>    chance to run mkimage with the private key, like you would do during
>    development.

That's OK, so long as there is a way to get the data to be signed in,
and the public key and signature out.

>
> B) It avoids having to run the signing process in a specific order
>    because it already embeds the public key during build, thus
>    generates everything that shall be signed upfront.

The public key is not signed though. Whether it is present at the
start or not is not important.

Regards,
Simon

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 09:49, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 17:31, Simon Glass wrote:
> > Hi Jan,
> >
> > On Wed, 10 Nov 2021 at 00:20, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 10.11.21 07:55, Jan Kiszka wrote:
> >>> On 10.11.21 01:58, Simon Glass wrote:
> >>>> Hi,
> >>>>
> >>>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>>
> >>>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>>> In order to reduce the coupling between building the kernel and
> >>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>>> in a single .c file.
> >>>>>>
> >>>>>> I'm still working on the details of my proposed "require just k out
> >>>>>> these n required keys" and how it should be implemented, but it will
> >>>>>> probably involve teaching this tool a bunch of new options. These
> >>>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>>>> it out for early comments.
> >>>>>
> >>>>> I'd also like to see the usage of this hooked into the build process.
> >>>>>
> >>>>> And to my understanding of [1], that approach will provide a feature
> >>>>> that permits hooking with the build but would expect the key as dtsi
> >>>>> fragment. Can we consolidate the approaches?
> >>>>>
> >>>>> My current vision of a user interface would be a Kconfig option that
> >>>>> takes a list of key files to be injected. Maybe make that three lists,
> >>>>> one for "required=image", one for "required=conf", and one for optional
> >>>>> keys (if that has a use case in practice, no idea).
> >>>>
> >>>> Also please take a look at binman which is designed to handle create
> >>>> (or later updating from Yocto) the devicetree or firmware image.
> >>>>
> >>>
> >>> Yes, binman is another problem area, but not for the public key
> >>> injection, rather for permitting to sign fit images that are described
> >>> for binman (rather than for mkimage). I'm currently back to dd for
> >>> signing the U-Boot container in
> >>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
> >>> that FIT image description from that file - both not optimal.
> >
> > Well I don't think binman supports that at present, or at least I'm
> > not sure what it would do. We don't have a test case for it. If you
> > have an idea for how it should work, please send some ideas and I can
> > look at it.
> >
> >>
> >> OK, this can already be optimized with "binman replace" - once I
> >> understood where fdtmap can go and where not. Why no support for using
> >> map files?
> >
> > The fdtmap provides enough information to extract anything from the
> > image and regenerate/replace things.
> >
> > What is a map file?
>
> *.map, e.g. image.map? Also generated by many binmap <cmd> -m?

Using map files for what? Do you mean passing it to Binman in lieu of
an in-image fdtmap? If so, they are not equivalent. The map is just a
simple text output of offsets and sizes. The fdtmap contains the full
image description.

>
> >
> >>
> >> Jan
> >>
> >>>
> >>> And another area: Trust centers that perform the signing (and only that)
> >>> usually do not support random formats and workflows but just few common
> >>> ones, e.g. x509. It would be nice to have a way to route out the payload
> >>> (hashes etc.) that mkimage would sign, ideally into a standard signing
> >>> request, and permit to inject the resulting signature at the right
> >>> places into the FIT image.
> >
> > Well that needs to be provided somewhere. It should be fairly easy to
> > get Binman to do this, so long as the image description has info about
> > what is being signed.
>
> I would assume that it has to have that information, already to use
> mkimage on it or its parts.

Well, at present the information is there but Binman does not fully
parse the mkimage subnodes. E.g. it doesn't look to see what things
are signed/hashed. It just runs mkimage. If we want to output the hash
for signing, we would need to implement that somewhere. Binman could
do this after the image is build, i.e. look at the various signature
nodes, hash the appropriate data and write out an 'instructions' file
in a suitable format.

>
> >
> >>>
> >>> But one after the other.
> >
> > Possibly, but sometimes it is best to design things up-front.
> >
>
> True as well.

Regards,
Simon

binman replace broken? (was: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool)

[Jan Kiszka]

On 10.11.21 08:20, Jan Kiszka wrote:
> On 10.11.21 07:55, Jan Kiszka wrote:
>> On 10.11.21 01:58, Simon Glass wrote:
>>> Hi,
>>>
>>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>>> In order to reduce the coupling between building the kernel and
>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>>> in a single .c file.
>>>>>
>>>>> I'm still working on the details of my proposed "require just k out
>>>>> these n required keys" and how it should be implemented, but it will
>>>>> probably involve teaching this tool a bunch of new options. These
>>>>> patches are not necessarily ready for inclusion (unless someone else
>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>>> it out for early comments.
>>>>
>>>> I'd also like to see the usage of this hooked into the build process.
>>>>
>>>> And to my understanding of [1], that approach will provide a feature
>>>> that permits hooking with the build but would expect the key as dtsi
>>>> fragment. Can we consolidate the approaches?
>>>>
>>>> My current vision of a user interface would be a Kconfig option that
>>>> takes a list of key files to be injected. Maybe make that three lists,
>>>> one for "required=image", one for "required=conf", and one for optional
>>>> keys (if that has a use case in practice, no idea).
>>>
>>> Also please take a look at binman which is designed to handle create
>>> (or later updating from Yocto) the devicetree or firmware image.
>>>
>>
>> Yes, binman is another problem area, but not for the public key
>> injection, rather for permitting to sign fit images that are described
>> for binman (rather than for mkimage). I'm currently back to dd for
>> signing the U-Boot container in
>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
>> that FIT image description from that file - both not optimal.
> 
> OK, this can already be optimized with "binman replace" - once I
> understood where fdtmap can go and where not. Why no support for using
> map files?
> 

Well, too quick: "binman replace" writes everything into a temporary
directory, including the updated image - and then deletes this directory
on exit. So the original image will not be updated, and the update is lost.

I tried to quickly fix it by adding a rename before FinaliseOutputDir,
but it feels like I'm working against the design of the internal
interfaces here.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 20:36, Simon Glass wrote:
> Hi Jan,
> 
> On Wed, 10 Nov 2021 at 09:48, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 10.11.21 17:31, Simon Glass wrote:
>>> Hi Jan,
>>>
>>> On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 10.11.21 01:58, Simon Glass wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>
>>>>>> On 09.11.21 10:37, Roman Kopytin wrote:
>>>>>>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
>>>>>>>
>>>>>>
>>>>>> Please do not top-post.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> Sent: Tuesday, November 9, 2021 12:17 PM
>>>>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
>>>>>>>
>>>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>>>>>> In order to reduce the coupling between building the kernel and
>>>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>>>>>> in a single .c file.
>>>>>>>>
>>>>>>>> I'm still working on the details of my proposed "require just k out
>>>>>>>> these n required keys" and how it should be implemented, but it will
>>>>>>>> probably involve teaching this tool a bunch of new options. These
>>>>>>>> patches are not necessarily ready for inclusion (unless someone else
>>>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>>>>>> it out for early comments.
>>>>>>>
>>>>>>> I'd also like to see the usage of this hooked into the build process.
>>>>>>>
>>>>>>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
>>>>>>>
>>>>>>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>> [1]
>>>>>>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
>>>>>>>
>>>>>>> --
>>>>>>> Siemens AG, T RDA IOT
>>>>>>> Corporate Competence Center Embedded Linux
>>>>>>>
>>>>>>
>>>>>> For what would you like to have code? The kconfig addition?
>>>>>>
>>>>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>>>>>> index d3a12be228..a9ed4d4ec4 100644
>>>>>> --- a/common/Kconfig.boot
>>>>>> +++ b/common/Kconfig.boot
>>>>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
>>>>>>
>>>>>>  endif # SPL
>>>>>>
>>>>>> +config FIT_SIGNATURE_PUB_KEYS
>>>>>> +       string "Public keys to use for FIT image verification"
>>>>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
>>>>>> +       help
>>>>>> +         Public keys, or certificate files to extract them from, that shall
>>>>>> +         be used to verify signed FIT images. The keys will be embedded into
>>>>>> +         the control device tree of U-Boot.
>>>>>> +
>>>>>>  endif # FIT
>>>>>>
>>>>>>  config LEGACY_IMAGE_FORMAT
>>>>>>
>>>>>>
>>>>>> But note that we are in a design discussion here, and I'm at least
>>>>>> reluctant to code up n-versions without having some common idea where
>>>>>> things should move.
>>>>>
>>>>> I'm not sure we want this built into U-Boot. I see signing of a
>>>>> firmware image as a final step, with the keys being added then, e.g.
>>>>> by binman.
>>>>
>>>> This is not signing, this in embedding public key information into build
>>>> artifacts before they can be signed. As pointed out in my other thread,
>>>> not having an embedding feature is a major drawback of the current
>>>> workflow. It easily forces you to rebuild existing build flows in
>>>> out-of-tree scripts.
>>>
>>> The public key is not needed for signing to work, right? I don't
>>> understand what you are getting at here. If you want to add the public
>>> key to the image before it is signed, that's fine. I just don't
>>> understand why you want to do that. Why not have the signer do
>>> everything?
>>
>> A) Because sensitive signing environments will not run arbitrary logic.
>>    They will hand out the public key, but they may not give you the
>>    chance to run mkimage with the private key, like you would do during
>>    development.
> 
> That's OK, so long as there is a way to get the data to be signed in,
> and the public key and signature out.
> 
>>
>> B) It avoids having to run the signing process in a specific order
>>    because it already embeds the public key during build, thus
>>    generates everything that shall be signed upfront.
> 
> The public key is not signed though. Whether it is present at the
> start or not is not important.

The public key is signed when it is placed into an image that is signed.
That is the case, e.g., when injecting it into the SPL control FDT and
then signing the SPL image. Or when injecting it into the main control
FDT and signing U-Boot proper afterwards.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Jan Kiszka]

On 10.11.21 20:36, Simon Glass wrote:
> Hi Jan,
> 
> On Wed, 10 Nov 2021 at 09:49, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 10.11.21 17:31, Simon Glass wrote:
>>> Hi Jan,
>>>
>>> On Wed, 10 Nov 2021 at 00:20, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 10.11.21 07:55, Jan Kiszka wrote:
>>>>> On 10.11.21 01:58, Simon Glass wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>
>>>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
>>>>>>>> In order to reduce the coupling between building the kernel and
>>>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
>>>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
>>>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
>>>>>>>> in a single .c file.
>>>>>>>>
>>>>>>>> I'm still working on the details of my proposed "require just k out
>>>>>>>> these n required keys" and how it should be implemented, but it will
>>>>>>>> probably involve teaching this tool a bunch of new options. These
>>>>>>>> patches are not necessarily ready for inclusion (unless someone else
>>>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
>>>>>>>> it out for early comments.
>>>>>>>
>>>>>>> I'd also like to see the usage of this hooked into the build process.
>>>>>>>
>>>>>>> And to my understanding of [1], that approach will provide a feature
>>>>>>> that permits hooking with the build but would expect the key as dtsi
>>>>>>> fragment. Can we consolidate the approaches?
>>>>>>>
>>>>>>> My current vision of a user interface would be a Kconfig option that
>>>>>>> takes a list of key files to be injected. Maybe make that three lists,
>>>>>>> one for "required=image", one for "required=conf", and one for optional
>>>>>>> keys (if that has a use case in practice, no idea).
>>>>>>
>>>>>> Also please take a look at binman which is designed to handle create
>>>>>> (or later updating from Yocto) the devicetree or firmware image.
>>>>>>
>>>>>
>>>>> Yes, binman is another problem area, but not for the public key
>>>>> injection, rather for permitting to sign fit images that are described
>>>>> for binman (rather than for mkimage). I'm currently back to dd for
>>>>> signing the U-Boot container in
>>>>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
>>>>> that FIT image description from that file - both not optimal.
>>>
>>> Well I don't think binman supports that at present, or at least I'm
>>> not sure what it would do. We don't have a test case for it. If you
>>> have an idea for how it should work, please send some ideas and I can
>>> look at it.
>>>
>>>>
>>>> OK, this can already be optimized with "binman replace" - once I
>>>> understood where fdtmap can go and where not. Why no support for using
>>>> map files?
>>>
>>> The fdtmap provides enough information to extract anything from the
>>> image and regenerate/replace things.
>>>
>>> What is a map file?
>>
>> *.map, e.g. image.map? Also generated by many binmap <cmd> -m?
> 
> Using map files for what? Do you mean passing it to Binman in lieu of
> an in-image fdtmap? If so, they are not equivalent. The map is just a
> simple text output of offsets and sizes. The fdtmap contains the full
> image description.

Too bad. I was looking for a way to avoid having to add fdtmap to an
image when all information is already on the build host - and should
actually only remain there. Embedding fdtmap into the image solely for
build/post-process purposes looks like overkill to me.

> 
>>
>>>
>>>>
>>>> Jan
>>>>
>>>>>
>>>>> And another area: Trust centers that perform the signing (and only that)
>>>>> usually do not support random formats and workflows but just few common
>>>>> ones, e.g. x509. It would be nice to have a way to route out the payload
>>>>> (hashes etc.) that mkimage would sign, ideally into a standard signing
>>>>> request, and permit to inject the resulting signature at the right
>>>>> places into the FIT image.
>>>
>>> Well that needs to be provided somewhere. It should be fairly easy to
>>> get Binman to do this, so long as the image description has info about
>>> what is being signed.
>>
>> I would assume that it has to have that information, already to use
>> mkimage on it or its parts.
> 
> Well, at present the information is there but Binman does not fully
> parse the mkimage subnodes. E.g. it doesn't look to see what things
> are signed/hashed. It just runs mkimage. If we want to output the hash
> for signing, we would need to implement that somewhere. Binman could
> do this after the image is build, i.e. look at the various signature
> nodes, hash the appropriate data and write out an 'instructions' file
> in a suitable format.

Yep, that would be nice. Or would mkimage have more of the needed logic
already on board and would better be extended to write them out?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

On 08.11.21 16:28, Roman Kopytin wrote:
> 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);
> +}
> +
> 

I'm playing with this diff on top in order to support embedding into SPL 
control FDTs:

diff --git a/tools/fdt_add_pubkey.c b/tools/fdt_add_pubkey.c
index 9306ecedd1..176b6bd37d 100755
--- a/tools/fdt_add_pubkey.c
+++ b/tools/fdt_add_pubkey.c
@@ -50,10 +50,11 @@ static void process_args(int argc, char *argv[])
 int main(int argc, char *argv[])
 {
 	struct image_sign_info info;
-	int destfd, ret;
+	int signode, keynode, ret;
 	void *dest_blob = NULL;
 	struct stat dest_sbuf;
 	size_t size_inc = 0;
+	int destfd = -1;
 
 	cmdname = argv[0];
 
@@ -71,20 +72,41 @@ int main(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	while (1) {
+	do {
+		if (destfd >= 0) {
+			munmap(dest_blob, dest_sbuf.st_size);
+			close(destfd);
+
+			fprintf(stderr, ".dtb too small, increasing size by 1024 bytes\n");
+			size_inc = 1024;
+		}
+
 		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)
+		if (ret == -ENOSPC)
+			continue;
+		else if (ret)
 			break;
-		fprintf(stderr, ".dtb too small, increasing size by 1024 bytes\n");
-		size_inc = 1024;
-	}
+
+		signode = fdt_path_offset(dest_blob, "/signature");
+		if (signode < 0) {
+			fprintf(stderr, "%s: /signature node not found?!\n",
+				cmdname);
+			exit(EXIT_FAILURE);
+		}
+
+		keynode = fdt_first_subnode(dest_blob, signode);
+		if (keynode < 0) {
+			fprintf(stderr, "%s: /signature/<key> node not found?!\n",
+				cmdname);
+			exit(EXIT_FAILURE);
+		}
+
+		ret = fdt_appendprop(dest_blob, keynode, "u-boot,dm-spl", NULL, 0);
+	} while (ret == -ENOSPC);
 
 	if (ret) {
 		fprintf(stderr, "%s: Cannot add public key to FIT blob: %s\n",
@@ -94,4 +116,3 @@ int main(int argc, char *argv[])
 
 	exit(EXIT_SUCCESS);
 }
-


This is step one. Step two is a diff - actually still rather a hack due 
to some hard-coded options - to use the tool during dtb builds:

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index d3a12be228..a9ed4d4ec4 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
 
 endif # SPL
 
+config FIT_SIGNATURE_PUB_KEYS
+	string "Public keys to use for FIT image verification"
+	depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
+	help
+	  Public keys, or certificate files to extract them from, that shall
+	  be used to verify signed FIT images. The keys will be embedded into
+	  the control device tree of U-Boot.
+
 endif # FIT
 
 config LEGACY_IMAGE_FORMAT
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 39f03398ed..65852dc1d9 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -326,9 +326,12 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 		-d $(depfile).dtc.tmp $(dtc-tmp) || \
 		(echo "Check $(shell pwd)/$(pre-tmp) for errors" && false) \
 		; \
+	$(foreach key,$(subst $(quote),,$(CONFIG_FIT_SIGNATURE_PUB_KEYS)), \
+		tools/fdt_add_pubkey -a sha256,rsa4096 -k $(shell dirname $(key)) \
+			-n $(subst .key,,$(shell basename $(key))) -r conf $@;) \
 	sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
 
-$(obj)/%.dtb: $(src)/%.dts FORCE
+$(obj)/%.dtb: $(src)/%.dts tools/fdt_add_pubkey FORCE
 	$(call if_changed_dep,dtc)
 
 pre-tmp = $(subst $(comma),_,$(dot-target).pre.tmp)


This permits the workflow:

- make flash.bin (via binman)
- mkimage -r -F fit@0x280000.fit (an embedded FIT in flash.bin)
- binman replace -i flash.bin -f fit@0x280000.fit fit@0x280000
(the latter on in theory, that command is broken ATM)

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 13:58, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 20:36, Simon Glass wrote:
> > Hi Jan,
> >
> > On Wed, 10 Nov 2021 at 09:49, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 10.11.21 17:31, Simon Glass wrote:
> >>> Hi Jan,
> >>>
> >>> On Wed, 10 Nov 2021 at 00:20, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 10.11.21 07:55, Jan Kiszka wrote:
> >>>>> On 10.11.21 01:58, Simon Glass wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>>>>
> >>>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>>>>> In order to reduce the coupling between building the kernel and
> >>>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>>>>> in a single .c file.
> >>>>>>>>
> >>>>>>>> I'm still working on the details of my proposed "require just k out
> >>>>>>>> these n required keys" and how it should be implemented, but it will
> >>>>>>>> probably involve teaching this tool a bunch of new options. These
> >>>>>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>>>>>> it out for early comments.
> >>>>>>>
> >>>>>>> I'd also like to see the usage of this hooked into the build process.
> >>>>>>>
> >>>>>>> And to my understanding of [1], that approach will provide a feature
> >>>>>>> that permits hooking with the build but would expect the key as dtsi
> >>>>>>> fragment. Can we consolidate the approaches?
> >>>>>>>
> >>>>>>> My current vision of a user interface would be a Kconfig option that
> >>>>>>> takes a list of key files to be injected. Maybe make that three lists,
> >>>>>>> one for "required=image", one for "required=conf", and one for optional
> >>>>>>> keys (if that has a use case in practice, no idea).
> >>>>>>
> >>>>>> Also please take a look at binman which is designed to handle create
> >>>>>> (or later updating from Yocto) the devicetree or firmware image.
> >>>>>>
> >>>>>
> >>>>> Yes, binman is another problem area, but not for the public key
> >>>>> injection, rather for permitting to sign fit images that are described
> >>>>> for binman (rather than for mkimage). I'm currently back to dd for
> >>>>> signing the U-Boot container in
> >>>>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
> >>>>> that FIT image description from that file - both not optimal.
> >>>
> >>> Well I don't think binman supports that at present, or at least I'm
> >>> not sure what it would do. We don't have a test case for it. If you
> >>> have an idea for how it should work, please send some ideas and I can
> >>> look at it.
> >>>
> >>>>
> >>>> OK, this can already be optimized with "binman replace" - once I
> >>>> understood where fdtmap can go and where not. Why no support for using
> >>>> map files?
> >>>
> >>> The fdtmap provides enough information to extract anything from the
> >>> image and regenerate/replace things.
> >>>
> >>> What is a map file?
> >>
> >> *.map, e.g. image.map? Also generated by many binmap <cmd> -m?
> >
> > Using map files for what? Do you mean passing it to Binman in lieu of
> > an in-image fdtmap? If so, they are not equivalent. The map is just a
> > simple text output of offsets and sizes. The fdtmap contains the full
> > image description.
>
> Too bad. I was looking for a way to avoid having to add fdtmap to an
> image when all information is already on the build host - and should
> actually only remain there. Embedding fdtmap into the image solely for
> build/post-process purposes looks like overkill to me.

and for run-time access and for being able to list the image and
extract things from it.

Regards,
Simon


>
> >
> >>
> >>>
> >>>>
> >>>> Jan
> >>>>
> >>>>>
> >>>>> And another area: Trust centers that perform the signing (and only that)
> >>>>> usually do not support random formats and workflows but just few common
> >>>>> ones, e.g. x509. It would be nice to have a way to route out the payload
> >>>>> (hashes etc.) that mkimage would sign, ideally into a standard signing
> >>>>> request, and permit to inject the resulting signature at the right
> >>>>> places into the FIT image.
> >>>
> >>> Well that needs to be provided somewhere. It should be fairly easy to
> >>> get Binman to do this, so long as the image description has info about
> >>> what is being signed.
> >>
> >> I would assume that it has to have that information, already to use
> >> mkimage on it or its parts.
> >
> > Well, at present the information is there but Binman does not fully
> > parse the mkimage subnodes. E.g. it doesn't look to see what things
> > are signed/hashed. It just runs mkimage. If we want to output the hash
> > for signing, we would need to implement that somewhere. Binman could
> > do this after the image is build, i.e. look at the various signature
> > nodes, hash the appropriate data and write out an 'instructions' file
> > in a suitable format.
>
> Yep, that would be nice. Or would mkimage have more of the needed logic
> already on board and would better be extended to write them out?
>
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux

Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 13:51, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 20:36, Simon Glass wrote:
> > Hi Jan,
> >
> > On Wed, 10 Nov 2021 at 09:48, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 10.11.21 17:31, Simon Glass wrote:
> >>> Hi Jan,
> >>>
> >>> On Tue, 9 Nov 2021 at 23:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 10.11.21 01:58, Simon Glass wrote:
> >>>>> Hi Jan,
> >>>>>
> >>>>> On Tue, 9 Nov 2021 at 03:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>>>
> >>>>>> On 09.11.21 10:37, Roman Kopytin wrote:
> >>>>>>> Can we have discussion with code lines? For me it is not very clear, because it isn't my code.
> >>>>>>>
> >>>>>>
> >>>>>> Please do not top-post.
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>> Sent: Tuesday, November 9, 2021 12:17 PM
> >>>>>>> To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de; Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >>>>>>> Subject: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool
> >>>>>>>
> >>>>>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>>>>> In order to reduce the coupling between building the kernel and
> >>>>>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>>>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>>>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>>>>> in a single .c file.
> >>>>>>>>
> >>>>>>>> I'm still working on the details of my proposed "require just k out
> >>>>>>>> these n required keys" and how it should be implemented, but it will
> >>>>>>>> probably involve teaching this tool a bunch of new options. These
> >>>>>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>>>>>> it out for early comments.
> >>>>>>>
> >>>>>>> I'd also like to see the usage of this hooked into the build process.
> >>>>>>>
> >>>>>>> And to my understanding of [1], that approach will provide a feature that permits hooking with the build but would expect the key as dtsi fragment. Can we consolidate the approaches?
> >>>>>>>
> >>>>>>> My current vision of a user interface would be a Kconfig option that takes a list of key files to be injected. Maybe make that three lists, one for "required=image", one for "required=conf", and one for optional keys (if that has a use case in practice, no idea).
> >>>>>>>
> >>>>>>> Jan
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> https://lore.kernel.org/u-boot/20210928085651.619892-1-rasmus.villemoes@prevas.dk/
> >>>>>>>
> >>>>>>> --
> >>>>>>> Siemens AG, T RDA IOT
> >>>>>>> Corporate Competence Center Embedded Linux
> >>>>>>>
> >>>>>>
> >>>>>> For what would you like to have code? The kconfig addition?
> >>>>>>
> >>>>>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> >>>>>> index d3a12be228..a9ed4d4ec4 100644
> >>>>>> --- a/common/Kconfig.boot
> >>>>>> +++ b/common/Kconfig.boot
> >>>>>> @@ -279,6 +279,14 @@ config SPL_FIT_GENERATOR
> >>>>>>
> >>>>>>  endif # SPL
> >>>>>>
> >>>>>> +config FIT_SIGNATURE_PUB_KEYS
> >>>>>> +       string "Public keys to use for FIT image verification"
> >>>>>> +       depends on FIT_SIGNATURE || SPL_FIT_SIGNATURE
> >>>>>> +       help
> >>>>>> +         Public keys, or certificate files to extract them from, that shall
> >>>>>> +         be used to verify signed FIT images. The keys will be embedded into
> >>>>>> +         the control device tree of U-Boot.
> >>>>>> +
> >>>>>>  endif # FIT
> >>>>>>
> >>>>>>  config LEGACY_IMAGE_FORMAT
> >>>>>>
> >>>>>>
> >>>>>> But note that we are in a design discussion here, and I'm at least
> >>>>>> reluctant to code up n-versions without having some common idea where
> >>>>>> things should move.
> >>>>>
> >>>>> I'm not sure we want this built into U-Boot. I see signing of a
> >>>>> firmware image as a final step, with the keys being added then, e.g.
> >>>>> by binman.
> >>>>
> >>>> This is not signing, this in embedding public key information into build
> >>>> artifacts before they can be signed. As pointed out in my other thread,
> >>>> not having an embedding feature is a major drawback of the current
> >>>> workflow. It easily forces you to rebuild existing build flows in
> >>>> out-of-tree scripts.
> >>>
> >>> The public key is not needed for signing to work, right? I don't
> >>> understand what you are getting at here. If you want to add the public
> >>> key to the image before it is signed, that's fine. I just don't
> >>> understand why you want to do that. Why not have the signer do
> >>> everything?
> >>
> >> A) Because sensitive signing environments will not run arbitrary logic.
> >>    They will hand out the public key, but they may not give you the
> >>    chance to run mkimage with the private key, like you would do during
> >>    development.
> >
> > That's OK, so long as there is a way to get the data to be signed in,
> > and the public key and signature out.
> >
> >>
> >> B) It avoids having to run the signing process in a specific order
> >>    because it already embeds the public key during build, thus
> >>    generates everything that shall be signed upfront.
> >
> > The public key is not signed though. Whether it is present at the
> > start or not is not important.
>
> The public key is signed when it is placed into an image that is signed.
> That is the case, e.g., when injecting it into the SPL control FDT and
> then signing the SPL image. Or when injecting it into the main control
> FDT and signing U-Boot proper afterwards.

Ah OK, so it is signed along with everything else.

Regards,
Simon


>
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux

Re: binman replace broken? (was: Re: [PATCH 0/2] RFC: add fdt_add_pubkey tool)

[Simon Glass]

Hi Jan,

On Wed, 10 Nov 2021 at 13:49, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 10.11.21 08:20, Jan Kiszka wrote:
> > On 10.11.21 07:55, Jan Kiszka wrote:
> >> On 10.11.21 01:58, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Tue, 9 Nov 2021 at 02:17, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 08.11.21 16:28, Roman Kopytin wrote:
> >>>>> In order to reduce the coupling between building the kernel and
> >>>>> U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
> >>>>> without simultaneously signing a FIT image. That tool doesn't seem to
> >>>>> exist, so I stole the necessary pieces from mkimage et al and put it
> >>>>> in a single .c file.
> >>>>>
> >>>>> I'm still working on the details of my proposed "require just k out
> >>>>> these n required keys" and how it should be implemented, but it will
> >>>>> probably involve teaching this tool a bunch of new options. These
> >>>>> patches are not necessarily ready for inclusion (unless someone else
> >>>>> finds fdt_add_pubkey useful as is), but I thought I might as well send
> >>>>> it out for early comments.
> >>>>
> >>>> I'd also like to see the usage of this hooked into the build process.
> >>>>
> >>>> And to my understanding of [1], that approach will provide a feature
> >>>> that permits hooking with the build but would expect the key as dtsi
> >>>> fragment. Can we consolidate the approaches?
> >>>>
> >>>> My current vision of a user interface would be a Kconfig option that
> >>>> takes a list of key files to be injected. Maybe make that three lists,
> >>>> one for "required=image", one for "required=conf", and one for optional
> >>>> keys (if that has a use case in practice, no idea).
> >>>
> >>> Also please take a look at binman which is designed to handle create
> >>> (or later updating from Yocto) the devicetree or firmware image.
> >>>
> >>
> >> Yes, binman is another problem area, but not for the public key
> >> injection, rather for permitting to sign fit images that are described
> >> for binman (rather than for mkimage). I'm currently back to dd for
> >> signing the U-Boot container in
> >> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi, or I would have to split
> >> that FIT image description from that file - both not optimal.
> >
> > OK, this can already be optimized with "binman replace" - once I
> > understood where fdtmap can go and where not. Why no support for using
> > map files?
> >
>
> Well, too quick: "binman replace" writes everything into a temporary
> directory, including the updated image - and then deletes this directory
> on exit. So the original image will not be updated, and the update is lost.
>
> I tried to quickly fix it by adding a rename before FinaliseOutputDir,
> but it feels like I'm working against the design of the internal
> interfaces here.

If you want to see how things work, check out ftest.py which has all
the tests. If there is a test for it, it probably works. If not, it
might not.

Regards,
SImon

RE: [PATCH 1/2] tools: add fdt_add_pubkey

[Roman Kopytin]

Thanks, I found example in fdtgrep.
What do you think about function like:

static void print_usage(const char *msg)
{
	if (msg != NULL)
		fprintf(stderr, "Error: %s\n", msg);
	fprintf(stderr, "Usage: %s [-a <algo>] [-k <keydir>] [-n <keyname>] [-r <conf|image>] <fdt blob>\n"
		"Options:\n"

		"\t-a <algo>       Cryptographic algorithm. Optional parameter, default: sha1,rsa2048\n"
		"\t-k <keydir>     Directory with public key. Optional parameter, default: .\n"
		"\t-n <keyname>    Public key name. Optional parameter, default: key\n"
		"\t-r <conf|image> Required: If present this indicates that the key must be verified for the image / configuration to be considered valid\n"
		"\t<fdt blob>      FDT blob file for adding of the public key. Required parameter.\n",
		cmdname);
	exit(EXIT_FAILURE);
}


Is it ok?

-----Original Message-----
From: Jan Kiszka <jan.kiszka@siemens.com> 
Sent: Wednesday, November 10, 2021 10:22 PM
To: Roman Kopytin <Roman.Kopytin@kaspersky.com>; u-boot@lists.denx.de
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH 1/2] tools: add fdt_add_pubkey

On 10.11.21 09:26, Roman Kopytin wrote:
> Could you please provide good example with needed style for helper?
> In tools I saw a lot of programs w/o help.
> 

Have a look at binman to see this full-blown - not a completely fair comparison as it benefits from Python argparse.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] tools: add fdt_add_pubkey

[Jan Kiszka]

On 11.11.21 06:26, Roman Kopytin wrote:
> Thanks, I found example in fdtgrep.
> What do you think about function like:
> 
> static void print_usage(const char *msg)
> {
> 	if (msg != NULL)
> 		fprintf(stderr, "Error: %s\n", msg);
> 	fprintf(stderr, "Usage: %s [-a <algo>] [-k <keydir>] [-n <keyname>] [-r <conf|image>] <fdt blob>\n"
> 		"Options:\n"
> 
> 		"\t-a <algo>       Cryptographic algorithm. Optional parameter, default: sha1,rsa2048\n"
> 		"\t-k <keydir>     Directory with public key. Optional parameter, default: .\n"
> 		"\t-n <keyname>    Public key name. Optional parameter, default: key\n"
> 		"\t-r <conf|image> Required: If present this indicates that the key must be verified for the image / configuration to be considered valid\n"

Maybe reorder to "configuration / image" (or reorder to "<image|conf>").

> 		"\t<fdt blob>      FDT blob file for adding of the public key. Required parameter.\n",
> 		cmdname);
> 	exit(EXIT_FAILURE);
> }
> 
> 
> Is it ok?

Yes, looks good to me.

I'm still looking for a way to overcome -a, though...

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[PATCH 0/2] RFC: add fdt_add_pubkey tool

[Roman Kopytin]

In order to reduce the coupling between building the kernel and
U-Boot, I'd like a tool that can add a public key to U-Boot's dtb
without simultaneously signing a FIT image. That tool doesn't seem to
exist, so I stole the necessary pieces from mkimage et al and put it
in a single .c file.

I'm still working on the details of my proposed "require just k out
these n required keys" and how it should be implemented, but it will
probably involve teaching this tool a bunch of new options. These
patches are not necessarily ready for inclusion (unless someone else
finds fdt_add_pubkey useful as is), but I thought I might as well send
it out for early comments.

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      | 130 ++++++++++++++++++++++++++++++++++++
 4 files changed, 142 insertions(+)
 create mode 100644 tools/fdt_add_pubkey.c

-- 
2.25.1