[PATCH v7 08/11] binman: capsule: Add support for generating EFI capsules

[Simon Glass]

Hi Sughosh,

On Sat, 5 Aug 2023 at 13:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Aug 2023 at 00:35, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 12:42, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > Add support in binman for generating EFI capsules. The capsule
> > > > > parameters can be specified through the capsule binman entry. Also add
> > > > > test cases in binman for testing capsule generation.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > ---
> > > > > Changes since V6:
> > > > > * Add macros for the GUID strings in sandbox_efi_capsule.h
> > > > > * Highlight that the private and public keys are mandatory for capsule
> > > > >   signing.
> > > > > * Add a URL link to the UEFI spec, as used in the rst files.
> > > > > * Use local vars for private and public keys in BuildSectionData()
> > > > > * Use local vars for input payload and capsule filenames in
> > > > >   BuildSectionData().
> > > > > * Drop the ProcessContents() and SetImagePos() as the superclass
> > > > >   functions suffice.
> > > > > * Use GUID macro names in the capsule test dts files.
> > > > > * Rename efi_capsule_payload.bin to capsule_input.bin.
> > > > >
> > > > >
> > > > >  include/sandbox_efi_capsule.h                 |  14 ++
> > > >
> > > > Please move this file to a later patch - see below.
> > >
> > > The idea was to also be able to run the binman capsule tests once this
> > > patch was applied. If we are to move this to a separate patch, it
> > > should be the one before this patch. But I guess based on your other
> > > reply, this might not be needed after all.
> >
> > Yes, it should not be needed if we name the test GUIDs. Remember that
> > binman is a standalone tool so cannot reference files outside
> > tools/...although there is no test for that so some things may have
> > crept in.
> >
> > >
> > > >
> > > > Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> > >
> > > Umm, I am not too sure. Maybe we can take a call at a later point if
> > > there are too many files that start cropping up.
> >
> > OK
> >
> > >
> > > >
> > > > >  tools/binman/entries.rst                      |  62 ++++++++
> > > > >  tools/binman/etype/efi_capsule.py             | 143 ++++++++++++++++++
> > > > >  tools/binman/ftest.py                         | 121 +++++++++++++++
> > > > >  tools/binman/test/307_capsule.dts             |  23 +++
> > > > >  tools/binman/test/308_capsule_signed.dts      |  25 +++
> > > > >  tools/binman/test/309_capsule_version.dts     |  24 +++
> > > > >  tools/binman/test/310_capsule_signed_ver.dts  |  26 ++++
> > > > >  tools/binman/test/311_capsule_oemflags.dts    |  24 +++
> > > > >  tools/binman/test/312_capsule_missing_key.dts |  24 +++
> > > > >  .../binman/test/313_capsule_missing_index.dts |  22 +++
> > > > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > > > >  .../test/315_capsule_missing_payload.dts      |  19 +++
> > > > >  13 files changed, 546 insertions(+)
> > > > >  create mode 100644 include/sandbox_efi_capsule.h
> > > > >  create mode 100644 tools/binman/etype/efi_capsule.py
> > > > >  create mode 100644 tools/binman/test/307_capsule.dts
> > > > >  create mode 100644 tools/binman/test/308_capsule_signed.dts
> > > > >  create mode 100644 tools/binman/test/309_capsule_version.dts
> > > > >  create mode 100644 tools/binman/test/310_capsule_signed_ver.dts
> > > > >  create mode 100644 tools/binman/test/311_capsule_oemflags.dts
> > > > >  create mode 100644 tools/binman/test/312_capsule_missing_key.dts
> > > > >  create mode 100644 tools/binman/test/313_capsule_missing_index.dts
> > > > >  create mode 100644 tools/binman/test/314_capsule_missing_guid.dts
> > > > >  create mode 100644 tools/binman/test/315_capsule_missing_payload.dts
> > > > >
> >
> > [..]
> >
> > > > > +
> > > > > +    def ReadNode(self):
> > > > > +        self.ReadEntries()
> > > > > +        super().ReadNode()
> > > >
> > > > I believe those two lines should be swapped.
> > >
> > > Again, like my earlier code for ProcessContents() and SetImagePos(),
> > > which was taken from mkimage.py as reference, this code is on similar
> > > lines to what is in intel_ifwi.py. Both these files are authored by
> > > you, so I took this as reference, especially mkimage.py.
> >
> > OK, then take a look at mkimage.py and follow that. Yes intel_ifwi is
> > around the wrong way. Although these days ReadEntries() is called
> > automatically from entry_Section so you don't need to call it here.
> >
> > >
> > > >
> > > > > +
> > > > > +        self.image_index = fdt_util.GetInt(self._node, 'image-index')
> > > > > +        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
> > > > > +        self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
> > > > > +        self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
> > > > > +        self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
> > > > > +        self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> > > > > +
> > > > > +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> > > > > +        self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
> > > > > +        if ((self.private_key and not self.public_key_cert) or (self.public_key_cert and not self.private_key)):
> > > > > +            self.Raise('Both private key and public key certificate need to be provided')
> > > > > +        elif not (self.private_key and self.public_key_cert):
> > > > > +            self.auth = 0
> > > > > +        else:
> > > > > +            self.auth = 1
> > > > > +
> > > > > +    def ReadEntries(self):
> > > > > +        """Read the subnode to get the payload for this capsule"""
> > > > > +        # We can have a single payload per capsule
> > > > > +        if len(self._node.subnodes) == 0:
> > > > > +            self.Raise('The capsule entry expects at least one subnode for payload')
> > > >
> > > > Still need to drop this
> > >
> > > ?
> > > Should we not check if the input payload is missing? We cannot call
> > > the mkeficapsule tool without an input image(binary).
> >
> > Why not?
>
> The mkeficapsule tool expects a input binary(image blob as it calls
> it) for generation of a normal capsule. Not passing the input image
> and the capsule filename to the tool results in the help being
> printed.
> For e.g. the below command is not passing one filename.
>
> $ ./tools/mkeficapsule -i 0x1 -g 09d7cf52-0720-4710-91d1-08469b7fe9c8
> foo.capsule
> Usage: mkeficapsule [options] <image blob> <output file>
> Options:
> -g, --guid <guid string>    guid for image blob type
> -i, --index <index>         update image index
> -I, --instance <instance>   update hardware instance
> -v, --fw-version <version>  firmware version
> -p, --private-key <privkey file>  private key file
> -c, --certificate <cert file>     signer's certificate file
> -m, --monotonic-count <count>     monotonic count
> -d, --dump_sig              dump signature (*.p7)
> -A, --fw-accept  firmware accept capsule, requires GUID, no image blob
> -R, --fw-revert  firmware revert capsule, takes no GUID, no image blob
> -o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff
> -h, --help                  print a help message
>
>
> So we need an input binary for a normal capsule.

Yes of course I understand that, but it can be empty, I expect. If you
don't have any entries, then your etype implementation should use an
empty file, as written.

No other etype has this sort of restriction so I don't think we should
add it now.

Regards,
Simon