[PATCH v6 6/9] binman: capsule: Add support for generating EFI capsules

Simon Glass sjg at chromium.org
Fri Aug 4 05:02:23 CEST 2023


Hi Sughosh,

On Thu, 3 Aug 2023 at 05:08, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 2 Aug 2023 at 18:23, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Aug 2023 at 11:41, 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 V5:
> > > * Add support for the oemflag parameter used in FWU A/B updates. This
> > >   was missed in the earlier version.
> > > * Use a single function, generate_capsule in the mkeficapsule bintool,
> > >   instead of the multiple functions in earlier version.
> > > * Remove the logic for generating capsules from config file as
> > >   suggested by Simon.
> > > * Use required_props for image index and GUID parameters.
> > > * Use a subnode for the capsule payload instead of using a filename
> > >   for the payload, as suggested by Simon.
> > > * Add a capsule generation test with oemflag parameter being passed.
> > >
> > >
> > >  configs/sandbox_spl_defconfig                 |   1 +
> >
> > move to a later commit
>
> Okay. I think it'd be better to have this change before adding support
> for efi_capsule entry in binman.

OK, I see that it is currently only built for tools-only and a
smattering of other boards. Tools should be built for all boards. I
suppose for now you can create a patch to enable it for all sandbox
boards, e.g. 'default y if SANDBOX' in the Kconfing, in a patch before
this one.

>
>
> >
> > >  tools/binman/btool/mkeficapsule.py            | 101 +++++++++++
> >
> > nit:  Please split this into its own commit, with the etype in the
> > second commit.
>
> Okay
>
> >
> > >  tools/binman/entries.rst                      |  64 +++++++
> > >  tools/binman/etype/efi_capsule.py             | 160 ++++++++++++++++++
> > >  tools/binman/ftest.py                         | 122 +++++++++++++
> > >  tools/binman/test/307_capsule.dts             |  21 +++
> > >  tools/binman/test/308_capsule_signed.dts      |  23 +++
> > >  tools/binman/test/309_capsule_version.dts     |  22 +++
> > >  tools/binman/test/310_capsule_signed_ver.dts  |  24 +++
> > >  tools/binman/test/311_capsule_oemflags.dts    |  22 +++
> > >  tools/binman/test/312_capsule_missing_key.dts |  22 +++
> > >  .../binman/test/313_capsule_missing_index.dts |  20 +++
> > >  .../binman/test/314_capsule_missing_guid.dts  |  19 +++
> > >  .../test/315_capsule_missing_payload.dts      |  17 ++
> > >  14 files changed, 638 insertions(+)
> > >  create mode 100644 tools/binman/btool/mkeficapsule.py
> > >  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
> > >
> > > diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
> > > index 8d50162b27..65223475ab 100644
> > > --- a/configs/sandbox_spl_defconfig
> > > +++ b/configs/sandbox_spl_defconfig
> > > @@ -249,3 +249,4 @@ CONFIG_UNIT_TEST=y
> > >  CONFIG_SPL_UNIT_TEST=y
> > >  CONFIG_UT_TIME=y
> > >  CONFIG_UT_DM=y
> > > +CONFIG_TOOLS_MKEFICAPSULE=y

[..]

> > > +    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) != 1:
> > > +            self.Raise('The capsule entry expects a single subnode for payload')
> >
> > No, we mustn't restruit this. Why would we?
>
> The mkeficapsule tool that generates the capsule expects a single
> image as it's input payload. Why do you see the need to support
> multiple entries as payload?

That's how binman works...it isn't the entry type's job to decide how
the image is laid out. The user could add a section in any cases and
get around any restriction you put here. But the restriction isn't
necessary and is just strange.

>
> mkeficapsule [options] <image blob> <output file>
>
>
> >
> > > +
> > > +        for node in self._node.subnodes:
> > > +            entry = Entry.Create(self, node)
> > > +            entry.ReadNode()
> > > +            self._entries[entry.name] = entry
> > > +
> > > +    def _GenCapsule(self):
> > > +        return self.mkeficapsule.generate_capsule(self.image_index,
> > > +                                                  self.image_guid,
> > > +                                                  self.hardware_instance,
> > > +                                                  self.payload,
> > > +                                                  self.capsule_fname,
> > > +                                                  self.private_key,
> > > +                                                  self.pub_key_cert,
> > > +                                                  self.monotonic_count,
> > > +                                                  self.fw_version,
> > > +                                                  self.capoemflags)
> > > +
> > > +    def BuildSectionData(self, required):
> > > +        if self.auth:
> > > +            if not os.path.isabs(self.private_key):
> > > +                self.private_key =  tools.get_input_filename(self.private_key)
> > > +            if not os.path.isabs(self.pub_key_cert):
> > > +                self.pub_key_cert = tools.get_input_filename(self.pub_key_cert)
> >
> > These should be local vars, not class vars.
>
> ?
>
> These are properties being passed by the user. Why should only these
> be local vars?

Not really.

self.private_key is a string, but here you turn it into an input
filename. You are changing the original property. Just use a local
var.

>
> >  Also these are properties
> > passed in by the user, so you should not be converting them to
> > filenames.
>
> These properties are actually paths to the private and public key cert
> files that a user will pass. In the normal usage, the user would be
> expected to provide the absolute path to the private and public key
> cert files. The above logic is needed for binman tests and out of tree
> builds with relative paths.

I'm fine with the logic, just don't update the property.

>
> >
> > > +        data, self.payload, _ = self.collect_contents_to_file(
> >
> > Please don't add new things to the class in the middle of the class.
> > Doesn't pylint warn about this? This can just be a local var.
>
> Changed this to a local var.
>
> > > +            self._entries.values(), 'capsule_payload')
> > > +        outfile = self._filename if self._filename else self._node.name
> > > +        self.capsule_fname = tools.get_output_filename(outfile)
> >
> > No, the class is for holding properties...you don't use this outside
> > the function, so just:

[..]

> > > +    def ProcessContents(self):
> > > +        ok = super().ProcessContents()
> > > +        data = self.BuildSectionData(True)
> > > +        ok2 = self.ProcessContentsUpdate(data)
> > > +        return ok and ok2
> >
> > If that is the same as the entry_Section function, you can drop it
>
> There are differences, so this is needed.

Why is it different? Deleting the code has no effect on the tests that
I can see.

[..]

> > > +        payload_data = EFI_CAPSULE_DATA
> > > +
> > > +        # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
> > > +        self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
> > > +        # Image GUID - offset(96 - 128)
> > > +        self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
> > > +
> > > +        if capoemflags:
> > > +            # OEM Flags - offset(40 - 44)
> > > +            self.assertEqual(oemflag, data.hex()[40:44])
> > > +        if signed_capsule and version_check:
> > > +            # FMP header signature - offset(4770 - 4778)
> > > +            self.assertEqual(fmp_signature, data.hex()[4770:4778])
> > > +            # FMP header size - offset(4778 - 4780)
> >
> > Can you mention where these values came from and how you created them?
> > This all seems very brittle. Is there a tool that prints them out,
> > or..?
>
> Like I mentioned earlier as well, the contents of a capsule are
> dependent on the input parameters specified in it's generation. For
> e.g. the capsule generated would be different when opting for passing
> the version param. Similarly when generating a signed capsule -- even
> with a signed capsule, the contents would depend on the keys. The
> offsets therefore are not fixed but depend on the inputs specified. I
> have added comments here for better understanding. Not sure what can
> be done to improve this.
>
> This is also one reason why the testing of the EFI capsule update
> feature should use capsules generated as part of the sandbox platform
> build. That makes the testing that much more robust.

We have the same problem with mkimage. Is there a 'dump_capsule'
command that can output information about it? Then you could use that
in this test.

>
> >
> > > +            self.assertEqual(fmp_size, data.hex()[4778:4780])
> > > +            # firmware version - offset(4786 - 4788)
> > > +            self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
> > > +            # payload offset signed capsule(4802 - 4808)
> > > +            self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
> > > +        elif signed_capsule:
> > > +            # payload offset signed capsule(4770 - 4776)
> > > +            self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
> > > +        elif version_check:
> > > +            # FMP header signature - offset(184 - 192)
> > > +            self.assertEqual(fmp_signature, data.hex()[184:192])
> > > +            # FMP header size - offset(192 - 194)
> > > +            self.assertEqual(fmp_size, data.hex()[192:194])
> > > +            # firmware version - offset(200 - 202)
> > > +            self.assertEqual(fmp_fw_version, data.hex()[200:202])
> > > +            # payload offset for non-signed capsule with version header(216 - 222)

[..]

>
> >
> > > +                       hardware-instance = <0x0>;
> > > +
> > > +                       blob-ext {
> > > +                               filename = "efi_capsule_payload.bin";
> >
> > Do you need this filename?
>
> Yes, this is the payload that will be used in generating the capsule.

Oh, this is so confusing. I think you can just make this a 'blob'
instead of a 'blob-ext'. How about capsule_input.bin as it is shorter?

[..]

Regards,
Simon


More information about the U-Boot mailing list