[PATCH v2 0/5] Introduce new sign binman's option

[PATCH v2 0/5] Introduce new sign binman's option

[Ivan Mikhaylov]

This patch introduces prototype of new sign binman's option.
Enhancing the sign procedure, as example:

mkimage -G privateky -r -o sha256,rsa4096 -F fit.fit
binman replace -i flash.bin -f fit.fit fit

into:
binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

It works with extracted FIT container and image, which provides key signing
of FIT container and replacing of it in directed image.

Also it is possible to sign exact FIT container in place.
As example:

binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit

Add fdt_add_pubkey utility which provides possibility of add pub keys
into DTB. This one needed mostly for test coverage of binman sign option
but could be useful when private and pub keys are separated.

Depends on "binman: Support updating section contents".

Ivan Mikhaylov (3):
  binman: add documentation for binman sign option
  binman: add sign option for binman
  binman: add tests for sign option

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/binman/binman.rst            |  18 ++++
 tools/binman/cmdline.py            |  13 +++
 tools/binman/control.py            |  29 +++++-
 tools/binman/etype/fit.py          |  18 ++++
 tools/binman/etype/section.py      |   3 +
 tools/binman/ftest.py              |  61 +++++++++++++
 tools/binman/test/277_fit_sign.dts |  63 +++++++++++++
 tools/fdt_add_pubkey.c             | 138 +++++++++++++++++++++++++++++
 11 files changed, 354 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/277_fit_sign.dts
 create mode 100644 tools/fdt_add_pubkey.c

-- 
2.39.1

[PATCH v2 1/5] binman: add documentation for binman sign option

[Ivan Mikhaylov]

Add the documentation about binman sign option and providing an
example.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/binman.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 0921e31878..c9d352e3ba 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1330,6 +1330,24 @@ You can also replace just a selection of entries::
 
 .. _`BinmanLogging`:
 
+Signing FIT container with private key in an image
+--------------------------------------------------
+
+You can sign FIT container with private key in your image.
+For example::
+
+    $ binman sign -i image.bin -k privatekey -a sha256,rsa4096 fit
+
+binman will extract FIT container, sign and replace it immediately.
+
+If you want to sign and replace FIT container in place::
+
+    $ binman sign -i image.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit
+
+which will sign FIT container with private key and replace it immediately
+inside your image.
+
+
 Logging
 -------
 
-- 
2.39.1

[PATCH v2 2/5] binman: add sign option for binman

[Ivan Mikhaylov]

Introduce proof of concept for binman's new option which provides sign
and replace FIT containers in binary images.

Usage as example:

from:
mkimage -G privateky -r -o sha256,rsa4096 -F fit
binman replace -i flash.bin -f fit.fit fit

to:
binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit

and to this one if it's need to be extracted, signed with key and put it
back in image:
binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/cmdline.py       | 13 +++++++++++++
 tools/binman/control.py       | 29 ++++++++++++++++++++++++++++-
 tools/binman/etype/fit.py     | 18 ++++++++++++++++++
 tools/binman/etype/section.py |  3 +++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
index 986d6f1a31..e2961ed11f 100644
--- a/tools/binman/cmdline.py
+++ b/tools/binman/cmdline.py
@@ -163,6 +163,19 @@ controlled by a description in the board device tree.'''
     replace_parser.add_argument('paths', type=str, nargs='*',
                                 help='Paths within file to replace (wildcard)')
 
+    sign_parser = subparsers.add_parser('sign',
+                                           help='Sign entries in image')
+    sign_parser.add_argument('-a', '--algo', type=str, required=True,
+                                help='Hash algorithm e.g. sha256,rsa4096')
+    sign_parser.add_argument('-f', '--file', type=str, required=False,
+                                help='Input filename to sign')
+    sign_parser.add_argument('-i', '--image', type=str, required=True,
+                                help='Image filename to update')
+    sign_parser.add_argument('-k', '--key', type=str, required=True,
+                                help='Private key file for signing')
+    sign_parser.add_argument('paths', type=str, nargs='*',
+                                help='Paths within file to sign (wildcard)')
+
     test_parser = subparsers.add_parser('test', help='Run tests')
     test_parser.add_argument('-P', '--processes', type=int,
         help='set number of processes to use for running tests')
diff --git a/tools/binman/control.py b/tools/binman/control.py
index e64740094f..9ebd73913a 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -20,6 +20,7 @@ from patman import command
 from binman import elf
 from binman import entry
 from patman import tout
+from patman import tools
 
 # These are imported if needed since they import libfdt
 state = None
@@ -445,6 +446,29 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths,
     AfterReplace(image, allow_resize=allow_resize, write_map=write_map)
     return image
 
+def SignEntries(image_fname, input_fname, privatekey_fname, algo, entry_paths,
+                write_map=False):
+    """Sign and replace the data from one or more entries from input files
+
+    Args:
+        image_fname: Image filename to process
+        input_fname: Single input filename to use if replacing one file, None
+            otherwise
+        algo: Hashing algorithm
+        entry_paths: List of entry paths to sign
+        privatekey_fname: Private key filename
+        write_map (bool): True to write the map file
+    """
+    image_fname = os.path.abspath(image_fname)
+    image = Image.FromFile(image_fname)
+
+    BeforeReplace(image, allow_resize=True)
+
+    for entry_path in entry_paths:
+        entry = image.FindEntryPath(entry_path)
+        entry.UpdateSignatures(privatekey_fname, algo, input_fname)
+
+    AfterReplace(image, allow_resize=True, write_map=write_map)
 
 def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded):
     """Prepare the images to be processed and select the device tree
@@ -650,7 +674,7 @@ def Binman(args):
     from binman.image import Image
     from binman import state
 
-    if args.cmd in ['ls', 'extract', 'replace', 'tool']:
+    if args.cmd in ['ls', 'extract', 'replace', 'tool', 'sign']:
         try:
             tout.init(args.verbosity)
             tools.prepare_output_dir(None)
@@ -666,6 +690,9 @@ def Binman(args):
                                do_compress=not args.compressed,
                                allow_resize=not args.fix_size, write_map=args.map)
 
+            if args.cmd == 'sign':
+                SignEntries(args.image, args.file, args.key, args.algo, args.paths)
+
             if args.cmd == 'tool':
                 tools.set_tool_paths(args.toolpath)
                 if args.list:
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index cd2943533c..c0451fe765 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -828,3 +828,21 @@ class Entry_fit(Entry_section):
         # missing
         for entry in self._priv_entries.values():
             entry.CheckMissing(missing_list)
+
+    def UpdateSignatures(self, privatekey_fname, algo, input_fname):
+        uniq = self.GetUniqueName()
+        args = [ '-G', privatekey_fname, '-r', '-o', algo, '-F' ]
+        if input_fname:
+            fname = input_fname
+        else:
+            fname = tools.get_output_filename('%s.fit' % uniq)
+            tools.write_file(fname, self.GetData())
+        args.append(fname)
+
+        if self.mkimage.run_cmd(*args) is None:
+            # Bintool is missing; just use empty data as the output
+            self.record_missing_bintool(self.mkimage)
+            return
+
+        data = tools.read_file(fname)
+        self.WriteData(data)
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 57b91ff726..4d0152e194 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -1000,3 +1000,6 @@ class Entry_section(Entry):
             for entry in entries.values():
                 return entry.read_elf_segments()
         return None
+
+    def UpdateSignatures(self, privatekey_fname, algo, input_fname):
+        self.Raise('Updating signatures is not supported with this entry type')
-- 
2.39.1

[PATCH v2 3/5] binman: add tests for sign option

[Ivan Mikhaylov]

Add the test which provides sequence of actions:
  1. create the image from binman dts
  2. create public and private keys
  3. add public key into dtb with fdt_add_pubkey
  4. 1. sign FIT container with new sign option with extracting from
        image
     2. sign exact FIT container with replacing of it in image
  5. check with fit_check_sign

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/ftest.py              | 61 +++++++++++++++++++++++++++++
 tools/binman/test/277_fit_sign.dts | 63 ++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 tools/binman/test/277_fit_sign.dts

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index d74aa90a62..84b2370271 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -709,6 +709,14 @@ class TestFunctional(unittest.TestCase):
         AddNode(dtb.GetRoot(), '')
         return tree
 
+    def _CheckSign(self, fit, key):
+        try:
+            tools.run('fit_check_sign', '-k', key, '-f', fit)
+        except:
+            self.fail('Expected signed FIT container')
+            return False
+        return True
+
     def testRun(self):
         """Test a basic run with valid args"""
         result = self._RunBinman('-h')
@@ -6404,6 +6412,59 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
             self._DoTestFile('278_mkimage_missing_multiple.dts', allow_missing=False)
         self.assertIn("not found in input path", str(e.exception))
 
+    def _PrepareSignEnv(self, dts='277_fit_sign.dts'):
+        """Prepare sign environment
+
+        Create private and public keys, add pubkey into dtb.
+
+        Returns:
+            Tuple:
+                FIT container
+                Image name
+                Private key
+                DTB
+        """
+
+        data = self._DoReadFileRealDtb(dts)
+        updated_fname = tools.get_output_filename('image-updated.bin')
+        tools.write_file(updated_fname, data)
+        dtb = tools.get_output_filename('source.dtb')
+        private_key = tools.get_output_filename('test_key.key')
+        public_key = tools.get_output_filename('test_key.crt')
+        fit = tools.get_output_filename('fit.fit')
+        key_dir = tools.get_output_dir()
+
+        tools.run('openssl', 'req', '-batch' , '-newkey', 'rsa:4096',
+                  '-sha256', '-new',  '-nodes',  '-x509', '-keyout',
+                  private_key, '-out', public_key)
+        tools.run('fdt_add_pubkey', '-a', 'sha256,rsa4096', '-k', key_dir,
+                  '-n', 'test_key', '-r', 'conf', dtb)
+
+        return fit, updated_fname, private_key, dtb
+
+    def testSignSimple(self):
+        """Test that a FIT container can be signed in image"""
+        is_signed = False
+        fit, fname, private_key, dtb = self._PrepareSignEnv()
+
+        # do sign with private key
+        control.SignEntries(fname, None, private_key, 'sha256,rsa4096',
+                            ['fit'])
+        is_signed = self._CheckSign(fit, dtb)
+
+        self.assertEqual(is_signed, True)
+
+    def testSignExactFIT(self):
+        """Test that a FIT container can be signed and replaced in image"""
+        is_signed = False
+        fit, fname, private_key, dtb = self._PrepareSignEnv()
+
+        # do sign with private key
+        self._DoBinman('sign', '-i', fname, '-k', private_key, '-a',
+                       'sha256,rsa4096', '-f', fit, 'fit')
+        is_signed = self._CheckSign(fit, dtb)
+
+        self.assertEqual(is_signed, True)
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/277_fit_sign.dts b/tools/binman/test/277_fit_sign.dts
new file mode 100644
index 0000000000..b9f17dc5c0
--- /dev/null
+++ b/tools/binman/test/277_fit_sign.dts
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <0x100000>;
+		allow-repack;
+
+		fit {
+			description = "U-Boot";
+			offset = <0x10000>;
+			images {
+				u-boot-1 {
+					description = "U-Boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					hash-1 {
+						algo = "sha256";
+					};
+					u-boot {
+					};
+				};
+
+				fdt-1 {
+					description = "test.dtb";
+					type = "flat_dt";
+					arch = "arm64";
+					compression = "none";
+					hash-1 {
+						algo = "sha256";
+					};
+					u-boot-spl-dtb {
+					};
+				};
+
+			};
+
+			configurations {
+				default = "conf-1";
+				conf-1 {
+					description = "u-boot with fdt";
+					firmware = "u-boot-1";
+					fdt = "fdt-1";
+					signature-1 {
+						algo = "sha256,rsa4096";
+						key-name-hint = "test_key";
+						sign-images = "firmware", "fdt";
+					};
+
+				};
+			};
+		};
+
+		fdtmap {
+		};
+	};
+};
-- 
2.39.1

[PATCH v2 4/5] tools: add fdt_add_pubkey

[Ivan Mikhaylov]

From: Roman Kopytin <Roman.Kopytin@kaspersky.com>

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>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/.gitignore       |   1 +
 tools/Makefile         |   3 +
 tools/fdt_add_pubkey.c | 138 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100644 tools/fdt_add_pubkey.c

diff --git a/tools/.gitignore b/tools/.gitignore
index 788ea260a0..cda3ea628c 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 e13effbb66..38699b069d 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -65,6 +65,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
 
 ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST)$(CONFIG_FWU_MDATA_GPT_BLK),)
 hostprogs-y += file2include
@@ -150,6 +151,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),)
@@ -187,6 +189,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 100644
index 0000000000..999f5a7e83
--- /dev/null
+++ b/tools/fdt_add_pubkey.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0+
+#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 print_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);
+	fprintf(stderr, "Help information: %s [-h]\n", cmdname);
+	exit(EXIT_FAILURE);
+}
+
+static void print_help(void)
+{
+	fprintf(stderr, "Options:\n"
+		"\t-a <algo>       Cryptographic algorithm. Optional parameter, default value: sha1,rsa2048\n"
+		"\t-k <keydir>     Directory with public key. Optional parameter, default value: .\n"
+		"\t-n <keyname>    Public key name. Optional parameter, default value: 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");
+	exit(EXIT_FAILURE);
+}
+
+static void process_args(int argc, char *argv[])
+{
+	int opt;
+
+	while ((opt = getopt(argc, argv, "a:k:n:r:h")) != -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;
+		case 'h':
+			print_help();
+		default:
+			print_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)
+		print_usage("Missing dtb file to update");
+}
+
+static void reset_info(struct image_sign_info *info)
+{
+	if (!info)
+		fprintf(stderr, "Error: info is NULL in %s\n", __func__);
+
+	memset(info, 0, sizeof(struct image_sign_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);
+	}
+}
+
+static int add_pubkey(struct image_sign_info *info)
+{
+	int destfd = -1, ret;
+	void *dest_blob = NULL;
+	struct stat dest_sbuf;
+	size_t size_inc = 0;
+
+	if (!info)
+		fprintf(stderr, "Error: info is NULL in %s\n", __func__);
+
+	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);
+		if (ret == -ENOSPC)
+			continue;
+		else if (ret < 0)
+			break;
+	} while (ret == -ENOSPC);
+
+	return ret;
+}
+
+int main(int argc, char *argv[])
+{
+	struct image_sign_info info;
+	int ret;
+
+	cmdname = argv[0];
+
+	process_args(argc, argv);
+	reset_info(&info);
+	ret = add_pubkey(&info);
+
+	if (ret < 0) {
+		fprintf(stderr, "%s: Cannot add public key to FIT blob: %s\n",
+			cmdname, strerror(ret));
+		exit(EXIT_FAILURE);
+	}
+
+	exit(EXIT_SUCCESS);
+}
+
-- 
2.39.1

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

[Ivan Mikhaylov]

From: Roman Kopytin <Roman.Kopytin@kaspersky.com>

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 e3e7ca4b21..956b8fcd43 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
 
         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
             # hashed nodes. If it isn't, a security bypass is possible.
@@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
     mkimage = cons.config.build_dir + '/tools/mkimage'
     binman = cons.config.source_dir + '/tools/binman/binman'
     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.39.1

Re: [PATCH v2 2/5] binman: add sign option for binman

[Simon Glass]

Hi Ivan,

On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> Introduce proof of concept for binman's new option which provides sign
> and replace FIT containers in binary images.
>
> Usage as example:
>
> from:
> mkimage -G privateky -r -o sha256,rsa4096 -F fit
> binman replace -i flash.bin -f fit.fit fit
>
> to:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit
>
> and to this one if it's need to be extracted, signed with key and put it
> back in image:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit
>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  tools/binman/cmdline.py       | 13 +++++++++++++
>  tools/binman/control.py       | 29 ++++++++++++++++++++++++++++-
>  tools/binman/etype/fit.py     | 18 ++++++++++++++++++
>  tools/binman/etype/section.py |  3 +++
>  4 files changed, 62 insertions(+), 1 deletion(-)

This needs a few tweaks to get the test coverage up to 100% and use
the mark_build_done() feature added a few days ago.

I have taken the liberty of updating it and will apply it soon, since
this has been outstanding for a while.

Regards,
Simon

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

[Simon Glass]

Hi Ivan,

On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
>
> 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 e3e7ca4b21..956b8fcd43 100644
> --- a/test/py/tests/test_vboot.py
> +++ b/test/py/tests/test_vboot.py
> @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
>
>          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
>              # hashed nodes. If it isn't, a security bypass is possible.
> @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
>      mkimage = cons.config.build_dir + '/tools/mkimage'
>      binman = cons.config.source_dir + '/tools/binman/binman'
>      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.39.1
>

Unfortunately this test fails on sandbox:

https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975

I think it would be better to put it in its own test (perhaps in the
same file) so we are not doing it on every test run. Also you could
check (in a very basic way) that it adds the key correctly since we
don't really need another test of the logic of doing that. We are just
checking that your tool calls that logic correctly.

I'll drop this one when applying, for now. Please take a look.

Regards,
Simon

Re: [PATCH v2 4/5] tools: add fdt_add_pubkey

[Simon Glass]

From: Roman Kopytin <Roman.Kopytin@kaspersky.com>

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>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/.gitignore       |   1 +
 tools/Makefile         |   3 +
 tools/fdt_add_pubkey.c | 138 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100644 tools/fdt_add_pubkey.c

Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 3/5] binman: add tests for sign option

[Simon Glass]

Add the test which provides sequence of actions:
  1. create the image from binman dts
  2. create public and private keys
  3. add public key into dtb with fdt_add_pubkey
  4. 1. sign FIT container with new sign option with extracting from
        image
     2. sign exact FIT container with replacing of it in image
  5. check with fit_check_sign

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/ftest.py              | 61 +++++++++++++++++++++++++++++
 tools/binman/test/277_fit_sign.dts | 63 ++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 tools/binman/test/277_fit_sign.dts

Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 2/5] binman: add sign option for binman

[Simon Glass]

Hi Ivan,

On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> Introduce proof of concept for binman's new option which provides sign
> and replace FIT containers in binary images.
>
> Usage as example:
>
> from:
> mkimage -G privateky -r -o sha256,rsa4096 -F fit
> binman replace -i flash.bin -f fit.fit fit
>
> to:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit
>
> and to this one if it's need to be extracted, signed with key and put it
> back in image:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit
>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  tools/binman/cmdline.py       | 13 +++++++++++++
>  tools/binman/control.py       | 29 ++++++++++++++++++++++++++++-
>  tools/binman/etype/fit.py     | 18 ++++++++++++++++++
>  tools/binman/etype/section.py |  3 +++
>  4 files changed, 62 insertions(+), 1 deletion(-)

This needs a few tweaks to get the test coverage up to 100% and use
the mark_build_done() feature added a few days ago.

I have taken the liberty of updating it and will apply it soon, since
this has been outstanding for a while.

Regards,
Simon

Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 1/5] binman: add documentation for binman sign option

[Simon Glass]

Add the documentation about binman sign option and providing an
example.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/binman.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 3/5] binman: add tests for sign option

[Simon Glass]

Hi Ivan,

On Fri, 10 Mar 2023 at 17:47, Simon Glass <sjg@chromium.org> wrote:
>
> Add the test which provides sequence of actions:
>   1. create the image from binman dts
>   2. create public and private keys
>   3. add public key into dtb with fdt_add_pubkey
>   4. 1. sign FIT container with new sign option with extracting from
>         image
>      2. sign exact FIT container with replacing of it in image
>   5. check with fit_check_sign
>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  tools/binman/ftest.py              | 61 +++++++++++++++++++++++++++++
>  tools/binman/test/277_fit_sign.dts | 63 ++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)
>  create mode 100644 tools/binman/test/277_fit_sign.dts
>
> Applied to u-boot-dm/next, thanks!

As mentioned on the other email I had a bit of trouble getting this
over the line Here is what I did:

Renumber test file from 277 to 280
Move UpdateSignatures() to Entry base class
Don't allow missing mkimage as it doesn't make sense
Propagate --toolpath for CI
Call mark_build_done() to avoid regenerating FIT

Regards,
Simon

Re: [PATCH v2 3/5] binman: add tests for sign option

[Simon Glass]

Hi Ivan,

On Fri, 10 Mar 2023 at 17:47, Simon Glass <sjg@chromium.org> wrote:
>
> Add the test which provides sequence of actions:
>   1. create the image from binman dts
>   2. create public and private keys
>   3. add public key into dtb with fdt_add_pubkey
>   4. 1. sign FIT container with new sign option with extracting from
>         image
>      2. sign exact FIT container with replacing of it in image
>   5. check with fit_check_sign
>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  tools/binman/ftest.py              | 61 +++++++++++++++++++++++++++++
>  tools/binman/test/277_fit_sign.dts | 63 ++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)
>  create mode 100644 tools/binman/test/277_fit_sign.dts
>
> Applied to u-boot-dm/next, thanks!

As mentioned on the other email I had a bit of trouble getting this
over the line Here is what I did:

Renumber test file from 277 to 280
Move UpdateSignatures() to Entry base class
Don't allow missing mkimage as it doesn't make sense
Propagate --toolpath for CI
Call mark_build_done() to avoid regenerating FIT

Regards,
Simon

Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 2/5] binman: add sign option for binman

[Simon Glass]

Hi Ivan,

On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> Introduce proof of concept for binman's new option which provides sign
> and replace FIT containers in binary images.
>
> Usage as example:
>
> from:
> mkimage -G privateky -r -o sha256,rsa4096 -F fit
> binman replace -i flash.bin -f fit.fit fit
>
> to:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f fit.fit fit
>
> and to this one if it's need to be extracted, signed with key and put it
> back in image:
> binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit
>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> ---
>  tools/binman/cmdline.py       | 13 +++++++++++++
>  tools/binman/control.py       | 29 ++++++++++++++++++++++++++++-
>  tools/binman/etype/fit.py     | 18 ++++++++++++++++++
>  tools/binman/etype/section.py |  3 +++
>  4 files changed, 62 insertions(+), 1 deletion(-)

This needs a few tweaks to get the test coverage up to 100% and use
the mark_build_done() feature added a few days ago.

I have taken the liberty of updating it and will apply it soon, since
this has been outstanding for a while.

Regards,
Simon

Applied to u-boot-dm/next, thanks!
Applied to u-boot-dm/next, thanks!

Re: [PATCH v2 1/5] binman: add documentation for binman sign option

[Simon Glass]

Add the documentation about binman sign option and providing an
example.

Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
---
 tools/binman/binman.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Applied to u-boot-dm/next, thanks!
Applied to u-boot-dm/next, thanks!

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

[Ivan Mikhaylov]

On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> Hi Ivan,
> 
> On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> wrote:
> > 
> > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > 
> > 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 e3e7ca4b21..956b8fcd43 100644
> > --- a/test/py/tests/test_vboot.py
> > +++ b/test/py/tests/test_vboot.py
> > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> > 
> >          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
> >              # hashed nodes. If it isn't, a security bypass is
> > possible.
> > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      mkimage = cons.config.build_dir + '/tools/mkimage'
> >      binman = cons.config.source_dir + '/tools/binman/binman'
> >      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.39.1
> > 
> 
> Unfortunately this test fails on sandbox:
> 
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> 
> I think it would be better to put it in its own test (perhaps in the
> same file) so we are not doing it on every test run. Also you could
> check (in a very basic way) that it adds the key correctly since we
> don't really need another test of the logic of doing that. We are
> just
> checking that your tool calls that logic correctly.
> 
> I'll drop this one when applying, for now. Please take a look.
> 
> Regards,
> Simon

Simon, does it look ok? Test for test_vboot is passed fine.

Thanks.

From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Thu, 11 Nov 2021 11:15:12 +0300
Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey tool

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

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index e3e7ca4b21..5ae622fe21 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
         # Check that the boot fails if the global signature is not
provided
         run_bootm(sha_algo, 'global image signature', 'signature is
mandatory', False)
 
+    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
+        """Test fdt_add_pubkey utility with given hash algorithm and
padding.
+
+        This function tests if fdt_add_pubkey utility may add public
keys into dtb.
+
+        Args:
+            sha_algo: Either 'sha1' or 'sha256', to select the
algorithm to use
+            padding: Either '' or '-pss', to select the padding to use
for the
+                    rsa signature algorithm.
+            sign_options: Options to mkimage when signing a fit image.
+        """
+
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
+
+        # Sign images with our dev keys
+        sign_fit(sha_algo, sign_options)
+
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+
+        cons.log.action('%s: Test fdt_add_pubkey with signed
configuration' % sha_algo)
+        # Then add the dev key via the fdt_add_pubkey tool
+        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
('sha256' if algo_arg else sha_algo, \
+                                'rsa3072' if sha_algo == 'sha384' else
'rsa2048'),
+                                '-k', tmpdir, '-n', 'dev', '-r',
'conf', dtb])
+
+        # Check with fit_check_sign that FIT is signed with key
+        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
+
     cons = u_boot_console
     tmpdir = os.path.join(cons.config.result_dir, name) + '/'
     if not os.path.exists(tmpdir):
@@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
     mkimage = cons.config.build_dir + '/tools/mkimage'
     binman = cons.config.source_dir + '/tools/binman/binman'
     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'
@@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
     with open(evil_kernel, 'wb') as fd:
         fd.write(500 * b'\x01')
 
+    test_fdt_add_pubkey(sha_algo, padding, sign_options)
     try:
         # We need to use our own device tree file. Remember to restore
it
         # afterwards.
-- 
2.39.1

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

[Simon Glass]

Hi Ivan,

On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > Hi Ivan,
> >
> > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > wrote:
> > >
> > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > >
> > > 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 e3e7ca4b21..956b8fcd43 100644
> > > --- a/test/py/tests/test_vboot.py
> > > +++ b/test/py/tests/test_vboot.py
> > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >
> > >          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
> > >              # hashed nodes. If it isn't, a security bypass is
> > > possible.
> > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > >      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.39.1
> > >
> >
> > Unfortunately this test fails on sandbox:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> >
> > I think it would be better to put it in its own test (perhaps in the
> > same file) so we are not doing it on every test run. Also you could
> > check (in a very basic way) that it adds the key correctly since we
> > don't really need another test of the logic of doing that. We are
> > just
> > checking that your tool calls that logic correctly.
> >
> > I'll drop this one when applying, for now. Please take a look.
> >
> > Regards,
> > Simon
>
> Simon, does it look ok? Test for test_vboot is passed fine.

Please see my message before:

> Unfortunately this test fails on sandbox:
>
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
>
> I think it would be better to put it in its own test (perhaps in the
> same file) so we are not doing it on every test run. Also you could
> check (in a very basic way) that it adds the key correctly since we
> don't really need another test of the logic of doing that. We are just
> checking that your tool calls that logic correctly.
>
> I'll drop this one when applying, for now. Please take a look.

I have not applied this patch due to the problem.

Regards,
Simon



>
> Thanks.
>
> From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00 2001
> From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Date: Thu, 11 Nov 2021 11:15:12 +0300
> Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey tool
>
> Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> index e3e7ca4b21..5ae622fe21 100644
> --- a/test/py/tests/test_vboot.py
> +++ b/test/py/tests/test_vboot.py
> @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>          # Check that the boot fails if the global signature is not
> provided
>          run_bootm(sha_algo, 'global image signature', 'signature is
> mandatory', False)
>
> +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> +        """Test fdt_add_pubkey utility with given hash algorithm and
> padding.
> +
> +        This function tests if fdt_add_pubkey utility may add public
> keys into dtb.
> +
> +        Args:
> +            sha_algo: Either 'sha1' or 'sha256', to select the
> algorithm to use
> +            padding: Either '' or '-pss', to select the padding to use
> for the
> +                    rsa signature algorithm.
> +            sign_options: Options to mkimage when signing a fit image.
> +        """
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> +
> +        # Sign images with our dev keys
> +        sign_fit(sha_algo, sign_options)
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +
> +        cons.log.action('%s: Test fdt_add_pubkey with signed
> configuration' % sha_algo)
> +        # Then add the dev key via the fdt_add_pubkey tool
> +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> ('sha256' if algo_arg else sha_algo, \
> +                                'rsa3072' if sha_algo == 'sha384' else
> 'rsa2048'),
> +                                '-k', tmpdir, '-n', 'dev', '-r',
> 'conf', dtb])
> +
> +        # Check with fit_check_sign that FIT is signed with key
> +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
> +
>      cons = u_boot_console
>      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
>      if not os.path.exists(tmpdir):
> @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>      mkimage = cons.config.build_dir + '/tools/mkimage'
>      binman = cons.config.source_dir + '/tools/binman/binman'
>      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'
> @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>      with open(evil_kernel, 'wb') as fd:
>          fd.write(500 * b'\x01')
>
> +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
>      try:
>          # We need to use our own device tree file. Remember to restore
> it
>          # afterwards.
> --
> 2.39.1
>
>

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

[Ivan Mikhaylov]

On Thu, 2023-03-16 at 07:59 -0600, Simon Glass wrote:
> Hi Ivan,
> 
> On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com>
> wrote:
> > 
> > On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > > Hi Ivan,
> > > 
> > > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > > wrote:
> > > > 
> > > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > 
> > > > 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 e3e7ca4b21..956b8fcd43 100644
> > > > --- a/test/py/tests/test_vboot.py
> > > > +++ b/test/py/tests/test_vboot.py
> > > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > > 
> > > >          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
> > > >              # hashed nodes. If it isn't, a security bypass is
> > > > possible.
> > > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > > >      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.39.1
> > > > 
> > > 
> > > Unfortunately this test fails on sandbox:
> > > 
> > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > > 
> > > I think it would be better to put it in its own test (perhaps in
> > > the
> > > same file) so we are not doing it on every test run. Also you
> > > could
> > > check (in a very basic way) that it adds the key correctly since
> > > we
> > > don't really need another test of the logic of doing that. We are
> > > just
> > > checking that your tool calls that logic correctly.
> > > 
> > > I'll drop this one when applying, for now. Please take a look.
> > > 
> > > Regards,
> > > Simon
> > 
> > Simon, does it look ok? Test for test_vboot is passed fine.
> 
> Please see my message before:
> 
> > Unfortunately this test fails on sandbox:
> > 
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > 
> > I think it would be better to put it in its own test (perhaps in
> > the
> > same file) so we are not doing it on every test run. Also you could
> > check (in a very basic way) that it adds the key correctly since we
> > don't really need another test of the logic of doing that. We are
> > just
> > checking that your tool calls that logic correctly.
> > 
> > I'll drop this one when applying, for now. Please take a look.
> 
> I have not applied this patch due to the problem.
> 
> Regards,
> Simon
> 

Simon, but I've changed the test and put it in previous note, maybe you
didn't notice, I did what you asked:
- made own test "test_fdt_add_pubkey"
- simple check that with clear dtb you can add keys with fdt_add_pubkey
  and check with fit_check_sign with signed fit.

I've checked that change with sandbox and it passes test_vboot well.

Need I re-submit that patch or whole series?

Thanks.

> 
> 
> > 
> > Thanks.
> > 
> > From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00
> > 2001
> > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > Date: Thu, 11 Nov 2021 11:15:12 +0300
> > Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey
> > tool
> > 
> > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/test/py/tests/test_vboot.py
> > b/test/py/tests/test_vboot.py
> > index e3e7ca4b21..5ae622fe21 100644
> > --- a/test/py/tests/test_vboot.py
> > +++ b/test/py/tests/test_vboot.py
> > @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >          # Check that the boot fails if the global signature is not
> > provided
> >          run_bootm(sha_algo, 'global image signature', 'signature
> > is
> > mandatory', False)
> > 
> > +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> > +        """Test fdt_add_pubkey utility with given hash algorithm
> > and
> > padding.
> > +
> > +        This function tests if fdt_add_pubkey utility may add
> > public
> > keys into dtb.
> > +
> > +        Args:
> > +            sha_algo: Either 'sha1' or 'sha256', to select the
> > algorithm to use
> > +            padding: Either '' or '-pss', to select the padding to
> > use
> > for the
> > +                    rsa signature algorithm.
> > +            sign_options: Options to mkimage when signing a fit
> > image.
> > +        """
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> > +
> > +        # Sign images with our dev keys
> > +        sign_fit(sha_algo, sign_options)
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +
> > +        cons.log.action('%s: Test fdt_add_pubkey with signed
> > configuration' % sha_algo)
> > +        # Then add the dev key via the fdt_add_pubkey tool
> > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> > ('sha256' if algo_arg else sha_algo, \
> > +                                'rsa3072' if sha_algo == 'sha384'
> > else
> > 'rsa2048'),
> > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > 'conf', dtb])
> > +
> > +        # Check with fit_check_sign that FIT is signed with key
> > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > dtb])
> > +
> >      cons = u_boot_console
> >      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
> >      if not os.path.exists(tmpdir):
> > @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      mkimage = cons.config.build_dir + '/tools/mkimage'
> >      binman = cons.config.source_dir + '/tools/binman/binman'
> >      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'
> > @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      with open(evil_kernel, 'wb') as fd:
> >          fd.write(500 * b'\x01')
> > 
> > +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
> >      try:
> >          # We need to use our own device tree file. Remember to
> > restore
> > it
> >          # afterwards.
> > --
> > 2.39.1
> > 
> > 

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

[Simon Glass]

Hi Ivan,

On Thu, 16 Mar 2023 at 08:45, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> On Thu, 2023-03-16 at 07:59 -0600, Simon Glass wrote:
> > Hi Ivan,
> >
> > On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com>
> > wrote:
> > >
> > > On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > > > Hi Ivan,
> > > >
> > > > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > wrote:
> > > > >
> > > > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > >
> > > > > 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 e3e7ca4b21..956b8fcd43 100644
> > > > > --- a/test/py/tests/test_vboot.py
> > > > > +++ b/test/py/tests/test_vboot.py
> > > > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name,
> > > > > sha_algo,
> > > > > padding, sign_options, required,
> > > > >
> > > > >          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
> > > > >              # hashed nodes. If it isn't, a security bypass is
> > > > > possible.
> > > > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name,
> > > > > sha_algo,
> > > > > padding, sign_options, required,
> > > > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > > > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > > > >      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.39.1
> > > > >
> > > >
> > > > Unfortunately this test fails on sandbox:
> > > >
> > > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > > >
> > > > I think it would be better to put it in its own test (perhaps in
> > > > the
> > > > same file) so we are not doing it on every test run. Also you
> > > > could
> > > > check (in a very basic way) that it adds the key correctly since
> > > > we
> > > > don't really need another test of the logic of doing that. We are
> > > > just
> > > > checking that your tool calls that logic correctly.
> > > >
> > > > I'll drop this one when applying, for now. Please take a look.
> > > >
> > > > Regards,
> > > > Simon
> > >
> > > Simon, does it look ok? Test for test_vboot is passed fine.
> >
> > Please see my message before:
> >
> > > Unfortunately this test fails on sandbox:
> > >
> > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > >
> > > I think it would be better to put it in its own test (perhaps in
> > > the
> > > same file) so we are not doing it on every test run. Also you could
> > > check (in a very basic way) that it adds the key correctly since we
> > > don't really need another test of the logic of doing that. We are
> > > just
> > > checking that your tool calls that logic correctly.
> > >
> > > I'll drop this one when applying, for now. Please take a look.
> >
> > I have not applied this patch due to the problem.
> >
> > Regards,
> > Simon
> >
>
> Simon, but I've changed the test and put it in previous note, maybe you
> didn't notice, I did what you asked:
> - made own test "test_fdt_add_pubkey"
> - simple check that with clear dtb you can add keys with fdt_add_pubkey
>   and check with fit_check_sign with signed fit.
>
> I've checked that change with sandbox and it passes test_vboot well.
>
> Need I re-submit that patch or whole series?

OK, great. Yes please send that patch by itself (rebased to
u-boot-dm/next) so it is picked up by patchwork. You'll find the rest
of your patches in dm/next but they have not made it upstream yet.

Regards,
Simon


>
> Thanks.
>
> >
> >
> > >
> > > Thanks.
> > >
> > > From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00
> > > 2001
> > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > Date: Thu, 11 Nov 2021 11:15:12 +0300
> > > Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey
> > > tool
> > >
> > > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > >
> > > diff --git a/test/py/tests/test_vboot.py
> > > b/test/py/tests/test_vboot.py
> > > index e3e7ca4b21..5ae622fe21 100644
> > > --- a/test/py/tests/test_vboot.py
> > > +++ b/test/py/tests/test_vboot.py
> > > @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >          # Check that the boot fails if the global signature is not
> > > provided
> > >          run_bootm(sha_algo, 'global image signature', 'signature
> > > is
> > > mandatory', False)
> > >
> > > +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> > > +        """Test fdt_add_pubkey utility with given hash algorithm
> > > and
> > > padding.
> > > +
> > > +        This function tests if fdt_add_pubkey utility may add
> > > public
> > > keys into dtb.
> > > +
> > > +        Args:
> > > +            sha_algo: Either 'sha1' or 'sha256', to select the
> > > algorithm to use
> > > +            padding: Either '' or '-pss', to select the padding to
> > > use
> > > for the
> > > +                    rsa signature algorithm.
> > > +            sign_options: Options to mkimage when signing a fit
> > > image.
> > > +        """
> > > +
> > > +        # Create a fresh .dtb without the public keys
> > > +        dtc('sandbox-u-boot.dts')
> > > +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> > > +
> > > +        # Sign images with our dev keys
> > > +        sign_fit(sha_algo, sign_options)
> > > +
> > > +        # Create a fresh .dtb without the public keys
> > > +        dtc('sandbox-u-boot.dts')
> > > +
> > > +        cons.log.action('%s: Test fdt_add_pubkey with signed
> > > configuration' % sha_algo)
> > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> > > ('sha256' if algo_arg else sha_algo, \
> > > +                                'rsa3072' if sha_algo == 'sha384'
> > > else
> > > 'rsa2048'),
> > > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > > 'conf', dtb])
> > > +
> > > +        # Check with fit_check_sign that FIT is signed with key
> > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > > dtb])
> > > +
> > >      cons = u_boot_console
> > >      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
> > >      if not os.path.exists(tmpdir):
> > > @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > >      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'
> > > @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      with open(evil_kernel, 'wb') as fd:
> > >          fd.write(500 * b'\x01')
> > >
> > > +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
> > >      try:
> > >          # We need to use our own device tree file. Remember to
> > > restore
> > > it
> > >          # afterwards.
> > > --
> > > 2.39.1
> > >
> > >
>