Hi Sughosh, On Sat, 15 Jul 2023 at 07:46, Sughosh Ganu <sughosh.g...@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.g...@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