Re: [PATCH v4 06/12] binman: capsule: Add support for generating capsules

Hi Sughosh,

On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu sughosh.ganu@linaro.org wrote:

Add support in binman for generating capsules. The capsule parameters can be specified either through a config file or through the capsule binman entry. Also add test cases in binman for capsule generation, and enable this testing on the sandbox_spl variant.

Can you use sandbox instead, or perhaps sandbox_spl? SPL is really for SPL testing.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V3:

• Add test cases for covering the various capsule generation scenarios.
• Add function comments in the mkeficapsule bintool.
• Fix the fetch method of the mkeficapsule bintool to enable building the tool.
• Add more details about the capsule parameters in the documentation as well as the code.
• Fix order of module imports, and addition of blank lines in the capsule.py file.
• Use SetContents in the ObtainContents method.

configs/sandbox_spl_defconfig | 1 + tools/binman/btool/mkeficapsule.py | 158 ++++++++++++++++++ tools/binman/entries.rst | 37 ++++ tools/binman/etype/capsule.py | 132 +++++++++++++++ tools/binman/ftest.py | 127 ++++++++++++++ tools/binman/test/282_capsule.dts | 18 ++ tools/binman/test/283_capsule_signed.dts | 20 +++ tools/binman/test/284_capsule_conf.dts | 14 ++ tools/binman/test/285_capsule_missing_key.dts | 19 +++ .../binman/test/286_capsule_missing_index.dts | 17 ++ .../binman/test/287_capsule_missing_guid.dts | 17 ++ .../test/288_capsule_missing_payload.dts | 17 ++ tools/binman/test/289_capsule_missing.dts | 17 ++ tools/binman/test/290_capsule_version.dts | 19 +++ tools/binman/test/capsule_cfg.txt | 6 + 15 files changed, 619 insertions(+) create mode 100644 tools/binman/btool/mkeficapsule.py create mode 100644 tools/binman/etype/capsule.py create mode 100644 tools/binman/test/282_capsule.dts create mode 100644 tools/binman/test/283_capsule_signed.dts create mode 100644 tools/binman/test/284_capsule_conf.dts create mode 100644 tools/binman/test/285_capsule_missing_key.dts create mode 100644 tools/binman/test/286_capsule_missing_index.dts create mode 100644 tools/binman/test/287_capsule_missing_guid.dts create mode 100644 tools/binman/test/288_capsule_missing_payload.dts create mode 100644 tools/binman/test/289_capsule_missing.dts create mode 100644 tools/binman/test/290_capsule_version.dts create mode 100644 tools/binman/test/capsule_cfg.txt

This looks pretty good to me. Some nits below

diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index dd848c57c6..2fcc789347 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -248,3 +248,4 @@ CONFIG_UNIT_TEST=y CONFIG_SPL_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y +CONFIG_TOOLS_MKEFICAPSULE=y

Why enabling this here? I don't think it is needed in sandbox_spl, but in any case it should be in a different patch if needed.

diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py new file mode 100644 index 0000000000..ba6b666714 --- /dev/null +++ b/tools/binman/btool/mkeficapsule.py @@ -0,0 +1,158 @@ +# 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
  

•   -f, --cfg-file <config file> config file with capsule parameters
  

•   -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.
• """
• def __init__(self, name):

•    super().__init__(name, 'mkeficapsule tool for generating capsules')
  

• def capsule_cfg_file(self, cfg_file):

•    """Generate a capsule reading parameters from config file
  

•    Args:
  

•        cfg_file (str): Path to the config file
  

•    Returns:
  

•        str: Tool output
  

•    """
  

nit: drop blank lines after """ function comment (please fix throughout)

•    args = [
  

•        f'--cfg-file={cfg_file}'
  

•    ]
  

•    return self.run_cmd(*args)
  

• def cmdline_capsule(self, image_index, image_guid, hardware_instance,

•                    payload, output_fname, version=0):
  

•    """Generate a capsule through commandline provided parameters
  

•    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
  

•        version (int): Image version (Optional)
  

•    Returns:
  

•        str: Tool output
  

•    """
  

•    if version:
  

•        args = [
  

•            f'--index={image_index}',
  

•            f'--fw-version={version}',
  

•            f'--guid={image_guid}',
  

•            f'--instance={hardware_instance}',
  

•            payload,
  

•            output_fname
  

•        ]
  

•    else:
  

•        args = [
  

•            f'--index={image_index}',
  

•            f'--guid={image_guid}',
  

•            f'--instance={hardware_instance}',
  

•            payload,
  

•            output_fname
  

•        ]
  

•    return self.run_cmd(*args)
  

• def cmdline_auth_capsule(self, image_index, image_guid, hardware_instance,

•                         monotonic_count, priv_key, pub_key,
  

•                         payload, output_fname, version=0):
  

•    """Generate a signed capsule through commandline provided parameters
  

•    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
  

•        monotonic_count (int): Count used when signing an image
  

•        priv_key (str): Path to the private key
  

•        pub_key(str): Path to the public key
  

•        payload (str): Path to the input payload image
  

•        output_fname (str): Path to the output capsule file
  

•        version (int): Image version (Optional)
  

•    Returns:
  

•        str: Tool output
  

•    """
  

•    if version:
  

•        args = [
  

•            f'--index={image_index}',
  

•            f'--guid={image_guid}',
  

•            f'--instance={hardware_instance}',
  

•            f'--monotonic-count={monotonic_count}',
  

•            f'--private-key={priv_key}',
  

•            f'--certificate={pub_key}',
  

•            f'--fw-version={version}',
  

•            payload,
  

•            output_fname
  

•        ]
  

•    else:
  

•        args = [
  

•            f'--index={image_index}',
  

•            f'--guid={image_guid}',
  

•            f'--instance={hardware_instance}',
  

•            f'--monotonic-count={monotonic_count}',
  

•            f'--private-key={priv_key}',
  

•            f'--certificate={pub_key}',
  

•            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 b71af801fd..523c8222f5 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -283,6 +283,43 @@ entry; similarly for SPL.

+.. _etype_capsule:

+Entry: capsule: Entry for generating EFI Capsule files +------------------------------------------------------

+This is an entry for generating EFI capsules.

+The parameters needed for generation of the capsules can either be +provided separately, or through a config file.

+Properties / Entry arguments:

• • cfg-file: Config file for providing capsule

•  parameters. These are parameters needed for generating the
  

•  capsules. The parameters can be listed by running the
  

•  './tools/mkeficapsule -h' command.
  

• • 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.
  

• • monotomic-count: Count used when signing an image.
• • private-key: Path to PEM formatted .key private key file.
• • pub-key-cert: Path to PEM formatted .crt public key certificate

•  file.
  

• • filename: Path to the input(payload) file. File can be any

•  format, a binary or an elf, platform specific.
  

• • capsule: Path to the output capsule file. A capsule is a

•  continous set of data as defined by the EFI specification. Refer
  

•  to the specification for more details.
  

.. _etype_cbfs:

Entry: cbfs: Coreboot Filesystem (CBFS) diff --git a/tools/binman/etype/capsule.py b/tools/binman/etype/capsule.py new file mode 100644 index 0000000000..46e5507704 --- /dev/null +++ b/tools/binman/etype/capsule.py @@ -0,0 +1,132 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2023 Linaro Limited +# +# Entry-type module for producing a capsule +#

+import os

+from binman.entry import Entry +from dtoc import fdt_util +from u_boot_pylib import tools

+class Entry_capsule(Entry):

• """Entry for generating EFI capsules
• This is an entry for generating EFI capsules.
• The parameters needed for generation of the capsules can
• either be provided separately, or through a config file.
• Properties / Entry arguments:
• • cfg-file: Config file for providing capsule

•  parameters. These are parameters needed for generating the
  

•  capsules. The parameters can be listed by running the
  

•  './tools/mkeficapsule -h' command.
  

• • 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.
  

• • monotomic-count: Count used when signing an image.

monotonic

• • private-key: Path to PEM formatted .key private key file.
• • pub-key-cert: Path to PEM formatted .crt public key certificate

•  file.
  

• • filename: Path to the input(payload) file. File can be any

•  format, a binary or an elf, platform specific.
  

• • capsule: Path to the output capsule file. A capsule is a

•  continous set of data as defined by the EFI specification. Refer
  

continuous

Can you add a link to EFI spec so it appears in the docs here?

•  to the specification for more details.
  

• """
• def __init__(self, section, etype, node):

•    super().__init__(section, etype, node)
  

•    self.image_index = 0
  

•    self.image_guid = ""
  

•    self.hardware_instance = 0
  

•    self.monotonic_count = 0
  

•    self.fw_version = 0
  

•    self.private_key = ""
  

•    self.pub_key_cert = ""
  

•    self.auth = 0
  

•    self.payload = ""
  

•    self.capsule_fname = ""
  

Please remember to use single quotes

• def ReadNode(self):

•    super().ReadNode()
  

•    self.cfg_file = fdt_util.GetString(self._node, 'cfg-file')
  

•    if not self.cfg_file:
  

•        self.image_index = fdt_util.GetInt(self._node, 'image-index')
  

•        if not self.image_index:
  

•            self.Raise('mkeficapsule must be provided an Image Index')
  

•        self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
  

•        if not self.image_guid:
  

•            self.Raise('mkeficapsule must be provided an Image GUID')
  

Use self.required_props = ['image-type-id', ...] in your __init__(). Then this is automatic

•        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.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
  

•        self.payload = fdt_util.GetString(self._node, 'filename')
  

•        if not self.payload:
  

•            self.Raise('mkeficapsule must be provided an input filename(payload)')
  

•        if not os.path.isabs(self.payload):
  

•            self.payload_path = tools.get_input_filename(self.payload)
  

•            if not os.path.exists(self.payload_path):
  

•                self.Raise('Cannot resolve path to the input filename(payload)')
  

•            else:
  

•                self.payload = self.payload_path
  

•        self.capsule_fname = fdt_util.GetString(self._node, 'capsule')
  

•        if not self.capsule_fname:
  

•            self.Raise('Specify the output capsule file')
  

•        if not os.path.isabs(self.capsule_fname):
  

•            self.capsule_path = tools.get_output_filename(self.capsule_fname)
  

•            self.capsule_fname = self.capsule_path
  

• def _GenCapsule(self):

What if some of the inputs are missing? Does this handle missing blobs?

•    if self.cfg_file:
  

•        return self.mkeficapsule.capsule_cfg_file(self.cfg_file)
  

•    elif self.auth:
  

•        return self.mkeficapsule.cmdline_auth_capsule(self.image_index,
  

•                                                      self.image_guid,
  

•                                                      self.hardware_instance,
  

•                                                      self.monotonic_count,
  

•                                                      self.private_key,
  

•                                                      self.pub_key_cert,
  

•                                                      self.payload,
  

•                                                      self.capsule_fname,
  

•                                                      self.fw_version)
  

•    else:
  

•        return self.mkeficapsule.cmdline_capsule(self.image_index,
  

•                                                 self.image_guid,
  

•                                                 self.hardware_instance,
  

•                                                 self.payload,
  

•                                                 self.capsule_fname,
  

•                                                 self.fw_version)
  

• def ObtainContents(self):

•    self.SetContents(tools.to_bytes(self._GenCapsule()))
  

•    return True
  

• def AddBintools(self, btools):

•    self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
  

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 43b4f850a6..f5dd62d028 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6676,6 +6676,133 @@ fdt fdtmap Extract the devicetree blob from the fdtmap ['fit']) self.assertIn("Node '/fit': Missing tool: 'mkimage'", str(e.exception))

• def _CheckCapsule(self, signed_capsule=False, version_check=False):

•    # Firmware Management Protocol GUID used in capsule header
  

•    self.capsule_guid = "edd5cb6d2de8444cbda17194199ad92a"
  

Please add a constant FW_MGMT_GUID = '' at the top of the file

We really should not have GUIDs in the code...they are a mess.

•    # Image GUID specified in the DTS
  

•    self.image_guid = "52cfd7092007104791d108469b7fe9c8"
  

•    self.fmp_signature = "4d535331"
  

•    self.fmp_size = "10"
  

•    self.fmp_fw_version = "02"
  

These should really be local vars, not members.

•    self.capsule_data = tools.read_file(self.capsule_fname)
  

Pass the data in here and then you don't need to read the file

•    self.assertEqual(self.capsule_guid, self.capsule_data.hex()[:32])
  

•    self.assertEqual(self.image_guid, self.capsule_data.hex()[96:128])
  

•    if version_check:
  

•        self.assertEqual(self.fmp_signature, self.capsule_data.hex()[184:192])
  

•        self.assertEqual(self.fmp_size, self.capsule_data.hex()[192:194])
  

•        self.assertEqual(self.fmp_fw_version, self.capsule_data.hex()[200:202])
  

•    if signed_capsule:
  

•        self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[4770:4778])
  

Where do these integer offsets come from? Please add a comment

•    elif version_check:
  

•        self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[216:224])
  

•    else:
  

•        self.assertEqual(self.payload_data.hex(), self.capsule_data.hex()[184:192])
  

• def _GenCapsuleCfgPayload(self, fname, contents):

•    capsule_dir = '/tmp/capsules/'
  

You can't write to /tmp

Please use self._indir for input files - see how other tests do it

•    pathname = os.path.join(capsule_dir, fname)
  

•    dirname = os.path.dirname(pathname)
  

•    if dirname and not os.path.exists(dirname):
  

•        os.makedirs(dirname)
  

•    with open(pathname, 'wb') as fd:
  

•        fd.write(contents)
  

tools.write_file(pathname, contents)

• def testCapsuleGen(self):

•    """Test generation of EFI capsule"""
  

•    self.payload_data = tools.to_bytes(TEXT_DATA)
  

Please can you use your own test data, like EFI_DATA ? Also if you declare it as a binary string you can drop the call.

For example:

EFI_DATA = b'efi'

•    TestFunctional._MakeInputFile('payload.txt', self.payload_data)
  

•    self._DoReadFile('282_capsule.dts')
  

data = self...

•    self.capsule_fname = tools.get_output_filename('test.capsule')
  

•    self.assertTrue(os.path.exists(self.capsule_fname))
  

You can drop that line

•    self._CheckCapsule()
  

self._CheckCapsule(data)

• def testSignedCapsuleGen(self):

•    """Test generation of EFI capsule"""
  

•    self.payload_data = tools.to_bytes(TEXT_DATA)
  

•    TestFunctional._MakeInputFile('payload.txt', self.payload_data)
  

•    self._DoReadFile('283_capsule_signed.dts')
  

data = self...

That is the actual data, so you don't need to read it below.

•    self.capsule_fname = tools.get_output_filename('test.capsule')
  

•    self.assertTrue(os.path.exists(self.capsule_fname))
  

•    self._CheckCapsule(signed_capsule=True)
  

• def testCapsuleGenCfgFile(self):

•    """Test generation of EFI capsule through config file"""
  

•    self.payload_data = tools.to_bytes(TEXT_DATA)
  

•    self._GenCapsuleCfgPayload('payload.txt', self.payload_data)
  

•    self._DoReadFile('284_capsule_conf.dts')
  

•    self.capsule_fname = '/tmp/capsules/test.capsule'
  

•    self.assertTrue(os.path.exists(self.capsule_fname))
  

•    self._CheckCapsule()
  

• def testCapsuleGenVersionSupport(self):

•    """Test generation of EFI capsule with version support"""
  

•    self.payload_data = tools.to_bytes(TEXT_DATA)
  
  

•    TestFunctional._MakeInputFile('payload.txt', self.payload_data)
  

•    self._DoReadFile('290_capsule_version.dts')
  

•    self.capsule_fname = tools.get_output_filename('test.capsule')
  

•    self.assertTrue(os.path.exists(self.capsule_fname))
  

•    self._CheckCapsule(version_check=True)
  

• def testCapsuleGenKeyMissing(self):

•    """Test that binman errors out on missing key"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('285_capsule_missing_key.dts')
  

•    self.assertIn("Both private-key and public key Certificate need to be provided",
  

private key certificate

•                  str(e.exception))
  

• def testCapsuleGenIndexMissing(self):

•    """Test that binman errors out on missing image index"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('286_capsule_missing_index.dts')
  

•    self.assertIn("mkeficapsule must be provided an 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('287_capsule_missing_guid.dts')
  

•    self.assertIn("mkeficapsule must be provided an Image GUID",
  

•                  str(e.exception))
  

• def testCapsuleGenPayloadMissing(self):

•    """Test that binman errors out on missing input(payload)image"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('288_capsule_missing_payload.dts')
  

•    self.assertIn("mkeficapsule must be provided an input filename(payload)",
  

•                  str(e.exception))
  

• def testCapsuleGenCapsuleFileMissing(self):

•    """Test that binman errors out on missing output capsule file"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('289_capsule_missing.dts')
  

•    self.assertIn("Specify the output capsule file",
  

•                  str(e.exception))
  

This looks good. It is a pain that you need to cover each missing arg. I'm not sure I can think of a better way.

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/282_capsule.dts b/tools/binman/test/282_capsule.dts new file mode 100644 index 0000000000..0e7fcdf8ab --- /dev/null +++ b/tools/binman/test/282_capsule.dts @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           capsule {
  

•                   image-index = <0x1>;
  

•                   image-type-id = "09D7CF52-0720-4710-91D1-08469B7FE9C8";
  

Is there a #define somewhere for this? Perhaps you can add a #define, or a comment as to what this is.

Also, please use lower case.

•                   hardware-instance = <0x0>;
  

•                   filename = "payload.txt";
  

•                   capsule = "test.capsule";
  

•           };
  

•   };
  

+};

Regards, Simon