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

Simon Glass sjg at chromium.org
Wed Aug 2 14:52:51 CEST 2023


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

>  tools/binman/btool/mkeficapsule.py            | 101 +++++++++++

nit:  Please split this into its own commit, with the etype in the
second commit.

>  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
> diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py
> new file mode 100644
> index 0000000000..da1d5de873
> --- /dev/null
> +++ b/tools/binman/btool/mkeficapsule.py
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright 2023 Linaro Limited
> +#
> +"""Bintool implementation for mkeficapsule tool
> +
> +mkeficapsule is a tool used for generating EFI capsules.
> +
> +The following are the command-line options to be provided
> +to the tool
> +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
> +"""
> +
> +from binman import bintool
> +
> +class Bintoolmkeficapsule(bintool.Bintool):
> +    """Handles the 'mkeficapsule' tool
> +
> +    This bintool is used for generating the EFI capsules. The
> +    capsule generation parameters can either be specified through
> +    command-line, or through a config file.

commandline (or command line, but please be consistent
)
> +    """
> +    def __init__(self, name):
> +        super().__init__(name, 'mkeficapsule tool for generating capsules')
> +
> +    def generate_capsule(self, image_index, image_guid, hardware_instance,
> +                         payload, output_fname, priv_key, pub_key,
> +                         monotonic_count=0, version=0, oemflags=0):
> +        """Generate a capsule through commandline provided parameters

commandline-provided

> +
> +        Args:
> +            image_index (int): Unique number for identifying payload image
> +            image_guid (str): GUID used for identifying the image
> +            hardware_instance (int): Optional unique hardware instance of
> +            a device in the system. 0 if not being used
> +            payload (str): Path to the input payload image
> +            output_fname (str): Path to the output capsule file
> +            priv_key (str): Path to the private key
> +            pub_key(str): Path to the public key
> +            monotonic_count (int): Count used when signing an image
> +            version (int): Image version (Optional)
> +            oemflags (int): Optional 16 bit OEM flags
> +
> +        Returns:
> +            str: Tool output
> +        """
> +        args = [
> +            f'--index={image_index}',
> +            f'--guid={image_guid}',
> +            f'--instance={hardware_instance}'
> +        ]
> +
> +        if version:
> +            args += [f'--fw-version={version}']
> +        if oemflags:
> +            args += [f'--capoemflag={oemflags}']
> +        if priv_key and pub_key:
> +            args += [
> +                f'--monotonic-count={monotonic_count}',
> +                f'--private-key={priv_key}',
> +                f'--certificate={pub_key}'
> +            ]
> +
> +        args += [
> +            payload,
> +            output_fname
> +        ]
> +
> +        return self.run_cmd(*args)
> +
> +    def fetch(self, method):
> +        """Fetch handler for mkeficapsule
> +
> +        This builds the tool from source
> +
> +        Returns:
> +            tuple:
> +                str: Filename of fetched file to copy to a suitable directory
> +                str: Name of temp directory to remove, or None
> +        """
> +        if method != bintool.FETCH_BUILD:
> +            return None
> +
> +        cmd = ['tools-only_defconfig', 'tools']
> +        result = self.build_from_git(
> +            'https://source.denx.de/u-boot/u-boot.git',
> +            cmd,
> +            'tools/mkeficapsule')
> +        return result
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index f2376932be..cfe699361c 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -468,6 +468,70 @@ updating the EC on startup via software sync.
>
>

The btool looks fine to me

[..]

> diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
> new file mode 100644
> index 0000000000..b8a269e247
> --- /dev/null
> +++ b/tools/binman/etype/efi_capsule.py
> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2023 Linaro Limited
> +#
> +# Entry-type module for producing a EFI capsule
> +#
> +
> +from collections import OrderedDict
> +import os
> +
> +from binman.entry import Entry
> +from binman.etype.section import Entry_section
> +from dtoc import fdt_util
> +from u_boot_pylib import tools
> +
> +class Entry_efi_capsule(Entry_section):
> +    """Entry for generating EFI capsules

"""Generate EPI capsules

(as we know it is an entry type)

> +
> +    This is an entry for generating EFI capsules.

Drop that as it repeats the above line

> +
> +    The parameters needed for generation of the capsules can
> +    either be provided as properties in the entry.

or...?

> +
> +    Properties / Entry arguments:
> +    - image-index: Unique number for identifying corresponding
> +      payload image. Number between 1 and descriptor count, i.e.
> +      the total number of firmware images that can be updated.
> +    - image-type-id: Image GUID which will be used for identifying the
> +      updatable image on the board.
> +    - hardware-instance: Optional number for identifying unique
> +      hardware instance of a device in the system. Default value of 0
> +      for images where value is not to be used.
> +    - fw-version: Optional value of image version that can be put on
> +      the capsule through the Firmware Management Protocol(FMP) header.
> +    - monotonic-count: Count used when signing an image.
> +    - private-key: Path to PEM formatted .key private key file.

Can you perhaps put these in a different order do you can say that if
private-key is provided, you must also provide public-key-cert ?

> +    - pub-key-cert: Path to PEM formatted .crt public key certificate

public, I think, since you write out private in full

> +      file.
> +    - capoemflags - Optional OEM flags to be passed through capsule header.

oem-flags ?

If 'cap' refers to capsule, we already know it is a capsule

> +    - filename:

Technically this is not true...it is the section output. I think it is
better to mention that it inherits the section property, which allows
an output filename.

Optional path to the output capsule file. A capsule is a
> +      continuous set of data as defined by the EFI specification. Refer
> +      to the specification for more details.

Good comments. You can drop filename, since you subclass entry_Section
which always provides this feature. But it's OK to mention it if you
like, e.g. "All section properties are supported, include filename".

> +
> +    For more details on the description of the capsule format, and the capsule
> +    update functionality, refer Section 8.5 and Chapter 23 in the UEFI
> +    specification.
> +    https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf

I suppose this is OK, but can you make it into a text link? e.g. see
how doc/usage/cmd/acpi.rst does it.

> +
> +    The capsule parameters like image index and image GUID are passed as
> +    properties in the entry. The payload to be used in the capsule is to be
> +    provided as a subnode of the capsule entry.
> +
> +    A typical capsule entry node would then look something like this
> +
> +    capsule {
> +            type = "efi-capsule";
> +            image-index = <0x1>;
> +            /* Image GUID for testing capsule update */
> +            image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";

lower-case. Also please use a name here, e.g. a #fdefine above, if
this isn't something already in U-Boot. I very much want to avoid
people adding random UUIDs everywhere.

> +            hardware-instance = <0x0>;
> +            private-key = "tools/binman/test/key.key";
> +            pub-key-cert = "tools/binman/test/key.pem";

Drop the path for these

> +            capoemflags = <0x8000>;
> +
> +            u-boot {
> +            };
> +    };
> +
> +    In the above example, the capsule payload is the u-boot image. The

U-Boot

> +    capsule entry would read the contents of the payload and put them
> +    into the capsule. Any external file can also be specified as the
> +    payload using the blob-ext subnode.
> +    """
> +    def __init__(self, section, etype, node):
> +        super().__init__(section, etype, node)
> +        self.required_props = ['image-index', 'image-type-id']
> +        self.image_index = 0
> +        self.image_guid = ''
> +        self.hardware_instance = 0
> +        self.monotonic_count = 0
> +        self.fw_version = 0
> +        self.capoemflags = 0
> +        self.private_key = ''
> +        self.pub_key_cert = ''
> +        self.auth = 0
> +        self.capsule_fname = ''

Please drop

> +        self._entries = OrderedDict()

entry_Section already does this

> +
> +    def ReadNode(self):
> +        self.ReadEntries()
> +        super().ReadNode()
> +
> +        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.capoemflags = fdt_util.GetInt(self._node, 'capoemflags')
> +
> +        self.private_key = fdt_util.GetString(self._node, 'private-key')
> +        self.pub_key_cert = fdt_util.GetString(self._node, 'pub-key-cert')
> +        if ((self.private_key and not self.pub_key_cert) or (self.pub_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.pub_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) != 1:
> +            self.Raise('The capsule entry expects a single subnode for payload')

No, we mustn't restruit this. Why would we?

> +
> +        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. Also these are properties
passed in by the user, so you should not be converting them to
filenames.

> +        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.
> +            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:

fname = tools....

> +        if self._GenCapsule() is not None:

Here you need to pass some of the params. Honestly, why not drop
_GenCapsule() and just call generate_capsule() here? It seems easier.

> +            os.remove(self.payload)
> +            return tools.read_file(self.capsule_fname)

return tools.read_file(fname)

> +
> +    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

> +
> +    def SetImagePos(self, image_pos):
> +        upto = 0
> +        for entry in self.GetEntries().values():
> +            entry.SetOffsetSize(upto, None)
> +            upto += entry.size
> +
> +        super().SetImagePos(image_pos)

This function looks the same as entry_Section. Drop it?

> +
> +    def AddBintools(self, btools):
> +        self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 1cfa349d38..7e7926b607 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -48,6 +48,7 @@ U_BOOT_VPL_DATA       = b'vpl76543210fedcbazywxyz_'
>  BLOB_DATA             = b'89'
>  ME_DATA               = b'0abcd'
>  VGA_DATA              = b'vga'
> +EFI_CAPSULE_DATA      = b'efi'
>  U_BOOT_DTB_DATA       = b'udtb'
>  U_BOOT_SPL_DTB_DATA   = b'spldtb'
>  U_BOOT_TPL_DTB_DATA   = b'tpldtb'
> @@ -119,6 +120,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
>
>  TEE_ADDR = 0x5678
>
> +# Firmware Management Protocol(FMP) GUID
> +FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> +# Image GUID specified in the DTS
> +CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> +
>  class TestFunctional(unittest.TestCase):
>      """Functional tests for binman
>
> @@ -215,6 +221,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>          TestFunctional._MakeInputFile('rockchip-tpl.bin', ROCKCHIP_TPL_DATA)
>          TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)
> +        TestFunctional._MakeInputFile('efi_capsule_payload.bin', EFI_CAPSULE_DATA)
>
>          # Add a few .dtb files for testing
>          TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> @@ -7087,5 +7094,120 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>           self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"),
>                            "key")
>
> +    def _CheckCapsule(self, data, signed_capsule=False, version_check=False,
> +                      capoemflags=False):
> +        fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
> +        fmp_size = "10"
> +        fmp_fw_version = "02"
> +        oemflag = "0080"
> +
> +        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..?

> +            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)
> +            self.assertEqual(payload_data.hex(), data.hex()[216:222])
> +        else:
> +            # payload offset for non-signed capsule with no version header(184 - 190)
> +            self.assertEqual(payload_data.hex(), data.hex()[184:190])
> +
> +    def testCapsuleGen(self):
> +        """Test generation of EFI capsule"""
> +        data = self._DoReadFile('307_capsule.dts')
> +
> +        self._CheckCapsule(data)
> +
> +    def testSignedCapsuleGen(self):
> +        """Test generation of EFI capsule"""
> +        data = tools.read_file(self.TestFile("key.key"))
> +        self._MakeInputFile("key.key", data)
> +        data = tools.read_file(self.TestFile("key.pem"))
> +        self._MakeInputFile("key.crt", data)
> +
> +        data = self._DoReadFile('308_capsule_signed.dts')
> +
> +        self._CheckCapsule(data, signed_capsule=True)
> +
> +    def testCapsuleGenVersionSupport(self):
> +        """Test generation of EFI capsule with version support"""
> +        data = self._DoReadFile('309_capsule_version.dts')
> +
> +        self._CheckCapsule(data, version_check=True)
> +
> +    def testCapsuleGenSignedVer(self):
> +        """Test generation of signed EFI capsule with version information"""
> +        data = tools.read_file(self.TestFile("key.key"))
> +        self._MakeInputFile("key.key", data)
> +        data = tools.read_file(self.TestFile("key.pem"))
> +        self._MakeInputFile("key.crt", data)
> +
> +        data = self._DoReadFile('310_capsule_signed_ver.dts')
> +
> +        self._CheckCapsule(data, signed_capsule=True, version_check=True)
> +
> +    def testCapsuleGenCapOemFlags(self):
> +        """Test generation of EFI capsule with OEM Flags set"""
> +        data = self._DoReadFile('311_capsule_oemflags.dts')
> +
> +        self._CheckCapsule(data, capoemflags=True)
> +
> +
> +    def testCapsuleGenKeyMissing(self):
> +        """Test that binman errors out on missing key"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('312_capsule_missing_key.dts')
> +
> +        self.assertIn("Both private key and public key certificate need to be provided",
> +                      str(e.exception))
> +
> +    def testCapsuleGenIndexMissing(self):
> +        """Test that binman errors out on missing image index"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('313_capsule_missing_index.dts')
> +
> +        self.assertIn("entry is missing properties: image-index",
> +                      str(e.exception))
> +
> +    def testCapsuleGenGuidMissing(self):
> +        """Test that binman errors out on missing image GUID"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('314_capsule_missing_guid.dts')
> +
> +        self.assertIn("entry is missing properties: image-type-id",
> +                      str(e.exception))
> +
> +    def testCapsuleGenPayloadMissing(self):
> +        """Test that binman errors out on missing input(payload)image"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoReadFile('315_capsule_missing_payload.dts')
> +
> +        self.assertIn("The capsule entry expects a single subnode for payload",
> +                      str(e.exception))
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts
> new file mode 100644
> index 0000000000..dd5b6f1c11
> --- /dev/null
> +++ b/tools/binman/test/307_capsule.dts
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               efi-capsule {
> +                       image-index = <0x1>;
> +                       /* Image GUID for testing capsule update */
> +                       image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";

Please create a #include files for these (and use lower case). Please
fix globally.

> +                       hardware-instance = <0x0>;
> +
> +                       blob-ext {
> +                               filename = "efi_capsule_payload.bin";

Do you need this filename?

> +                       };
> +               };
> +       };
> +};

[..]

Regards,
Simon


More information about the U-Boot mailing list