[PATCH v4 06/12] binman: capsule: Add support for generating capsules
Simon Glass
sjg at chromium.org
Sun Jul 16 01:40:38 CEST 2023
Hi Sughosh,
On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.ganu at 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 at 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
More information about the U-Boot
mailing list