Re: [PATCH v7 08/11] binman: capsule: Add support for generating EFI capsules

hi Simon,

On Sat, 5 Aug 2023 at 20:34, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@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.ganu@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.

The idea was to also be able to run the binman capsule tests once this patch was applied. If we are to move this to a separate patch, it should be the one before this patch. But I guess based on your other reply, this might not be needed after all.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

Umm, I am not too sure. Maybe we can take a call at a later point if there are too many files that start cropping up.

...

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.

Again, like my earlier code for ProcessContents() and SetImagePos(), which was taken from mkimage.py as reference, this code is on similar lines to what is in intel_ifwi.py. Both these files are authored by you, so I took this as reference, especially mkimage.py.

...

•    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

? Should we not check if the input payload is missing? We cannot call the mkeficapsule tool without an input image(binary).

...

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

Will check, but again, referenced from mkimage.py.

...

• 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

Okay

...

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

Okay

-sughosh

...

•    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