Re: [PATCH 03/21] tools: binman: add ti-secure entry type

Hi Neha,

\ On Fri, 20 Jan 2023 at 03:19, Neha Malcom Francis n-francis@ti.com wrote:

This entry type is used to create a secured binary for use with K3 High Security (HS) devices.

This allows us to no longer depend on k3_fit_atf.sh for A53 SPL and u-boot image generation even for HS devices.

We still depend on the availability of an external tool provided by the TI_SECURE_DEV_PKG environment variable to secure the binaries.

Signed-off-by: Roger Quadros rogerq@kernel.org [n-francis@ti.com: enabled signing for all K3 boot binaries for all different boot flows] Signed-off-by: Neha Malcom Francis n-francis@ti.com

---

Makefile | 1 + tools/binman/entries.rst | 15 ++++ tools/binman/etype/ti_secure.py | 133 ++++++++++++++++++++++++++++++++ tools/binman/ftest.py | 8 ++ 4 files changed, 157 insertions(+) create mode 100644 tools/binman/etype/ti_secure.py

Sorry, I have rather a lot of comments, so will do a round of just the major ones for this version.

diff --git a/Makefile b/Makefile index eb354c045c..c568a6e59a 100644 --- a/Makefile +++ b/Makefile @@ -1329,6 +1329,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \ $(foreach f,$(BINMAN_INDIRS),-I $(f)) \ -a atf-bl31-path=${BL31} \ -a tee-os-path=${TEE} \

•           -a ti-secure-dev-pkg-path=${TI_SECURE_DEV_PKG} \
  

Cam we use ti-secure-path ? The 'dev' and 'pkg' seem to be noise.

            -a opensbi-path=${OPENSBI} \
            -a default-dt=$(default_dt) \
            -a scp-path=$(SCP) \

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index 2b32c131ed..bf363434a2 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -2361,3 +2361,18 @@ may be used instead.

+Entry: ti-secure: Entry containing a Secured binary blob +--------------------------------------------------------

+Properties / Entry arguments:

• • filename: Filename of file to sign and read into entry

+Texas Instruments High-Security (HS) devices need secure binaries to be +provided. This entry uses an external tool to append a x509 certificate +to the file provided in the filename property and places it in the entry.

+The path for the external tool is fetched from TI_SECURE_DEV_PKG +environment variable.

diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py new file mode 100644 index 0000000000..5447bb61df --- /dev/null +++ b/tools/binman/etype/ti_secure.py @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/ +#

+# Support for signed binaries for TI K3 platform

+from collections import OrderedDict +import os

+from binman.entry import Entry, EntryArg

+from dtoc import fdt_util +from patman import tools

+class Entry_ti_secure(Entry):

So shouldn't this be Entry_blob ?

• """An entry which contains a signed x509 binary for signing TI
• General Purpose as well as High-Security devices.

First line of comment must be the summary. Then add a blank line and the extra detail. See how this is done elsewhere

• Properties / Entry arguments:

•   - filename: filename of binary file to be secured
  

Please describe all the properties

• Output files:

•    - filename_x509 - output file generated by secure x509 signing script (which
  

•        used as entry contents)
  

• """
• def __init__(self, section, etype, node):

•    super().__init__(section, etype, node)
  

•    self.filename = fdt_util.GetString(self._node, 'filename')
  

•    self.key = fdt_util.GetString(self._node, 'key', "")
  

If key is '' does it work?

Please use single quotes consistently except for function comments.

•    self.core = fdt_util.GetInt(self._node, 'core', 16)
  

•    self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
  

•    self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev')
  

•    self.cert3 = fdt_util.GetBool(self._node, 'sysfw-cert', False)
  

try to use the same name for the property as the DT one if you can

•    self.secure = fdt_util.GetBool(self._node, 'secure', False)
  

•    self.combined = fdt_util.GetBool(self._node, 'combined', False)
  

•    self.split_dm = fdt_util.GetBool(self._node, 'split-dm', False)
  

•    self.sysfw_filename = fdt_util.GetString(self._node, 'sysfw-filename')
  

•    self.sysfw_load_addr = fdt_util.GetInt(self._node, 'sysfw-load')
  

•    self.sysfw_data_filename = fdt_util.GetString(self._node, 'sysfw-data-filename')
  

•    self.sysfw_data_load_addr = fdt_util.GetInt(self._node, 'sysfw-data-load')
  

•    self.sysfw_inner_cert = fdt_util.GetString(self._node, 'sysfw-inner-cert', "")
  

•    self.dm_data_filename = fdt_util.GetString(self._node, 'dm-data-filename')
  

•    self.dm_data_load_addr = fdt_util.GetInt(self._node, 'dm-data-load')
  

•    self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
  

•    self.sysfw_inner_cert_load_addr = fdt_util.GetInt(self._node, 'sysfw-inner-cert-load')
  

•    self.toolpresent = False
  

•    if not self.filename:
  

•        self.Raise("ti_secure must have a 'filename' property")
  

This is handled by putting something like this in __init__

self.required_props = ['filename']

•    self.toolspath, = self.GetEntryArgsOrProps(
  

•        [EntryArg('ti-secure-dev-pkg-path', str)])
  

•    if not self.toolspath:
  

•        print("WARNING: TI_SECURE_DEV_PKG environment " \
  

•            "variable must be defined for TI GP and HS devices! " +
  

•            self.filename + " was NOT signed!")
  

•        return
  

This is handled by the bintool being 'missing'.

See how the other bintools are implemented. Create a bintool for your tool.

•    if self.cert3 == True:
  

•        self.tool = self.toolspath + "/scripts/gen_x509_cert3.sh"
  

•        self.core = "m3"
  

•    elif self.secure == True:
  

•        self.tool = self.toolspath + "/scripts/secure-binary-image.sh"
  

•    elif self.combined:
  

•        self.tool = self.toolspath + "/scripts/gen_x509_combined_cert.sh"
  

•    else:
  

•        self.tool = self.toolspath + "/scripts/gen_x509_cert.sh"
  

These need to be bintools too, or perhaps other entry types. I'm not sure...

•    self.toolpresent = os.path.exists(self.tool)
  

•    if not self.toolpresent:
  

•        print(self.tool + " not found. " +
  

•            self.filename + " was NOT signed! ")
  

•    if self.key == "" and not self.secure:
  

•        self.key = self.toolspath + "/keys/ti-degenerate-key.pem"
  

•        self.keypresent = os.path.exists(self.key)
  

•        if not self.keypresent:
  

•            print(self.key + " not found. " +
  

•                self.filename + " was NOT signed! ")
  

•        else:
  

•            print("Signing " + self.filename + " with degenerate RSA key...")
  

•    else:
  

•        self.key = self.toolspath + self.key
  

•        print("Signing " + self.filename + " with " + self.key)
  

You can't use 'print' in binman. You can use things like tout.info() to emit info.

•    if self.sw_rev is None and not self.secure:
  

•        self.sw_revfile = self.toolspath + "/keys/swrv.txt"
  

•        with open(self.sw_revfile) as f:
  

•            self.sw_rev = int(f.read())
  

•        self.swrevpresent = os.path.exists(self.sw_rev)
  

•        if not self.swrevpresent:
  

•            print(self.sw_rev + " not found. " +
  

•                "Software revision file not found. Default may not work on HS hardware.")
  

•            self.sw_rev = 1
  

• def ObtainContents(self):

•    input_fname = self.filename
  

•    output_fname = input_fname + "_x509"
  

•    if self.secure:
  

•        args = [
  

•            input_fname, output_fname,
  

•        ]
  

•    elif self.combined:
  

•        args = [
  

•            '-b', input_fname,
  

•            '-l', hex(self.load_addr),
  

•            '-s', self.sysfw_filename,
  

•            '-m', hex(self.sysfw_load_addr),
  

•            '-c', self.sysfw_inner_cert,
  

•            '-d', self.sysfw_data_filename,
  

•            '-n', hex(self.sysfw_data_load_addr),
  

•            '-k', self.key,
  

•            '-r', str(self.sw_rev),
  

•            '-o', output_fname,
  

•        ]
  

•        if self.split_dm:
  

•            args.extend(['-t', self.dm_data_filename, '-y', hex(self.dm_data_load_addr)])
  

•    else:
  

•        args = [
  

•            '-c', str(self.core),
  

•            '-b', input_fname,
  

•            '-o', output_fname,
  

•            '-l', hex(self.load_addr),
  

•            '-r', str(self.sw_rev),
  

•            '-k', self.key,
  

•        ]
  

•        if self.cert3 == True:
  

nit: use:

if self.cert3:

•            args.insert(0, '-d')
  

•    if self.toolpresent:
  

•        stdout = tools.run(self.tool, *args)
  

•    else:
  

•        stdout = tools.run('cp', *args)
  

•        print(output_fname + ' not signed!')
  

•    self.SetContents(tools.read_file(output_fname))
  

•    return True
  

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index be0aea49ce..aaa2c610b0 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -93,6 +93,7 @@ SCP_DATA = b'scp' TEST_FDT1_DATA = b'fdt1' TEST_FDT2_DATA = b'test-fdt2' ENV_DATA = b'var1=1\nvar2="2"' +TI_UNSECURE_DATA = b'this is some unsecure data' PRE_LOAD_MAGIC = b'UBSH' PRE_LOAD_VERSION = 0x11223344.to_bytes(4, 'big') PRE_LOAD_HDR_SIZE = 0x00001000.to_bytes(4, 'big') @@ -213,6 +214,7 @@ class TestFunctional(unittest.TestCase): TEST_FDT2_DATA)

     TestFunctional._MakeInputFile('env.txt', ENV_DATA)

•    TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)
  
     # ELF file with two sections in different parts of memory, used for both
     # ATF and OP_TEE
  

@@ -5545,6 +5547,12 @@ fdt fdtmap Extract the devicetree blob from the fdtmap err)

• def testPackTisecure(self):

•    """Test that an image with a TI secured binary can be created"""
  

•    data = self._DoReadFile('187_ti_secure.dts')
  

•       securedata = tools.ReadFile('ti_unsecure.bin_HS')
  

•       self.assertEquals(data, securedata)
  

The test needs to cover the tool being missing.

 def testFitSplitElfMissing(self):
     """Test an split-elf FIT with a missing ELF file"""
     if not elf.ELF_TOOLS:

-- 2.34.1

Regards, Simon