Re: [Buildroot] [PATCH v9 07/11] package/ti-core-secdev-k3: new package

[Andreas Dannenberg via buildroot <buildroot@buildroot.org>]

Hi Patrick,

On Sat, Jun 24, 2023 at 10:32:02AM +1000, Patrick Oppenlander wrote:
> On Sat, Jun 24, 2023 at 12:53 AM Andreas Dannenberg <dannenberg@ti.com> wrote:
> >
> > Hi Patrick,
> > thanks for the feedback. Comments inlined...
> >
> > On Fri, Jun 23, 2023 at 01:48:30PM +1000, Patrick Oppenlander wrote:
> > > On Fri, Jun 23, 2023 at 2:06 AM Andreas Dannenberg via buildroot
> > > <buildroot@buildroot.org> wrote:
> > > >
> > > > The ti-core-secdev-k3 package is used to provide binary image signing
> > > > tools and keys to the build process needed to build boot artifacts for
> > > > the secure boot flow on TI K3 platform "High Security" SoCs (device
> > > > variants "HS-FS" and "HS-SE"). This package is not needed building for
> > > > "General Purpose" ("GP") SoC variants.
> > > >
> > > > This commit also updates the ti-k3-image-gen, ti-k3-r5-loader, and uboot
> > > > packages which are all used as part of the boot flow of TI K3 platform
> > > > devices to make use of the ti-core-secdev-k3 package if enabled.
> > > >
> > > > Note that although the use of the underlying 'core-secdev-k3' tool
> > > > provided by TI is required to generate bootable images for HS-FS and
> > > > HS-SE device variants, the use of this Buildroot package itself should
> > > > remain optional, hence no hard dependencies are being established. The
> > > > reason is that a user often wants to provide their own signing tool
> > > > through the use of the TI_SECURE_DEV_PKG environmental variable set
> > > > outside Buildroot on their specific build machine, especially for HS-SE
> > > > device variants where the signing tool would contain the user's private
> > > > keys.
> > > >
> > > > https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> > > >
> > > > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > > > ---
> > > >  boot/ti-k3-image-gen/ti-k3-image-gen.mk       | 10 ++++++
> > > >  boot/ti-k3-r5-loader/ti-k3-r5-loader.mk       | 10 ++++++
> > > >  boot/uboot/uboot.mk                           | 10 ++++++
> > > >  package/Config.in.host                        |  1 +
> > > >  package/ti-core-secdev-k3/Config.in.host      | 11 +++++++
> > > >  .../ti-core-secdev-k3/ti-core-secdev-k3.hash  |  3 ++
> > > >  .../ti-core-secdev-k3/ti-core-secdev-k3.mk    | 31 +++++++++++++++++++
> > > >  7 files changed, 76 insertions(+)
> > > >  create mode 100644 package/ti-core-secdev-k3/Config.in.host
> > > >  create mode 100644 package/ti-core-secdev-k3/ti-core-secdev-k3.hash
> > > >  create mode 100644 package/ti-core-secdev-k3/ti-core-secdev-k3.mk
> > > >
> > > > diff --git a/boot/ti-k3-image-gen/ti-k3-image-gen.mk b/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> > > > index b624f93771..57202d6918 100644
> > > > --- a/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> > > > +++ b/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> > > > @@ -70,6 +70,16 @@ TI_K3_IMAGE_GEN_MAKE_OPTS = \
> > > >         O=$(@D)/tmp \
> > > >         BIN_DIR=$(@D)
> > > >
> > > > +ifneq ($(TI_CORE_SECDEV_K3_INSTALL_DIR),)
> > > > +# Only set TI_SECURE_DEV_PKG make option if not already defined in the
> > > > +# environment, thus allowing the user to unconditionally override this
> > > > +# setting with a custom location on their build machine containing their
> > > > +# private keys, etc.
> > > > +ifeq ($(TI_SECURE_DEV_PKG),)
> > > > +TI_K3_IMAGE_GEN_MAKE_OPTS += TI_SECURE_DEV_PKG=$(TI_CORE_SECDEV_K3_INSTALL_DIR)
> > > > +endif
> > > > +endif
> > > > +
> > > >  define TI_K3_IMAGE_GEN_BUILD_CMDS
> > > >         $(TI_K3_IMAGE_GEN_MAKE) -C $(@D) $(TI_K3_IMAGE_GEN_MAKE_OPTS)
> > > >  endef
> > > > diff --git a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > > > index 341888623e..8311e1b401 100644
> > > > --- a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > > > +++ b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > > > @@ -67,6 +67,16 @@ TI_K3_R5_LOADER_MAKE_OPTS = \
> > > >         HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \
> > > >         HOSTLDFLAGS="$(HOST_LDFLAGS)"
> > > >
> > > > +ifneq ($(TI_CORE_SECDEV_K3_INSTALL_DIR),)
> > > > +# Only set TI_SECURE_DEV_PKG make option if not already defined in the
> > > > +# environment, thus allowing the user to unconditionally override this
> > > > +# setting with a custom location on their build machine containing their
> > > > +# private keys, etc.
> > > > +ifeq ($(TI_SECURE_DEV_PKG),)
> > > > +TI_K3_R5_LOADER_MAKE_OPTS += TI_SECURE_DEV_PKG=$(TI_CORE_SECDEV_K3_INSTALL_DIR)
> > > > +endif
> > > > +endif
> > > > +
> > > >  define TI_K3_R5_LOADER_BUILD_CMDS
> > > >         $(TARGET_CONFIGURE_OPTS) $(TI_K3_R5_LOADER_MAKE) -C $(@D) $(TI_K3_R5_LOADER_MAKE_OPTS)
> > > >  endef
> > > > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > > > index 48af69bd26..472fec8619 100644
> > > > --- a/boot/uboot/uboot.mk
> > > > +++ b/boot/uboot/uboot.mk
> > > > @@ -184,6 +184,16 @@ UBOOT_DEPENDENCIES += optee-os
> > > >  UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf
> > > >  endif
> > > >
> > > > +ifneq ($(TI_CORE_SECDEV_K3_INSTALL_DIR),)
> > > > +# Only set TI_SECURE_DEV_PKG make option if not already defined in the
> > > > +# environment, thus allowing the user to unconditionally override this
> > > > +# setting with a custom location on their build machine containing their
> > > > +# private keys, etc.
> > > > +ifeq ($(TI_SECURE_DEV_PKG),)
> > > > +UBOOT_MAKE_OPTS += TI_SECURE_DEV_PKG=$(TI_CORE_SECDEV_K3_INSTALL_DIR)
> > > > +endif
> > > > +endif
> > > > +
> > > >  ifeq ($(BR2_TARGET_UBOOT_NEEDS_TI_K3_DM),y)
> > > >  # Currently supports the FW from Git tag 08.06.00.006 by default
> > > >  TI_K3_DM_VERSION = 340194800a581baf976360386dfc7b5acab8d948
> > > > diff --git a/package/Config.in.host b/package/Config.in.host
> > > > index dcadbfdfc1..eed39a4102 100644
> > > > --- a/package/Config.in.host
> > > > +++ b/package/Config.in.host
> > > > @@ -103,6 +103,7 @@ menu "Host utilities"
> > > >         source "package/systemd/Config.in.host"
> > > >         source "package/tegrarcm/Config.in.host"
> > > >         source "package/ti-cgt-pru/Config.in.host"
> > > > +       source "package/ti-core-secdev-k3/Config.in.host"
> > > >         source "package/uboot-tools/Config.in.host"
> > > >         source "package/util-linux/Config.in.host"
> > > >         source "package/utp_com/Config.in.host"
> > > > diff --git a/package/ti-core-secdev-k3/Config.in.host b/package/ti-core-secdev-k3/Config.in.host
> > > > new file mode 100644
> > > > index 0000000000..364619d824
> > > > --- /dev/null
> > > > +++ b/package/ti-core-secdev-k3/Config.in.host
> > > > @@ -0,0 +1,11 @@
> > > > +config BR2_PACKAGE_HOST_TI_CORE_SECDEV_K3
> > > > +       bool "host ti-core-secdev-k3"
> > > > +       help
> > > > +         ti-core-secdev-k3 is used to provide binary image signing
> > > > +         tools and keys to the build process needed to build boot
> > > > +         artifacts for the secure boot flow on TI K3 platform
> > > > +         "High Security" SoCs (device variants "HS-FS" and "HS-SE").
> > > > +         This package is not needed building for "General Purpose"
> > > > +         ("GP") SoC variants.
> > > > +
> > > > +         https://git.ti.com/cgit/security-development-tools/core-secdev-k3
> > > > diff --git a/package/ti-core-secdev-k3/ti-core-secdev-k3.hash b/package/ti-core-secdev-k3/ti-core-secdev-k3.hash
> > > > new file mode 100644
> > > > index 0000000000..526ed29514
> > > > --- /dev/null
> > > > +++ b/package/ti-core-secdev-k3/ti-core-secdev-k3.hash
> > > > @@ -0,0 +1,3 @@
> > > > +# Locally calculated
> > > > +sha256  eb637ed54204b64e98ae07070e0f2ebd36eed228ecc108dae0e7be6e38edde74  core-secdev-k3-08.06.00.007.tar.gz
> > > > +sha256  3e5cf4f5ab9f0333f46cd68fabede3f21e55de1a9e3c6ad673f241f4514d8369  manifest/k3-secdev-0.2-manifest.html
> > > > diff --git a/package/ti-core-secdev-k3/ti-core-secdev-k3.mk b/package/ti-core-secdev-k3/ti-core-secdev-k3.mk
> > > > new file mode 100644
> > > > index 0000000000..c388af2865
> > > > --- /dev/null
> > > > +++ b/package/ti-core-secdev-k3/ti-core-secdev-k3.mk
> > > > @@ -0,0 +1,31 @@
> > > > +################################################################################
> > > > +#
> > > > +# ti-core-secdev-k3
> > > > +#
> > > > +################################################################################
> > > > +
> > > > +TI_CORE_SECDEV_K3_VERSION = 08.06.00.007
> > > > +TI_CORE_SECDEV_K3_SITE = https://git.ti.com/cgit/security-development-tools/core-secdev-k3/snapshot
> > > > +TI_CORE_SECDEV_K3_LICENSE = TI TSPA License
> > > > +TI_CORE_SECDEV_K3_LICENSE_FILES = manifest/k3-secdev-0.2-manifest.html
> > > > +TI_CORE_SECDEV_K3_SOURCE = core-secdev-k3-$(TI_CORE_SECDEV_K3_VERSION).tar.gz
> > >
> > > Hi Andreas,
> > >
> > > Would it make sense to support user customisation of this via Kconfig
> > > instead of using the magic TI_SECURE_DEV_PKG environment variable?
> > >
> > > I'm slightly confused about how this is intended to work.
> > > core-secdev-k3 contains both infrastructure (scripts) and data (keys)
> > > to secure the image. It looks like someone tried to split these
> > > concerns by supporting customisation of the key location.
> > > secure-binary-image.sh expects TI_SECURE_DEV_PKG to point to something
> > > containing keys/custMpk.pem, but the rest of the process expects
> > > TI_SECURE_DEV_PKG to point to something containing scripts to secure
> > > the binary blobs.
> > >
> > > It would make more sense to me to support overriding just the key,
> > > rather than having to duplicate all the scripts too.
> > >
> > > Could we support configuring the location of the custMpk.pem key and
> > > have the build process replace the default key in
> > > build/host-ti-core-secdev-k3-08.06.00.007/keys?
> >
> > Well, this is how the "TI secure dev package" has been for the last 10+
> > years, and that's how it's being used by 100% of our customers today
> > doing Linux image builds. It's hard-wired into Yocto builds, even the
> > official U-Boot tree...
> >
> > https://source.denx.de/u-boot/u-boot/-/blob/master/doc/README.ti-secure
> 
> Thanks for the reference!
> 
> > I agree improvements could be made for example to point to just a
> > private key via Kconfig but this would also create an inconsistency with
> > all solutions currently in production, and with what folks are used to,
> > and go against enabling an easier and more seamless "switch" from Yocto
> > for example, which is what I'd like people to do.
> 
> If you look in the section in README.ti-secure about K3 secure devices
> it supports setting the key with the Kconfig option "K3_KEY", so it
> looks like we can support key customisation using the u-boot config
> and don't need to support this in the buildroot config.

Yes this option is available, but this in by itself won't be a complete
solution to customize the key. For example the key is also used as part
of ti-k3-image-gen, which is a wrapper around U-Boot SPL on R5 (which is
provided through ti-k3-r5-loader). So if you customize it in U-Boot
Kconfig you still need to somehow feed that same key into ti-k3-image-gen.
Furthermore, the key _should_ also be used to sign Kernel / DTBs /
Kernel FIT image (this part of the authenticated boot chain is not
implemented with this patch series, but will be added in a future step).

--
Andreas Dannenberg
Texas Instruments Inc

> Patrick
> 
> > This doesn't say it should not or never be improved, but I think such
> > changes could be managed more smoothly by establishing a known baseline
> > first (which is what this entire series intends to do, based on a
> > comparable variant of the official TI production SDK), and then over
> > time improve things, also as Yocto, U-Boot, and other projects evolve
> > (there's constant development on those including on the security front,
> > so what we have here will not be the "forever solution).
> >
> > > Is there a reason why a customer is expected to replace (or, more
> > > likely, duplicate) the scripts rather than just supply a key?
> >
> > The key is the most obvious thing, but often customers also want to
> > customize the certificate to add additional items for example to control
> > certain device features such as whether JTAG should be locked or
> > unlocked, etc., all things not readily controllable through some knobs
> > but needing to be done through customization of sripts / templates.
> >
> >
> > --
> > Andreas Dannenberg
> > Texas Instruments Inc
> >
> >
> > >
> > > Patrick
> > >
> > > > +# To allow the image signing process for various firmware artifacts to work the
> > > > +# build process for TI K3 platform HS-FS and HS-SE device variants is using the
> > > > +# 'core-secdev-k3' tool provided by TI. Its location must be made available to
> > > > +# the build process of dependent packages by exporting it through the use of an
> > > > +# environmental variable. In order to not pollute the global Buildroot
> > > > +# environment let's record the package's location and then define the actual
> > > > +# environmental variable needed for the build only in the packages that need it.
> > > > +TI_CORE_SECDEV_K3_INSTALL_DIR = $(HOST_DIR)/opt/ti-core-secdev-k3
> > > > +
> > > > +define HOST_TI_CORE_SECDEV_K3_INSTALL_CMDS
> > > > +       mkdir -p $(TI_CORE_SECDEV_K3_INSTALL_DIR)/keys
> > > > +       cp -dpfr $(@D)/keys/* $(TI_CORE_SECDEV_K3_INSTALL_DIR)/keys
> > > > +       mkdir -p $(TI_CORE_SECDEV_K3_INSTALL_DIR)/scripts
> > > > +       cp -dpfr $(@D)/scripts/* $(TI_CORE_SECDEV_K3_INSTALL_DIR)/scripts
> > > > +       mkdir -p $(TI_CORE_SECDEV_K3_INSTALL_DIR)/templates
> > > > +       cp -dpfr $(@D)/scripts/* $(TI_CORE_SECDEV_K3_INSTALL_DIR)/templates
> > > > +endef
> > > > +
> > > > +$(eval $(host-generic-package))
> > > > --
> > > > 2.34.1
> > > >
> > > > _______________________________________________
> > > > buildroot mailing list
> > > > buildroot@buildroot.org
> > > > https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot