diff mbox series

[RFC] binman: bintool: etype: Add support for ti-secure entry

Message ID 20230224120340.587786-1-n-francis@ti.com
State RFC
Delegated to: Simon Glass
Headers show
Series [RFC] binman: bintool: etype: Add support for ti-secure entry | expand

Commit Message

Neha Malcom Francis Feb. 24, 2023, 12:03 p.m. UTC 
core-secdev-k3 is the TI security development package provided for K3
platform devices. This tool helps sign bootloader images with the x509
ceritificate header.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
This patch depends on https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
and on ongoing development in https://git.ti.com/cgit/security-development-tools/core-secdev-k3

 tools/binman/btool/tisecuretool.py            |  72 +++++++++++
 tools/binman/etype/ti_secure.py               | 114 ++++++++++++++++++
 tools/binman/ftest.py                         |  36 ++++++
 tools/binman/test/278_ti_secure_rom.dts       |  11 ++
 tools/binman/test/279_ti_secure.dts           |  11 ++
 .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
 tools/binman/test/281_ti_secure_combined.dts  |  12 ++
 7 files changed, 266 insertions(+)
 create mode 100644 tools/binman/btool/tisecuretool.py
 create mode 100644 tools/binman/etype/ti_secure.py
 create mode 100644 tools/binman/test/278_ti_secure_rom.dts
 create mode 100644 tools/binman/test/279_ti_secure.dts
 create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
 create mode 100644 tools/binman/test/281_ti_secure_combined.dts

Comments

Simon Glass Feb. 28, 2023, 12:35 a.m. UTC | #1 
Hi Neha,

On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> core-secdev-k3 is the TI security development package provided for K3
> platform devices. This tool helps sign bootloader images with the x509
> ceritificate header.
>
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
> This patch depends on https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
> and on ongoing development in https://git.ti.com/cgit/security-development-tools/core-secdev-k3
>
>  tools/binman/btool/tisecuretool.py            |  72 +++++++++++
>  tools/binman/etype/ti_secure.py               | 114 ++++++++++++++++++
>  tools/binman/ftest.py                         |  36 ++++++
>  tools/binman/test/278_ti_secure_rom.dts       |  11 ++
>  tools/binman/test/279_ti_secure.dts           |  11 ++
>  .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
>  tools/binman/test/281_ti_secure_combined.dts  |  12 ++
>  7 files changed, 266 insertions(+)
>  create mode 100644 tools/binman/btool/tisecuretool.py
>  create mode 100644 tools/binman/etype/ti_secure.py
>  create mode 100644 tools/binman/test/278_ti_secure_rom.dts
>  create mode 100644 tools/binman/test/279_ti_secure.dts
>  create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
>  create mode 100644 tools/binman/test/281_ti_secure_combined.dts

Now that I see what you are doing, this it not quite the right way.

See this hack-up of how you can call the openssl thing. Basically you
should not have a shell script in the way, but instead make your
bintool do it.

https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804

I suppose we can have an openssl bintool that others build on top of?

Regards,
Simon


>
> diff --git a/tools/binman/btool/tisecuretool.py b/tools/binman/btool/tisecuretool.py
> new file mode 100644
> index 0000000000..5102bb1f7d
> --- /dev/null
> +++ b/tools/binman/btool/tisecuretool.py
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
> +# Written by Neha Malcom Francis <n-francis@ti.com>
> +#
> +"""Bintool implementation for TI security development tools
> +
> +tisecuretool helps add x509 certification for bootloader images for K3 platform devices
> +
> +Source code:
> +https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
> +
> +import os
> +
> +from binman import bintool
> +from patman import tout
> +from patman import tools
> +
> +class Bintooltisecuretool(bintool.Bintool):
> +    """Signing tool for TI bootloaders"""
> +    name = 'tisecuretool'
> +    def __init__(self, name):
> +        super().__init__(name, 'TI secure tool')
> +
> +    def sign_binary_secure(self, fname, out_fname):
> +        """Create a signed binary
> +
> +        Args:
> +            fname (str): Filename to sign
> +            out_fname (str): Output filename
> +
> +        Returns:
> +            str: Tool output
> +            or None
> +        """
> +        tool_path = self.get_path()
> +        script_path = os.path.join(tool_path, 'scripts/secure-binary-image.sh')
> +        args = [
> +            'sh',
> +            script_path,
> +            fname,
> +            out_fname
> +            ]
> +        output = self.run_cmd(*args, add_name=False)
> +        return output
> +
> +    def sign_binary_rom(self, args):
> +        """Create a signed binary that is booted by ROM
> +
> +        Args:
> +            fname (str): Filename to sign
> +            out_fname (str): Output filename"""
> +        tool_path = self.get_path()
> +        script_path = os.path.join(tool_path, 'scripts/secure-rom-boot-image.sh')
> +        #args.insert(0, script_path)
> +        args.insert(0, script_path)
> +        output = self.run_cmd(*args, add_name=False)
> +        return output
> +
> +    def fetch(self, method):
> +        """Fetch handler for TI secure tool
> +
> +        This builds the tool from source
> +
> +        Returns:
> +            True if the file was fetched, None if a method other than FETCH_SOURCE
> +            was requested
> +        """
> +        if method != bintool.FETCH_SOURCE:
> +            return None
> +        result = self.fetch_from_git(
> +            'git://git.ti.com/security-development-tools/core-secdev-k3.git', 'tisecuretool')
> +        return result
> diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py
> new file mode 100644
> index 0000000000..26f81ff8e8
> --- /dev/null
> +++ b/tools/binman/etype/ti_secure.py
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
> +# Written by Neha Malcom Francis <n-francis@ti.com>
> +#
> +# Entry-type module for signed binaries for TI K3 platform
> +#
> +
> +from binman.etype.blob import Entry_blob
> +from binman import bintool
> +
> +from dtoc import fdt_util
> +from patman import terminal
> +from patman import tools
> +from patman import tout
> +
> +class Entry_ti_secure(Entry_blob):
> +    """An entry which contains a signed x509 binary for signing TI
> +    General Purpose as well as High-Security devices.
> +
> +    Properties / Entry arguments:
> +       - filename: filename of binary file to be secured
> +
> +    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.core = fdt_util.GetString(self._node, 'core', 'secure')
> +        self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
> +        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
> +        self.sysfw_cert = fdt_util.GetBool(self._node, 'sysfw-cert', False)
> +        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', 0x40000)
> +        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', 0x7f000)
> +        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', 0x41c80000)
> +        self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
> +        self.toolpresent = False
> +        if not self.filename:
> +            self.Raise("ti_secure must have a 'filename' property")
> +
> +    def _SignBinarySecure(self):
> +        infilename = self.filename
> +        outfilename = infilename + '_signed'
> +
> +        data = self.tisecuretool.sign_binary_secure(infilename, outfilename)
> +
> +        if data is None:
> +            # Bintool is missing, create a zeroed binary
> +            self.record_missing_bintool(self.tisecuretool)
> +            return tools.get_bytes(0, 1024)
> +
> +        return tools.read_file(outfilename)
> +
> +    def _SignBinaryROM(self):
> +        input_fname = self.filename
> +        output_fname = input_fname + "_x509"
> +        if self.combined:
> +            args = [
> +                '-b', input_fname,
> +                '-l', hex(self.load_addr),
> +                '-s', self.sysfw_filename,
> +                '-m', hex(self.sysfw_load_addr),
> +                '-a', self.sysfw_inner_cert,
> +                '-d', self.sysfw_data_filename,
> +                '-n', hex(self.sysfw_data_load_addr),
> +                '-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 = [
> +                '-a', self.core,
> +                '-b', input_fname,
> +                '-o', output_fname,
> +                '-l', hex(self.load_addr),
> +                '-r', str(self.sw_rev),
> +
> +            ]
> +            if self.sysfw_cert == True:
> +                args.insert(0, '-u')
> +
> +        out = self.tisecuretool.sign_binary_rom(args)
> +
> +        if out is None:
> +            # Bintool is missing, create a zeroed binary
> +            self.record_missing_bintool(self.tisecuretool)
> +            return tools.get_bytes(0, 1024)
> +
> +        return tools.read_file(output_fname)
> +
> +    def ProcessContents(self):
> +        if self.secure:
> +            data = self._SignBinarySecure()
> +        else:
> +            data = self._SignBinaryROM()
> +        return self.ProcessContentsUpdate(data)
> +
> +    def AddBintools(self, btools):
> +        #super().AddBintools(btools)
> +        col = terminal.Color()
> +        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
> +        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, col, True)
> +        if out == bintool.FAIL:
> +            # Bintool is missing, record missing
> +            self.record_missing_bintool(self.tisecuretool)
> \ No newline at end of file
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index bf902341c5..72cce0ef02 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -97,6 +97,7 @@ PRE_LOAD_MAGIC        = b'UBSH'
>  PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
>  PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
>  TI_BOARD_CONFIG_DATA  = b'\x00\x01'
> +TIX509DATA            = b'letsgo'
>
>  # Subdirectory of the input dir to use to put test FDTs
>  TEST_FDT_SUBDIR       = 'fdts'
> @@ -206,6 +207,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>          TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
> +        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
>
>          # Add a few .dtb files for testing
>          TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> @@ -6398,5 +6400,39 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          configlen_noheader = TI_BOARD_CONFIG_DATA*4
>          self.assertGreater(data, configlen_noheader)
>
> +    def testTISecureROMImageMissingTool(self):
> +        """Test that a TI secure signed ROM booted image can be generated although tisecuretool is missing"""
> +        with test_util.capture_sys_output() as (_, stderr):
> +            data = self._DoTestFile('278_ti_secure_rom.dts',
> +                force_missing_bintools='tisecuretool')
> +        err = stderr.getvalue()
> +        self.assertRegex(err,
> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> +
> +    def testTISecureImageMissingTool(self):
> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
> +        with test_util.capture_sys_output() as (_, stderr):
> +            data = self._DoTestFile('279_ti_secure.dts',
> +                force_missing_bintools='tisecuretool')
> +        err = stderr.getvalue()
> +        self.assertRegex(err,
> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> +
> +    def testTISecureNoFilename(self):
> +        """Test that a missing 'filename' property is detected"""
> +        with self.assertRaises(ValueError) as e:
> +            self._DoTestFile('280_ti_secure_nofilename.dts')
> +        self.assertIn("ti_secure must have a 'filename' property",
> +                      str(e.exception))
> +
> +    def testTISecureImageCombinedMissingTool(self):
> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
> +        with test_util.capture_sys_output() as (_, stderr):
> +            data = self._DoTestFile('281_ti_secure_combined.dts',
> +                force_missing_bintools='tisecuretool')
> +        err = stderr.getvalue()
> +        self.assertRegex(err,
> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/278_ti_secure_rom.dts b/tools/binman/test/278_ti_secure_rom.dts
> new file mode 100644
> index 0000000000..67a6a9de8d
> --- /dev/null
> +++ b/tools/binman/test/278_ti_secure_rom.dts
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +       binman {
> +                       ti-secure {
> +                               filename = "to_sign.bin";
> +                               sysfw-cert;
> +                       };
> +       };
> +};
> diff --git a/tools/binman/test/279_ti_secure.dts b/tools/binman/test/279_ti_secure.dts
> new file mode 100644
> index 0000000000..0e60984013
> --- /dev/null
> +++ b/tools/binman/test/279_ti_secure.dts
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +       binman {
> +                       ti-secure {
> +                               filename = "to_sign.bin";
> +                               secure;
> +                       };
> +       };
> +};
> diff --git a/tools/binman/test/280_ti_secure_nofilename.dts b/tools/binman/test/280_ti_secure_nofilename.dts
> new file mode 100644
> index 0000000000..928a50a499
> --- /dev/null
> +++ b/tools/binman/test/280_ti_secure_nofilename.dts
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +       binman {
> +                       ti-secure {
> +                               secure;
> +                       };
> +       };
> +};
> diff --git a/tools/binman/test/281_ti_secure_combined.dts b/tools/binman/test/281_ti_secure_combined.dts
> new file mode 100644
> index 0000000000..aadb5a4306
> --- /dev/null
> +++ b/tools/binman/test/281_ti_secure_combined.dts
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +/ {
> +       binman {
> +                       ti-secure {
> +                               filename = "to_sign.bin";
> +                               combined;
> +                               split-dm;
> +                       };
> +       };
> +};
> --
> 2.34.1
>
Neha Malcom Francis Feb. 28, 2023, 9:50 a.m. UTC | #2 
Hi Simon,

On 28/02/23 06:05, Simon Glass wrote:
> Hi Neha,
> 
> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com> wrote:
>>
>> core-secdev-k3 is the TI security development package provided for K3
>> platform devices. This tool helps sign bootloader images with the x509
>> ceritificate header.
>>
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>> This patch depends on https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
>> and on ongoing development in https://git.ti.com/cgit/security-development-tools/core-secdev-k3
>>
>>   tools/binman/btool/tisecuretool.py            |  72 +++++++++++
>>   tools/binman/etype/ti_secure.py               | 114 ++++++++++++++++++
>>   tools/binman/ftest.py                         |  36 ++++++
>>   tools/binman/test/278_ti_secure_rom.dts       |  11 ++
>>   tools/binman/test/279_ti_secure.dts           |  11 ++
>>   .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
>>   tools/binman/test/281_ti_secure_combined.dts  |  12 ++
>>   7 files changed, 266 insertions(+)
>>   create mode 100644 tools/binman/btool/tisecuretool.py
>>   create mode 100644 tools/binman/etype/ti_secure.py
>>   create mode 100644 tools/binman/test/278_ti_secure_rom.dts
>>   create mode 100644 tools/binman/test/279_ti_secure.dts
>>   create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
>>   create mode 100644 tools/binman/test/281_ti_secure_combined.dts
> 
> Now that I see what you are doing, this it not quite the right way.
> 
> See this hack-up of how you can call the openssl thing. Basically you
> should not have a shell script in the way, but instead make your
> bintool do it.
> 
> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
> 
> I suppose we can have an openssl bintool that others build on top of?
> 
> Regards,
> Simon
> 
> 

That is possible, maybe ti-secure extends from x509_cert etype so as to 
support the TI specific certificate extensions. I'll start working on this.

However the patches I've sent support external production flow where 
signing need not necessarily be carried out from U-Boot. An external 
repo that acts as what is core-secdev-k3 here, would be the repo 
responsible for signing.

Could we find a way to combine both so as to support production flow 
mandating an external agency, as well as a completely within U-Boot flow 
using bintool? i.e. we modify ti-secure etype to be able to support both 
external git repo/executable script signing as well as default signing 
using openssl bintool.

>>
>> diff --git a/tools/binman/btool/tisecuretool.py b/tools/binman/btool/tisecuretool.py
>> new file mode 100644
>> index 0000000000..5102bb1f7d
>> --- /dev/null
>> +++ b/tools/binman/btool/tisecuretool.py
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>> +#
>> +"""Bintool implementation for TI security development tools
>> +
>> +tisecuretool helps add x509 certification for bootloader images for K3 platform devices
>> +
>> +Source code:
>> +https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
>> +
>> +import os
>> +
>> +from binman import bintool
>> +from patman import tout
>> +from patman import tools
>> +
>> +class Bintooltisecuretool(bintool.Bintool):
>> +    """Signing tool for TI bootloaders"""
>> +    name = 'tisecuretool'
>> +    def __init__(self, name):
>> +        super().__init__(name, 'TI secure tool')
>> +
>> +    def sign_binary_secure(self, fname, out_fname):
>> +        """Create a signed binary
>> +
>> +        Args:
>> +            fname (str): Filename to sign
>> +            out_fname (str): Output filename
>> +
>> +        Returns:
>> +            str: Tool output
>> +            or None
>> +        """
>> +        tool_path = self.get_path()
>> +        script_path = os.path.join(tool_path, 'scripts/secure-binary-image.sh')
>> +        args = [
>> +            'sh',
>> +            script_path,
>> +            fname,
>> +            out_fname
>> +            ]
>> +        output = self.run_cmd(*args, add_name=False)
>> +        return output
>> +
>> +    def sign_binary_rom(self, args):
>> +        """Create a signed binary that is booted by ROM
>> +
>> +        Args:
>> +            fname (str): Filename to sign
>> +            out_fname (str): Output filename"""
>> +        tool_path = self.get_path()
>> +        script_path = os.path.join(tool_path, 'scripts/secure-rom-boot-image.sh')
>> +        #args.insert(0, script_path)
>> +        args.insert(0, script_path)
>> +        output = self.run_cmd(*args, add_name=False)
>> +        return output
>> +
>> +    def fetch(self, method):
>> +        """Fetch handler for TI secure tool
>> +
>> +        This builds the tool from source
>> +
>> +        Returns:
>> +            True if the file was fetched, None if a method other than FETCH_SOURCE
>> +            was requested
>> +        """
>> +        if method != bintool.FETCH_SOURCE:
>> +            return None
>> +        result = self.fetch_from_git(
>> +            'git://git.ti.com/security-development-tools/core-secdev-k3.git', 'tisecuretool')
>> +        return result
>> diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py
>> new file mode 100644
>> index 0000000000..26f81ff8e8
>> --- /dev/null
>> +++ b/tools/binman/etype/ti_secure.py
>> @@ -0,0 +1,114 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>> +#
>> +# Entry-type module for signed binaries for TI K3 platform
>> +#
>> +
>> +from binman.etype.blob import Entry_blob
>> +from binman import bintool
>> +
>> +from dtoc import fdt_util
>> +from patman import terminal
>> +from patman import tools
>> +from patman import tout
>> +
>> +class Entry_ti_secure(Entry_blob):
>> +    """An entry which contains a signed x509 binary for signing TI
>> +    General Purpose as well as High-Security devices.
>> +
>> +    Properties / Entry arguments:
>> +       - filename: filename of binary file to be secured
>> +
>> +    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.core = fdt_util.GetString(self._node, 'core', 'secure')
>> +        self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
>> +        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
>> +        self.sysfw_cert = fdt_util.GetBool(self._node, 'sysfw-cert', False)
>> +        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', 0x40000)
>> +        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', 0x7f000)
>> +        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', 0x41c80000)
>> +        self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
>> +        self.toolpresent = False
>> +        if not self.filename:
>> +            self.Raise("ti_secure must have a 'filename' property")
>> +
>> +    def _SignBinarySecure(self):
>> +        infilename = self.filename
>> +        outfilename = infilename + '_signed'
>> +
>> +        data = self.tisecuretool.sign_binary_secure(infilename, outfilename)
>> +
>> +        if data is None:
>> +            # Bintool is missing, create a zeroed binary
>> +            self.record_missing_bintool(self.tisecuretool)
>> +            return tools.get_bytes(0, 1024)
>> +
>> +        return tools.read_file(outfilename)
>> +
>> +    def _SignBinaryROM(self):
>> +        input_fname = self.filename
>> +        output_fname = input_fname + "_x509"
>> +        if self.combined:
>> +            args = [
>> +                '-b', input_fname,
>> +                '-l', hex(self.load_addr),
>> +                '-s', self.sysfw_filename,
>> +                '-m', hex(self.sysfw_load_addr),
>> +                '-a', self.sysfw_inner_cert,
>> +                '-d', self.sysfw_data_filename,
>> +                '-n', hex(self.sysfw_data_load_addr),
>> +                '-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 = [
>> +                '-a', self.core,
>> +                '-b', input_fname,
>> +                '-o', output_fname,
>> +                '-l', hex(self.load_addr),
>> +                '-r', str(self.sw_rev),
>> +
>> +            ]
>> +            if self.sysfw_cert == True:
>> +                args.insert(0, '-u')
>> +
>> +        out = self.tisecuretool.sign_binary_rom(args)
>> +
>> +        if out is None:
>> +            # Bintool is missing, create a zeroed binary
>> +            self.record_missing_bintool(self.tisecuretool)
>> +            return tools.get_bytes(0, 1024)
>> +
>> +        return tools.read_file(output_fname)
>> +
>> +    def ProcessContents(self):
>> +        if self.secure:
>> +            data = self._SignBinarySecure()
>> +        else:
>> +            data = self._SignBinaryROM()
>> +        return self.ProcessContentsUpdate(data)
>> +
>> +    def AddBintools(self, btools):
>> +        #super().AddBintools(btools)
>> +        col = terminal.Color()
>> +        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
>> +        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, col, True)
>> +        if out == bintool.FAIL:
>> +            # Bintool is missing, record missing
>> +            self.record_missing_bintool(self.tisecuretool)
>> \ No newline at end of file
>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>> index bf902341c5..72cce0ef02 100644
>> --- a/tools/binman/ftest.py
>> +++ b/tools/binman/ftest.py
>> @@ -97,6 +97,7 @@ PRE_LOAD_MAGIC        = b'UBSH'
>>   PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
>>   PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
>>   TI_BOARD_CONFIG_DATA  = b'\x00\x01'
>> +TIX509DATA            = b'letsgo'
>>
>>   # Subdirectory of the input dir to use to put test FDTs
>>   TEST_FDT_SUBDIR       = 'fdts'
>> @@ -206,6 +207,7 @@ class TestFunctional(unittest.TestCase):
>>           TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>>           TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>>           TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>> +        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
>>
>>           # Add a few .dtb files for testing
>>           TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
>> @@ -6398,5 +6400,39 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>>           configlen_noheader = TI_BOARD_CONFIG_DATA*4
>>           self.assertGreater(data, configlen_noheader)
>>
>> +    def testTISecureROMImageMissingTool(self):
>> +        """Test that a TI secure signed ROM booted image can be generated although tisecuretool is missing"""
>> +        with test_util.capture_sys_output() as (_, stderr):
>> +            data = self._DoTestFile('278_ti_secure_rom.dts',
>> +                force_missing_bintools='tisecuretool')
>> +        err = stderr.getvalue()
>> +        self.assertRegex(err,
>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>> +
>> +    def testTISecureImageMissingTool(self):
>> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
>> +        with test_util.capture_sys_output() as (_, stderr):
>> +            data = self._DoTestFile('279_ti_secure.dts',
>> +                force_missing_bintools='tisecuretool')
>> +        err = stderr.getvalue()
>> +        self.assertRegex(err,
>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>> +
>> +    def testTISecureNoFilename(self):
>> +        """Test that a missing 'filename' property is detected"""
>> +        with self.assertRaises(ValueError) as e:
>> +            self._DoTestFile('280_ti_secure_nofilename.dts')
>> +        self.assertIn("ti_secure must have a 'filename' property",
>> +                      str(e.exception))
>> +
>> +    def testTISecureImageCombinedMissingTool(self):
>> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
>> +        with test_util.capture_sys_output() as (_, stderr):
>> +            data = self._DoTestFile('281_ti_secure_combined.dts',
>> +                force_missing_bintools='tisecuretool')
>> +        err = stderr.getvalue()
>> +        self.assertRegex(err,
>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>> +
>>   if __name__ == "__main__":
>>       unittest.main()
>> diff --git a/tools/binman/test/278_ti_secure_rom.dts b/tools/binman/test/278_ti_secure_rom.dts
>> new file mode 100644
>> index 0000000000..67a6a9de8d
>> --- /dev/null
>> +++ b/tools/binman/test/278_ti_secure_rom.dts
>> @@ -0,0 +1,11 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +/ {
>> +       binman {
>> +                       ti-secure {
>> +                               filename = "to_sign.bin";
>> +                               sysfw-cert;
>> +                       };
>> +       };
>> +};
>> diff --git a/tools/binman/test/279_ti_secure.dts b/tools/binman/test/279_ti_secure.dts
>> new file mode 100644
>> index 0000000000..0e60984013
>> --- /dev/null
>> +++ b/tools/binman/test/279_ti_secure.dts
>> @@ -0,0 +1,11 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +/ {
>> +       binman {
>> +                       ti-secure {
>> +                               filename = "to_sign.bin";
>> +                               secure;
>> +                       };
>> +       };
>> +};
>> diff --git a/tools/binman/test/280_ti_secure_nofilename.dts b/tools/binman/test/280_ti_secure_nofilename.dts
>> new file mode 100644
>> index 0000000000..928a50a499
>> --- /dev/null
>> +++ b/tools/binman/test/280_ti_secure_nofilename.dts
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +/ {
>> +       binman {
>> +                       ti-secure {
>> +                               secure;
>> +                       };
>> +       };
>> +};
>> diff --git a/tools/binman/test/281_ti_secure_combined.dts b/tools/binman/test/281_ti_secure_combined.dts
>> new file mode 100644
>> index 0000000000..aadb5a4306
>> --- /dev/null
>> +++ b/tools/binman/test/281_ti_secure_combined.dts
>> @@ -0,0 +1,12 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +/ {
>> +       binman {
>> +                       ti-secure {
>> +                               filename = "to_sign.bin";
>> +                               combined;
>> +                               split-dm;
>> +                       };
>> +       };
>> +};
>> --
>> 2.34.1
>>
Simon Glass Feb. 28, 2023, 3:10 p.m. UTC | #3 
Hi Neha,

On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Simon,
>
> On 28/02/23 06:05, Simon Glass wrote:
> > Hi Neha,
> >
> > On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com> wrote:
> >>
> >> core-secdev-k3 is the TI security development package provided for K3
> >> platform devices. This tool helps sign bootloader images with the x509
> >> ceritificate header.
> >>
> >> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> >> ---
> >> This patch depends on https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
> >> and on ongoing development in https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> >>
> >>   tools/binman/btool/tisecuretool.py            |  72 +++++++++++
> >>   tools/binman/etype/ti_secure.py               | 114 ++++++++++++++++++
> >>   tools/binman/ftest.py                         |  36 ++++++
> >>   tools/binman/test/278_ti_secure_rom.dts       |  11 ++
> >>   tools/binman/test/279_ti_secure.dts           |  11 ++
> >>   .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
> >>   tools/binman/test/281_ti_secure_combined.dts  |  12 ++
> >>   7 files changed, 266 insertions(+)
> >>   create mode 100644 tools/binman/btool/tisecuretool.py
> >>   create mode 100644 tools/binman/etype/ti_secure.py
> >>   create mode 100644 tools/binman/test/278_ti_secure_rom.dts
> >>   create mode 100644 tools/binman/test/279_ti_secure.dts
> >>   create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
> >>   create mode 100644 tools/binman/test/281_ti_secure_combined.dts
> >
> > Now that I see what you are doing, this it not quite the right way.
> >
> > See this hack-up of how you can call the openssl thing. Basically you
> > should not have a shell script in the way, but instead make your
> > bintool do it.
> >
> > https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
> >
> > I suppose we can have an openssl bintool that others build on top of?
> >
> > Regards,
> > Simon
> >
> >
>
> That is possible, maybe ti-secure extends from x509_cert etype so as to
> support the TI specific certificate extensions. I'll start working on this.
>
> However the patches I've sent support external production flow where
> signing need not necessarily be carried out from U-Boot. An external
> repo that acts as what is core-secdev-k3 here, would be the repo
> responsible for signing.
>
> Could we find a way to combine both so as to support production flow
> mandating an external agency, as well as a completely within U-Boot flow
> using bintool? i.e. we modify ti-secure etype to be able to support both
> external git repo/executable script signing as well as default signing
> using openssl bintool.

Yes that seems important.

One option is to have binman emit some instructions on how to sign the
image, perhaps a simple data format similar to an fdtmap, with a basic
shell script plus whatever the etype provides. Then the signer can
follow the instructions or run the script.

Another option is to run binman on the signer and have it do the
signing. Would that work?

Regards,
Simon


>
> >>
> >> diff --git a/tools/binman/btool/tisecuretool.py b/tools/binman/btool/tisecuretool.py
> >> new file mode 100644
> >> index 0000000000..5102bb1f7d
> >> --- /dev/null
> >> +++ b/tools/binman/btool/tisecuretool.py
> >> @@ -0,0 +1,72 @@
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
> >> +# Written by Neha Malcom Francis <n-francis@ti.com>
> >> +#
> >> +"""Bintool implementation for TI security development tools
> >> +
> >> +tisecuretool helps add x509 certification for bootloader images for K3 platform devices
> >> +
> >> +Source code:
> >> +https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
> >> +
> >> +import os
> >> +
> >> +from binman import bintool
> >> +from patman import tout
> >> +from patman import tools
> >> +
> >> +class Bintooltisecuretool(bintool.Bintool):
> >> +    """Signing tool for TI bootloaders"""
> >> +    name = 'tisecuretool'
> >> +    def __init__(self, name):
> >> +        super().__init__(name, 'TI secure tool')
> >> +
> >> +    def sign_binary_secure(self, fname, out_fname):
> >> +        """Create a signed binary
> >> +
> >> +        Args:
> >> +            fname (str): Filename to sign
> >> +            out_fname (str): Output filename
> >> +
> >> +        Returns:
> >> +            str: Tool output
> >> +            or None
> >> +        """
> >> +        tool_path = self.get_path()
> >> +        script_path = os.path.join(tool_path, 'scripts/secure-binary-image.sh')
> >> +        args = [
> >> +            'sh',
> >> +            script_path,
> >> +            fname,
> >> +            out_fname
> >> +            ]
> >> +        output = self.run_cmd(*args, add_name=False)
> >> +        return output
> >> +
> >> +    def sign_binary_rom(self, args):
> >> +        """Create a signed binary that is booted by ROM
> >> +
> >> +        Args:
> >> +            fname (str): Filename to sign
> >> +            out_fname (str): Output filename"""
> >> +        tool_path = self.get_path()
> >> +        script_path = os.path.join(tool_path, 'scripts/secure-rom-boot-image.sh')
> >> +        #args.insert(0, script_path)
> >> +        args.insert(0, script_path)
> >> +        output = self.run_cmd(*args, add_name=False)
> >> +        return output
> >> +
> >> +    def fetch(self, method):
> >> +        """Fetch handler for TI secure tool
> >> +
> >> +        This builds the tool from source
> >> +
> >> +        Returns:
> >> +            True if the file was fetched, None if a method other than FETCH_SOURCE
> >> +            was requested
> >> +        """
> >> +        if method != bintool.FETCH_SOURCE:
> >> +            return None
> >> +        result = self.fetch_from_git(
> >> +            'git://git.ti.com/security-development-tools/core-secdev-k3.git', 'tisecuretool')
> >> +        return result
> >> diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py
> >> new file mode 100644
> >> index 0000000000..26f81ff8e8
> >> --- /dev/null
> >> +++ b/tools/binman/etype/ti_secure.py
> >> @@ -0,0 +1,114 @@
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
> >> +# Written by Neha Malcom Francis <n-francis@ti.com>
> >> +#
> >> +# Entry-type module for signed binaries for TI K3 platform
> >> +#
> >> +
> >> +from binman.etype.blob import Entry_blob
> >> +from binman import bintool
> >> +
> >> +from dtoc import fdt_util
> >> +from patman import terminal
> >> +from patman import tools
> >> +from patman import tout
> >> +
> >> +class Entry_ti_secure(Entry_blob):
> >> +    """An entry which contains a signed x509 binary for signing TI
> >> +    General Purpose as well as High-Security devices.
> >> +
> >> +    Properties / Entry arguments:
> >> +       - filename: filename of binary file to be secured
> >> +
> >> +    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.core = fdt_util.GetString(self._node, 'core', 'secure')
> >> +        self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
> >> +        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
> >> +        self.sysfw_cert = fdt_util.GetBool(self._node, 'sysfw-cert', False)
> >> +        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', 0x40000)
> >> +        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', 0x7f000)
> >> +        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', 0x41c80000)
> >> +        self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
> >> +        self.toolpresent = False
> >> +        if not self.filename:
> >> +            self.Raise("ti_secure must have a 'filename' property")
> >> +
> >> +    def _SignBinarySecure(self):
> >> +        infilename = self.filename
> >> +        outfilename = infilename + '_signed'
> >> +
> >> +        data = self.tisecuretool.sign_binary_secure(infilename, outfilename)
> >> +
> >> +        if data is None:
> >> +            # Bintool is missing, create a zeroed binary
> >> +            self.record_missing_bintool(self.tisecuretool)
> >> +            return tools.get_bytes(0, 1024)
> >> +
> >> +        return tools.read_file(outfilename)
> >> +
> >> +    def _SignBinaryROM(self):
> >> +        input_fname = self.filename
> >> +        output_fname = input_fname + "_x509"
> >> +        if self.combined:
> >> +            args = [
> >> +                '-b', input_fname,
> >> +                '-l', hex(self.load_addr),
> >> +                '-s', self.sysfw_filename,
> >> +                '-m', hex(self.sysfw_load_addr),
> >> +                '-a', self.sysfw_inner_cert,
> >> +                '-d', self.sysfw_data_filename,
> >> +                '-n', hex(self.sysfw_data_load_addr),
> >> +                '-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 = [
> >> +                '-a', self.core,
> >> +                '-b', input_fname,
> >> +                '-o', output_fname,
> >> +                '-l', hex(self.load_addr),
> >> +                '-r', str(self.sw_rev),
> >> +
> >> +            ]
> >> +            if self.sysfw_cert == True:
> >> +                args.insert(0, '-u')
> >> +
> >> +        out = self.tisecuretool.sign_binary_rom(args)
> >> +
> >> +        if out is None:
> >> +            # Bintool is missing, create a zeroed binary
> >> +            self.record_missing_bintool(self.tisecuretool)
> >> +            return tools.get_bytes(0, 1024)
> >> +
> >> +        return tools.read_file(output_fname)
> >> +
> >> +    def ProcessContents(self):
> >> +        if self.secure:
> >> +            data = self._SignBinarySecure()
> >> +        else:
> >> +            data = self._SignBinaryROM()
> >> +        return self.ProcessContentsUpdate(data)
> >> +
> >> +    def AddBintools(self, btools):
> >> +        #super().AddBintools(btools)
> >> +        col = terminal.Color()
> >> +        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
> >> +        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, col, True)
> >> +        if out == bintool.FAIL:
> >> +            # Bintool is missing, record missing
> >> +            self.record_missing_bintool(self.tisecuretool)
> >> \ No newline at end of file
> >> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> >> index bf902341c5..72cce0ef02 100644
> >> --- a/tools/binman/ftest.py
> >> +++ b/tools/binman/ftest.py
> >> @@ -97,6 +97,7 @@ PRE_LOAD_MAGIC        = b'UBSH'
> >>   PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
> >>   PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
> >>   TI_BOARD_CONFIG_DATA  = b'\x00\x01'
> >> +TIX509DATA            = b'letsgo'
> >>
> >>   # Subdirectory of the input dir to use to put test FDTs
> >>   TEST_FDT_SUBDIR       = 'fdts'
> >> @@ -206,6 +207,7 @@ class TestFunctional(unittest.TestCase):
> >>           TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
> >>           TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
> >>           TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
> >> +        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
> >>
> >>           # Add a few .dtb files for testing
> >>           TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
> >> @@ -6398,5 +6400,39 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >>           configlen_noheader = TI_BOARD_CONFIG_DATA*4
> >>           self.assertGreater(data, configlen_noheader)
> >>
> >> +    def testTISecureROMImageMissingTool(self):
> >> +        """Test that a TI secure signed ROM booted image can be generated although tisecuretool is missing"""
> >> +        with test_util.capture_sys_output() as (_, stderr):
> >> +            data = self._DoTestFile('278_ti_secure_rom.dts',
> >> +                force_missing_bintools='tisecuretool')
> >> +        err = stderr.getvalue()
> >> +        self.assertRegex(err,
> >> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> >> +
> >> +    def testTISecureImageMissingTool(self):
> >> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
> >> +        with test_util.capture_sys_output() as (_, stderr):
> >> +            data = self._DoTestFile('279_ti_secure.dts',
> >> +                force_missing_bintools='tisecuretool')
> >> +        err = stderr.getvalue()
> >> +        self.assertRegex(err,
> >> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> >> +
> >> +    def testTISecureNoFilename(self):
> >> +        """Test that a missing 'filename' property is detected"""
> >> +        with self.assertRaises(ValueError) as e:
> >> +            self._DoTestFile('280_ti_secure_nofilename.dts')
> >> +        self.assertIn("ti_secure must have a 'filename' property",
> >> +                      str(e.exception))
> >> +
> >> +    def testTISecureImageCombinedMissingTool(self):
> >> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
> >> +        with test_util.capture_sys_output() as (_, stderr):
> >> +            data = self._DoTestFile('281_ti_secure_combined.dts',
> >> +                force_missing_bintools='tisecuretool')
> >> +        err = stderr.getvalue()
> >> +        self.assertRegex(err,
> >> +                         "Image 'image'.*missing bintools.*: tisecuretool")
> >> +
> >>   if __name__ == "__main__":
> >>       unittest.main()
> >> diff --git a/tools/binman/test/278_ti_secure_rom.dts b/tools/binman/test/278_ti_secure_rom.dts
> >> new file mode 100644
> >> index 0000000000..67a6a9de8d
> >> --- /dev/null
> >> +++ b/tools/binman/test/278_ti_secure_rom.dts
> >> @@ -0,0 +1,11 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/dts-v1/;
> >> +
> >> +/ {
> >> +       binman {
> >> +                       ti-secure {
> >> +                               filename = "to_sign.bin";
> >> +                               sysfw-cert;
> >> +                       };
> >> +       };
> >> +};
> >> diff --git a/tools/binman/test/279_ti_secure.dts b/tools/binman/test/279_ti_secure.dts
> >> new file mode 100644
> >> index 0000000000..0e60984013
> >> --- /dev/null
> >> +++ b/tools/binman/test/279_ti_secure.dts
> >> @@ -0,0 +1,11 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/dts-v1/;
> >> +
> >> +/ {
> >> +       binman {
> >> +                       ti-secure {
> >> +                               filename = "to_sign.bin";
> >> +                               secure;
> >> +                       };
> >> +       };
> >> +};
> >> diff --git a/tools/binman/test/280_ti_secure_nofilename.dts b/tools/binman/test/280_ti_secure_nofilename.dts
> >> new file mode 100644
> >> index 0000000000..928a50a499
> >> --- /dev/null
> >> +++ b/tools/binman/test/280_ti_secure_nofilename.dts
> >> @@ -0,0 +1,10 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/dts-v1/;
> >> +
> >> +/ {
> >> +       binman {
> >> +                       ti-secure {
> >> +                               secure;
> >> +                       };
> >> +       };
> >> +};
> >> diff --git a/tools/binman/test/281_ti_secure_combined.dts b/tools/binman/test/281_ti_secure_combined.dts
> >> new file mode 100644
> >> index 0000000000..aadb5a4306
> >> --- /dev/null
> >> +++ b/tools/binman/test/281_ti_secure_combined.dts
> >> @@ -0,0 +1,12 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/dts-v1/;
> >> +
> >> +/ {
> >> +       binman {
> >> +                       ti-secure {
> >> +                               filename = "to_sign.bin";
> >> +                               combined;
> >> +                               split-dm;
> >> +                       };
> >> +       };
> >> +};
> >> --
> >> 2.34.1
> >>
>
> --
> Thanking You
> Neha Malcom Francis
Andrew Davis March 1, 2023, 5:11 p.m. UTC | #4 
On 2/28/23 9:10 AM, Simon Glass wrote:
> Hi Neha,
> 
> On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 28/02/23 06:05, Simon Glass wrote:
>>> Hi Neha,
>>>
>>> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com> wrote:
>>>>
>>>> core-secdev-k3 is the TI security development package provided for K3
>>>> platform devices. This tool helps sign bootloader images with the x509
>>>> ceritificate header.
>>>>
>>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>>>> ---
>>>> This patch depends on https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
>>>> and on ongoing development in https://git.ti.com/cgit/security-development-tools/core-secdev-k3
>>>>
>>>>    tools/binman/btool/tisecuretool.py            |  72 +++++++++++
>>>>    tools/binman/etype/ti_secure.py               | 114 ++++++++++++++++++
>>>>    tools/binman/ftest.py                         |  36 ++++++
>>>>    tools/binman/test/278_ti_secure_rom.dts       |  11 ++
>>>>    tools/binman/test/279_ti_secure.dts           |  11 ++
>>>>    .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
>>>>    tools/binman/test/281_ti_secure_combined.dts  |  12 ++
>>>>    7 files changed, 266 insertions(+)
>>>>    create mode 100644 tools/binman/btool/tisecuretool.py
>>>>    create mode 100644 tools/binman/etype/ti_secure.py
>>>>    create mode 100644 tools/binman/test/278_ti_secure_rom.dts
>>>>    create mode 100644 tools/binman/test/279_ti_secure.dts
>>>>    create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
>>>>    create mode 100644 tools/binman/test/281_ti_secure_combined.dts
>>>
>>> Now that I see what you are doing, this it not quite the right way.
>>>
>>> See this hack-up of how you can call the openssl thing. Basically you
>>> should not have a shell script in the way, but instead make your
>>> bintool do it.
>>>
>>> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
>>>
>>> I suppose we can have an openssl bintool that others build on top of?
>>>
>>> Regards,
>>> Simon
>>>
>>>
>>
>> That is possible, maybe ti-secure extends from x509_cert etype so as to
>> support the TI specific certificate extensions. I'll start working on this.
>>
>> However the patches I've sent support external production flow where
>> signing need not necessarily be carried out from U-Boot. An external
>> repo that acts as what is core-secdev-k3 here, would be the repo
>> responsible for signing.
>>
>> Could we find a way to combine both so as to support production flow
>> mandating an external agency, as well as a completely within U-Boot flow
>> using bintool? i.e. we modify ti-secure etype to be able to support both
>> external git repo/executable script signing as well as default signing
>> using openssl bintool.
> 
> Yes that seems important.
> 
> One option is to have binman emit some instructions on how to sign the
> image, perhaps a simple data format similar to an fdtmap, with a basic
> shell script plus whatever the etype provides. Then the signer can
> follow the instructions or run the script.
> 
> Another option is to run binman on the signer and have it do the
> signing. Would that work?
> 

I'd like to keep the dependencies needed on the signing server as
minimal as possible. We do require the "u-boot-tools" package on
the key servers if folks want to re-package signed FIT images, so
if binman could be reasonably packaged in that package someday
it could be an option.

For now, if binman could generate the x509 file and leave it with the
other boot artifacts then if one needs to re-sign with real keys they
will only need to have openssl sign that cert and append to the image
on the key server, that is simple enough document.

The more difficult part is in re-packaging these signed images.
Our security tools already have a tool to disasemble a FIT image,
sign the components, then re-assemble it[0]. This would work for
u-boot.img and the kernel FIT images, but we would need something
new for re-assembling boot3.bin and tispl.bin. The first boot files
(boot3.bin) is also more complex that simple FIT images in that it
has several variations in format and content based on device family
and type.

The ideal case would be we do not need to pull in the TI security tools
package at all and we could drop this patch. The tools would then only be
needed by folks wanting to sign their images using an external key
server.

If we could have binman learn how to generate/template x509 certs and
have it sign them with openssl, plus add the TI dummy[1] and degenerate[2]
keys to the u-boot source repo, then we would not need TI security
tools any more here.

This might be a longer term goal though, and I think we are already
trying to do too much all at once as is. Perhaps we could take
this current solution posted here with the intent to remove it in
the near future. Thoughts?

Andrew

[0] https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/scripts/fit-image-secure.sh
[1] https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/custMpk.pem
[2] https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/ti-degenerate-key.pem

> Regards,
> Simon
> 
> 
>>
>>>>
>>>> diff --git a/tools/binman/btool/tisecuretool.py b/tools/binman/btool/tisecuretool.py
>>>> new file mode 100644
>>>> index 0000000000..5102bb1f7d
>>>> --- /dev/null
>>>> +++ b/tools/binman/btool/tisecuretool.py
>>>> @@ -0,0 +1,72 @@
>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>>>> +#
>>>> +"""Bintool implementation for TI security development tools
>>>> +
>>>> +tisecuretool helps add x509 certification for bootloader images for K3 platform devices
>>>> +
>>>> +Source code:
>>>> +https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
>>>> +
>>>> +import os
>>>> +
>>>> +from binman import bintool
>>>> +from patman import tout
>>>> +from patman import tools
>>>> +
>>>> +class Bintooltisecuretool(bintool.Bintool):
>>>> +    """Signing tool for TI bootloaders"""
>>>> +    name = 'tisecuretool'
>>>> +    def __init__(self, name):
>>>> +        super().__init__(name, 'TI secure tool')
>>>> +
>>>> +    def sign_binary_secure(self, fname, out_fname):
>>>> +        """Create a signed binary
>>>> +
>>>> +        Args:
>>>> +            fname (str): Filename to sign
>>>> +            out_fname (str): Output filename
>>>> +
>>>> +        Returns:
>>>> +            str: Tool output
>>>> +            or None
>>>> +        """
>>>> +        tool_path = self.get_path()
>>>> +        script_path = os.path.join(tool_path, 'scripts/secure-binary-image.sh')
>>>> +        args = [
>>>> +            'sh',
>>>> +            script_path,
>>>> +            fname,
>>>> +            out_fname
>>>> +            ]
>>>> +        output = self.run_cmd(*args, add_name=False)
>>>> +        return output
>>>> +
>>>> +    def sign_binary_rom(self, args):
>>>> +        """Create a signed binary that is booted by ROM
>>>> +
>>>> +        Args:
>>>> +            fname (str): Filename to sign
>>>> +            out_fname (str): Output filename"""
>>>> +        tool_path = self.get_path()
>>>> +        script_path = os.path.join(tool_path, 'scripts/secure-rom-boot-image.sh')
>>>> +        #args.insert(0, script_path)
>>>> +        args.insert(0, script_path)
>>>> +        output = self.run_cmd(*args, add_name=False)
>>>> +        return output
>>>> +
>>>> +    def fetch(self, method):
>>>> +        """Fetch handler for TI secure tool
>>>> +
>>>> +        This builds the tool from source
>>>> +
>>>> +        Returns:
>>>> +            True if the file was fetched, None if a method other than FETCH_SOURCE
>>>> +            was requested
>>>> +        """
>>>> +        if method != bintool.FETCH_SOURCE:
>>>> +            return None
>>>> +        result = self.fetch_from_git(
>>>> +            'git://git.ti.com/security-development-tools/core-secdev-k3.git', 'tisecuretool')
>>>> +        return result
>>>> diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py
>>>> new file mode 100644
>>>> index 0000000000..26f81ff8e8
>>>> --- /dev/null
>>>> +++ b/tools/binman/etype/ti_secure.py
>>>> @@ -0,0 +1,114 @@
>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>> +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>>>> +#
>>>> +# Entry-type module for signed binaries for TI K3 platform
>>>> +#
>>>> +
>>>> +from binman.etype.blob import Entry_blob
>>>> +from binman import bintool
>>>> +
>>>> +from dtoc import fdt_util
>>>> +from patman import terminal
>>>> +from patman import tools
>>>> +from patman import tout
>>>> +
>>>> +class Entry_ti_secure(Entry_blob):
>>>> +    """An entry which contains a signed x509 binary for signing TI
>>>> +    General Purpose as well as High-Security devices.
>>>> +
>>>> +    Properties / Entry arguments:
>>>> +       - filename: filename of binary file to be secured
>>>> +
>>>> +    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.core = fdt_util.GetString(self._node, 'core', 'secure')
>>>> +        self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
>>>> +        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
>>>> +        self.sysfw_cert = fdt_util.GetBool(self._node, 'sysfw-cert', False)
>>>> +        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', 0x40000)
>>>> +        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', 0x7f000)
>>>> +        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', 0x41c80000)
>>>> +        self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
>>>> +        self.toolpresent = False
>>>> +        if not self.filename:
>>>> +            self.Raise("ti_secure must have a 'filename' property")
>>>> +
>>>> +    def _SignBinarySecure(self):
>>>> +        infilename = self.filename
>>>> +        outfilename = infilename + '_signed'
>>>> +
>>>> +        data = self.tisecuretool.sign_binary_secure(infilename, outfilename)
>>>> +
>>>> +        if data is None:
>>>> +            # Bintool is missing, create a zeroed binary
>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>> +            return tools.get_bytes(0, 1024)
>>>> +
>>>> +        return tools.read_file(outfilename)
>>>> +
>>>> +    def _SignBinaryROM(self):
>>>> +        input_fname = self.filename
>>>> +        output_fname = input_fname + "_x509"
>>>> +        if self.combined:
>>>> +            args = [
>>>> +                '-b', input_fname,
>>>> +                '-l', hex(self.load_addr),
>>>> +                '-s', self.sysfw_filename,
>>>> +                '-m', hex(self.sysfw_load_addr),
>>>> +                '-a', self.sysfw_inner_cert,
>>>> +                '-d', self.sysfw_data_filename,
>>>> +                '-n', hex(self.sysfw_data_load_addr),
>>>> +                '-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 = [
>>>> +                '-a', self.core,
>>>> +                '-b', input_fname,
>>>> +                '-o', output_fname,
>>>> +                '-l', hex(self.load_addr),
>>>> +                '-r', str(self.sw_rev),
>>>> +
>>>> +            ]
>>>> +            if self.sysfw_cert == True:
>>>> +                args.insert(0, '-u')
>>>> +
>>>> +        out = self.tisecuretool.sign_binary_rom(args)
>>>> +
>>>> +        if out is None:
>>>> +            # Bintool is missing, create a zeroed binary
>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>> +            return tools.get_bytes(0, 1024)
>>>> +
>>>> +        return tools.read_file(output_fname)
>>>> +
>>>> +    def ProcessContents(self):
>>>> +        if self.secure:
>>>> +            data = self._SignBinarySecure()
>>>> +        else:
>>>> +            data = self._SignBinaryROM()
>>>> +        return self.ProcessContentsUpdate(data)
>>>> +
>>>> +    def AddBintools(self, btools):
>>>> +        #super().AddBintools(btools)
>>>> +        col = terminal.Color()
>>>> +        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
>>>> +        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, col, True)
>>>> +        if out == bintool.FAIL:
>>>> +            # Bintool is missing, record missing
>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>> \ No newline at end of file
>>>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>>>> index bf902341c5..72cce0ef02 100644
>>>> --- a/tools/binman/ftest.py
>>>> +++ b/tools/binman/ftest.py
>>>> @@ -97,6 +97,7 @@ PRE_LOAD_MAGIC        = b'UBSH'
>>>>    PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
>>>>    PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
>>>>    TI_BOARD_CONFIG_DATA  = b'\x00\x01'
>>>> +TIX509DATA            = b'letsgo'
>>>>
>>>>    # Subdirectory of the input dir to use to put test FDTs
>>>>    TEST_FDT_SUBDIR       = 'fdts'
>>>> @@ -206,6 +207,7 @@ class TestFunctional(unittest.TestCase):
>>>>            TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>>>>            TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>>>>            TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>>>> +        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
>>>>
>>>>            # Add a few .dtb files for testing
>>>>            TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
>>>> @@ -6398,5 +6400,39 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>>>>            configlen_noheader = TI_BOARD_CONFIG_DATA*4
>>>>            self.assertGreater(data, configlen_noheader)
>>>>
>>>> +    def testTISecureROMImageMissingTool(self):
>>>> +        """Test that a TI secure signed ROM booted image can be generated although tisecuretool is missing"""
>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>> +            data = self._DoTestFile('278_ti_secure_rom.dts',
>>>> +                force_missing_bintools='tisecuretool')
>>>> +        err = stderr.getvalue()
>>>> +        self.assertRegex(err,
>>>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>>>> +
>>>> +    def testTISecureImageMissingTool(self):
>>>> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>> +            data = self._DoTestFile('279_ti_secure.dts',
>>>> +                force_missing_bintools='tisecuretool')
>>>> +        err = stderr.getvalue()
>>>> +        self.assertRegex(err,
>>>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>>>> +
>>>> +    def testTISecureNoFilename(self):
>>>> +        """Test that a missing 'filename' property is detected"""
>>>> +        with self.assertRaises(ValueError) as e:
>>>> +            self._DoTestFile('280_ti_secure_nofilename.dts')
>>>> +        self.assertIn("ti_secure must have a 'filename' property",
>>>> +                      str(e.exception))
>>>> +
>>>> +    def testTISecureImageCombinedMissingTool(self):
>>>> +        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>> +            data = self._DoTestFile('281_ti_secure_combined.dts',
>>>> +                force_missing_bintools='tisecuretool')
>>>> +        err = stderr.getvalue()
>>>> +        self.assertRegex(err,
>>>> +                         "Image 'image'.*missing bintools.*: tisecuretool")
>>>> +
>>>>    if __name__ == "__main__":
>>>>        unittest.main()
>>>> diff --git a/tools/binman/test/278_ti_secure_rom.dts b/tools/binman/test/278_ti_secure_rom.dts
>>>> new file mode 100644
>>>> index 0000000000..67a6a9de8d
>>>> --- /dev/null
>>>> +++ b/tools/binman/test/278_ti_secure_rom.dts
>>>> @@ -0,0 +1,11 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> +       binman {
>>>> +                       ti-secure {
>>>> +                               filename = "to_sign.bin";
>>>> +                               sysfw-cert;
>>>> +                       };
>>>> +       };
>>>> +};
>>>> diff --git a/tools/binman/test/279_ti_secure.dts b/tools/binman/test/279_ti_secure.dts
>>>> new file mode 100644
>>>> index 0000000000..0e60984013
>>>> --- /dev/null
>>>> +++ b/tools/binman/test/279_ti_secure.dts
>>>> @@ -0,0 +1,11 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> +       binman {
>>>> +                       ti-secure {
>>>> +                               filename = "to_sign.bin";
>>>> +                               secure;
>>>> +                       };
>>>> +       };
>>>> +};
>>>> diff --git a/tools/binman/test/280_ti_secure_nofilename.dts b/tools/binman/test/280_ti_secure_nofilename.dts
>>>> new file mode 100644
>>>> index 0000000000..928a50a499
>>>> --- /dev/null
>>>> +++ b/tools/binman/test/280_ti_secure_nofilename.dts
>>>> @@ -0,0 +1,10 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> +       binman {
>>>> +                       ti-secure {
>>>> +                               secure;
>>>> +                       };
>>>> +       };
>>>> +};
>>>> diff --git a/tools/binman/test/281_ti_secure_combined.dts b/tools/binman/test/281_ti_secure_combined.dts
>>>> new file mode 100644
>>>> index 0000000000..aadb5a4306
>>>> --- /dev/null
>>>> +++ b/tools/binman/test/281_ti_secure_combined.dts
>>>> @@ -0,0 +1,12 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/dts-v1/;
>>>> +
>>>> +/ {
>>>> +       binman {
>>>> +                       ti-secure {
>>>> +                               filename = "to_sign.bin";
>>>> +                               combined;
>>>> +                               split-dm;
>>>> +                       };
>>>> +       };
>>>> +};
>>>> --
>>>> 2.34.1
>>>>
>>
>> --
>> Thanking You
>> Neha Malcom Francis
Neha Malcom Francis March 10, 2023, 5:35 a.m. UTC | #5 
Hi Andrew, Simon

On 01/03/23 22:41, Andrew Davis wrote:
> On 2/28/23 9:10 AM, Simon Glass wrote:
>> Hi Neha,
>>
>> On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com> 
>> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 28/02/23 06:05, Simon Glass wrote:
>>>> Hi Neha,
>>>>
>>>> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com> 
>>>> wrote:
>>>>>
>>>>> core-secdev-k3 is the TI security development package provided for K3
>>>>> platform devices. This tool helps sign bootloader images with the x509
>>>>> ceritificate header.
>>>>>
>>>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>>>>> ---
>>>>> This patch depends on 
>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
>>>>> and on ongoing development in 
>>>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3
>>>>>
>>>>>    tools/binman/btool/tisecuretool.py            |  72 +++++++++++
>>>>>    tools/binman/etype/ti_secure.py               | 114 
>>>>> ++++++++++++++++++
>>>>>    tools/binman/ftest.py                         |  36 ++++++
>>>>>    tools/binman/test/278_ti_secure_rom.dts       |  11 ++
>>>>>    tools/binman/test/279_ti_secure.dts           |  11 ++
>>>>>    .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
>>>>>    tools/binman/test/281_ti_secure_combined.dts  |  12 ++
>>>>>    7 files changed, 266 insertions(+)
>>>>>    create mode 100644 tools/binman/btool/tisecuretool.py
>>>>>    create mode 100644 tools/binman/etype/ti_secure.py
>>>>>    create mode 100644 tools/binman/test/278_ti_secure_rom.dts
>>>>>    create mode 100644 tools/binman/test/279_ti_secure.dts
>>>>>    create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
>>>>>    create mode 100644 tools/binman/test/281_ti_secure_combined.dts
>>>>
>>>> Now that I see what you are doing, this it not quite the right way.
>>>>
>>>> See this hack-up of how you can call the openssl thing. Basically you
>>>> should not have a shell script in the way, but instead make your
>>>> bintool do it.
>>>>
>>>> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
>>>>
>>>> I suppose we can have an openssl bintool that others build on top of?
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>
>>>
>>> That is possible, maybe ti-secure extends from x509_cert etype so as to
>>> support the TI specific certificate extensions. I'll start working on 
>>> this.
>>>
>>> However the patches I've sent support external production flow where
>>> signing need not necessarily be carried out from U-Boot. An external
>>> repo that acts as what is core-secdev-k3 here, would be the repo
>>> responsible for signing.
>>>
>>> Could we find a way to combine both so as to support production flow
>>> mandating an external agency, as well as a completely within U-Boot flow
>>> using bintool? i.e. we modify ti-secure etype to be able to support both
>>> external git repo/executable script signing as well as default signing
>>> using openssl bintool.
>>
>> Yes that seems important.
>>
>> One option is to have binman emit some instructions on how to sign the
>> image, perhaps a simple data format similar to an fdtmap, with a basic
>> shell script plus whatever the etype provides. Then the signer can
>> follow the instructions or run the script.
>>
>> Another option is to run binman on the signer and have it do the
>> signing. Would that work?
>>
> 
> I'd like to keep the dependencies needed on the signing server as
> minimal as possible. We do require the "u-boot-tools" package on
> the key servers if folks want to re-package signed FIT images, so
> if binman could be reasonably packaged in that package someday
> it could be an option.
> 
> For now, if binman could generate the x509 file and leave it with the
> other boot artifacts then if one needs to re-sign with real keys they
> will only need to have openssl sign that cert and append to the image
> on the key server, that is simple enough document.
> 
> The more difficult part is in re-packaging these signed images.
> Our security tools already have a tool to disasemble a FIT image,
> sign the components, then re-assemble it[0]. This would work for
> u-boot.img and the kernel FIT images, but we would need something
> new for re-assembling boot3.bin and tispl.bin. The first boot files
> (boot3.bin) is also more complex that simple FIT images in that it
> has several variations in format and content based on device family
> and type.
> 
> The ideal case would be we do not need to pull in the TI security tools
> package at all and we could drop this patch. The tools would then only be
> needed by folks wanting to sign their images using an external key
> server.
> 
> If we could have binman learn how to generate/template x509 certs and
> have it sign them with openssl, plus add the TI dummy[1] and degenerate[2]
> keys to the u-boot source repo, then we would not need TI security
> tools any more here.
> 
> This might be a longer term goal though, and I think we are already
> trying to do too much all at once as is. Perhaps we could take
> this current solution posted here with the intent to remove it in
> the near future. Thoughts?
> 
> Andrew
> 
> [0] 
> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/scripts/fit-image-secure.sh
> [1] 
> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/custMpk.pem
> [2] 
> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/ti-degenerate-key.pem
> 

I think leaving the boot artifacts while going with the current solution 
would be a better option. As Andrew mentioned, the complexity mainly 
comes with the way our tiboot3.bin is packaged. It seems cleaner for now 
to go with this since this patch series is only a part in the larger 
series of packaging all the bootloader images.

  I will take future action to eventually completely remove using TI 
security tools. What do you think, Simon?

>> Regards,
>> Simon
>>
>>
>>>
>>>>>
>>>>> diff --git a/tools/binman/btool/tisecuretool.py 
>>>>> b/tools/binman/btool/tisecuretool.py
>>>>> new file mode 100644
>>>>> index 0000000000..5102bb1f7d
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/btool/tisecuretool.py
>>>>> @@ -0,0 +1,72 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>> +# Copyright (c) 2022 Texas Instruments Incorporated - 
>>>>> https://www.ti.com/
>>>>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>>>>> +#
>>>>> +"""Bintool implementation for TI security development tools
>>>>> +
>>>>> +tisecuretool helps add x509 certification for bootloader images 
>>>>> for K3 platform devices
>>>>> +
>>>>> +Source code:
>>>>> +https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
>>>>> +
>>>>> +import os
>>>>> +
>>>>> +from binman import bintool
>>>>> +from patman import tout
>>>>> +from patman import tools
>>>>> +
>>>>> +class Bintooltisecuretool(bintool.Bintool):
>>>>> +    """Signing tool for TI bootloaders"""
>>>>> +    name = 'tisecuretool'
>>>>> +    def __init__(self, name):
>>>>> +        super().__init__(name, 'TI secure tool')
>>>>> +
>>>>> +    def sign_binary_secure(self, fname, out_fname):
>>>>> +        """Create a signed binary
>>>>> +
>>>>> +        Args:
>>>>> +            fname (str): Filename to sign
>>>>> +            out_fname (str): Output filename
>>>>> +
>>>>> +        Returns:
>>>>> +            str: Tool output
>>>>> +            or None
>>>>> +        """
>>>>> +        tool_path = self.get_path()
>>>>> +        script_path = os.path.join(tool_path, 
>>>>> 'scripts/secure-binary-image.sh')
>>>>> +        args = [
>>>>> +            'sh',
>>>>> +            script_path,
>>>>> +            fname,
>>>>> +            out_fname
>>>>> +            ]
>>>>> +        output = self.run_cmd(*args, add_name=False)
>>>>> +        return output
>>>>> +
>>>>> +    def sign_binary_rom(self, args):
>>>>> +        """Create a signed binary that is booted by ROM
>>>>> +
>>>>> +        Args:
>>>>> +            fname (str): Filename to sign
>>>>> +            out_fname (str): Output filename"""
>>>>> +        tool_path = self.get_path()
>>>>> +        script_path = os.path.join(tool_path, 
>>>>> 'scripts/secure-rom-boot-image.sh')
>>>>> +        #args.insert(0, script_path)
>>>>> +        args.insert(0, script_path)
>>>>> +        output = self.run_cmd(*args, add_name=False)
>>>>> +        return output
>>>>> +
>>>>> +    def fetch(self, method):
>>>>> +        """Fetch handler for TI secure tool
>>>>> +
>>>>> +        This builds the tool from source
>>>>> +
>>>>> +        Returns:
>>>>> +            True if the file was fetched, None if a method other 
>>>>> than FETCH_SOURCE
>>>>> +            was requested
>>>>> +        """
>>>>> +        if method != bintool.FETCH_SOURCE:
>>>>> +            return None
>>>>> +        result = self.fetch_from_git(
>>>>> +            
>>>>> 'git://git.ti.com/security-development-tools/core-secdev-k3.git', 
>>>>> 'tisecuretool')
>>>>> +        return result
>>>>> diff --git a/tools/binman/etype/ti_secure.py 
>>>>> b/tools/binman/etype/ti_secure.py
>>>>> new file mode 100644
>>>>> index 0000000000..26f81ff8e8
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/etype/ti_secure.py
>>>>> @@ -0,0 +1,114 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>> +# Copyright (c) 2022 Texas Instruments Incorporated - 
>>>>> https://www.ti.com/
>>>>> +# Written by Neha Malcom Francis <n-francis@ti.com>
>>>>> +#
>>>>> +# Entry-type module for signed binaries for TI K3 platform
>>>>> +#
>>>>> +
>>>>> +from binman.etype.blob import Entry_blob
>>>>> +from binman import bintool
>>>>> +
>>>>> +from dtoc import fdt_util
>>>>> +from patman import terminal
>>>>> +from patman import tools
>>>>> +from patman import tout
>>>>> +
>>>>> +class Entry_ti_secure(Entry_blob):
>>>>> +    """An entry which contains a signed x509 binary for signing TI
>>>>> +    General Purpose as well as High-Security devices.
>>>>> +
>>>>> +    Properties / Entry arguments:
>>>>> +       - filename: filename of binary file to be secured
>>>>> +
>>>>> +    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.core = fdt_util.GetString(self._node, 'core', 'secure')
>>>>> +        self.load_addr = fdt_util.GetInt(self._node, 'load', 
>>>>> 0x41c00000)
>>>>> +        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
>>>>> +        self.sysfw_cert = fdt_util.GetBool(self._node, 
>>>>> 'sysfw-cert', False)
>>>>> +        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', 0x40000)
>>>>> +        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', 0x7f000)
>>>>> +        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', 0x41c80000)
>>>>> +        self.sysfw_inner_cert_filename = 
>>>>> fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
>>>>> +        self.toolpresent = False
>>>>> +        if not self.filename:
>>>>> +            self.Raise("ti_secure must have a 'filename' property")
>>>>> +
>>>>> +    def _SignBinarySecure(self):
>>>>> +        infilename = self.filename
>>>>> +        outfilename = infilename + '_signed'
>>>>> +
>>>>> +        data = self.tisecuretool.sign_binary_secure(infilename, 
>>>>> outfilename)
>>>>> +
>>>>> +        if data is None:
>>>>> +            # Bintool is missing, create a zeroed binary
>>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>>> +            return tools.get_bytes(0, 1024)
>>>>> +
>>>>> +        return tools.read_file(outfilename)
>>>>> +
>>>>> +    def _SignBinaryROM(self):
>>>>> +        input_fname = self.filename
>>>>> +        output_fname = input_fname + "_x509"
>>>>> +        if self.combined:
>>>>> +            args = [
>>>>> +                '-b', input_fname,
>>>>> +                '-l', hex(self.load_addr),
>>>>> +                '-s', self.sysfw_filename,
>>>>> +                '-m', hex(self.sysfw_load_addr),
>>>>> +                '-a', self.sysfw_inner_cert,
>>>>> +                '-d', self.sysfw_data_filename,
>>>>> +                '-n', hex(self.sysfw_data_load_addr),
>>>>> +                '-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 = [
>>>>> +                '-a', self.core,
>>>>> +                '-b', input_fname,
>>>>> +                '-o', output_fname,
>>>>> +                '-l', hex(self.load_addr),
>>>>> +                '-r', str(self.sw_rev),
>>>>> +
>>>>> +            ]
>>>>> +            if self.sysfw_cert == True:
>>>>> +                args.insert(0, '-u')
>>>>> +
>>>>> +        out = self.tisecuretool.sign_binary_rom(args)
>>>>> +
>>>>> +        if out is None:
>>>>> +            # Bintool is missing, create a zeroed binary
>>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>>> +            return tools.get_bytes(0, 1024)
>>>>> +
>>>>> +        return tools.read_file(output_fname)
>>>>> +
>>>>> +    def ProcessContents(self):
>>>>> +        if self.secure:
>>>>> +            data = self._SignBinarySecure()
>>>>> +        else:
>>>>> +            data = self._SignBinaryROM()
>>>>> +        return self.ProcessContentsUpdate(data)
>>>>> +
>>>>> +    def AddBintools(self, btools):
>>>>> +        #super().AddBintools(btools)
>>>>> +        col = terminal.Color()
>>>>> +        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
>>>>> +        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, 
>>>>> col, True)
>>>>> +        if out == bintool.FAIL:
>>>>> +            # Bintool is missing, record missing
>>>>> +            self.record_missing_bintool(self.tisecuretool)
>>>>> \ No newline at end of file
>>>>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>>>>> index bf902341c5..72cce0ef02 100644
>>>>> --- a/tools/binman/ftest.py
>>>>> +++ b/tools/binman/ftest.py
>>>>> @@ -97,6 +97,7 @@ PRE_LOAD_MAGIC        = b'UBSH'
>>>>>    PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
>>>>>    PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
>>>>>    TI_BOARD_CONFIG_DATA  = b'\x00\x01'
>>>>> +TIX509DATA            = b'letsgo'
>>>>>
>>>>>    # Subdirectory of the input dir to use to put test FDTs
>>>>>    TEST_FDT_SUBDIR       = 'fdts'
>>>>> @@ -206,6 +207,7 @@ class TestFunctional(unittest.TestCase):
>>>>>            TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
>>>>>            TestFunctional._MakeInputFile('fw_dynamic.bin', 
>>>>> OPENSBI_DATA)
>>>>>            TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>>>>> +        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
>>>>>
>>>>>            # Add a few .dtb files for testing
>>>>>            TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % 
>>>>> TEST_FDT_SUBDIR,
>>>>> @@ -6398,5 +6400,39 @@ fdt         fdtmap                Extract 
>>>>> the devicetree blob from the fdtmap
>>>>>            configlen_noheader = TI_BOARD_CONFIG_DATA*4
>>>>>            self.assertGreater(data, configlen_noheader)
>>>>>
>>>>> +    def testTISecureROMImageMissingTool(self):
>>>>> +        """Test that a TI secure signed ROM booted image can be 
>>>>> generated although tisecuretool is missing"""
>>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>>> +            data = self._DoTestFile('278_ti_secure_rom.dts',
>>>>> +                force_missing_bintools='tisecuretool')
>>>>> +        err = stderr.getvalue()
>>>>> +        self.assertRegex(err,
>>>>> +                         "Image 'image'.*missing bintools.*: 
>>>>> tisecuretool")
>>>>> +
>>>>> +    def testTISecureImageMissingTool(self):
>>>>> +        """Test that a TI secure signed image can be generated 
>>>>> although tisecuretool is missing"""
>>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>>> +            data = self._DoTestFile('279_ti_secure.dts',
>>>>> +                force_missing_bintools='tisecuretool')
>>>>> +        err = stderr.getvalue()
>>>>> +        self.assertRegex(err,
>>>>> +                         "Image 'image'.*missing bintools.*: 
>>>>> tisecuretool")
>>>>> +
>>>>> +    def testTISecureNoFilename(self):
>>>>> +        """Test that a missing 'filename' property is detected"""
>>>>> +        with self.assertRaises(ValueError) as e:
>>>>> +            self._DoTestFile('280_ti_secure_nofilename.dts')
>>>>> +        self.assertIn("ti_secure must have a 'filename' property",
>>>>> +                      str(e.exception))
>>>>> +
>>>>> +    def testTISecureImageCombinedMissingTool(self):
>>>>> +        """Test that a TI secure signed image can be generated 
>>>>> although tisecuretool is missing"""
>>>>> +        with test_util.capture_sys_output() as (_, stderr):
>>>>> +            data = self._DoTestFile('281_ti_secure_combined.dts',
>>>>> +                force_missing_bintools='tisecuretool')
>>>>> +        err = stderr.getvalue()
>>>>> +        self.assertRegex(err,
>>>>> +                         "Image 'image'.*missing bintools.*: 
>>>>> tisecuretool")
>>>>> +
>>>>>    if __name__ == "__main__":
>>>>>        unittest.main()
>>>>> diff --git a/tools/binman/test/278_ti_secure_rom.dts 
>>>>> b/tools/binman/test/278_ti_secure_rom.dts
>>>>> new file mode 100644
>>>>> index 0000000000..67a6a9de8d
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/test/278_ti_secure_rom.dts
>>>>> @@ -0,0 +1,11 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> +       binman {
>>>>> +                       ti-secure {
>>>>> +                               filename = "to_sign.bin";
>>>>> +                               sysfw-cert;
>>>>> +                       };
>>>>> +       };
>>>>> +};
>>>>> diff --git a/tools/binman/test/279_ti_secure.dts 
>>>>> b/tools/binman/test/279_ti_secure.dts
>>>>> new file mode 100644
>>>>> index 0000000000..0e60984013
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/test/279_ti_secure.dts
>>>>> @@ -0,0 +1,11 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> +       binman {
>>>>> +                       ti-secure {
>>>>> +                               filename = "to_sign.bin";
>>>>> +                               secure;
>>>>> +                       };
>>>>> +       };
>>>>> +};
>>>>> diff --git a/tools/binman/test/280_ti_secure_nofilename.dts 
>>>>> b/tools/binman/test/280_ti_secure_nofilename.dts
>>>>> new file mode 100644
>>>>> index 0000000000..928a50a499
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/test/280_ti_secure_nofilename.dts
>>>>> @@ -0,0 +1,10 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> +       binman {
>>>>> +                       ti-secure {
>>>>> +                               secure;
>>>>> +                       };
>>>>> +       };
>>>>> +};
>>>>> diff --git a/tools/binman/test/281_ti_secure_combined.dts 
>>>>> b/tools/binman/test/281_ti_secure_combined.dts
>>>>> new file mode 100644
>>>>> index 0000000000..aadb5a4306
>>>>> --- /dev/null
>>>>> +++ b/tools/binman/test/281_ti_secure_combined.dts
>>>>> @@ -0,0 +1,12 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> +       binman {
>>>>> +                       ti-secure {
>>>>> +                               filename = "to_sign.bin";
>>>>> +                               combined;
>>>>> +                               split-dm;
>>>>> +                       };
>>>>> +       };
>>>>> +};
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>
>>> -- 
>>> Thanking You
>>> Neha Malcom Francis
Simon Glass March 10, 2023, 8:49 p.m. UTC | #6 
Hi,

On Thu, 9 Mar 2023 at 21:35, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Andrew, Simon
>
> On 01/03/23 22:41, Andrew Davis wrote:
> > On 2/28/23 9:10 AM, Simon Glass wrote:
> >> Hi Neha,
> >>
> >> On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com>
> >> wrote:
> >>>
> >>> Hi Simon,
> >>>
> >>> On 28/02/23 06:05, Simon Glass wrote:
> >>>> Hi Neha,
> >>>>
> >>>> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com>
> >>>> wrote:
> >>>>>
> >>>>> core-secdev-k3 is the TI security development package provided for K3
> >>>>> platform devices. This tool helps sign bootloader images with the x509
> >>>>> ceritificate header.
> >>>>>
> >>>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> >>>>> ---
> >>>>> This patch depends on
> >>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
> >>>>> and on ongoing development in
> >>>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> >>>>>
> >>>>>    tools/binman/btool/tisecuretool.py            |  72 +++++++++++
> >>>>>    tools/binman/etype/ti_secure.py               | 114
> >>>>> ++++++++++++++++++
> >>>>>    tools/binman/ftest.py                         |  36 ++++++
> >>>>>    tools/binman/test/278_ti_secure_rom.dts       |  11 ++
> >>>>>    tools/binman/test/279_ti_secure.dts           |  11 ++
> >>>>>    .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
> >>>>>    tools/binman/test/281_ti_secure_combined.dts  |  12 ++
> >>>>>    7 files changed, 266 insertions(+)
> >>>>>    create mode 100644 tools/binman/btool/tisecuretool.py
> >>>>>    create mode 100644 tools/binman/etype/ti_secure.py
> >>>>>    create mode 100644 tools/binman/test/278_ti_secure_rom.dts
> >>>>>    create mode 100644 tools/binman/test/279_ti_secure.dts
> >>>>>    create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
> >>>>>    create mode 100644 tools/binman/test/281_ti_secure_combined.dts
> >>>>
> >>>> Now that I see what you are doing, this it not quite the right way.
> >>>>
> >>>> See this hack-up of how you can call the openssl thing. Basically you
> >>>> should not have a shell script in the way, but instead make your
> >>>> bintool do it.
> >>>>
> >>>> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
> >>>>
> >>>> I suppose we can have an openssl bintool that others build on top of?
> >>>>
> >>>> Regards,
> >>>> Simon
> >>>>
> >>>>
> >>>
> >>> That is possible, maybe ti-secure extends from x509_cert etype so as to
> >>> support the TI specific certificate extensions. I'll start working on
> >>> this.
> >>>
> >>> However the patches I've sent support external production flow where
> >>> signing need not necessarily be carried out from U-Boot. An external
> >>> repo that acts as what is core-secdev-k3 here, would be the repo
> >>> responsible for signing.
> >>>
> >>> Could we find a way to combine both so as to support production flow
> >>> mandating an external agency, as well as a completely within U-Boot flow
> >>> using bintool? i.e. we modify ti-secure etype to be able to support both
> >>> external git repo/executable script signing as well as default signing
> >>> using openssl bintool.
> >>
> >> Yes that seems important.
> >>
> >> One option is to have binman emit some instructions on how to sign the
> >> image, perhaps a simple data format similar to an fdtmap, with a basic
> >> shell script plus whatever the etype provides. Then the signer can
> >> follow the instructions or run the script.
> >>
> >> Another option is to run binman on the signer and have it do the
> >> signing. Would that work?
> >>
> >
> > I'd like to keep the dependencies needed on the signing server as
> > minimal as possible. We do require the "u-boot-tools" package on
> > the key servers if folks want to re-package signed FIT images, so
> > if binman could be reasonably packaged in that package someday
> > it could be an option.

At present binman is packaged as binary-manager (with 'pip install').
Is that good enough? We could add it to u-boot-tools, perhaps, but I
have found that packing Python things together with non-Python things
seems to be tricky. We gave up with libfdt and ended up using a
separate package for pylibfdt.

> >
> > For now, if binman could generate the x509 file and leave it with the
> > other boot artifacts then if one needs to re-sign with real keys they
> > will only need to have openssl sign that cert and append to the image
> > on the key server, that is simple enough document.

I sent a patch to add x509 cert using the openssl tool[1]. If there is
more needed, can you take a look or let me know what is missing?

> >
> > The more difficult part is in re-packaging these signed images.
> > Our security tools already have a tool to disasemble a FIT image,
> > sign the components, then re-assemble it[0]. This would work for
> > u-boot.img and the kernel FIT images, but we would need something
> > new for re-assembling boot3.bin and tispl.bin. The first boot files
> > (boot3.bin) is also more complex that simple FIT images in that it
> > has several variations in format and content based on device family
> > and type.
> >
> > The ideal case would be we do not need to pull in the TI security tools
> > package at all and we could drop this patch. The tools would then only be
> > needed by folks wanting to sign their images using an external key
> > server.

Yes, that seems to be the bare minimum of what we should reach here.
Any tools we need should be available in 'binman tool -l' and be
fetchable / buildable with binman. It is such a pain for users if
everyone does their own thing. It is even more difficult for people
who develop U-Boot generally, since they just want to build and run.
For example, in my lab I want to be able to build U-Boot in a generic
way and have it boot on a board.

> >
> > If we could have binman learn how to generate/template x509 certs and
> > have it sign them with openssl, plus add the TI dummy[1] and degenerate[2]
> > keys to the u-boot source repo, then we would not need TI security
> > tools any more here.

OK, see the patch.

> >
> > This might be a longer term goal though, and I think we are already
> > trying to do too much all at once as is. Perhaps we could take
> > this current solution posted here with the intent to remove it in
> > the near future. Thoughts?

IMO the problem here is starting with shell scripts. They are OK for
hacking but we are trying to use a data-driven image description,
rather than having a lot of shell scripts.

It took ages to remove the SPL_FIT_GENERATOR stuff

> >
> > Andrew
> >
> > [0]
> > https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/scripts/fit-image-secure.sh
> > [1]
> > https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/custMpk.pem
> > [2]
> > https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/ti-degenerate-key.pem
> >
>
> I think leaving the boot artifacts while going with the current solution
> would be a better option. As Andrew mentioned, the complexity mainly
> comes with the way our tiboot3.bin is packaged. It seems cleaner for now
> to go with this since this patch series is only a part in the larger
> series of packaging all the bootloader images.
>
>   I will take future action to eventually completely remove using TI
> security tools. What do you think, Simon?

I am not convinced. We still have SPL_FIT_GENERATOR after 5 years.

Can we not just do this work now? I am happy to help on the binman
side, but I have not heard back on the series I sent, so I am not sure
what is missing.

[..]
Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230303000246.2640473-4-sjg@chromium.org/
Neha Malcom Francis March 14, 2023, 3:38 a.m. UTC | #7 
Hi Simon

On 11/03/23 02:19, Simon Glass wrote:
> Hi,
> 
> On Thu, 9 Mar 2023 at 21:35, Neha Malcom Francis <n-francis@ti.com> wrote:
>>
>> Hi Andrew, Simon
>>
>> On 01/03/23 22:41, Andrew Davis wrote:
>>> On 2/28/23 9:10 AM, Simon Glass wrote:
>>>> Hi Neha,
>>>>
>>>> On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com>
>>>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 28/02/23 06:05, Simon Glass wrote:
>>>>>> Hi Neha,
>>>>>>
>>>>>> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> core-secdev-k3 is the TI security development package provided for K3
>>>>>>> platform devices. This tool helps sign bootloader images with the x509
>>>>>>> ceritificate header.
>>>>>>>
>>>>>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>>>>>>> ---
>>>>>>> This patch depends on
>>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
>>>>>>> and on ongoing development in
>>>>>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3
>>>>>>>
>>>>>>>     tools/binman/btool/tisecuretool.py            |  72 +++++++++++
>>>>>>>     tools/binman/etype/ti_secure.py               | 114
>>>>>>> ++++++++++++++++++
>>>>>>>     tools/binman/ftest.py                         |  36 ++++++
>>>>>>>     tools/binman/test/278_ti_secure_rom.dts       |  11 ++
>>>>>>>     tools/binman/test/279_ti_secure.dts           |  11 ++
>>>>>>>     .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
>>>>>>>     tools/binman/test/281_ti_secure_combined.dts  |  12 ++
>>>>>>>     7 files changed, 266 insertions(+)
>>>>>>>     create mode 100644 tools/binman/btool/tisecuretool.py
>>>>>>>     create mode 100644 tools/binman/etype/ti_secure.py
>>>>>>>     create mode 100644 tools/binman/test/278_ti_secure_rom.dts
>>>>>>>     create mode 100644 tools/binman/test/279_ti_secure.dts
>>>>>>>     create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
>>>>>>>     create mode 100644 tools/binman/test/281_ti_secure_combined.dts
>>>>>>
>>>>>> Now that I see what you are doing, this it not quite the right way.
>>>>>>
>>>>>> See this hack-up of how you can call the openssl thing. Basically you
>>>>>> should not have a shell script in the way, but instead make your
>>>>>> bintool do it.
>>>>>>
>>>>>> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
>>>>>>
>>>>>> I suppose we can have an openssl bintool that others build on top of?
>>>>>>
>>>>>> Regards,
>>>>>> Simon
>>>>>>
>>>>>>
>>>>>
>>>>> That is possible, maybe ti-secure extends from x509_cert etype so as to
>>>>> support the TI specific certificate extensions. I'll start working on
>>>>> this.
>>>>>
>>>>> However the patches I've sent support external production flow where
>>>>> signing need not necessarily be carried out from U-Boot. An external
>>>>> repo that acts as what is core-secdev-k3 here, would be the repo
>>>>> responsible for signing.
>>>>>
>>>>> Could we find a way to combine both so as to support production flow
>>>>> mandating an external agency, as well as a completely within U-Boot flow
>>>>> using bintool? i.e. we modify ti-secure etype to be able to support both
>>>>> external git repo/executable script signing as well as default signing
>>>>> using openssl bintool.
>>>>
>>>> Yes that seems important.
>>>>
>>>> One option is to have binman emit some instructions on how to sign the
>>>> image, perhaps a simple data format similar to an fdtmap, with a basic
>>>> shell script plus whatever the etype provides. Then the signer can
>>>> follow the instructions or run the script.
>>>>
>>>> Another option is to run binman on the signer and have it do the
>>>> signing. Would that work?
>>>>
>>>
>>> I'd like to keep the dependencies needed on the signing server as
>>> minimal as possible. We do require the "u-boot-tools" package on
>>> the key servers if folks want to re-package signed FIT images, so
>>> if binman could be reasonably packaged in that package someday
>>> it could be an option.
> 
> At present binman is packaged as binary-manager (with 'pip install').
> Is that good enough? We could add it to u-boot-tools, perhaps, but I
> have found that packing Python things together with non-Python things
> seems to be tricky. We gave up with libfdt and ended up using a
> separate package for pylibfdt.
> 
>>>
>>> For now, if binman could generate the x509 file and leave it with the
>>> other boot artifacts then if one needs to re-sign with real keys they
>>> will only need to have openssl sign that cert and append to the image
>>> on the key server, that is simple enough document.
> 
> I sent a patch to add x509 cert using the openssl tool[1]. If there is
> more needed, can you take a look or let me know what is missing?
> 

I had looked at the patch and I think it should support custom 
certificates as well. I think an x509 etype should be able to produce a 
general certificate like what you have done, as well as have the 
capability to add more extensions, change parameters etc. I am thinking 
of a template and dict sort of method where you pass your custom 
template as well as a dict/config file containing 
parameter:parameter-valye pairs for the etype to fill in, does that 
sound okay? Then in use cases like ours where there's more done we could 
extend this etype and use that parse-and-replace mechanism for our 
certificate.

>>>
>>> The more difficult part is in re-packaging these signed images.
>>> Our security tools already have a tool to disasemble a FIT image,
>>> sign the components, then re-assemble it[0]. This would work for
>>> u-boot.img and the kernel FIT images, but we would need something
>>> new for re-assembling boot3.bin and tispl.bin. The first boot files
>>> (boot3.bin) is also more complex that simple FIT images in that it
>>> has several variations in format and content based on device family
>>> and type.
>>>
>>> The ideal case would be we do not need to pull in the TI security tools
>>> package at all and we could drop this patch. The tools would then only be
>>> needed by folks wanting to sign their images using an external key
>>> server.
> 
> Yes, that seems to be the bare minimum of what we should reach here.
> Any tools we need should be available in 'binman tool -l' and be
> fetchable / buildable with binman. It is such a pain for users if
> everyone does their own thing. It is even more difficult for people
> who develop U-Boot generally, since they just want to build and run.
> For example, in my lab I want to be able to build U-Boot in a generic
> way and have it boot on a board.
> 
>>>
>>> If we could have binman learn how to generate/template x509 certs and
>>> have it sign them with openssl, plus add the TI dummy[1] and degenerate[2]
>>> keys to the u-boot source repo, then we would not need TI security
>>> tools any more here.
> 
> OK, see the patch.
> 
>>>
>>> This might be a longer term goal though, and I think we are already
>>> trying to do too much all at once as is. Perhaps we could take
>>> this current solution posted here with the intent to remove it in
>>> the near future. Thoughts?
> 
> IMO the problem here is starting with shell scripts. They are OK for
> hacking but we are trying to use a data-driven image description,
> rather than having a lot of shell scripts.
> 
> It took ages to remove the SPL_FIT_GENERATOR stuff
> 
>>>
>>> Andrew
>>>
>>> [0]
>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/scripts/fit-image-secure.sh
>>> [1]
>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/custMpk.pem
>>> [2]
>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/ti-degenerate-key.pem
>>>
>>
>> I think leaving the boot artifacts while going with the current solution
>> would be a better option. As Andrew mentioned, the complexity mainly
>> comes with the way our tiboot3.bin is packaged. It seems cleaner for now
>> to go with this since this patch series is only a part in the larger
>> series of packaging all the bootloader images.
>>
>>    I will take future action to eventually completely remove using TI
>> security tools. What do you think, Simon?
> 
> I am not convinced. We still have SPL_FIT_GENERATOR after 5 years.
> 
> Can we not just do this work now? I am happy to help on the binman
> side, but I have not heard back on the series I sent, so I am not sure
> what is missing.
> 
> [..]
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230303000246.2640473-4-sjg@chromium.org/
Simon Glass March 14, 2023, 11:05 p.m. UTC | #8 
Hi Neha,

On Mon, 13 Mar 2023 at 21:38, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Simon
>
> On 11/03/23 02:19, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 9 Mar 2023 at 21:35, Neha Malcom Francis <n-francis@ti.com> wrote:
> >>
> >> Hi Andrew, Simon
> >>
> >> On 01/03/23 22:41, Andrew Davis wrote:
> >>> On 2/28/23 9:10 AM, Simon Glass wrote:
> >>>> Hi Neha,
> >>>>
> >>>> On Tue, 28 Feb 2023 at 02:50, Neha Malcom Francis <n-francis@ti.com>
> >>>> wrote:
> >>>>>
> >>>>> Hi Simon,
> >>>>>
> >>>>> On 28/02/23 06:05, Simon Glass wrote:
> >>>>>> Hi Neha,
> >>>>>>
> >>>>>> On Fri, 24 Feb 2023 at 05:03, Neha Malcom Francis <n-francis@ti.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> core-secdev-k3 is the TI security development package provided for K3
> >>>>>>> platform devices. This tool helps sign bootloader images with the x509
> >>>>>>> ceritificate header.
> >>>>>>>
> >>>>>>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> >>>>>>> ---
> >>>>>>> This patch depends on
> >>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230224115101.563729-1-n-francis@ti.com/
> >>>>>>> and on ongoing development in
> >>>>>>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> >>>>>>>
> >>>>>>>     tools/binman/btool/tisecuretool.py            |  72 +++++++++++
> >>>>>>>     tools/binman/etype/ti_secure.py               | 114
> >>>>>>> ++++++++++++++++++
> >>>>>>>     tools/binman/ftest.py                         |  36 ++++++
> >>>>>>>     tools/binman/test/278_ti_secure_rom.dts       |  11 ++
> >>>>>>>     tools/binman/test/279_ti_secure.dts           |  11 ++
> >>>>>>>     .../binman/test/280_ti_secure_nofilename.dts  |  10 ++
> >>>>>>>     tools/binman/test/281_ti_secure_combined.dts  |  12 ++
> >>>>>>>     7 files changed, 266 insertions(+)
> >>>>>>>     create mode 100644 tools/binman/btool/tisecuretool.py
> >>>>>>>     create mode 100644 tools/binman/etype/ti_secure.py
> >>>>>>>     create mode 100644 tools/binman/test/278_ti_secure_rom.dts
> >>>>>>>     create mode 100644 tools/binman/test/279_ti_secure.dts
> >>>>>>>     create mode 100644 tools/binman/test/280_ti_secure_nofilename.dts
> >>>>>>>     create mode 100644 tools/binman/test/281_ti_secure_combined.dts
> >>>>>>
> >>>>>> Now that I see what you are doing, this it not quite the right way.
> >>>>>>
> >>>>>> See this hack-up of how you can call the openssl thing. Basically you
> >>>>>> should not have a shell script in the way, but instead make your
> >>>>>> bintool do it.
> >>>>>>
> >>>>>> https://github.com/sjg20/u-boot/commit/03c0d74f81106570b18d8e4fe7a3355bfeb0d5da#r100378804
> >>>>>>
> >>>>>> I suppose we can have an openssl bintool that others build on top of?
> >>>>>>
> >>>>>> Regards,
> >>>>>> Simon
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> That is possible, maybe ti-secure extends from x509_cert etype so as to
> >>>>> support the TI specific certificate extensions. I'll start working on
> >>>>> this.
> >>>>>
> >>>>> However the patches I've sent support external production flow where
> >>>>> signing need not necessarily be carried out from U-Boot. An external
> >>>>> repo that acts as what is core-secdev-k3 here, would be the repo
> >>>>> responsible for signing.
> >>>>>
> >>>>> Could we find a way to combine both so as to support production flow
> >>>>> mandating an external agency, as well as a completely within U-Boot flow
> >>>>> using bintool? i.e. we modify ti-secure etype to be able to support both
> >>>>> external git repo/executable script signing as well as default signing
> >>>>> using openssl bintool.
> >>>>
> >>>> Yes that seems important.
> >>>>
> >>>> One option is to have binman emit some instructions on how to sign the
> >>>> image, perhaps a simple data format similar to an fdtmap, with a basic
> >>>> shell script plus whatever the etype provides. Then the signer can
> >>>> follow the instructions or run the script.
> >>>>
> >>>> Another option is to run binman on the signer and have it do the
> >>>> signing. Would that work?
> >>>>
> >>>
> >>> I'd like to keep the dependencies needed on the signing server as
> >>> minimal as possible. We do require the "u-boot-tools" package on
> >>> the key servers if folks want to re-package signed FIT images, so
> >>> if binman could be reasonably packaged in that package someday
> >>> it could be an option.
> >
> > At present binman is packaged as binary-manager (with 'pip install').
> > Is that good enough? We could add it to u-boot-tools, perhaps, but I
> > have found that packing Python things together with non-Python things
> > seems to be tricky. We gave up with libfdt and ended up using a
> > separate package for pylibfdt.
> >
> >>>
> >>> For now, if binman could generate the x509 file and leave it with the
> >>> other boot artifacts then if one needs to re-sign with real keys they
> >>> will only need to have openssl sign that cert and append to the image
> >>> on the key server, that is simple enough document.
> >
> > I sent a patch to add x509 cert using the openssl tool[1]. If there is
> > more needed, can you take a look or let me know what is missing?
> >
>
> I had looked at the patch and I think it should support custom
> certificates as well. I think an x509 etype should be able to produce a
> general certificate like what you have done, as well as have the
> capability to add more extensions, change parameters etc. I am thinking
> of a template and dict sort of method where you pass your custom
> template as well as a dict/config file containing
> parameter:parameter-valye pairs for the etype to fill in, does that
> sound okay? Then in use cases like ours where there's more done we could

Yes that seems OK to me. I do like the idea of have specific methods
in the bintool class which do useful things. But if the amount of
diversity is too large, then a template/dict can provide flexibility.
Of course it means the caller needs to understand the format, but I
suppose that is fair enough.

Still, if we have a few specific use cases, then I would prefer a
method for each, rather than a generic function called with lots of
args / dict.

> extend this etype and use that parse-and-replace mechanism for our
> certificate.

OK.

Regards,
Simon

>
> >>>
> >>> The more difficult part is in re-packaging these signed images.
> >>> Our security tools already have a tool to disasemble a FIT image,
> >>> sign the components, then re-assemble it[0]. This would work for
> >>> u-boot.img and the kernel FIT images, but we would need something
> >>> new for re-assembling boot3.bin and tispl.bin. The first boot files
> >>> (boot3.bin) is also more complex that simple FIT images in that it
> >>> has several variations in format and content based on device family
> >>> and type.
> >>>
> >>> The ideal case would be we do not need to pull in the TI security tools
> >>> package at all and we could drop this patch. The tools would then only be
> >>> needed by folks wanting to sign their images using an external key
> >>> server.
> >
> > Yes, that seems to be the bare minimum of what we should reach here.
> > Any tools we need should be available in 'binman tool -l' and be
> > fetchable / buildable with binman. It is such a pain for users if
> > everyone does their own thing. It is even more difficult for people
> > who develop U-Boot generally, since they just want to build and run.
> > For example, in my lab I want to be able to build U-Boot in a generic
> > way and have it boot on a board.
> >
> >>>
> >>> If we could have binman learn how to generate/template x509 certs and
> >>> have it sign them with openssl, plus add the TI dummy[1] and degenerate[2]
> >>> keys to the u-boot source repo, then we would not need TI security
> >>> tools any more here.
> >
> > OK, see the patch.
> >
> >>>
> >>> This might be a longer term goal though, and I think we are already
> >>> trying to do too much all at once as is. Perhaps we could take
> >>> this current solution posted here with the intent to remove it in
> >>> the near future. Thoughts?
> >
> > IMO the problem here is starting with shell scripts. They are OK for
> > hacking but we are trying to use a data-driven image description,
> > rather than having a lot of shell scripts.
> >
> > It took ages to remove the SPL_FIT_GENERATOR stuff
> >
> >>>
> >>> Andrew
> >>>
> >>> [0]
> >>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/scripts/fit-image-secure.sh
> >>> [1]
> >>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/custMpk.pem
> >>> [2]
> >>> https://git.ti.com/cgit/security-development-tools/core-secdev-k3/tree/keys/ti-degenerate-key.pem
> >>>
> >>
> >> I think leaving the boot artifacts while going with the current solution
> >> would be a better option. As Andrew mentioned, the complexity mainly
> >> comes with the way our tiboot3.bin is packaged. It seems cleaner for now
> >> to go with this since this patch series is only a part in the larger
> >> series of packaging all the bootloader images.
> >>
> >>    I will take future action to eventually completely remove using TI
> >> security tools. What do you think, Simon?
> >
> > I am not convinced. We still have SPL_FIT_GENERATOR after 5 years.
> >
> > Can we not just do this work now? I am happy to help on the binman
> > side, but I have not heard back on the series I sent, so I am not sure
> > what is missing.
> >
> > [..]
> > Regards,
> > Simon
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/patch/20230303000246.2640473-4-sjg@chromium.org/
>
> --
> Thanking You
> Neha Malcom Francis
diff mbox series

Patch

diff --git a/tools/binman/btool/tisecuretool.py b/tools/binman/btool/tisecuretool.py
new file mode 100644
index 0000000000..5102bb1f7d
--- /dev/null
+++ b/tools/binman/btool/tisecuretool.py
@@ -0,0 +1,72 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
+# Written by Neha Malcom Francis <n-francis@ti.com>
+#
+"""Bintool implementation for TI security development tools
+
+tisecuretool helps add x509 certification for bootloader images for K3 platform devices
+
+Source code:
+https://git.ti.com/cgit/security-development-tools/core-secdev-k3/"""
+
+import os
+
+from binman import bintool
+from patman import tout
+from patman import tools
+
+class Bintooltisecuretool(bintool.Bintool):
+    """Signing tool for TI bootloaders"""
+    name = 'tisecuretool'
+    def __init__(self, name):
+        super().__init__(name, 'TI secure tool')
+
+    def sign_binary_secure(self, fname, out_fname):
+        """Create a signed binary
+
+        Args:
+            fname (str): Filename to sign
+            out_fname (str): Output filename
+
+        Returns:
+            str: Tool output
+            or None
+        """
+        tool_path = self.get_path()
+        script_path = os.path.join(tool_path, 'scripts/secure-binary-image.sh')
+        args = [
+            'sh',
+            script_path,
+            fname,
+            out_fname
+            ]
+        output = self.run_cmd(*args, add_name=False)
+        return output
+
+    def sign_binary_rom(self, args):
+        """Create a signed binary that is booted by ROM
+
+        Args:
+            fname (str): Filename to sign
+            out_fname (str): Output filename"""
+        tool_path = self.get_path()
+        script_path = os.path.join(tool_path, 'scripts/secure-rom-boot-image.sh')
+        #args.insert(0, script_path)
+        args.insert(0, script_path)
+        output = self.run_cmd(*args, add_name=False)
+        return output
+
+    def fetch(self, method):
+        """Fetch handler for TI secure tool
+
+        This builds the tool from source
+
+        Returns:
+            True if the file was fetched, None if a method other than FETCH_SOURCE
+            was requested
+        """
+        if method != bintool.FETCH_SOURCE:
+            return None
+        result = self.fetch_from_git(
+            'git://git.ti.com/security-development-tools/core-secdev-k3.git', 'tisecuretool')
+        return result
diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py
new file mode 100644
index 0000000000..26f81ff8e8
--- /dev/null
+++ b/tools/binman/etype/ti_secure.py
@@ -0,0 +1,114 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/
+# Written by Neha Malcom Francis <n-francis@ti.com>
+#
+# Entry-type module for signed binaries for TI K3 platform
+#
+
+from binman.etype.blob import Entry_blob
+from binman import bintool
+
+from dtoc import fdt_util
+from patman import terminal
+from patman import tools
+from patman import tout
+
+class Entry_ti_secure(Entry_blob):
+    """An entry which contains a signed x509 binary for signing TI
+    General Purpose as well as High-Security devices.
+
+    Properties / Entry arguments:
+	- filename: filename of binary file to be secured
+
+    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.core = fdt_util.GetString(self._node, 'core', 'secure')
+        self.load_addr = fdt_util.GetInt(self._node, 'load', 0x41c00000)
+        self.sw_rev = fdt_util.GetInt(self._node, 'sw-rev', 1)
+        self.sysfw_cert = fdt_util.GetBool(self._node, 'sysfw-cert', False)
+        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', 0x40000)
+        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', 0x7f000)
+        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', 0x41c80000)
+        self.sysfw_inner_cert_filename = fdt_util.GetString(self._node, 'sysfw-inner-cert-filename')
+        self.toolpresent = False
+        if not self.filename:
+            self.Raise("ti_secure must have a 'filename' property")
+
+    def _SignBinarySecure(self):
+        infilename = self.filename
+        outfilename = infilename + '_signed'
+
+        data = self.tisecuretool.sign_binary_secure(infilename, outfilename)
+
+        if data is None:
+            # Bintool is missing, create a zeroed binary
+            self.record_missing_bintool(self.tisecuretool)
+            return tools.get_bytes(0, 1024)
+
+        return tools.read_file(outfilename)
+
+    def _SignBinaryROM(self):
+        input_fname = self.filename
+        output_fname = input_fname + "_x509"
+        if self.combined:
+            args = [
+                '-b', input_fname,
+                '-l', hex(self.load_addr),
+                '-s', self.sysfw_filename,
+                '-m', hex(self.sysfw_load_addr),
+                '-a', self.sysfw_inner_cert,
+                '-d', self.sysfw_data_filename,
+                '-n', hex(self.sysfw_data_load_addr),
+                '-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 = [
+                '-a', self.core,
+                '-b', input_fname,
+                '-o', output_fname,
+                '-l', hex(self.load_addr),
+                '-r', str(self.sw_rev),
+
+            ]
+            if self.sysfw_cert == True:
+                args.insert(0, '-u')
+
+        out = self.tisecuretool.sign_binary_rom(args)
+
+        if out is None:
+            # Bintool is missing, create a zeroed binary
+            self.record_missing_bintool(self.tisecuretool)
+            return tools.get_bytes(0, 1024)
+
+        return tools.read_file(output_fname)
+
+    def ProcessContents(self):
+        if self.secure:
+            data = self._SignBinarySecure()
+        else:
+            data = self._SignBinaryROM()
+        return self.ProcessContentsUpdate(data)
+
+    def AddBintools(self, btools):
+        #super().AddBintools(btools)
+        col = terminal.Color()
+        self.tisecuretool = self.AddBintool(btools, 'tisecuretool')
+        out = self.tisecuretool.fetch_tool(bintool.FETCH_SOURCE, col, True)
+        if out == bintool.FAIL:
+            # Bintool is missing, record missing
+            self.record_missing_bintool(self.tisecuretool)
\ No newline at end of file
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index bf902341c5..72cce0ef02 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -97,6 +97,7 @@  PRE_LOAD_MAGIC        = b'UBSH'
 PRE_LOAD_VERSION      = 0x11223344.to_bytes(4, 'big')
 PRE_LOAD_HDR_SIZE     = 0x00001000.to_bytes(4, 'big')
 TI_BOARD_CONFIG_DATA  = b'\x00\x01'
+TIX509DATA            = b'letsgo'
 
 # Subdirectory of the input dir to use to put test FDTs
 TEST_FDT_SUBDIR       = 'fdts'
@@ -206,6 +207,7 @@  class TestFunctional(unittest.TestCase):
         TestFunctional._MakeInputFile('bl2u.bin', ATF_BL2U_DATA)
         TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
         TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
+        TestFunctional._MakeInputFile('to_sign.bin', TIX509DATA)
 
         # Add a few .dtb files for testing
         TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
@@ -6398,5 +6400,39 @@  fdt         fdtmap                Extract the devicetree blob from the fdtmap
         configlen_noheader = TI_BOARD_CONFIG_DATA*4
         self.assertGreater(data, configlen_noheader)
 
+    def testTISecureROMImageMissingTool(self):
+        """Test that a TI secure signed ROM booted image can be generated although tisecuretool is missing"""
+        with test_util.capture_sys_output() as (_, stderr):
+            data = self._DoTestFile('278_ti_secure_rom.dts',
+                force_missing_bintools='tisecuretool')
+        err = stderr.getvalue()
+        self.assertRegex(err,
+                         "Image 'image'.*missing bintools.*: tisecuretool")
+
+    def testTISecureImageMissingTool(self):
+        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
+        with test_util.capture_sys_output() as (_, stderr):
+            data = self._DoTestFile('279_ti_secure.dts',
+                force_missing_bintools='tisecuretool')
+        err = stderr.getvalue()
+        self.assertRegex(err,
+                         "Image 'image'.*missing bintools.*: tisecuretool")
+
+    def testTISecureNoFilename(self):
+        """Test that a missing 'filename' property is detected"""
+        with self.assertRaises(ValueError) as e:
+            self._DoTestFile('280_ti_secure_nofilename.dts')
+        self.assertIn("ti_secure must have a 'filename' property",
+                      str(e.exception))
+
+    def testTISecureImageCombinedMissingTool(self):
+        """Test that a TI secure signed image can be generated although tisecuretool is missing"""
+        with test_util.capture_sys_output() as (_, stderr):
+            data = self._DoTestFile('281_ti_secure_combined.dts',
+                force_missing_bintools='tisecuretool')
+        err = stderr.getvalue()
+        self.assertRegex(err,
+                         "Image 'image'.*missing bintools.*: tisecuretool")
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/278_ti_secure_rom.dts b/tools/binman/test/278_ti_secure_rom.dts
new file mode 100644
index 0000000000..67a6a9de8d
--- /dev/null
+++ b/tools/binman/test/278_ti_secure_rom.dts
@@ -0,0 +1,11 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+			ti-secure {
+				filename = "to_sign.bin";
+				sysfw-cert;
+			};
+	};
+};
diff --git a/tools/binman/test/279_ti_secure.dts b/tools/binman/test/279_ti_secure.dts
new file mode 100644
index 0000000000..0e60984013
--- /dev/null
+++ b/tools/binman/test/279_ti_secure.dts
@@ -0,0 +1,11 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+			ti-secure {
+				filename = "to_sign.bin";
+				secure;
+			};
+	};
+};
diff --git a/tools/binman/test/280_ti_secure_nofilename.dts b/tools/binman/test/280_ti_secure_nofilename.dts
new file mode 100644
index 0000000000..928a50a499
--- /dev/null
+++ b/tools/binman/test/280_ti_secure_nofilename.dts
@@ -0,0 +1,10 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+			ti-secure {
+				secure;
+			};
+	};
+};
diff --git a/tools/binman/test/281_ti_secure_combined.dts b/tools/binman/test/281_ti_secure_combined.dts
new file mode 100644
index 0000000000..aadb5a4306
--- /dev/null
+++ b/tools/binman/test/281_ti_secure_combined.dts
@@ -0,0 +1,12 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+			ti-secure {
+				filename = "to_sign.bin";
+				combined;
+				split-dm;
+			};
+	};
+};