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

[Simon Glass]

Hi Christian,

On Wed, 12 Jul 2023 at 03:20, Taedcke, Christian
<christian.taedcke-oss at weidmueller.com> wrote:
>
> Hello Simon,
>
> thank you for your help so far.
>
> On 11.07.2023 17:01, Simon Glass wrote:
> > Hi Christian,
> >
> > On Tue, 11 Jul 2023 at 08:58, Taedcke, Christian
> > <christian.taedcke-oss at weidmueller.com> wrote:
> >>
> >> Hello Jonas,
> >>
> >> On 10.07.2023 12:48, Jonas Karlman wrote:
> >>> Hi Christian,
> >>>
> >>> On 2023-07-10 11:25, 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.
> >>>>
> >>>> Signed-off-by: Christian Taedcke <christian.taedcke at weidmueller.com>
> >>>> ---
> >>>>
> >>>> (no changes since v3)
> >>>>
> >>>> Changes in v3:
> >>>> - rebase on u-boot-dm/mkim-working
> >>>> - update doc for functions ObtainContents and ProcessContents
> >>>> - update entries.rst
> >>>>
> >>>> Changes in v2:
> >>>> - add entry documentation
> >>>> - remove global /cipher node
> >>>> - replace key-name-hint with key-source property
> >>>>
> >>>>    tools/binman/entries.rst        |  88 ++++++++++++++++++
> >>>>    tools/binman/etype/encrypted.py | 157 ++++++++++++++++++++++++++++++++
> >>>>    2 files changed, 245 insertions(+)
> >>>>    create mode 100644 tools/binman/etype/encrypted.py
> >>>>
> >>>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> >>>> index b55f424620..d4bc5de1d3 100644
> >>>> --- a/tools/binman/entries.rst
> >>>> +++ b/tools/binman/entries.rst
> >>>> @@ -468,6 +468,94 @@ updating the EC on startup via software sync.
> >>>>
> >>>>
> >>>>
> >>>> +.. _etype_encrypted:
> >>>> +
> >>>> +Entry: encrypted: Externally built encrypted binary blob
> >>>> +--------------------------------------------------------
> >>>> +
> >>>> +This entry provides the functionality to include information about how to
> >>>> +decrypt an encrypted binary. This information is added to the
> >>>> +resulting device tree by adding a new cipher node in the entry's parent
> >>>> +node (i.e. the binary).
> >>>> +
> >>>> +The key that must be used to decrypt the binary is either directly embedded
> >>>> +in the device tree or indirectly by specifying a key source. The key source
> >>>> +can be used as an id of a key that is stored in an external device.
> >>>> +
> >>>> +Using an embedded key
> >>>> +~~~~~~~~~~~~~~~~~~~~~
> >>>> +
> >>>> +This is an example using an embedded key::
> >>>> +
> >>>> +    encrypted_blob: blob-ext {
> >>>> +        filename = "encrypted-blob.bin";
> >>>> +    };
> >>>> +
> >>>> +    encrypted {
> >>>> +        content = <&encrypted_blob>;
> >>>
> >>> Why is this content reference needed?
> >>>
> >>> It does not look like this content is used by the etype and if the etype
> >>> intends to encrypt the content this etype should probably be a section
> >>> and wrap content nodes instead of referencing it.
> >>>
> >>> If the content is not intended to be encrypted by this etype the name
> >>> of the etype is misleading, cipher may be a better name if the intended
> >>> use is to produce a cipher node for an already encrypted blob.
> >>
> >> I tried renaming the etype to cipher, but this leads to further
> >> complications. After renaming the etype to cipher, the node cipher in
> >> the device tree has basically two different states/meanings.
> >> Initially it contains the input to binman, so binman passes the node and
> >> its properties to the etype, so the etype can evaluate it. After this is
> >> done the cipher node contains new, different properties that should not
> >> be evaluated by binman again, but simply written out to the generated
> >> device tree.
> >> But since binman does not simply parse the device tree once and writes
> >> out the result, i get an error when binman tries to pass the generated
> >> cipher node (containing the new properties that should end up in the
> >> resulting device tree) to the cipher etype again.
> >>
> >> So in the next version of the patch series i would keep the etype name
> >> encrypted.
> >
> > Could you please post the error you get? Binman should not process the
> > nodes more than once. I might be able to fix it.
>
> I misinterpreted my issue. So my etype is not processed twice.
>
> But i am not able to replace the cipher node or the content of the
> cipher node. When i try to get the binman output in the unit test, i
> always get the original cipher node, not the one that i am trying to
> create in my etype.
>
> The unit test looks like this:
>    data = self._DoReadFileDtb('295_cipher_key_file.dts')[0]
>    dtb = fdt.Fdt.FromData(data)
>    dtb.Scan()
>    node = dtb.GetNode('/images/u-boot/cipher')
>
> Is this the correct sequence for the unit test?

You should be aware that Binman fakes almost all data in tests, by
default. For example the  'u-boot' entry results in contents of
'1234'. This is for easy inspection and debugging.

If you use _DoReadFileRealDtb() it will avoid that.

>
> In the new cipher etype, i tried 2 approaches, none of them worked for me:
>
> 1. Delete all properties from the current node (self._node) and add the
> new properties. To delete the properties i used:
>    props = list(self._node.props)
>    for prop in props:
>      self._node.DeleteProp(prop)
>
>
> 2. Delete the current cipher node completely and create a new one.
>    parent = self._node.parent
>    self._node.purge()
>    cipher_node = state.AddSubnode(parent, "cipher")
>
> I tried this in the etype method "def gen_entries(self)" and also in
> "def ProcessFdt(self, fdt)"
>
> How can i replace the initial cipher node that is read from the dts file
> with a new cipher node that is created in the etype?

You should only need to add the new properties, as with 2. The purge
should not be necessary unless you are deleting things, etc.

Regards,
Simon