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