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

[Taedcke, Christian]

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?

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?

> 
> Regards,
> Simon

Regards,
Christian