Hi Philippe,
On Fri, 25 Feb 2022 at 07:58, Philippe Reynes
<philippe.rey...@softathome.com> wrote:
>
> Adds the support of the pre-load header with the image signature
> to binman.
>
> Signed-off-by: Philippe Reynes <philippe.rey...@softathome.com>
> ---
> tools/binman/etype/pre_load.py | 165 ++++++++++++++++++
> tools/binman/ftest.py | 45 +++++
> tools/binman/test/225_dev.key | 28 +++
> tools/binman/test/225_pre_load.dts | 22 +++
> tools/binman/test/226_pre_load_pkcs.dts | 23 +++
> tools/binman/test/227_pre_load_pss.dts | 23 +++
> .../test/228_pre_load_invalid_padding.dts | 23 +++
> .../binman/test/229_pre_load_invalid_sha.dts | 23 +++
> .../binman/test/230_pre_load_invalid_algo.dts | 23 +++
> .../binman/test/231_pre_load_invalid_key.dts | 23 +++
> 10 files changed, 398 insertions(+)
> create mode 100644 tools/binman/etype/pre_load.py
> create mode 100644 tools/binman/test/225_dev.key
> create mode 100644 tools/binman/test/225_pre_load.dts
> create mode 100644 tools/binman/test/226_pre_load_pkcs.dts
> create mode 100644 tools/binman/test/227_pre_load_pss.dts
> create mode 100644 tools/binman/test/228_pre_load_invalid_padding.dts
> create mode 100644 tools/binman/test/229_pre_load_invalid_sha.dts
> create mode 100644 tools/binman/test/230_pre_load_invalid_algo.dts
> create mode 100644 tools/binman/test/231_pre_load_invalid_key.dts
Reviewed-by: Simon Glass <s...@chromium.org>
But lots of nits below sorry
>
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> new file mode 100644
> index 0000000000..2af2857404
> --- /dev/null
> +++ b/tools/binman/etype/pre_load.py
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2022 Softathome
> +# Written by Philippe Reynes <philippe.rey...@softathome.com>
> +#
> +# Entry-type for the global header
> +#
> +
> +import struct
> +from dtoc import fdt_util
> +from patman import tools
> +
> +from binman.entry import Entry
> +from binman.etype.collection import Entry_collection
> +from binman.entry import EntryArg
> +
> +from Cryptodome.Hash import SHA256, SHA384, SHA512
> +from Cryptodome.PublicKey import RSA
> +from Cryptodome.Signature import pkcs1_15
> +from Cryptodome.Signature import pss
> +
> +PRE_LOAD_MAGIC = b'UBSH'
> +
> +RSAS = {
> + 'rsa1024': 1024 / 8,
> + 'rsa2048': 2048 / 8,
> + 'rsa4096': 4096 / 8
> +}
> +
> +SHAS = {
> + 'sha256': SHA256,
> + 'sha384': SHA384,
> + 'sha512': SHA512
> +}
> +
> +class Entry_pre_load(Entry_collection):
> + """Pre load image header
Run binman entry-docs >tools/binman/entries.rst to regen that file
> +
> + Properties / Entry arguments:
> + - key-path: Path of the directory that store key (provided by the
> environment variable KEY_PATH)
> + - content: List of phandles to entries to sign
> + - algo-name: Hash and signature algo to use for the signature
> + - padding-name: Name of the padding (pkcs-1.5 or pss)
> + - key-name: Filename of the private key to sign
> + - header-size: Total size of the header
> + - version: Version of the header
> +
> + This entry create a pre-load header that contain a global
creates
contains
> + image signature.
> +
> + For example, this creates an image with a pre-load header and a binary::
> +
> + binman {
> + image2 {
> + filename = "sandbox.bin";
> +
> + pre-load {
> + content = <&image>;
> + algo-name = "sha256,rsa2048";
> + padding-name = "pss";
> + key-name = "private.pem";
> + header-size = <4096>;
> + version = <1>;
> + };
> +
> + image: blob-ext {
> + filename = "sandbox.itb";
> + };
> + };
> + };
> + """
> +
> + def __init__(self, section, etype, node):
> + super().__init__(section, etype, node)
> + self.algo_name = fdt_util.GetString(self._node, 'algo-name')
> + self.padding_name = fdt_util.GetString(self._node, 'padding-name')
> + self.key_name = fdt_util.GetString(self._node, 'key-name')
> + self.header_size = fdt_util.GetInt(self._node, 'header-size')
> + self.version = fdt_util.GetInt(self._node, 'version')
> +
> + def _CreateHeader(self):
> + """Create a pre load header"""
> + hash_name, sign_name = self.algo_name.split(',')
> + padding_name=self.padding_name
> + key_path, = self.GetEntryArgsOrProps([EntryArg('key-path', str)])
Please read this into self.key_path in a ReadNode() method. We try to
do all the reading in one place. See other entry types for examples.
> + if key_path is None or key_path == "":
Please use single quotes by default
> + key_name = self.key_name
> + else:
> + key_name = key_path + "/" + self.key_name
key_name = os.path.join(key_path, self.key_name)
(then you don't need the 'if')
> +
> + # Check hash and signature name/type
> + if hash_name not in SHAS:
> + raise ValueError(hash_name + " is not supported")
self.Raise(...
> + if sign_name not in RSAS:
> + raise ValueError(sign_name + "is not supported")
self.Raise(...
> +
> + # Read the key
> + with open(key_name, 'rb') as pem:
> + key = RSA.import_key(pem.read())
> +
> + # Check if the key has the expected size
> + if key.size_in_bytes() != RSAS[sign_name]:
> + raise ValueError("The key " + self.key_name + " don't have the
> expected size")
self.Raise(...
(the reason for that is we want to report the node that had the problem)
> +
> + # Compute the hash
> + hash_image = SHAS[hash_name].new()
> + hash_image.update(self.image)
> +
> + # Compute the signature
> + if padding_name is None:
> + padding_name = "pkcs-1.5"
> + if padding_name == "pss":
> + salt_len = key.size_in_bytes() - hash_image.digest_size - 2
> + padding = pss
> + padding_args = {'salt_bytes': salt_len}
> + elif padding_name == "pkcs-1.5":
> + padding = pkcs1_15
> + padding_args = {}
> + else:
> + raise ValueError(padding_name + " is not supported")
> +
> + sig = padding.new(key, **padding_args).sign(hash_image)
> +
> + hash_sig = SHA256.new()
> + hash_sig.update(sig)
> +
> + version = self.version
> + header_size = self.header_size
> + image_size = len(self.image)
> + ofs_img_sig = 64 + len(sig)
> + flags = 0
> + reserved0 = 0
> + reserved1 = 0
> +
> + first_header = bytearray(64)
> + struct.pack_into('4s', first_header, 0, PRE_LOAD_MAGIC)
> + struct.pack_into('>I', first_header, 4, version)
> + struct.pack_into('>I', first_header, 8, header_size)
> + struct.pack_into('>I', first_header, 12, image_size)
> + struct.pack_into('>I', first_header, 16, ofs_img_sig)
> + struct.pack_into('>I', first_header, 20, flags)
> + struct.pack_into('>I', first_header, 24, reserved0)
> + struct.pack_into('>I', first_header, 28, reserved1)
> + struct.pack_into('32s', first_header, 32, hash_sig.digest())
Can you do
first_header = struct.pack('>4sIIIII...', PRE_LOAD_MAGIC, version,
header_size, ...
> +
> + hash_first_header = SHAS[hash_name].new()
> + hash_first_header.update(first_header)
> + sig_first_header = padding.new(key,
> **padding_args).sign(hash_first_header)
> +
> + data = first_header + sig_first_header + sig
> + pad = bytearray(self.header_size - len(data))
> +
> + return data + pad
> +
> + def ObtainContents(self):
> + """Obtain a placeholder for the header contents"""
> + # wait that the image is available
> + self.image = self.GetContents(False)
> + if self.image is None:
> + return False
> + self.SetContents(self._CreateHeader())
> + return True
> +
> + def ProcessContents(self):
> + data = self._CreateHeader()
> + return self.ProcessContentsUpdate(data)
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 8f00db6945..06b8546354 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -91,6 +91,9 @@ SCP_DATA = b'scp'
> TEST_FDT1_DATA = b'fdt1'
> TEST_FDT2_DATA = b'test-fdt2'
> ENV_DATA = b'var1=1\nvar2="2"'
> +PRE_LOAD_MAGIC = b'UBSH'
> +PRE_LOAD_VERSION = 0x11223344.to_bytes(4, 'big')
> +PRE_LOAD_HDR_SIZE = 0x00001000.to_bytes(4, 'big')
>
> # Subdirectory of the input dir to use to put test FDTs
> TEST_FDT_SUBDIR = 'fdts'
> @@ -5321,6 +5324,48 @@ fdt fdtmap Extract the
> devicetree blob from the fdtmap
> self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
> str(exc.exception))
>
> + def testPreLoad(self):
> + """Test an image with a pre-load header"""
> + entry_args = {
> + 'key-path': '.',
> + }
> + data, _, _, _ = self._DoReadFileDtb('225_pre_load.dts',
> + entry_args=entry_args)
> + self.assertEqual(PRE_LOAD_MAGIC, data[:len(PRE_LOAD_MAGIC)])
> + self.assertEqual(PRE_LOAD_VERSION, data[4:4+len(PRE_LOAD_VERSION)])
> + self.assertEqual(PRE_LOAD_HDR_SIZE, data[8:8+len(PRE_LOAD_HDR_SIZE)])
> + data = self._DoReadFile('225_pre_load.dts')
Please put each .dts file in its own test function, just because that
is how it currently works.
> + self.assertEqual(PRE_LOAD_MAGIC, data[:len(PRE_LOAD_MAGIC)])
> + self.assertEqual(PRE_LOAD_VERSION, data[4:4+len(PRE_LOAD_VERSION)])
> + self.assertEqual(PRE_LOAD_HDR_SIZE, data[8:8+len(PRE_LOAD_HDR_SIZE)])
> + data = self._DoReadFile('226_pre_load_pkcs.dts')
> + self.assertEqual(PRE_LOAD_MAGIC, data[:len(PRE_LOAD_MAGIC)])
> + self.assertEqual(PRE_LOAD_VERSION, data[4:4+len(PRE_LOAD_VERSION)])
> + self.assertEqual(PRE_LOAD_HDR_SIZE, data[8:8+len(PRE_LOAD_HDR_SIZE)])
> + data = self._DoReadFile('227_pre_load_pss.dts')
> + self.assertEqual(PRE_LOAD_MAGIC, data[:len(PRE_LOAD_MAGIC)])
> + self.assertEqual(PRE_LOAD_VERSION, data[4:4+len(PRE_LOAD_VERSION)])
> + self.assertEqual(PRE_LOAD_HDR_SIZE, data[8:8+len(PRE_LOAD_HDR_SIZE)])
Spaces around + please
Regards,
Simon
