Hi Sughosh,
On Sat, 5 Aug 2023 at 05:35, 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 V6:
> * Add macros for the GUID strings in sandbox_efi_capsule.h
> * Highlight that the private and public keys are mandatory for capsule
> signing.
> * Add a URL link to the UEFI spec, as used in the rst files.
> * Use local vars for private and public keys in BuildSectionData()
> * Use local vars for input payload and capsule filenames in
> BuildSectionData().
> * Drop the ProcessContents() and SetImagePos() as the superclass
> functions suffice.
> * Use GUID macro names in the capsule test dts files.
> * Rename efi_capsule_payload.bin to capsule_input.bin.
>
>
> include/sandbox_efi_capsule.h | 14 ++
Please move this file to a later patch - see below.
Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.
> tools/binman/entries.rst | 62 ++++++++
> tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++
> tools/binman/ftest.py | 121 +++++++++++++++
> tools/binman/test/307_capsule.dts | 23 +++
> tools/binman/test/308_capsule_signed.dts | 25 +++
> tools/binman/test/309_capsule_version.dts | 24 +++
> tools/binman/test/310_capsule_signed_ver.dts | 26 ++++
> tools/binman/test/311_capsule_oemflags.dts | 24 +++
> tools/binman/test/312_capsule_missing_key.dts | 24 +++
> .../binman/test/313_capsule_missing_index.dts | 22 +++
> .../binman/test/314_capsule_missing_guid.dts | 19 +++
> .../test/315_capsule_missing_payload.dts | 19 +++
> 13 files changed, 546 insertions(+)
> create mode 100644 include/sandbox_efi_capsule.h
> 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/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h
> new file mode 100644
> index 0000000000..da71b87a18
> --- /dev/null
> +++ b/include/sandbox_efi_capsule.h
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#if !defined(_SANDBOX_EFI_CAPSULE_H_)
> +#define _SANDBOX_EFI_CAPSULE_H_
> +
> +#define SANDBOX_UBOOT_IMAGE_GUID "09d7cf52-0720-4710-91d1-08469b7fe9c8"
> +#define SANDBOX_UBOOT_ENV_IMAGE_GUID "5a7021f5-fef2-48b4-aaba-832e777418c0"
> +#define SANDBOX_FIT_IMAGE_GUID "3673b45d-6a7c-46f3-9e60-adabb03f7937"
> +#define SANDBOX_INCORRECT_GUID "058b7d83-50d5-4c47-a195-60d86ad341c4"
These should not be needed in binman tests.
> +
> +#endif /* _SANDBOX_EFI_CAPSULE_H_ */
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index f2376932be..fc094b9c08 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -468,6 +468,68 @@ updating the EC on startup via software sync.
>
>
>
> +.. _etype_efi_capsule:
> +
> +Entry: capsule: Entry for generating EFI Capsule files
> +------------------------------------------------------
> +
> +The parameters needed for generation of the capsules can
> +be provided as properties in the entry.
> +
> +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. This property
> + is mandatory for generating signed capsules.
> + - public-key-cert: Path to PEM formatted .crt public key certificate
> + file. This property is mandatory for generating signed capsules.
> + - oem-flags - Optional OEM flags to be passed through capsule header.
> +
> + Since this is a subclass of Entry_section, all properties of the parent
> + class also apply here.
> +
> +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`_.
> +
> +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 = SANDBOX_UBOOT_IMAGE_GUID;
> + hardware-instance = <0x0>;
> + private-key = "path/to/the/private/key";
> + public-key-cert = "path/to/the/public-key-cert";
> + oem-flags = <0x8000>;
> +
> + u-boot {
> + };
> + };
> +
> +In the above example, the capsule payload is the U-Boot image. The
> +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.
> +
> +.. _`UEFI specification`:
> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> +
> +
> +
> .. _etype_encrypted:
>
> Entry: encrypted: Externally built encrypted binary blob
> diff --git a/tools/binman/etype/efi_capsule.py
> b/tools/binman/etype/efi_capsule.py
> new file mode 100644
> index 0000000000..bfdca94e26
> --- /dev/null
> +++ b/tools/binman/etype/efi_capsule.py
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2023 Linaro Limited
> +#
> +# Entry-type module for producing a EFI capsule
> +#
> +
> +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):
> + """Generate EFI capsules
> +
> + The parameters needed for generation of the capsules can
> + be provided as properties in the entry.
> +
> + 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. This property
> + is mandatory for generating signed capsules.
> + - public-key-cert: Path to PEM formatted .crt public key certificate
> + file. This property is mandatory for generating signed capsules.
> + - oem-flags - Optional OEM flags to be passed through capsule header.
> +
> + Since this is a subclass of Entry_section, all properties of the parent
> + class also apply here.
> +
> + 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`_.
> +
> + 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 = SANDBOX_UBOOT_IMAGE_GUID;
> + hardware-instance = <0x0>;
> + private-key = "path/to/the/private/key";
> + public-key-cert = "path/to/the/public-key-cert";
> + oem-flags = <0x8000>;
> +
> + u-boot {
> + };
> + };
> +
> + In the above example, the capsule payload is the U-Boot image. The
> + 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.
> +
> + .. _`UEFI specification`:
> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
> + """
> + 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.oem_flags = 0
> + self.private_key = ''
> + self.public_key_cert = ''
> + self.auth = 0
> +
> + def ReadNode(self):
> + self.ReadEntries()
> + super().ReadNode()
I believe those two lines should be swapped.
> +
> + 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.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
> +
> + self.private_key = fdt_util.GetString(self._node, 'private-key')
> + self.public_key_cert = fdt_util.GetString(self._node,
> 'public-key-cert')
> + if ((self.private_key and not self.public_key_cert) or
> (self.public_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.public_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) == 0:
> + self.Raise('The capsule entry expects at least one subnode for
> payload')
Still need to drop this
> +
> + for node in self._node.subnodes:
> + entry = Entry.Create(self, node)
> + entry.ReadNode()
> + self._entries[entry.name] = entry
I think you can drop this method, since it should be the same as entry_Sectoin ?
> +
> + def BuildSectionData(self, required):
> + private_key = ''
> + public_key_cert = ''
> + if self.auth:
> + if not os.path.isabs(self.private_key):
> + private_key = tools.get_input_filename(self.private_key)
> + if not os.path.isabs(self.public_key_cert):
> + public_key_cert =
> tools.get_input_filename(self.public_key_cert)
> + data, payload, _ = self.collect_contents_to_file(
s/_/uniq/ since you need this below
> + self._entries.values(), 'capsule_payload')
'capsule_in' is better as it is an input to this entry type
> + outfile = self._filename if self._filename else self._node.name
You need a unique filename so cannot use self._node.name since it is
not guaranteed to be unique.
See how mkimage does this, using uniq
> + capsule_fname = tools.get_output_filename(outfile)
> + ret = self.mkeficapsule.generate_capsule(self.image_index,
> + self.image_guid,
> + self.hardware_instance,
> + payload,
> + capsule_fname,
> + private_key,
> + public_key_cert,
> + self.monotonic_count,
> + self.fw_version,
> + self.oem_flags)
> + if ret is not None:
> + os.remove(payload)
> + return tools.read_file(capsule_fname)
> +
> + def AddBintools(self, btools):
> + self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 36428ec343..9bda0157f7 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('capsule_input.bin', EFI_CAPSULE_DATA)
>
> # Add a few .dtb files for testing
> TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> @@ -7139,5 +7146,119 @@ 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])
> +
As discussed please add a TODO here, so it is clear that you are
planning to write a decoder tool like dump_image.
> + 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)
> + 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 at least one 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..86552ff83f
> --- /dev/null
> +++ b/tools/binman/test/307_capsule.dts
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +#include <sandbox_efi_capsule.h>
We can't include sandbox files in binman! I Does it actually matter
what the GUID value is? Can they all be the same? For now I think it
is best to have
#define BINMAN_TEST_GUID "xxx"
repeated at the top of each .dts file. Hopefully at some point we can
figure out a sensible way to provide a decoder ring for this mess, so
we can use actually names in the binman definition instead of GUIDs.
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + binman {
> + efi-capsule {
> + image-index = <0x1>;
> + /* Image GUID for testing capsule update */
> + image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
> + hardware-instance = <0x0>;
> +
> + blob {
> + filename = "capsule_input.bin";
> + };
> + };
> + };
> +};
Regards,
Simon
