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

[Simon Glass]

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.

Regards,
Simon