From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1F055C25B50 for ; Fri, 20 Jan 2023 19:47:11 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6220385694; Fri, 20 Jan 2023 20:46:53 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="DNhRuS6j"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5C6B88543D; Fri, 20 Jan 2023 20:46:26 +0100 (CET) Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 911D08551B for ; Fri, 20 Jan 2023 20:46:22 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ej1-x62f.google.com with SMTP id kt14so16619321ejc.3 for ; Fri, 20 Jan 2023 11:46:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=lWFjqMDvQE8Pxh4E92IxwuES9WeBN62RJWZJlYPQZtw=; b=DNhRuS6jPFKuVjXGczsFPiIsAHyE/MySiFCrYCE+nxVOWkcmOTXn3TvF6jR3TpVewn 7zy1dUx474QUN5oImekJIRite2206dFu1d65MOv38xl+sWufPrW+p5GawHYj1wbntDlX zJmfkG+nlahd3P4QiEjfmYBiCrclMhK2YemKE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=lWFjqMDvQE8Pxh4E92IxwuES9WeBN62RJWZJlYPQZtw=; b=pv8pn4VciGal/llnU8cM2VUuzitDI5kQ0DjqdGgb0g5jkWR2E3KQwl9vS4rdASZe6c wxnYJ5IQVZlTpDscOHUP9mvzScY6lmOm+vLqEvpRA3R2/hbxNNeUs05i27HiBQMPO8qG ekPnkX778jcxNXouE1IX7fLaAT3RZaJeAardoq473A6z2YMxqLhiZGcT8ifVmw8efL4v 3SjeO3JipgdcX8jf9dTsXF8K+V2xExvB3AXNxTMRLNZXOPy4Tfx9d0qODjjWmbvlUYuS Apw+JGFByE2m+5OfmVvrHoetYNeaCuUXbiHCyQrXvaBLP0173GcsqNVzjCpYHJbRH1dj EpnA== X-Gm-Message-State: AFqh2kqIb+C3IiTm8ZEpKQzBBkQ1gzHsrbQbT6qiqZwaiwRjv2akjw1N WPPNhfuVLrXfdbrFo2ZhKRW5tfL/C47XDgZlF0YEVw== X-Google-Smtp-Source: AMrXdXvN1iSeOqh/pFS0rbKujORZOasZmDdjzwwPWaUf7PvB9YQB6xdde/K9zXBc5h4COg1H0YgPlP3kz/P5WCA/2Qg= X-Received: by 2002:a17:906:fa08:b0:871:3919:cbe1 with SMTP id lo8-20020a170906fa0800b008713919cbe1mr1286356ejb.193.1674243981820; Fri, 20 Jan 2023 11:46:21 -0800 (PST) MIME-Version: 1.0 References: <20230120101903.179959-1-n-francis@ti.com> <20230120101903.179959-4-n-francis@ti.com> In-Reply-To: <20230120101903.179959-4-n-francis@ti.com> From: Simon Glass Date: Fri, 20 Jan 2023 12:46:07 -0700 Message-ID: Subject: Re: [PATCH 03/21] tools: binman: add ti-secure entry type To: Neha Malcom Francis Cc: u-boot@lists.denx.de, trini@konsulko.com, afd@ti.com, vigneshr@ti.com, rogerq@kernel.org, alpernebiyasak@gmail.com, nm@ti.com, bb@ti.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Neha, \ On Fri, 20 Jan 2023 at 03:19, Neha Malcom Francis 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 > [n-francis@ti.com: enabled signing for all K3 boot binaries for all > different boot flows] > Signed-off-by: Neha Malcom Francis > --- > 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