Hi Neha, \ On Fri, 20 Jan 2023 at 03:19, Neha Malcom Francis <n-fran...@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 <rog...@kernel.org> > [n-fran...@ti.com: enabled signing for all K3 boot binaries for all > different boot flows] > Signed-off-by: Neha Malcom Francis <n-fran...@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