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

Hi Sughosh,

On Tue, 1 Aug 2023 at 11:41, Sughosh Ganu sughosh.ganu@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@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