[PATCH 1/3] binman: Add support for externally encrypted blobs

[Simon Glass]

Hi Christian,

On Tue, 27 Jun 2023 at 08:39, <christian.taedcke-oss at weidmueller.com> wrote:
>
> From: Christian Taedcke <christian.taedcke at weidmueller.com>
>
> This adds a new etype encrypted that is derived from collection.
>
> It creates a new cipher node in the related image similar to the
> cipher node used by u-boot, see boot/image-cipher.c.
> Optionally it creates a global /cipher node containing a key and iv
> using the same nameing convention as used in boot/image-cipher.c.
>
> Signed-off-by: Christian Taedcke <christian.taedcke at weidmueller.com>
> ---
>
>  tools/binman/etype/encrypted.py | 98 +++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 tools/binman/etype/encrypted.py
>
> diff --git a/tools/binman/etype/encrypted.py b/tools/binman/etype/encrypted.py
> new file mode 100644
> index 0000000000..005ae56acf
> --- /dev/null
> +++ b/tools/binman/etype/encrypted.py
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright 2023 Weidmüller Interface GmbH & Co. KG
> +# Written by Christian Taedcke <christian.taedcke at weidmueller.com>
> +#
> +# Entry-type module for cipher information of encrypted blobs/images
> +#
> +
> +from binman.etype.collection import Entry_collection
> +from dtoc import fdt_util
> +from u_boot_pylib import tools
> +
> +# This is imported if needed
> +state = None
> +
> +
> +class Entry_encrypted(Entry_collection):
> +    """Externally built encrypted binary blob
> +
> +    If the file providing this blob is missing, binman can optionally ignore it
> +    and produce a broken image with a warning.
> +
> +    In addition to the inherited 'collection' for Properties / Entry arguments:
> +        - algo: The encryption algorithm

What possible values are available? Please list them

> +        - iv-name-hint: The name hint for the iv

what is the iv?

> +        - key-name-hint: The name hint for the key
> +        - iv-filename: The name of the file containing the iv
> +        - key-filename: The name of the file containing the key
> +
> +    This entry creates a cipher node in the entries' parent node (i.e. the

entry's

> +    image). Optionally it also creates a cipher node in the root of the device
> +    tree containg key and iv information.

containing

Overall I think this documentation needs to be expanded.

I wonder why this needs to be an entry type? Could the node be added
to the description by the user, instead of the entry adding it? It
feels a little strange to me, but perhaps I just need more info.

> +    """
> +
> +    def __init__(self, section, etype, node):
> +        # Put this here to allow entry-docs and help to work without libfdt
> +        global state
> +        from binman import state
> +
> +        super().__init__(section, etype, node)
> +        # The property key-filename is not required, because some implementations use keys that
> +        # are not embedded in the device tree, but e.g. in the device itself
> +        self.required_props = ['algo', 'key-name-hint', 'iv-filename']
> +        self._algo = fdt_util.GetString(self._node, 'algo')
> +        self._iv_name_hint = fdt_util.GetString(self._node, 'iv-name-hint')
> +        self._key_name_hint = fdt_util.GetString(self._node, 'key-name-hint')
> +        self._filename_iv = fdt_util.GetString(self._node, 'iv-filename')
> +        self._filename_key = fdt_util.GetString(self._node, 'key-filename')

Here you should set these variables to None. Move the reading to ReadNode()

Sorry if there are counter-examples in the source code. But this is
the correct way.

> +
> +    def ReadNode(self):
> +        super().ReadNode()
> +
> +        iv_filename = tools.get_input_filename(self._filename_iv)
> +        self._iv = tools.read_file(iv_filename, binary=True)

Please only read the node in this method. Move file reading until
where it is needed.

> +
> +        self._key = None
> +        if self._filename_key:
> +            key_filename = tools.get_input_filename(self._filename_key)
> +            self._key = tools.read_file(key_filename, binary=True)
> +
> +    def gen_entries(self):
> +        super().gen_entries()
> +
> +        cipher_node = state.AddSubnode(self._node.parent, "cipher")
> +        cipher_node.AddString("algo", self._algo)
> +        cipher_node.AddString("key-name-hint", self._key_name_hint)
> +        if self._iv_name_hint:
> +            cipher_node.AddString("iv-name-hint", self._iv_name_hint)
> +        else:
> +            cipher_node.AddData("iv", self._iv)
> +
> +        if self._key or self._iv_name_hint:
> +            # add cipher node in root
> +            root_node = self._node.parent.parent.parent

The root node is self.GetImage()._node

But why are you adding something to the root node? This seems quite strange.

> +            name = "cipher"
> +            cipher_node = root_node.FindNode(name)
> +            if not cipher_node:
> +                cipher_node = state.AddSubnode(root_node, name)
> +            key_node_name = (
> +                f"key-{self._algo}-{self._key_name_hint}-{self._iv_name_hint}"
> +                if self._iv_name_hint
> +                else f"key-{self._algo}-{self._key_name_hint}"
> +            )
> +            key_node = cipher_node.FindNode(key_node_name)
> +            if not key_node:

This behaviour is not clearly documented above.

> +                key_node = state.AddSubnode(cipher_node, key_node_name)
> +                if self._key:
> +                    key_node.AddData("key", self._key)
> +                if self._iv_name_hint:
> +                    key_node.AddData("iv", self._iv)
> +
> +    def ObtainContents(self):
> +        # ensure that linked content is not added to the device tree again from this entry
> +        self.SetContents(b'')
> +        return True
> +
> +    def ProcessContents(self):
> +        # ensure that linked content is not added to the device tree again from this entry
> +        return self.ProcessContentsUpdate(b'')
> --
> 2.34.1
>

Regards,
Simon