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

Hi Christian,

On Tue, 27 Jun 2023 at 08:39, christian.taedcke-oss@weidmueller.com wrote:

From: Christian Taedcke christian.taedcke@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@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@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