[PATCH v4 0/4] sunxi: TOC0 image type support

[PATCH v4 0/4] sunxi: TOC0 image type support

[Samuel Holland]

This series adds support for the TOC0 image format used by the Allwinner
secure boot ROM (SBROM). This series has been tested on the following
SoCs/boards, with the eFuse burnt to enable secure mode:
 - A50: Ainol Q88 Tablet
 - A64: Pine A64 Plus
 - H5: Orange Pi Zero Plus
 - H6: Pine H64 Model B
 - H616: Orange Pi Zero 2

This time I also tested it on boards that are not switched to secure
mode (with A64, H3, and H5).

Due to both series changing Makefile.spl, the last patch depends on:
https://patchwork.ozlabs.org/project/uboot/list/?series=267136

Since this series no longer selects TOOLS_LIBCRYPTO anywhere, building
certain platforms/options may fail with an error like the following if
TOOLS_LIBCRYPTO is disabled:

    MKIMAGE spl/sunxi-spl.bin
  ./tools/mkimage: unsupported type Allwinner TOC0 Boot Image
  make[1]: *** [scripts/Makefile.spl:426: spl/sunxi-spl.bin] Error 1
  make: *** [Makefile:1982: spl/u-boot-spl] Error 2

Hopefully this is clear enough.

Changes in v4:
 - Do not select TOOLS_LIBCRYPTO anywhere

Changes in v3:
 - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
   as I can tell, using the suggestions from Pali Rohár)
 - Removed TOOLS_LIBCRYPTO selection for sunxi, since most boards
   do not need it
 - Added __packed to all new "ABI" structs
 - Added entry to MAINTAINERS for sunxi tools
 - Fixed offset of magic passed to memcmp
 - Refactored functions to not return pointers (fixes ambiguous NULL)

Changes in v2:
 - Refactored the first patch on top of TOOLS_LIBCRYPTO
 - Moved certificate and key item structures out of sunxi_image.h
 - Renamed "main" and "item" variables for clarity
 - Improved error messages, and added a hint about key generation
 - Added a comment explaining the purpose of the various key files
 - Mentioned testing this code on A50 in the commit message
 - Moved SPL header signature checks out of sunxi_image.h
 - Refactored SPL header signature checks to use fewer casts
 - Rebase on top of Icenowy's RISC-V support series
 - Rename Kconfig symbols to include the full image type name

Samuel Holland (4):
  tools: Separate image types which depend on OpenSSL
  tools: mkimage: Add Allwinner TOC0 support
  sunxi: Support SPL in both eGON and TOC0 images
  sunxi: Support building a SPL as a TOC0 image

 MAINTAINERS                           |   1 +
 arch/arm/include/asm/arch-sunxi/spl.h |   2 -
 arch/arm/mach-sunxi/Kconfig           |   2 +
 arch/arm/mach-sunxi/board.c           |  34 +-
 board/sunxi/Kconfig                   |  24 +
 common/image.c                        |   1 +
 include/image.h                       |   1 +
 include/sunxi_image.h                 |  37 ++
 scripts/Makefile.spl                  |   5 +-
 scripts/config_whitelist.txt          |   1 -
 tools/Makefile                        |  20 +-
 tools/mxsimage.c                      |   3 -
 tools/sunxi_toc0.c                    | 907 ++++++++++++++++++++++++++
 13 files changed, 1011 insertions(+), 27 deletions(-)
 create mode 100644 board/sunxi/Kconfig
 create mode 100644 tools/sunxi_toc0.c

-- 
2.32.0

[PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Samuel Holland]

Some image types (kwbimage and mxsimage) always depend on OpenSSL, so
they can only be included in mkimage when TOOLS_LIBCRYPTO is selected.
Use Makefile logic to conditionally link the files.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v4:
 - Do not select TOOLS_LIBCRYPTO anywhere

Changes in v3:
 - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
   as I can tell, using the suggestions from Pali Rohár)

Changes in v2:
 - Refactored the first patch on top of TOOLS_LIBCRYPTO

 scripts/config_whitelist.txt |  1 -
 tools/Makefile               | 19 +++++--------------
 tools/mxsimage.c             |  3 ---
 3 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index cd94b5777a..affae6875d 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE
 CONFIG_MXC_USB_FLAGS
 CONFIG_MXC_USB_PORT
 CONFIG_MXC_USB_PORTSC
-CONFIG_MXS
 CONFIG_MXS_AUART
 CONFIG_MXS_AUART_BASE
 CONFIG_MXS_OCOTP
diff --git a/tools/Makefile b/tools/Makefile
index 999fd46531..a9b3d982d8 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.
 AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
 					aes-encrypt.o aes-decrypt.o)
 
-# Cryptographic helpers that depend on openssl/libcrypto
-LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
-					fdt-libcrypto.o)
+# Cryptographic helpers and image types that depend on openssl/libcrypto
+LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
+			lib/fdt-libcrypto.o \
+			kwbimage.o \
+			mxsimage.o
 
 ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
 
@@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \
 			imximage.o \
 			imx8image.o \
 			imx8mimage.o \
-			kwbimage.o \
 			lib/md5.o \
 			lpc32xximage.o \
-			mxsimage.o \
 			omapimage.o \
 			os_support.o \
 			pblimage.o \
@@ -156,22 +156,13 @@ fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
 fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
 file2include-objs := file2include.o
 
-ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
-# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register
-# the mxsimage support within tools/mxsimage.c .
-HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS
-endif
-
 ifdef CONFIG_TOOLS_LIBCRYPTO
 # This affects include/image.h, but including the board config file
 # is tricky, so manually define this options here.
 HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE
 HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
 HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
-endif
 
-# MXSImage needs LibSSL
-ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
 HOSTCFLAGS_kwbimage.o += \
 	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
 HOSTLDLIBS_mkimage += \
diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..2bfbb421eb 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -5,8 +5,6 @@
  * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de>
  */
 
-#ifdef CONFIG_MXS
-
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
@@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE(
 	NULL,
 	mxsimage_generate
 );
-#endif
-- 
2.32.0

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Pali Rohár]

On Tuesday 19 October 2021 21:44:51 Samuel Holland wrote:
> Some image types (kwbimage and mxsimage) always depend on OpenSSL, so
> they can only be included in mkimage when TOOLS_LIBCRYPTO is selected.
> Use Makefile logic to conditionally link the files.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

NAK.

As explained in previous email [1], kwbimage is required for building
Kirkwood, Dove, A370, AXP, A375, A38x, A39x and MSYS platforms.
Therefore it cannot be disabled or hidden behind some user config
options for these platforms (and it does not matter if it is crypto
option or any other option). kwbimage must be unconditionally enabled on
these platforms like it was before this change, as it is crucial part of
build.

[1] - https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/

> ---
> 
> Changes in v4:
>  - Do not select TOOLS_LIBCRYPTO anywhere
> 
> Changes in v3:
>  - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
>    as I can tell, using the suggestions from Pali Rohár)
> 
> Changes in v2:
>  - Refactored the first patch on top of TOOLS_LIBCRYPTO
> 
>  scripts/config_whitelist.txt |  1 -
>  tools/Makefile               | 19 +++++--------------
>  tools/mxsimage.c             |  3 ---
>  3 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> index cd94b5777a..affae6875d 100644
> --- a/scripts/config_whitelist.txt
> +++ b/scripts/config_whitelist.txt
> @@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE
>  CONFIG_MXC_USB_FLAGS
>  CONFIG_MXC_USB_PORT
>  CONFIG_MXC_USB_PORTSC
> -CONFIG_MXS
>  CONFIG_MXS_AUART
>  CONFIG_MXS_AUART_BASE
>  CONFIG_MXS_OCOTP
> diff --git a/tools/Makefile b/tools/Makefile
> index 999fd46531..a9b3d982d8 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.
>  AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
>  					aes-encrypt.o aes-decrypt.o)
>  
> -# Cryptographic helpers that depend on openssl/libcrypto
> -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
> -					fdt-libcrypto.o)
> +# Cryptographic helpers and image types that depend on openssl/libcrypto
> +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
> +			lib/fdt-libcrypto.o \
> +			kwbimage.o \
> +			mxsimage.o
>  
>  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
>  
> @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \
>  			imximage.o \
>  			imx8image.o \
>  			imx8mimage.o \
> -			kwbimage.o \
>  			lib/md5.o \
>  			lpc32xximage.o \
> -			mxsimage.o \
>  			omapimage.o \
>  			os_support.o \
>  			pblimage.o \
> @@ -156,22 +156,13 @@ fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
>  fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
>  file2include-objs := file2include.o
>  
> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
> -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register
> -# the mxsimage support within tools/mxsimage.c .
> -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS
> -endif
> -
>  ifdef CONFIG_TOOLS_LIBCRYPTO
>  # This affects include/image.h, but including the board config file
>  # is tricky, so manually define this options here.
>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE
>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
> -endif
>  
> -# MXSImage needs LibSSL
> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
>  HOSTCFLAGS_kwbimage.o += \
>  	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
>  HOSTLDLIBS_mkimage += \
> diff --git a/tools/mxsimage.c b/tools/mxsimage.c
> index 002f4b525a..2bfbb421eb 100644
> --- a/tools/mxsimage.c
> +++ b/tools/mxsimage.c
> @@ -5,8 +5,6 @@
>   * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de>
>   */
>  
> -#ifdef CONFIG_MXS
> -
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <stdio.h>
> @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE(
>  	NULL,
>  	mxsimage_generate
>  );
> -#endif
> -- 
> 2.32.0
> 

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Andre Przywara]

On Wed, 20 Oct 2021 09:29:25 +0200
Pali Rohár <pali@kernel.org> wrote:

Hi,

> On Tuesday 19 October 2021 21:44:51 Samuel Holland wrote:
> > Some image types (kwbimage and mxsimage) always depend on OpenSSL, so
> > they can only be included in mkimage when TOOLS_LIBCRYPTO is selected.
> > Use Makefile logic to conditionally link the files.
> > 
> > Signed-off-by: Samuel Holland <samuel@sholland.org>  
> 
> NAK.
> 
> As explained in previous email [1], kwbimage is required for building
> Kirkwood, Dove, A370, AXP, A375, A38x, A39x and MSYS platforms.
> Therefore it cannot be disabled or hidden behind some user config
> options for these platforms (and it does not matter if it is crypto
> option or any other option).

So somehow we need to find a solution between your view and Alex' view of
things.
First: Pali, do you see any actual problem at the moment? TOOLS_LIBCRYPTO
defaults to y, so if I am not mistaken that means that a user would need
to deliberately turn that off to trigger build errors?
And also: the situation with this patch is not worse as before, isn't it?

So if it's just the missing improvement that you are concerned about, I am
not sure that should block this patch? I mean you could always propose
your own version of that missing piece, to improve the situation for the
boards you care about? And we should have this discussion there?

As it stands right now, this patch just improves things (just not
*everything*), and it is a prerequisite for the rest of the series
(unrelated to your problems), so I would like to go ahead on this one.

Cheers,
Andre.

> kwbimage must be unconditionally enabled on
> these platforms like it was before this change, as it is crucial part of
> build.
> 
> [1] - https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/
> 
> > ---
> > 
> > Changes in v4:
> >  - Do not select TOOLS_LIBCRYPTO anywhere
> > 
> > Changes in v3:
> >  - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
> >    as I can tell, using the suggestions from Pali Rohár)
> > 
> > Changes in v2:
> >  - Refactored the first patch on top of TOOLS_LIBCRYPTO
> > 
> >  scripts/config_whitelist.txt |  1 -
> >  tools/Makefile               | 19 +++++--------------
> >  tools/mxsimage.c             |  3 ---
> >  3 files changed, 5 insertions(+), 18 deletions(-)
> > 
> > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> > index cd94b5777a..affae6875d 100644
> > --- a/scripts/config_whitelist.txt
> > +++ b/scripts/config_whitelist.txt
> > @@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE
> >  CONFIG_MXC_USB_FLAGS
> >  CONFIG_MXC_USB_PORT
> >  CONFIG_MXC_USB_PORTSC
> > -CONFIG_MXS
> >  CONFIG_MXS_AUART
> >  CONFIG_MXS_AUART_BASE
> >  CONFIG_MXS_OCOTP
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 999fd46531..a9b3d982d8 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.
> >  AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
> >  					aes-encrypt.o aes-decrypt.o)
> >  
> > -# Cryptographic helpers that depend on openssl/libcrypto
> > -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
> > -					fdt-libcrypto.o)
> > +# Cryptographic helpers and image types that depend on openssl/libcrypto
> > +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
> > +			lib/fdt-libcrypto.o \
> > +			kwbimage.o \
> > +			mxsimage.o
> >  
> >  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
> >  
> > @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \
> >  			imximage.o \
> >  			imx8image.o \
> >  			imx8mimage.o \
> > -			kwbimage.o \
> >  			lib/md5.o \
> >  			lpc32xximage.o \
> > -			mxsimage.o \
> >  			omapimage.o \
> >  			os_support.o \
> >  			pblimage.o \
> > @@ -156,22 +156,13 @@ fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
> >  fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
> >  file2include-objs := file2include.o
> >  
> > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
> > -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register
> > -# the mxsimage support within tools/mxsimage.c .
> > -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS
> > -endif
> > -
> >  ifdef CONFIG_TOOLS_LIBCRYPTO
> >  # This affects include/image.h, but including the board config file
> >  # is tricky, so manually define this options here.
> >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE
> >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
> >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
> > -endif
> >  
> > -# MXSImage needs LibSSL
> > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
> >  HOSTCFLAGS_kwbimage.o += \
> >  	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
> >  HOSTLDLIBS_mkimage += \
> > diff --git a/tools/mxsimage.c b/tools/mxsimage.c
> > index 002f4b525a..2bfbb421eb 100644
> > --- a/tools/mxsimage.c
> > +++ b/tools/mxsimage.c
> > @@ -5,8 +5,6 @@
> >   * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de>
> >   */
> >  
> > -#ifdef CONFIG_MXS
> > -
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <stdio.h>
> > @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE(
> >  	NULL,
> >  	mxsimage_generate
> >  );
> > -#endif
> > -- 
> > 2.32.0
> >   

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Pali Rohár]

On Wednesday 20 October 2021 14:29:02 Andre Przywara wrote:
> On Wed, 20 Oct 2021 09:29:25 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> Hi,
> 
> > On Tuesday 19 October 2021 21:44:51 Samuel Holland wrote:
> > > Some image types (kwbimage and mxsimage) always depend on OpenSSL, so
> > > they can only be included in mkimage when TOOLS_LIBCRYPTO is selected.
> > > Use Makefile logic to conditionally link the files.
> > > 
> > > Signed-off-by: Samuel Holland <samuel@sholland.org>  
> > 
> > NAK.
> > 
> > As explained in previous email [1], kwbimage is required for building
> > Kirkwood, Dove, A370, AXP, A375, A38x, A39x and MSYS platforms.
> > Therefore it cannot be disabled or hidden behind some user config
> > options for these platforms (and it does not matter if it is crypto
> > option or any other option).
> 
> So somehow we need to find a solution between your view and Alex' view of
> things.
> First: Pali, do you see any actual problem at the moment? TOOLS_LIBCRYPTO
> defaults to y, so if I am not mistaken that means that a user would need
> to deliberately turn that off to trigger build errors?
> And also: the situation with this patch is not worse as before, isn't it?

I do not think so. Without this patch, kwbimage is always going
compiled, which means it is compiled also for mvebu platforms which
needs it.

With this patch it is possible via _valid_ config options to disable
compilation of kwbimage which would lead to totally bogus and
unintuitive behavior and errors. Without this patch compilation fails on
exact and clear error = cryto header files (or libs) were not found and
user / developer / package maintainer would know what is needed (= mean
to install missing dependency).

> So if it's just the missing improvement that you are concerned about, I am
> not sure that should block this patch?

V3 patch was improvement - it enforced required dependencies and I guess
it showed some build system error message when dependences were not met.
This is improvement from situation without patch when it failed on
compiling kwbimage which probably showed error message about missing
header files (or libs, depends on which stage it failed). It was not the
best choice as in some cases it enforced crypto dependencies also in
cases when they were not really required -- but it should have solve the
problem that dependences are there for platforms which require them.

v4 patch is not improvement, it is step backward - it started allowing
to compile mkimage without required functionality, even on platforms
which needs them, and effectively hides issue which is there.

> I mean you could always propose
> your own version of that missing piece, to improve the situation for the
> boards you care about? And we should have this discussion there?

I think I have written details and some proposed solution in that email.
What is needed is to ensure that kwbimage is always compiled for
platforms which require it (list of platform are in email 1 in previous
email). This can be done as hard dependency like it is currently without
this patch (ugly, does not show nice error message and enforce everybody
to have crypto dependency, even platforms which do not need it). Other
solution could be to define some select symbol which says that kwbimage
is required and then kwbimage would be unconditionally compiled when
this symbol is selected. And to make error message nice in build system,
this symbol could depends on crypto symbol to ensure that all
dependencies are met when trying to compile something which needs it.
Another option could be to implement required crypto functions directly
in u-boot source tree to remove external dependency. I do not know which
solution is the best or how hard they are to implement, and neither what
can be accepted by other u-boot developers / maintainers. I see an issue
here that fixing this problem need to touch more parts of u-boot source
code and build system which, which means that u-boot maintainers need to
say what they are willing to accept and what not.

> As it stands right now, this patch just improves things (just not
> *everything*), and it is a prerequisite for the rest of the series
> (unrelated to your problems), so I would like to go ahead on this one.

Well, maybe this patch improves some things about non-mvebu platforms,
but makes mvebu platform code / build system worse. And due to this fact
I cannot say that I want this change...

> Cheers,
> Andre.
> 
> > kwbimage must be unconditionally enabled on
> > these platforms like it was before this change, as it is crucial part of
> > build.
> > 
> > [1] - https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/
> > 
> > > ---
> > > 
> > > Changes in v4:
> > >  - Do not select TOOLS_LIBCRYPTO anywhere
> > > 
> > > Changes in v3:
> > >  - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
> > >    as I can tell, using the suggestions from Pali Rohár)
> > > 
> > > Changes in v2:
> > >  - Refactored the first patch on top of TOOLS_LIBCRYPTO
> > > 
> > >  scripts/config_whitelist.txt |  1 -
> > >  tools/Makefile               | 19 +++++--------------
> > >  tools/mxsimage.c             |  3 ---
> > >  3 files changed, 5 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> > > index cd94b5777a..affae6875d 100644
> > > --- a/scripts/config_whitelist.txt
> > > +++ b/scripts/config_whitelist.txt
> > > @@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE
> > >  CONFIG_MXC_USB_FLAGS
> > >  CONFIG_MXC_USB_PORT
> > >  CONFIG_MXC_USB_PORTSC
> > > -CONFIG_MXS
> > >  CONFIG_MXS_AUART
> > >  CONFIG_MXS_AUART_BASE
> > >  CONFIG_MXS_OCOTP
> > > diff --git a/tools/Makefile b/tools/Makefile
> > > index 999fd46531..a9b3d982d8 100644
> > > --- a/tools/Makefile
> > > +++ b/tools/Makefile
> > > @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.
> > >  AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
> > >  					aes-encrypt.o aes-decrypt.o)
> > >  
> > > -# Cryptographic helpers that depend on openssl/libcrypto
> > > -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
> > > -					fdt-libcrypto.o)
> > > +# Cryptographic helpers and image types that depend on openssl/libcrypto
> > > +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
> > > +			lib/fdt-libcrypto.o \
> > > +			kwbimage.o \
> > > +			mxsimage.o
> > >  
> > >  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
> > >  
> > > @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \
> > >  			imximage.o \
> > >  			imx8image.o \
> > >  			imx8mimage.o \
> > > -			kwbimage.o \
> > >  			lib/md5.o \
> > >  			lpc32xximage.o \
> > > -			mxsimage.o \
> > >  			omapimage.o \
> > >  			os_support.o \
> > >  			pblimage.o \
> > > @@ -156,22 +156,13 @@ fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
> > >  fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
> > >  file2include-objs := file2include.o
> > >  
> > > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
> > > -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register
> > > -# the mxsimage support within tools/mxsimage.c .
> > > -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS
> > > -endif
> > > -
> > >  ifdef CONFIG_TOOLS_LIBCRYPTO
> > >  # This affects include/image.h, but including the board config file
> > >  # is tricky, so manually define this options here.
> > >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE
> > >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
> > >  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
> > > -endif
> > >  
> > > -# MXSImage needs LibSSL
> > > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
> > >  HOSTCFLAGS_kwbimage.o += \
> > >  	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
> > >  HOSTLDLIBS_mkimage += \
> > > diff --git a/tools/mxsimage.c b/tools/mxsimage.c
> > > index 002f4b525a..2bfbb421eb 100644
> > > --- a/tools/mxsimage.c
> > > +++ b/tools/mxsimage.c
> > > @@ -5,8 +5,6 @@
> > >   * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de>
> > >   */
> > >  
> > > -#ifdef CONFIG_MXS
> > > -
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > >  #include <stdio.h>
> > > @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE(
> > >  	NULL,
> > >  	mxsimage_generate
> > >  );
> > > -#endif
> > > -- 
> > > 2.32.0
> > >   
> 

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Samuel Holland]

On 10/20/21 8:47 AM, Pali Rohár wrote:
> On Wednesday 20 October 2021 14:29:02 Andre Przywara wrote:
>> On Wed, 20 Oct 2021 09:29:25 +0200
>> Pali Rohár <pali@kernel.org> wrote:
>>
>> Hi,
>>
>>> On Tuesday 19 October 2021 21:44:51 Samuel Holland wrote:
>>>> Some image types (kwbimage and mxsimage) always depend on OpenSSL, so
>>>> they can only be included in mkimage when TOOLS_LIBCRYPTO is selected.
>>>> Use Makefile logic to conditionally link the files.
>>>>
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>  
>>>
>>> NAK.
>>>
>>> As explained in previous email [1], kwbimage is required for building
>>> Kirkwood, Dove, A370, AXP, A375, A38x, A39x and MSYS platforms.
>>> Therefore it cannot be disabled or hidden behind some user config
>>> options for these platforms (and it does not matter if it is crypto
>>> option or any other option).
>>
>> So somehow we need to find a solution between your view and Alex' view of
>> things.
>> First: Pali, do you see any actual problem at the moment? TOOLS_LIBCRYPTO
>> defaults to y, so if I am not mistaken that means that a user would need
>> to deliberately turn that off to trigger build errors?
>> And also: the situation with this patch is not worse as before, isn't it?
> 
> I do not think so. Without this patch, kwbimage is always going
> compiled, which means it is compiled also for mvebu platforms which
> needs it.
> 
> With this patch it is possible via _valid_ config options to disable
> compilation of kwbimage which would lead to totally bogus and
> unintuitive behavior and errors. Without this patch compilation fails on
> exact and clear error = cryto header files (or libs) were not found and
> user / developer / package maintainer would know what is needed (= mean
> to install missing dependency).

Andre is correct. No version of this patch makes *any* change to *any*
in-tree defconfig build, because TOOLS_LIBCRYPTO=y in all defconfigs.

>> So if it's just the missing improvement that you are concerned about, I am
>> not sure that should block this patch?
> 
> V3 patch was improvement - it enforced required dependencies and I guess
> it showed some build system error message when dependences were not met.
> This is improvement from situation without patch when it failed on
> compiling kwbimage which probably showed error message about missing
> header files (or libs, depends on which stage it failed). It was not the
> best choice as in some cases it enforced crypto dependencies also in
> cases when they were not really required -- but it should have solve the
> problem that dependences are there for platforms which require them.

No, you would have seen the exact same error message with v3 as you get
today without any patch. Because all v3 does is enforce that kwbimage.o
gets compiled on certain platforms (whereas today, it is compiled on
*all* platforms).

> v4 patch is not improvement, it is step backward - it started allowing
> to compile mkimage without required functionality, even on platforms
> which needs them, and effectively hides issue which is there.

Even if I was to accept your assertion that it hides a possible error on
platforms using kwbimage:

1) It does not *create* any errors for those platforms, and
2) It *fixes* possible errors for almost all other platforms.

>> I mean you could always propose
>> your own version of that missing piece, to improve the situation for the
>> boards you care about? And we should have this discussion there?
> 
> I think I have written details and some proposed solution in that email.

Yes, Pali, like I said in response to your comments on v2, you should
really be the one sending the patch here. I know next to nothing about
your platforms, and even giving me a list of SoC families does not tell
me where in the Kconfig you want the change to go. I am not interested
in continuing to play a guessing game (which so far I have been losing).

The TOOLS_LIBCRYPTO option has been around for several months now, so
there is no need to wait for my series to send your patch.

In fact, it was your change which broke the mkimage build with
TOOLS_LIBCRYPTO=n in the first place, so it is all the more
disappointing that, instead of fixing it, you are now trying to block
other people from fixing it.

> What is needed is to ensure that kwbimage is always compiled for
> platforms which require it (list of platform are in email 1 in previous
> email). This can be done as hard dependency like it is currently without
> this patch (ugly, does not show nice error message and enforce everybody
> to have crypto dependency, even platforms which do not need it). Other
> solution could be to define some select symbol which says that kwbimage
> is required and then kwbimage would be unconditionally compiled when
> this symbol is selected. And to make error message nice in build system,
> this symbol could depends on crypto symbol to ensure that all
> dependencies are met when trying to compile something which needs it.
> Another option could be to implement required crypto functions directly
> in u-boot source tree to remove external dependency. I do not know which
> solution is the best or how hard they are to implement, and neither what
> can be accepted by other u-boot developers / maintainers. I see an issue
> here that fixing this problem need to touch more parts of u-boot source
> code and build system which, which means that u-boot maintainers need to
> say what they are willing to accept and what not.
I don't see how "other bugs exist" is a reasonable justification for
NAKing this patch.

>> As it stands right now, this patch just improves things (just not
>> *everything*), and it is a prerequisite for the rest of the series
>> (unrelated to your problems), so I would like to go ahead on this one.
> 
> Well, maybe this patch improves some things about non-mvebu platforms,
> but makes mvebu platform code / build system worse. And due to this fact
> I cannot say that I want this change...

Previously, if I didn't have OpenSSL installed (or was not allowed to
link it for legal reasons):

1) mkimage could not build,
2) U-Boot for mvebu would not build.

With this patch v4, if I don't have OpenSSL installed (or am not allowed
to link it for legal reasons):

1) mkimage could build, with reduced functionality,
2) U-Boot for mvebu would not build.

That doesn't sound worse to me.

Regards,
Samuel

>> Cheers,
>> Andre.
>>
>>> kwbimage must be unconditionally enabled on
>>> these platforms like it was before this change, as it is crucial part of
>>> build.
>>>
>>> [1] - https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/
>>>
>>>> ---
>>>>
>>>> Changes in v4:
>>>>  - Do not select TOOLS_LIBCRYPTO anywhere
>>>>
>>>> Changes in v3:
>>>>  - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best
>>>>    as I can tell, using the suggestions from Pali Rohár)
>>>>
>>>> Changes in v2:
>>>>  - Refactored the first patch on top of TOOLS_LIBCRYPTO
>>>>
>>>>  scripts/config_whitelist.txt |  1 -
>>>>  tools/Makefile               | 19 +++++--------------
>>>>  tools/mxsimage.c             |  3 ---
>>>>  3 files changed, 5 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
>>>> index cd94b5777a..affae6875d 100644
>>>> --- a/scripts/config_whitelist.txt
>>>> +++ b/scripts/config_whitelist.txt
>>>> @@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE
>>>>  CONFIG_MXC_USB_FLAGS
>>>>  CONFIG_MXC_USB_PORT
>>>>  CONFIG_MXC_USB_PORTSC
>>>> -CONFIG_MXS
>>>>  CONFIG_MXS_AUART
>>>>  CONFIG_MXS_AUART_BASE
>>>>  CONFIG_MXS_OCOTP
>>>> diff --git a/tools/Makefile b/tools/Makefile
>>>> index 999fd46531..a9b3d982d8 100644
>>>> --- a/tools/Makefile
>>>> +++ b/tools/Makefile
>>>> @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.
>>>>  AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
>>>>  					aes-encrypt.o aes-decrypt.o)
>>>>  
>>>> -# Cryptographic helpers that depend on openssl/libcrypto
>>>> -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
>>>> -					fdt-libcrypto.o)
>>>> +# Cryptographic helpers and image types that depend on openssl/libcrypto
>>>> +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
>>>> +			lib/fdt-libcrypto.o \
>>>> +			kwbimage.o \
>>>> +			mxsimage.o
>>>>  
>>>>  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
>>>>  
>>>> @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \
>>>>  			imximage.o \
>>>>  			imx8image.o \
>>>>  			imx8mimage.o \
>>>> -			kwbimage.o \
>>>>  			lib/md5.o \
>>>>  			lpc32xximage.o \
>>>> -			mxsimage.o \
>>>>  			omapimage.o \
>>>>  			os_support.o \
>>>>  			pblimage.o \
>>>> @@ -156,22 +156,13 @@ fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
>>>>  fit_check_sign-objs   := $(dumpimage-mkimage-objs) fit_check_sign.o
>>>>  file2include-objs := file2include.o
>>>>  
>>>> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),)
>>>> -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register
>>>> -# the mxsimage support within tools/mxsimage.c .
>>>> -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS
>>>> -endif
>>>> -
>>>>  ifdef CONFIG_TOOLS_LIBCRYPTO
>>>>  # This affects include/image.h, but including the board config file
>>>>  # is tricky, so manually define this options here.
>>>>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE
>>>>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
>>>>  HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
>>>> -endif
>>>>  
>>>> -# MXSImage needs LibSSL
>>>> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),)
>>>>  HOSTCFLAGS_kwbimage.o += \
>>>>  	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
>>>>  HOSTLDLIBS_mkimage += \
>>>> diff --git a/tools/mxsimage.c b/tools/mxsimage.c
>>>> index 002f4b525a..2bfbb421eb 100644
>>>> --- a/tools/mxsimage.c
>>>> +++ b/tools/mxsimage.c
>>>> @@ -5,8 +5,6 @@
>>>>   * Copyright (C) 2012-2013 Marek Vasut <marex@denx.de>
>>>>   */
>>>>  
>>>> -#ifdef CONFIG_MXS
>>>> -
>>>>  #include <errno.h>
>>>>  #include <fcntl.h>
>>>>  #include <stdio.h>
>>>> @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE(
>>>>  	NULL,
>>>>  	mxsimage_generate
>>>>  );
>>>> -#endif
>>>> -- 
>>>> 2.32.0
>>>>   
>>

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Marek Behún]

Hi Samuel,

On Wed, 20 Oct 2021 09:14:11 -0500
Samuel Holland <samuel@sholland.org> wrote:

> Andre is correct. No version of this patch makes *any* change to *any*
> in-tree defconfig build, because TOOLS_LIBCRYPTO=y in all defconfigs.

> Even if I was to accept your assertion that it hides a possible error on
> platforms using kwbimage:
> 
> 1) It does not *create* any errors for those platforms, and
> 2) It *fixes* possible errors for almost all other platforms.

It does create possible errors, since it is possible to choose config
options which won't compile.

The fact that defconfig have this option enabled for those platforms
does not matter. What matters is that I can then validly change the
config with make menuconfig, and it won't compile. A compilation error
should not occur with valid config options. That's why dependencies
need to be made in Kconfig, so that if Kconfig decides that kwbimage is
to be built, then it will force crypto.

(Yes, I know that there currently are many ways to change Kconfig
 options and the result will fail to compile. That behaviour is
 unwanted and should be fixed by adding Kconfig dependencies so that it
 isn't possible to choose configuration that won't compile.)

Marek

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Marek Behún]

BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
platform in Kconfig?

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Pali Rohár]

On Thursday 21 October 2021 15:00:48 Marek Behún wrote:
> BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> platform in Kconfig?

FYI https://lore.kernel.org/u-boot/20211021093304.25399-1-pali@kernel.org/
(forgot to CC)

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Samuel Holland]

On 10/21/21 8:00 AM, Marek Behún wrote:
> BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> platform in Kconfig?

No, that would not do anything. For example:

	config ARMADA_32BIT
		imply TOOLS_LIBCRYPTO

is equivalent to:

	config TOOLS_LIBCRYPTO
		default y if ARMADA_32BIT

but TOOLS_LIBCRYPTO is already "default y". To force TOOLS_LIBCRYPTO=y,
you must select it.

Regards,
Samuel

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Heinrich Schuchardt]

On 10/21/21 15:00, Marek Behún wrote:
> BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> platform in Kconfig?
> 

We should only use 'imply' for suggested settings and never for hard 
requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
for mvebu would be redundant.

In an OS distribution we only want to ship a single version of mkimage. 
So it is good to elimate symbol CONFIG_MXS.

How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.

Tom wrote regarding this aspect in 
https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:

"if we're building a generically useful tool, we don't want another
symbol for it."

We should therefore remove the following dependencies in tools/Makefile:

CONFIG_MX23
CONFIG_MX28
CONFIG_ARMADA_38X

Best regards

Heinrich

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Marek Behún]

On Fri, 22 Oct 2021 12:09:19 +0200
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

> On 10/21/21 15:00, Marek Behún wrote:
> > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> > platform in Kconfig?
> >   
> 
> We should only use 'imply' for suggested settings and never for hard 
> requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> for mvebu would be redundant.
> 
> In an OS distribution we only want to ship a single version of mkimage. 
> So it is good to elimate symbol CONFIG_MXS.
> 
> How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> 
> Tom wrote regarding this aspect in 
> https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> 
> "if we're building a generically useful tool, we don't want another
> symbol for it."

OK, so mkimage and dumpimage should be always generic and always
support all platforms, that makes sense, since the tools can be
installed as a distribution package.

But I still think it should be possible to cripple these tools if the
developer wants to disable libcrypto due to embedded environment.

Marek

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:
> On Fri, 22 Oct 2021 12:09:19 +0200
> Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> 
> > On 10/21/21 15:00, Marek Behún wrote:
> > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> > > platform in Kconfig?
> > >   
> > 
> > We should only use 'imply' for suggested settings and never for hard 
> > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> > for mvebu would be redundant.
> > 
> > In an OS distribution we only want to ship a single version of mkimage. 
> > So it is good to elimate symbol CONFIG_MXS.
> > 
> > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> > 
> > Tom wrote regarding this aspect in 
> > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> > 
> > "if we're building a generically useful tool, we don't want another
> > symbol for it."
> 
> OK, so mkimage and dumpimage should be always generic and always
> support all platforms, that makes sense, since the tools can be
> installed as a distribution package.
> 
> But I still think it should be possible to cripple these tools if the
> developer wants to disable libcrypto due to embedded environment.

This is probably the time to reach out to some of the distro folks to
see how they would like to see things handled for "build the tools we
need to package for the user" and also "build the binary for the
platform".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Andre Przywara]

On Fri, 22 Oct 2021 11:09:27 -0400
Tom Rini <trini@konsulko.com> wrote:

Hi,

> On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:
> > On Fri, 22 Oct 2021 12:09:19 +0200
> > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> >   
> > > On 10/21/21 15:00, Marek Behún wrote:  
> > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> > > > platform in Kconfig?
> > > >     
> > > 
> > > We should only use 'imply' for suggested settings and never for hard 
> > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> > > for mvebu would be redundant.
> > > 
> > > In an OS distribution we only want to ship a single version of mkimage. 
> > > So it is good to elimate symbol CONFIG_MXS.
> > > 
> > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> > > 
> > > Tom wrote regarding this aspect in 
> > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> > > 
> > > "if we're building a generically useful tool, we don't want another
> > > symbol for it."  
> > 
> > OK, so mkimage and dumpimage should be always generic and always
> > support all platforms, that makes sense, since the tools can be
> > installed as a distribution package.
> > 
> > But I still think it should be possible to cripple these tools if the
> > developer wants to disable libcrypto due to embedded environment.  

Well, I don't think this is the real question here, is it?
I think the tools part is clear: distros want to build just mkimage,
supporting as many platforms as possible, and might need to avoid OpenSSL.
This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
tools-only_defconfg && make tools", and Samuel's patch actually fixes the
build (at least somewhat, I still get link errors).

The question at hand is whether *board* builds should be able to *force*
TOOLS_LIBCRYPTO, aka "select" this symbol. This was somewhat denied by
Alex, on the grounds of the top comment in tools/Makefile:
# Host tools can be used across multiple targets, or different configurations
# of the same target. Thus, host tools must be able to handle any combination
# of target configurations. To prevent having different variations of the same
# tool, the tool build options may not depend on target configuration.

I read this as: "a tool like mkimage should not use #ifdef
CONFIG_PLATFORM_FEATURE", but I don't see why a defconfig should not be
able to select TOOLS_LIBCRYPTO, if that's needed to package the firmware.

Do I get this correctly? If that's the case, then I think we should
actually stick more with the solution in v2, or maybe split that patch, so
v4 plus Pali's separate patch to select/depend on LIBCRYPTO for boards
using kwbimage.

Does that make sense?

Cheers,
Andre

> 
> This is probably the time to reach out to some of the distro folks to
> see how they would like to see things handled for "build the tools we
> need to package for the user" and also "build the binary for the
> platform".
> 

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On Fri, Oct 22, 2021 at 04:56:09PM +0100, Andre Przywara wrote:
> On Fri, 22 Oct 2021 11:09:27 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi,
> 
> > On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:
> > > On Fri, 22 Oct 2021 12:09:19 +0200
> > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> > >   
> > > > On 10/21/21 15:00, Marek Behún wrote:  
> > > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> > > > > platform in Kconfig?
> > > > >     
> > > > 
> > > > We should only use 'imply' for suggested settings and never for hard 
> > > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> > > > for mvebu would be redundant.
> > > > 
> > > > In an OS distribution we only want to ship a single version of mkimage. 
> > > > So it is good to elimate symbol CONFIG_MXS.
> > > > 
> > > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> > > > 
> > > > Tom wrote regarding this aspect in 
> > > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> > > > 
> > > > "if we're building a generically useful tool, we don't want another
> > > > symbol for it."  
> > > 
> > > OK, so mkimage and dumpimage should be always generic and always
> > > support all platforms, that makes sense, since the tools can be
> > > installed as a distribution package.
> > > 
> > > But I still think it should be possible to cripple these tools if the
> > > developer wants to disable libcrypto due to embedded environment.  
> 
> Well, I don't think this is the real question here, is it?
> I think the tools part is clear: distros want to build just mkimage,
> supporting as many platforms as possible, and might need to avoid OpenSSL.
> This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
> tools-only_defconfg && make tools", and Samuel's patch actually fixes the
> build (at least somewhat, I still get link errors).

The problem is, are distros doing a tools-only build, for tools, or are
they doing it per board?  Like, hey, ugh, OpenEmbedded uses
sandbox_defconfig and cross_tools as the targets.  That's not quite what
I was hoping to see.  So I want to know everyone else is doing, rather
than we hope they're doing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 3265 bytes --]

On 2021-10-22, Tom Rini wrote:
> On Fri, Oct 22, 2021 at 04:56:09PM +0100, Andre Przywara wrote:
>> On Fri, 22 Oct 2021 11:09:27 -0400
>> Tom Rini <trini@konsulko.com> wrote:

>> > On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:
>> > > On Fri, 22 Oct 2021 12:09:19 +0200
>> > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>> > >   
>> > > > On 10/21/21 15:00, Marek Behún wrote:  
>> > > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
>> > > > > platform in Kconfig?
>> > > > >     
>> > > > 
>> > > > We should only use 'imply' for suggested settings and never for hard 
>> > > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
>> > > > for mvebu would be redundant.
>> > > > 
>> > > > In an OS distribution we only want to ship a single version of mkimage. 
>> > > > So it is good to elimate symbol CONFIG_MXS.
>> > > > 
>> > > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
>> > > > 
>> > > > Tom wrote regarding this aspect in 
>> > > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
>> > > > 
>> > > > "if we're building a generically useful tool, we don't want another
>> > > > symbol for it."  
>> > > 
>> > > OK, so mkimage and dumpimage should be always generic and always
>> > > support all platforms, that makes sense, since the tools can be
>> > > installed as a distribution package.
>> > > 
>> > > But I still think it should be possible to cripple these tools if the
>> > > developer wants to disable libcrypto due to embedded environment.  
>> 
>> Well, I don't think this is the real question here, is it?
>> I think the tools part is clear: distros want to build just mkimage,
>> supporting as many platforms as possible, and might need to avoid OpenSSL.
>> This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
>> tools-only_defconfg && make tools", and Samuel's patch actually fixes the
>> build (at least somewhat, I still get link errors).
>
> The problem is, are distros doing a tools-only build, for tools, or are
> they doing it per board?  Like, hey, ugh, OpenEmbedded uses
> sandbox_defconfig and cross_tools as the targets.  That's not quite what
> I was hoping to see.  So I want to know everyone else is doing, rather
> than we hope they're doing.

Thanks for bringing this to my attention!

In Debian, the u-boot-tools package is built using tools-only, and for
each of the board-specific targets, it still ends up building the
relevent tools, but we throw them away and do not ship them in any
packages.

With 2021.10, the board-specific builds made it harder to avoid openssl
with the corresponding tools, and I reluctantly added a dependency on
openssl... (which is technically permitted in Debian, having declared
openssl as a system library to avoid the GPL incompatibilities, but
... meh.)

I also have been doing some packaging of u-boot for GNU Guix, where I
suspect the stance wouldn't be as willing to accept such a compromise...

So... I would *love* an option to be able to build a board-only config
without any of the tools; do some boards use board-specific tools as
part of their build processes?


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Pali Rohár]

On Friday 22 October 2021 09:47:35 Vagrant Cascadian wrote:
> do some boards use board-specific tools as part of their build processes?

Lot of boards are using mkimage for generating final U-Boot binary.

Prior U-Boot 2021.10 all 32-bit mvebu boards used own specific version
of mkimage to generate final U-Boot binary. mkimage had own compile time
options (defined in board include header files) which generated unique
./tools/mkimage binary, specific for just one board.

Since U-Boot 2021.10 now all 32-bit mvebu boards are using one common
mkimage binary. So you can even do defconfig for some x86 board, build
mkimage binary and then you can use this mkimage binary also for
building final mvebu (arm) U-Boot binary.

Side effect of this change was hard dependency on openssl, but we are
discussing how to solve and remove that hard dependency.

I do not know if there are any processors, SoCs or boards which require
board-specific version of some tool (e.g. mkimage) in U-Boot 2021.10.

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Andre Przywara]

On Fri, 22 Oct 2021 09:47:35 -0700
Vagrant Cascadian <vagrant@debian.org> wrote:

Hi,

> On 2021-10-22, Tom Rini wrote:
> > On Fri, Oct 22, 2021 at 04:56:09PM +0100, Andre Przywara wrote:  
> >> On Fri, 22 Oct 2021 11:09:27 -0400
> >> Tom Rini <trini@konsulko.com> wrote:  
> 
> >> > On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:  
> >> > > On Fri, 22 Oct 2021 12:09:19 +0200
> >> > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> >> > >     
> >> > > > On 10/21/21 15:00, Marek Behún wrote:    
> >> > > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> >> > > > > platform in Kconfig?
> >> > > > >       
> >> > > > 
> >> > > > We should only use 'imply' for suggested settings and never for hard 
> >> > > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> >> > > > for mvebu would be redundant.
> >> > > > 
> >> > > > In an OS distribution we only want to ship a single version of mkimage. 
> >> > > > So it is good to elimate symbol CONFIG_MXS.
> >> > > > 
> >> > > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> >> > > > 
> >> > > > Tom wrote regarding this aspect in 
> >> > > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> >> > > > 
> >> > > > "if we're building a generically useful tool, we don't want another
> >> > > > symbol for it."    
> >> > > 
> >> > > OK, so mkimage and dumpimage should be always generic and always
> >> > > support all platforms, that makes sense, since the tools can be
> >> > > installed as a distribution package.
> >> > > 
> >> > > But I still think it should be possible to cripple these tools if the
> >> > > developer wants to disable libcrypto due to embedded environment.    
> >> 
> >> Well, I don't think this is the real question here, is it?
> >> I think the tools part is clear: distros want to build just mkimage,
> >> supporting as many platforms as possible, and might need to avoid OpenSSL.
> >> This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
> >> tools-only_defconfg && make tools", and Samuel's patch actually fixes the
> >> build (at least somewhat, I still get link errors).  
> >
> > The problem is, are distros doing a tools-only build, for tools, or are
> > they doing it per board?  Like, hey, ugh, OpenEmbedded uses
> > sandbox_defconfig and cross_tools as the targets.  That's not quite what
> > I was hoping to see.  So I want to know everyone else is doing, rather
> > than we hope they're doing.  
> 
> Thanks for bringing this to my attention!
> 
> In Debian, the u-boot-tools package is built using tools-only, and for
> each of the board-specific targets, it still ends up building the
> relevent tools, but we throw them away and do not ship them in any
> packages.
> 
> With 2021.10, the board-specific builds made it harder to avoid openssl
> with the corresponding tools, and I reluctantly added a dependency on
> openssl... (which is technically permitted in Debian, having declared
> openssl as a system library to avoid the GPL incompatibilities, but
> ... meh.)

But this is purely a *build-time* dependency only, right? The resulting
images do not have any openssl code in them, they were just *created*
(signed) using that code.
I don't think this a legal issue? The problems are about *shipping*
openssl code, which you only do for u-boot-tools - where it now can be
disabled.

> I also have been doing some packaging of u-boot for GNU Guix, where I
> suspect the stance wouldn't be as willing to accept such a compromise...
> 
> So... I would *love* an option to be able to build a board-only config
> without any of the tools;

Why is this a problem (see above)? Who is building board builds? It's
either the maintainer when creating the binary package, or a curious user,
right? And they can surely *use* OpenSSL during build time - if it's
needed by the board.

Cheers,
Andre


> do some boards use board-specific tools as
> part of their build processes?
> 
> 
> live well,
>   vagrant

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 5067 bytes --]

On 2021-10-22, Andre Przywara wrote:
> On Fri, 22 Oct 2021 09:47:35 -0700
> Vagrant Cascadian <vagrant@debian.org> wrote:
>> On 2021-10-22, Tom Rini wrote:
>> > On Fri, Oct 22, 2021 at 04:56:09PM +0100, Andre Przywara wrote:  
>> >> On Fri, 22 Oct 2021 11:09:27 -0400
>> >> Tom Rini <trini@konsulko.com> wrote:  
>> 
>> >> > On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:  
>> >> > > On Fri, 22 Oct 2021 12:09:19 +0200
>> >> > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>> >> > >     
>> >> > > > On 10/21/21 15:00, Marek Behún wrote:    
>> >> > > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
>> >> > > > > platform in Kconfig?
>> >> > > > >       
>> >> > > > 
>> >> > > > We should only use 'imply' for suggested settings and never for hard 
>> >> > > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
>> >> > > > for mvebu would be redundant.
>> >> > > > 
>> >> > > > In an OS distribution we only want to ship a single version of mkimage. 
>> >> > > > So it is good to elimate symbol CONFIG_MXS.
>> >> > > > 
>> >> > > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
>> >> > > > 
>> >> > > > Tom wrote regarding this aspect in 
>> >> > > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
>> >> > > > 
>> >> > > > "if we're building a generically useful tool, we don't want another
>> >> > > > symbol for it."    
>> >> > > 
>> >> > > OK, so mkimage and dumpimage should be always generic and always
>> >> > > support all platforms, that makes sense, since the tools can be
>> >> > > installed as a distribution package.
>> >> > > 
>> >> > > But I still think it should be possible to cripple these tools if the
>> >> > > developer wants to disable libcrypto due to embedded environment.    
>> >> 
>> >> Well, I don't think this is the real question here, is it?
>> >> I think the tools part is clear: distros want to build just mkimage,
>> >> supporting as many platforms as possible, and might need to avoid OpenSSL.
>> >> This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
>> >> tools-only_defconfg && make tools", and Samuel's patch actually fixes the
>> >> build (at least somewhat, I still get link errors).  
>> >
>> > The problem is, are distros doing a tools-only build, for tools, or are
>> > they doing it per board?  Like, hey, ugh, OpenEmbedded uses
>> > sandbox_defconfig and cross_tools as the targets.  That's not quite what
>> > I was hoping to see.  So I want to know everyone else is doing, rather
>> > than we hope they're doing.  
>> 
>> Thanks for bringing this to my attention!
>> 
>> In Debian, the u-boot-tools package is built using tools-only, and for
>> each of the board-specific targets, it still ends up building the
>> relevent tools, but we throw them away and do not ship them in any
>> packages.
>> 
>> With 2021.10, the board-specific builds made it harder to avoid openssl
>> with the corresponding tools, and I reluctantly added a dependency on
>> openssl... (which is technically permitted in Debian, having declared
>> openssl as a system library to avoid the GPL incompatibilities, but
>> ... meh.)
>
> But this is purely a *build-time* dependency only, right? The resulting
> images do not have any openssl code in them, they were just *created*
> (signed) using that code.
> I don't think this a legal issue? 

The various .h includes are all that I saw, and I *think* all in the
tools/ directory, but yeah, if this is really the case that no openssl
code ends up in the board-specific binaries, that simplifies things
considerably.


> The problems are about *shipping* openssl code, which you only do for
> u-boot-tools - where it now can be disabled.

Probably won't disable it for u-boot-tools in Debian (reluctantly riding
on the system library exception), but the tools builds that are part of
the build process would be nice to be able to disable.



>> I also have been doing some packaging of u-boot for GNU Guix, where I
>> suspect the stance wouldn't be as willing to accept such a compromise...
>> 
>> So... I would *love* an option to be able to build a board-only config
>> without any of the tools;
>
> Why is this a problem (see above)? Who is building board builds? It's
> either the maintainer when creating the binary package, or a curious user,
> right? And they can surely *use* OpenSSL during build time - if it's
> needed by the board.

Sure, if there is no actual openssl code embedded in the resulting
binary with GPLv2 code, it shouldn't be a problem...


It's a mess of an issue to tease out exactly what codepaths trigger and
do not trigger the compatibility issues between openssl and GPL...


Depending on openssl in a project with GPLv2-only code does seem at risk
to introduce license compatibility issues without sufficient and
constant review and dilligence, even if it is technically ok how it is
done right now...


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5888 bytes --]

On Fri, Oct 22, 2021 at 12:46:59PM -0700, Vagrant Cascadian wrote:
> On 2021-10-22, Andre Przywara wrote:
> > On Fri, 22 Oct 2021 09:47:35 -0700
> > Vagrant Cascadian <vagrant@debian.org> wrote:
> >> On 2021-10-22, Tom Rini wrote:
> >> > On Fri, Oct 22, 2021 at 04:56:09PM +0100, Andre Przywara wrote:  
> >> >> On Fri, 22 Oct 2021 11:09:27 -0400
> >> >> Tom Rini <trini@konsulko.com> wrote:  
> >> 
> >> >> > On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:  
> >> >> > > On Fri, 22 Oct 2021 12:09:19 +0200
> >> >> > > Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
> >> >> > >     
> >> >> > > > On 10/21/21 15:00, Marek Behún wrote:    
> >> >> > > > > BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
> >> >> > > > > platform in Kconfig?
> >> >> > > > >       
> >> >> > > > 
> >> >> > > > We should only use 'imply' for suggested settings and never for hard 
> >> >> > > > requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it 
> >> >> > > > for mvebu would be redundant.
> >> >> > > > 
> >> >> > > > In an OS distribution we only want to ship a single version of mkimage. 
> >> >> > > > So it is good to elimate symbol CONFIG_MXS.
> >> >> > > > 
> >> >> > > > How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
> >> >> > > > 
> >> >> > > > Tom wrote regarding this aspect in 
> >> >> > > > https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
> >> >> > > > 
> >> >> > > > "if we're building a generically useful tool, we don't want another
> >> >> > > > symbol for it."    
> >> >> > > 
> >> >> > > OK, so mkimage and dumpimage should be always generic and always
> >> >> > > support all platforms, that makes sense, since the tools can be
> >> >> > > installed as a distribution package.
> >> >> > > 
> >> >> > > But I still think it should be possible to cripple these tools if the
> >> >> > > developer wants to disable libcrypto due to embedded environment.    
> >> >> 
> >> >> Well, I don't think this is the real question here, is it?
> >> >> I think the tools part is clear: distros want to build just mkimage,
> >> >> supporting as many platforms as possible, and might need to avoid OpenSSL.
> >> >> This should be covered by TOOLS_LIBCRYPTO=[yn] and "make
> >> >> tools-only_defconfg && make tools", and Samuel's patch actually fixes the
> >> >> build (at least somewhat, I still get link errors).  
> >> >
> >> > The problem is, are distros doing a tools-only build, for tools, or are
> >> > they doing it per board?  Like, hey, ugh, OpenEmbedded uses
> >> > sandbox_defconfig and cross_tools as the targets.  That's not quite what
> >> > I was hoping to see.  So I want to know everyone else is doing, rather
> >> > than we hope they're doing.  
> >> 
> >> Thanks for bringing this to my attention!
> >> 
> >> In Debian, the u-boot-tools package is built using tools-only, and for
> >> each of the board-specific targets, it still ends up building the
> >> relevent tools, but we throw them away and do not ship them in any
> >> packages.
> >> 
> >> With 2021.10, the board-specific builds made it harder to avoid openssl
> >> with the corresponding tools, and I reluctantly added a dependency on
> >> openssl... (which is technically permitted in Debian, having declared
> >> openssl as a system library to avoid the GPL incompatibilities, but
> >> ... meh.)
> >
> > But this is purely a *build-time* dependency only, right? The resulting
> > images do not have any openssl code in them, they were just *created*
> > (signed) using that code.
> > I don't think this a legal issue? 
> 
> The various .h includes are all that I saw, and I *think* all in the
> tools/ directory, but yeah, if this is really the case that no openssl
> code ends up in the board-specific binaries, that simplifies things
> considerably.
> 
> 
> > The problems are about *shipping* openssl code, which you only do for
> > u-boot-tools - where it now can be disabled.
> 
> Probably won't disable it for u-boot-tools in Debian (reluctantly riding
> on the system library exception), but the tools builds that are part of
> the build process would be nice to be able to disable.
> 
> 
> 
> >> I also have been doing some packaging of u-boot for GNU Guix, where I
> >> suspect the stance wouldn't be as willing to accept such a compromise...
> >> 
> >> So... I would *love* an option to be able to build a board-only config
> >> without any of the tools;
> >
> > Why is this a problem (see above)? Who is building board builds? It's
> > either the maintainer when creating the binary package, or a curious user,
> > right? And they can surely *use* OpenSSL during build time - if it's
> > needed by the board.
> 
> Sure, if there is no actual openssl code embedded in the resulting
> binary with GPLv2 code, it shouldn't be a problem...
> 
> 
> It's a mess of an issue to tease out exactly what codepaths trigger and
> do not trigger the compatibility issues between openssl and GPL...
> 
> 
> Depending on openssl in a project with GPLv2-only code does seem at risk
> to introduce license compatibility issues without sufficient and
> constant review and dilligence, even if it is technically ok how it is
> done right now...

There's still the long standing request to migrate the tooling to use a
different library, but it's apparently not been a large enough concern
of company with concerns to fund a developer of theirs to do the
migration.  I feel like that might be one of the better, at least in
terms of license, fixes for this issue.

And then maybe we do just need a way to say if you're building for
platform X then you must also have the crypto requirement resolved to
build mkimage.  And conversely if you aren't building those platforms,
it's OK to not.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Peter Robinson]

> > >> I also have been doing some packaging of u-boot for GNU Guix, where I
> > >> suspect the stance wouldn't be as willing to accept such a compromise...
> > >>
> > >> So... I would *love* an option to be able to build a board-only config
> > >> without any of the tools;
> > >
> > > Why is this a problem (see above)? Who is building board builds? It's
> > > either the maintainer when creating the binary package, or a curious user,
> > > right? And they can surely *use* OpenSSL during build time - if it's
> > > needed by the board.
> >
> > Sure, if there is no actual openssl code embedded in the resulting
> > binary with GPLv2 code, it shouldn't be a problem...
> >
> >
> > It's a mess of an issue to tease out exactly what codepaths trigger and
> > do not trigger the compatibility issues between openssl and GPL...
> >
> >
> > Depending on openssl in a project with GPLv2-only code does seem at risk
> > to introduce license compatibility issues without sufficient and
> > constant review and dilligence, even if it is technically ok how it is
> > done right now...
>
> There's still the long standing request to migrate the tooling to use a
> different library, but it's apparently not been a large enough concern
> of company with concerns to fund a developer of theirs to do the
> migration.  I feel like that might be one of the better, at least in
> terms of license, fixes for this issue.
>
> And then maybe we do just need a way to say if you're building for
> platform X then you must also have the crypto requirement resolved to
> build mkimage.  And conversely if you aren't building those platforms,
> it's OK to not.

Does the re-license [1] to Apache License 2.0 for openssl 3+ change
this situation at all?

For reference all the pieces of U-Boot that Fedora builds seem to
build fine with openssl 3.

[1] https://www.openssl.org/blog/blog/2021/09/07/OpenSSL3.Final/

Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL

[Matthias Brugger]

On 22/10/2021 17:09, Tom Rini wrote:
> On Fri, Oct 22, 2021 at 04:59:22PM +0200, Marek Behún wrote:
>> On Fri, 22 Oct 2021 12:09:19 +0200
>> Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>>
>>> On 10/21/21 15:00, Marek Behún wrote:
>>>> BTW, wouldn't it be enough to simply imply TOOLS_LIBCRYPTO for mvebu
>>>> platform in Kconfig?
>>>>    
>>>
>>> We should only use 'imply' for suggested settings and never for hard
>>> requirements. TOOLS_LIBCRYPTO already defaults to 'Y'. So implying it
>>> for mvebu would be redundant.
>>>
>>> In an OS distribution we only want to ship a single version of mkimage.
>>> So it is good to elimate symbol CONFIG_MXS.
>>>
>>> How mkimage is built should not depend on CONFIG_TOOLS_LIBCRYPTO.
>>>
>>> Tom wrote regarding this aspect in
>>> https://lists.denx.de/pipermail/u-boot/2021-September/460251.html:
>>>
>>> "if we're building a generically useful tool, we don't want another
>>> symbol for it."
>>
>> OK, so mkimage and dumpimage should be always generic and always
>> support all platforms, that makes sense, since the tools can be
>> installed as a distribution package.
>>
>> But I still think it should be possible to cripple these tools if the
>> developer wants to disable libcrypto due to embedded environment.
> 
> This is probably the time to reach out to some of the distro folks to
> see how they would like to see things handled for "build the tools we
> need to package for the user" and also "build the binary for the
> platform".
> 

in openSUSE we are building the tools for each u-boot binary but those are not 
part of our u-boot-tools package. For that package we use sandbox_defconfig to 
only build the tools. So we are using openSSL in our packaging. AFAIK that's not 
an issue for us.

Regards,
Matthias

[PATCH v4 2/4] tools: mkimage: Add Allwinner TOC0 support

[Samuel Holland]

Most Allwinner sunxi SoCs have separate boot ROMs in non-secure and
secure mode. The "non-secure" or "normal" boot ROM (NBROM) uses the
existing sunxi_egon image type. The secure boot ROM (SBROM) uses a
completely different image type, known as TOC0.

A TOC0 image is composed of a header and two or more items. One item
is the firmware binary. The others form a chain linking the firmware
signature to the root-of-trust public key (ROTPK), which has its hash
burned in the SoC's eFuses. Signatures are made using RSA-2048 + SHA256.

The pseudo-ASN.1 structure is manually assembled; this is done to work
around bugs/quirks in the boot ROM, which vary between SoCs. This TOC0
implementation has been verified to work with the A50, A64, H5, H6,
and H616 SBROMs, and it may work with other SoCs.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v3)

Changes in v3:
 - Removed TOOLS_LIBCRYPTO selection for sunxi, since most boards
   do not need it
 - Added __packed to all new "ABI" structs
 - Added entry to MAINTAINERS for sunxi tools

Changes in v2:
 - Moved certificate and key item structures out of sunxi_image.h
 - Renamed "main" and "item" variables for clarity
 - Improved error messages, and added a hint about key generation
 - Added a comment explaining the purpose of the various key files
 - Mentioned testing this code on A50 in the commit message

 MAINTAINERS           |   1 +
 common/image.c        |   1 +
 include/image.h       |   1 +
 include/sunxi_image.h |  37 ++
 tools/Makefile        |   3 +-
 tools/sunxi_toc0.c    | 907 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 949 insertions(+), 1 deletion(-)
 create mode 100644 tools/sunxi_toc0.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 71f468c00a..0d62829f51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -475,6 +475,7 @@ F:	board/sunxi/
 F:	drivers/clk/sunxi/
 F:	drivers/phy/allwinner/
 F:	drivers/video/sunxi/
+F:	tools/sunxi*
 
 ARM TEGRA
 M:	Tom Warren <twarren@nvidia.com>
diff --git a/common/image.c b/common/image.c
index 3fa60b5827..d15b47ebbe 100644
--- a/common/image.c
+++ b/common/image.c
@@ -185,6 +185,7 @@ static const table_entry_t uimage_type[] = {
 	{	IH_TYPE_MTKIMAGE,   "mtk_image",   "MediaTek BootROM loadable Image" },
 	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
 	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
+	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
 	{	-1,		    "",		  "",			},
 };
 
diff --git a/include/image.h b/include/image.h
index 34d13ada84..1547246ec8 100644
--- a/include/image.h
+++ b/include/image.h
@@ -227,6 +227,7 @@ enum {
 	IH_TYPE_IMX8IMAGE,		/* Freescale IMX8Boot Image	*/
 	IH_TYPE_COPRO,			/* Coprocessor Image for remoteproc*/
 	IH_TYPE_SUNXI_EGON,		/* Allwinner eGON Boot Image */
+	IH_TYPE_SUNXI_TOC0,		/* Allwinner TOC0 Boot Image */
 
 	IH_TYPE_COUNT,			/* Number of image types */
 };
diff --git a/include/sunxi_image.h b/include/sunxi_image.h
index 5b2055c0af..379ca9196e 100644
--- a/include/sunxi_image.h
+++ b/include/sunxi_image.h
@@ -9,9 +9,13 @@
  *
  * Shared between mkimage and the SPL.
  */
+
 #ifndef	SUNXI_IMAGE_H
 #define	SUNXI_IMAGE_H
 
+#include <linux/compiler_attributes.h>
+#include <linux/types.h>
+
 #define BOOT0_MAGIC		"eGON.BT0"
 #define BROM_STAMP_VALUE	0x5f0a6c39
 #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
@@ -79,4 +83,37 @@ struct boot_file_head {
 /* Compile time check to assure proper alignment of structure */
 typedef char boot_file_head_not_multiple_of_32[1 - 2*(sizeof(struct boot_file_head) % 32)];
 
+struct __packed toc0_main_info {
+	uint8_t	name[8];
+	__le32	magic;
+	__le32	checksum;
+	__le32	serial;
+	__le32	status;
+	__le32	num_items;
+	__le32	length;
+	uint8_t	platform[4];
+	uint8_t	reserved[8];
+	uint8_t	end[4];
+};
+
+#define TOC0_MAIN_INFO_NAME		"TOC0.GLH"
+#define TOC0_MAIN_INFO_MAGIC		0x89119800
+#define TOC0_MAIN_INFO_END		"MIE;"
+
+struct __packed toc0_item_info {
+	__le32	name;
+	__le32	offset;
+	__le32	length;
+	__le32	status;
+	__le32	type;
+	__le32	load_addr;
+	uint8_t	reserved[4];
+	uint8_t	end[4];
+};
+
+#define TOC0_ITEM_INFO_NAME_CERT	0x00010101
+#define TOC0_ITEM_INFO_NAME_FIRMWARE	0x00010202
+#define TOC0_ITEM_INFO_NAME_KEY		0x00010303
+#define TOC0_ITEM_INFO_END		"IIE;"
+
 #endif
diff --git a/tools/Makefile b/tools/Makefile
index a9b3d982d8..e2aeb097aa 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -98,7 +98,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
 LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
 			lib/fdt-libcrypto.o \
 			kwbimage.o \
-			mxsimage.o
+			mxsimage.o \
+			sunxi_toc0.o
 
 ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
 
diff --git a/tools/sunxi_toc0.c b/tools/sunxi_toc0.c
new file mode 100644
index 0000000000..58a6e7a0a1
--- /dev/null
+++ b/tools/sunxi_toc0.c
@@ -0,0 +1,907 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Arm Ltd.
+ * (C) Copyright 2020-2021 Samuel Holland <samuel@sholland.org>
+ */
+
+#include <assert.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <openssl/asn1t.h>
+#include <openssl/pem.h>
+#include <openssl/rsa.h>
+
+#include <image.h>
+#include <sunxi_image.h>
+
+#include "imagetool.h"
+#include "mkimage.h"
+
+/*
+ * NAND requires 8K padding. For other devices, BROM requires only
+ * 512B padding, but let's use the larger padding to cover everything.
+ */
+#define PAD_SIZE		8192
+
+#define pr_fmt(fmt)		"mkimage (TOC0): %s: " fmt
+#define pr_err(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "error", ##args)
+#define pr_warn(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "warning", ##args)
+#define pr_info(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "info", ##args)
+
+struct __packed toc0_key_item {
+	__le32  vendor_id;
+	__le32  key0_n_len;
+	__le32  key0_e_len;
+	__le32  key1_n_len;
+	__le32  key1_e_len;
+	__le32  sig_len;
+	uint8_t key0[512];
+	uint8_t key1[512];
+	uint8_t reserved[32];
+	uint8_t sig[256];
+};
+
+/*
+ * This looks somewhat like an X.509 certificate, but it is not valid BER.
+ *
+ * Some differences:
+ *  - Some X.509 certificate fields are missing or rearranged.
+ *  - Some sequences have the wrong tag.
+ *  - Zero-length sequences are accepted.
+ *  - Large strings and integers must be an even number of bytes long.
+ *  - Positive integers are not zero-extended to maintain their sign.
+ *
+ * See https://linux-sunxi.org/TOC0 for more information.
+ */
+struct __packed toc0_small_tag {
+	uint8_t tag;
+	uint8_t length;
+};
+
+typedef struct toc0_small_tag toc0_small_int;
+typedef struct toc0_small_tag toc0_small_oct;
+typedef struct toc0_small_tag toc0_small_seq;
+typedef struct toc0_small_tag toc0_small_exp;
+
+#define TOC0_SMALL_INT(len) { 0x02, (len) }
+#define TOC0_SMALL_SEQ(len) { 0x30, (len) }
+#define TOC0_SMALL_EXP(tag, len) { 0xa0 | (tag), len }
+
+struct __packed toc0_large_tag {
+	uint8_t tag;
+	uint8_t prefix;
+	uint8_t length_hi;
+	uint8_t length_lo;
+};
+
+typedef struct toc0_large_tag toc0_large_int;
+typedef struct toc0_large_tag toc0_large_bit;
+typedef struct toc0_large_tag toc0_large_seq;
+
+#define TOC0_LARGE_INT(len) { 0x02, 0x82, (len) >> 8, (len) & 0xff }
+#define TOC0_LARGE_BIT(len) { 0x03, 0x82, (len) >> 8, (len) & 0xff }
+#define TOC0_LARGE_SEQ(len) { 0x30, 0x82, (len) >> 8, (len) & 0xff }
+
+struct __packed toc0_cert_item {
+	toc0_large_seq tag_totalSequence;
+	struct __packed toc0_totalSequence {
+		toc0_large_seq tag_mainSequence;
+		struct __packed toc0_mainSequence {
+			toc0_small_exp tag_explicit0;
+			struct __packed toc0_explicit0 {
+				toc0_small_int tag_version;
+				uint8_t version;
+			} explicit0;
+			toc0_small_int tag_serialNumber;
+			uint8_t serialNumber;
+			toc0_small_seq tag_signature;
+			toc0_small_seq tag_issuer;
+			toc0_small_seq tag_validity;
+			toc0_small_seq tag_subject;
+			toc0_large_seq tag_subjectPublicKeyInfo;
+			struct __packed toc0_subjectPublicKeyInfo {
+				toc0_small_seq tag_algorithm;
+				toc0_large_seq tag_publicKey;
+				struct __packed toc0_publicKey {
+					toc0_large_int tag_n;
+					uint8_t n[256];
+					toc0_small_int tag_e;
+					uint8_t e[3];
+				} publicKey;
+			} subjectPublicKeyInfo;
+			toc0_small_exp tag_explicit3;
+			struct __packed toc0_explicit3 {
+				toc0_small_seq tag_extension;
+				struct __packed toc0_extension {
+					toc0_small_int tag_digest;
+					uint8_t digest[32];
+				} extension;
+			} explicit3;
+		} mainSequence;
+		toc0_large_bit tag_sigSequence;
+		struct __packed toc0_sigSequence {
+			toc0_small_seq tag_algorithm;
+			toc0_large_bit tag_signature;
+			uint8_t signature[256];
+		} sigSequence;
+	} totalSequence;
+};
+
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+
+static const struct toc0_cert_item cert_item_template = {
+	TOC0_LARGE_SEQ(sizeof(struct toc0_totalSequence)),
+	{
+		TOC0_LARGE_SEQ(sizeof(struct toc0_mainSequence)),
+		{
+			TOC0_SMALL_EXP(0, sizeof(struct toc0_explicit0)),
+			{
+				TOC0_SMALL_INT(sizeof_field(struct toc0_explicit0, version)),
+				0,
+			},
+			TOC0_SMALL_INT(sizeof_field(struct toc0_mainSequence, serialNumber)),
+			0,
+			TOC0_SMALL_SEQ(0),
+			TOC0_SMALL_SEQ(0),
+			TOC0_SMALL_SEQ(0),
+			TOC0_SMALL_SEQ(0),
+			TOC0_LARGE_SEQ(sizeof(struct toc0_subjectPublicKeyInfo)),
+			{
+				TOC0_SMALL_SEQ(0),
+				TOC0_LARGE_SEQ(sizeof(struct toc0_publicKey)),
+				{
+					TOC0_LARGE_INT(sizeof_field(struct toc0_publicKey, n)),
+					{},
+					TOC0_SMALL_INT(sizeof_field(struct toc0_publicKey, e)),
+					{},
+				},
+			},
+			TOC0_SMALL_EXP(3, sizeof(struct toc0_explicit3)),
+			{
+				TOC0_SMALL_SEQ(sizeof(struct toc0_extension)),
+				{
+					TOC0_SMALL_INT(sizeof_field(struct toc0_extension, digest)),
+					{},
+				},
+			},
+		},
+		TOC0_LARGE_BIT(sizeof(struct toc0_sigSequence)),
+		{
+			TOC0_SMALL_SEQ(0),
+			TOC0_LARGE_BIT(sizeof_field(struct toc0_sigSequence, signature)),
+			{},
+		},
+	},
+};
+
+#define TOC0_DEFAULT_NUM_ITEMS		3
+#define TOC0_DEFAULT_HEADER_LEN						  \
+	ALIGN(								  \
+		sizeof(struct toc0_main_info)				+ \
+		sizeof(struct toc0_item_info) *	TOC0_DEFAULT_NUM_ITEMS	+ \
+		sizeof(struct toc0_cert_item)				+ \
+		sizeof(struct toc0_key_item),				  \
+	32)
+
+static char *fw_key_file   = "fw_key.pem";
+static char *key_item_file = "key_item.bin";
+static char *root_key_file = "root_key.pem";
+
+/*
+ * Create a key item in @buf, containing the public keys @root_key and @fw_key,
+ * and signed by the RSA key @root_key.
+ */
+static int toc0_create_key_item(uint8_t *buf, uint32_t *len,
+				RSA *root_key, RSA *fw_key)
+{
+	struct toc0_key_item *key_item = (void *)buf;
+	uint8_t digest[SHA256_DIGEST_LENGTH];
+	int ret = EXIT_FAILURE;
+	unsigned int sig_len;
+	int n_len, e_len;
+
+	/* Store key 0. */
+	n_len = BN_bn2bin(RSA_get0_n(root_key), key_item->key0);
+	e_len = BN_bn2bin(RSA_get0_e(root_key), key_item->key0 + n_len);
+	if (n_len + e_len > sizeof(key_item->key0)) {
+		pr_err("Root key is too big for key item\n");
+		goto err;
+	}
+	key_item->key0_n_len = cpu_to_le32(n_len);
+	key_item->key0_e_len = cpu_to_le32(e_len);
+
+	/* Store key 1. */
+	n_len = BN_bn2bin(RSA_get0_n(fw_key), key_item->key1);
+	e_len = BN_bn2bin(RSA_get0_e(fw_key), key_item->key1 + n_len);
+	if (n_len + e_len > sizeof(key_item->key1)) {
+		pr_err("Firmware key is too big for key item\n");
+		goto err;
+	}
+	key_item->key1_n_len = cpu_to_le32(n_len);
+	key_item->key1_e_len = cpu_to_le32(e_len);
+
+	/* Sign the key item. */
+	key_item->sig_len = cpu_to_le32(RSA_size(root_key));
+	SHA256(buf, key_item->sig - buf, digest);
+	if (!RSA_sign(NID_sha256, digest, sizeof(digest),
+		      key_item->sig, &sig_len, root_key)) {
+		pr_err("Failed to sign key item\n");
+		goto err;
+	}
+	if (sig_len != sizeof(key_item->sig)) {
+		pr_err("Bad key item signature length\n");
+		goto err;
+	}
+
+	*len = sizeof(*key_item);
+	ret = EXIT_SUCCESS;
+
+err:
+	return ret;
+}
+
+/*
+ * Verify the key item in @buf, containing two public keys @key0 and @key1,
+ * and signed by the RSA key @key0. If @root_key is provided, only signatures
+ * by that key will be accepted. @key1 is returned in @key.
+ */
+static int toc0_verify_key_item(const uint8_t *buf, uint32_t len,
+				RSA *root_key, RSA **fw_key)
+{
+	struct toc0_key_item *key_item = (void *)buf;
+	uint8_t digest[SHA256_DIGEST_LENGTH];
+	int ret = EXIT_FAILURE;
+	int n_len, e_len;
+	RSA *key0 = NULL;
+	RSA *key1 = NULL;
+	BIGNUM *n, *e;
+
+	if (len < sizeof(*key_item))
+		goto err;
+
+	/* Load key 0. */
+	n_len = le32_to_cpu(key_item->key0_n_len);
+	e_len = le32_to_cpu(key_item->key0_e_len);
+	if (n_len + e_len > sizeof(key_item->key0)) {
+		pr_err("Bad root key size in key item\n");
+		goto err;
+	}
+	n = BN_bin2bn(key_item->key0, n_len, NULL);
+	e = BN_bin2bn(key_item->key0 + n_len, e_len, NULL);
+	key0 = RSA_new();
+	if (!key0)
+		goto err;
+	if (!RSA_set0_key(key0, n, e, NULL))
+		goto err;
+
+	/* If a root key was provided, compare it to key 0. */
+	if (root_key && (BN_cmp(n, RSA_get0_n(root_key)) ||
+			 BN_cmp(e, RSA_get0_e(root_key)))) {
+		pr_err("Wrong root key in key item\n");
+		goto err;
+	}
+
+	/* Verify the key item signature. */
+	SHA256(buf, key_item->sig - buf, digest);
+	if (!RSA_verify(NID_sha256, digest, sizeof(digest),
+			key_item->sig, le32_to_cpu(key_item->sig_len), key0)) {
+		pr_err("Bad key item signature\n");
+		goto err;
+	}
+
+	if (fw_key) {
+		/* Load key 1. */
+		n_len = le32_to_cpu(key_item->key1_n_len);
+		e_len = le32_to_cpu(key_item->key1_e_len);
+		if (n_len + e_len > sizeof(key_item->key1)) {
+			pr_err("Bad firmware key size in key item\n");
+			goto err;
+		}
+		n = BN_bin2bn(key_item->key1, n_len, NULL);
+		e = BN_bin2bn(key_item->key1 + n_len, e_len, NULL);
+		key1 = RSA_new();
+		if (!key1)
+			goto err;
+		if (!RSA_set0_key(key1, n, e, NULL))
+			goto err;
+
+		if (*fw_key) {
+			/* If a FW key was provided, compare it to key 1. */
+			if (BN_cmp(n, RSA_get0_n(*fw_key)) ||
+			    BN_cmp(e, RSA_get0_e(*fw_key))) {
+				pr_err("Wrong firmware key in key item\n");
+				goto err;
+			}
+		} else {
+			/* Otherwise, send key1 back to the caller. */
+			*fw_key = key1;
+			key1 = NULL;
+		}
+	}
+
+	ret = EXIT_SUCCESS;
+
+err:
+	RSA_free(key0);
+	RSA_free(key1);
+
+	return ret;
+}
+
+/*
+ * Create a certificate in @buf, describing the firmware with SHA256 digest
+ * @digest, and signed by the RSA key @fw_key.
+ */
+static int toc0_create_cert_item(uint8_t *buf, uint32_t *len, RSA *fw_key,
+				 uint8_t digest[static SHA256_DIGEST_LENGTH])
+{
+	struct toc0_cert_item *cert_item = (void *)buf;
+	uint8_t cert_digest[SHA256_DIGEST_LENGTH];
+	struct toc0_totalSequence *totalSequence;
+	struct toc0_sigSequence *sigSequence;
+	struct toc0_extension *extension;
+	struct toc0_publicKey *publicKey;
+	int ret = EXIT_FAILURE;
+	unsigned int sig_len;
+
+	memcpy(cert_item, &cert_item_template, sizeof(*cert_item));
+	*len = sizeof(*cert_item);
+
+	/*
+	 * Fill in the public key.
+	 *
+	 * Only 2048-bit RSA keys are supported. Since this uses a fixed-size
+	 * structure, it may fail for non-standard exponents.
+	 */
+	totalSequence = &cert_item->totalSequence;
+	publicKey = &totalSequence->mainSequence.subjectPublicKeyInfo.publicKey;
+	if (BN_bn2binpad(RSA_get0_n(fw_key), publicKey->n, sizeof(publicKey->n)) < 0 ||
+	    BN_bn2binpad(RSA_get0_e(fw_key), publicKey->e, sizeof(publicKey->e)) < 0) {
+		pr_err("Firmware key is too big for certificate\n");
+		goto err;
+	}
+
+	/* Fill in the firmware digest. */
+	extension = &totalSequence->mainSequence.explicit3.extension;
+	memcpy(&extension->digest, digest, SHA256_DIGEST_LENGTH);
+
+	/*
+	 * Sign the certificate.
+	 *
+	 * In older SBROM versions (and by default in newer versions),
+	 * the last 4 bytes of the certificate are not signed.
+	 *
+	 * (The buffer passed to SHA256 starts at tag_mainSequence, but
+	 *  the buffer size does not include the length of that tag.)
+	 */
+	SHA256((uint8_t *)totalSequence, sizeof(struct toc0_mainSequence), cert_digest);
+	sigSequence = &totalSequence->sigSequence;
+	if (!RSA_sign(NID_sha256, cert_digest, SHA256_DIGEST_LENGTH,
+		      sigSequence->signature, &sig_len, fw_key)) {
+		pr_err("Failed to sign certificate\n");
+		goto err;
+	}
+	if (sig_len != sizeof(sigSequence->signature)) {
+		pr_err("Bad certificate signature length\n");
+		goto err;
+	}
+
+	ret = EXIT_SUCCESS;
+
+err:
+	return ret;
+}
+
+/*
+ * Verify the certificate in @buf, describing the firmware with SHA256 digest
+ * @digest, and signed by the RSA key contained within. If @fw_key is provided,
+ * only that key will be accepted.
+ *
+ * This function is only expected to work with images created by mkimage.
+ */
+static int toc0_verify_cert_item(const uint8_t *buf, uint32_t len, RSA *fw_key,
+				 uint8_t digest[static SHA256_DIGEST_LENGTH])
+{
+	const struct toc0_cert_item *cert_item = (const void *)buf;
+	uint8_t cert_digest[SHA256_DIGEST_LENGTH];
+	const struct toc0_totalSequence *totalSequence;
+	const struct toc0_sigSequence *sigSequence;
+	const struct toc0_extension *extension;
+	const struct toc0_publicKey *publicKey;
+	int ret = EXIT_FAILURE;
+	RSA *key = NULL;
+	BIGNUM *n, *e;
+
+	/* Extract the public key from the certificate. */
+	totalSequence = &cert_item->totalSequence;
+	publicKey = &totalSequence->mainSequence.subjectPublicKeyInfo.publicKey;
+	n = BN_bin2bn(publicKey->n, sizeof(publicKey->n), NULL);
+	e = BN_bin2bn(publicKey->e, sizeof(publicKey->e), NULL);
+	key = RSA_new();
+	if (!key)
+		goto err;
+	if (!RSA_set0_key(key, n, e, NULL))
+		goto err;
+
+	/* If a key was provided, compare it to the embedded key. */
+	if (fw_key && (BN_cmp(RSA_get0_n(key), RSA_get0_n(fw_key)) ||
+		       BN_cmp(RSA_get0_e(key), RSA_get0_e(fw_key)))) {
+		pr_err("Wrong firmware key in certificate\n");
+		goto err;
+	}
+
+	/* If a digest was provided, compare it to the embedded digest. */
+	extension = &totalSequence->mainSequence.explicit3.extension;
+	if (digest && memcmp(&extension->digest, digest, SHA256_DIGEST_LENGTH)) {
+		pr_err("Wrong firmware digest in certificate\n");
+		goto err;
+	}
+
+	/* Verify the certificate's signature. See the comment above. */
+	SHA256((uint8_t *)totalSequence, sizeof(struct toc0_mainSequence), cert_digest);
+	sigSequence = &totalSequence->sigSequence;
+	if (!RSA_verify(NID_sha256, cert_digest, SHA256_DIGEST_LENGTH,
+			sigSequence->signature,
+			sizeof(sigSequence->signature), key)) {
+		pr_err("Bad certificate signature\n");
+		goto err;
+	}
+
+	ret = EXIT_SUCCESS;
+
+err:
+	RSA_free(key);
+
+	return ret;
+}
+
+/*
+ * Always create a TOC0 containing 3 items. The extra item will be ignored on
+ * SoCs which do not support it.
+ */
+static int toc0_create(uint8_t *buf, uint32_t len, RSA *root_key, RSA *fw_key,
+		       uint8_t *key_item, uint32_t key_item_len,
+		       uint8_t *fw_item, uint32_t fw_item_len, uint32_t fw_addr)
+{
+	struct toc0_main_info *main_info = (void *)buf;
+	struct toc0_item_info *item_info = (void *)(main_info + 1);
+	uint8_t digest[SHA256_DIGEST_LENGTH];
+	uint32_t *buf32 = (void *)buf;
+	RSA *orig_fw_key = fw_key;
+	int ret = EXIT_FAILURE;
+	uint32_t checksum = 0;
+	uint32_t item_offset;
+	uint32_t item_length;
+	int i;
+
+	/* Hash the firmware for inclusion in the certificate. */
+	SHA256(fw_item, fw_item_len, digest);
+
+	/* Create the main TOC0 header, containing three items. */
+	memcpy(main_info->name, TOC0_MAIN_INFO_NAME, sizeof(main_info->name));
+	main_info->magic	= cpu_to_le32(TOC0_MAIN_INFO_MAGIC);
+	main_info->checksum	= cpu_to_le32(BROM_STAMP_VALUE);
+	main_info->num_items	= cpu_to_le32(TOC0_DEFAULT_NUM_ITEMS);
+	memcpy(main_info->end, TOC0_MAIN_INFO_END, sizeof(main_info->end));
+
+	/* The first item links the ROTPK to the signing key. */
+	item_offset = sizeof(*main_info) +
+		      sizeof(*item_info) * TOC0_DEFAULT_NUM_ITEMS;
+	/* Using an existing key item avoids needing the root private key. */
+	if (key_item) {
+		item_length = sizeof(*key_item);
+		if (toc0_verify_key_item(key_item, item_length,
+					 root_key, &fw_key))
+			goto err;
+		memcpy(buf + item_offset, key_item, item_length);
+	} else if (toc0_create_key_item(buf + item_offset, &item_length,
+					root_key, fw_key)) {
+		goto err;
+	}
+
+	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_KEY);
+	item_info->offset	= cpu_to_le32(item_offset);
+	item_info->length	= cpu_to_le32(item_length);
+	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
+
+	/* The second item contains a certificate signed by the firmware key. */
+	item_offset = item_offset + item_length;
+	if (toc0_create_cert_item(buf + item_offset, &item_length,
+				  fw_key, digest))
+		goto err;
+
+	item_info++;
+	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_CERT);
+	item_info->offset	= cpu_to_le32(item_offset);
+	item_info->length	= cpu_to_le32(item_length);
+	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
+
+	/* The third item contains the actual boot code. */
+	item_offset = ALIGN(item_offset + item_length, 32);
+	item_length = fw_item_len;
+	if (buf + item_offset != fw_item)
+		memmove(buf + item_offset, fw_item, item_length);
+
+	item_info++;
+	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_FIRMWARE);
+	item_info->offset	= cpu_to_le32(item_offset);
+	item_info->length	= cpu_to_le32(item_length);
+	item_info->load_addr	= cpu_to_le32(fw_addr);
+	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
+
+	/* Pad to the required block size with 0xff to be flash-friendly. */
+	item_offset = item_offset + item_length;
+	item_length = ALIGN(item_offset, PAD_SIZE) - item_offset;
+	memset(buf + item_offset, 0xff, item_length);
+
+	/* Fill in the total padded file length. */
+	item_offset = item_offset + item_length;
+	main_info->length = cpu_to_le32(item_offset);
+
+	/* Verify enough space was provided when creating the image. */
+	assert(len >= item_offset);
+
+	/* Calculate the checksum. Yes, it's that simple. */
+	for (i = 0; i < item_offset / 4; ++i)
+		checksum += le32_to_cpu(buf32[i]);
+	main_info->checksum = cpu_to_le32(checksum);
+
+	ret = EXIT_SUCCESS;
+
+err:
+	if (fw_key != orig_fw_key)
+		RSA_free(fw_key);
+
+	return ret;
+}
+
+static const struct toc0_item_info *
+toc0_find_item(const struct toc0_main_info *main_info, uint32_t name,
+	       uint32_t *offset, uint32_t *length)
+{
+	const struct toc0_item_info *item_info = (void *)(main_info + 1);
+	uint32_t item_offset, item_length;
+	uint32_t num_items, main_length;
+	int i;
+
+	num_items   = le32_to_cpu(main_info->num_items);
+	main_length = le32_to_cpu(main_info->length);
+
+	for (i = 0; i < num_items; ++i, ++item_info) {
+		if (le32_to_cpu(item_info->name) != name)
+			continue;
+
+		item_offset = le32_to_cpu(item_info->offset);
+		item_length = le32_to_cpu(item_info->length);
+
+		if (item_offset > main_length ||
+		    item_length > main_length - item_offset)
+			continue;
+
+		*offset = item_offset;
+		*length = item_length;
+
+		return item_info;
+	}
+
+	return NULL;
+}
+
+static int toc0_verify(const uint8_t *buf, uint32_t len, RSA *root_key)
+{
+	const struct toc0_main_info *main_info = (void *)buf;
+	const struct toc0_item_info *item_info;
+	uint8_t digest[SHA256_DIGEST_LENGTH];
+	uint32_t main_length = le32_to_cpu(main_info->length);
+	uint32_t checksum = BROM_STAMP_VALUE;
+	uint32_t *buf32 = (void *)buf;
+	uint32_t length, offset;
+	int ret = EXIT_FAILURE;
+	RSA *fw_key = NULL;
+	int i;
+
+	if (len < main_length)
+		goto err;
+
+	/* Verify the main header. */
+	if (memcmp(main_info->name, TOC0_MAIN_INFO_NAME, sizeof(main_info->name)))
+		goto err;
+	if (le32_to_cpu(main_info->magic) != TOC0_MAIN_INFO_MAGIC)
+		goto err;
+	/* Verify the checksum without modifying the buffer. */
+	for (i = 0; i < main_length / 4; ++i)
+		checksum += le32_to_cpu(buf32[i]);
+	if (checksum != 2 * le32_to_cpu(main_info->checksum))
+		goto err;
+	/* The length must be at least 512 byte aligned. */
+	if (main_length % 512)
+		goto err;
+	if (memcmp(main_info->end, TOC0_MAIN_INFO_END, sizeof(main_info->end)))
+		goto err;
+
+	/* Verify the key item if present (it is optional). */
+	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_KEY,
+				   &offset, &length);
+	if (!item_info)
+		fw_key = root_key;
+	else if (toc0_verify_key_item(buf + offset, length, root_key, &fw_key))
+		goto err;
+
+	/* Hash the firmware to compare with the certificate. */
+	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_FIRMWARE,
+				   &offset, &length);
+	if (!item_info) {
+		pr_err("Missing firmware item\n");
+		goto err;
+	}
+	SHA256(buf + offset, length, digest);
+
+	/* Verify the certificate item. */
+	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_CERT,
+				   &offset, &length);
+	if (!item_info) {
+		pr_err("Missing certificate item\n");
+		goto err;
+	}
+	if (toc0_verify_cert_item(buf + offset, length, fw_key, digest))
+		goto err;
+
+	ret = EXIT_SUCCESS;
+
+err:
+	if (fw_key != root_key)
+		RSA_free(fw_key);
+
+	return ret;
+}
+
+static int toc0_check_params(struct image_tool_params *params)
+{
+	if (!params->dflag)
+		return -EINVAL;
+
+	/*
+	 * If a key directory was provided, look for key files there.
+	 * Otherwise, look for them in the current directory. The key files are
+	 * the "quoted" terms in the description below.
+	 *
+	 * A summary of the chain of trust on most SoCs:
+	 *  1) eFuse contains a SHA256 digest of the public "root key".
+	 *  2) Private "root key" signs the certificate item (generated here).
+	 *  3) Certificate item contains a SHA256 digest of the firmware item.
+	 *
+	 * A summary of the chain of trust on the H6 (by default; a bit in the
+	 * BROM_CONFIG eFuse makes it work like above):
+	 *  1) eFuse contains a SHA256 digest of the public "root key".
+	 *  2) Private "root key" signs the "key item" (generated here).
+	 *  3) "Key item" contains the public "root key" and public "fw key".
+	 *  4) Private "fw key" signs the certificate item (generated here).
+	 *  5) Certificate item contains a SHA256 digest of the firmware item.
+	 *
+	 * This means there are three valid ways to generate a TOC0:
+	 *  1) Provide the private "root key" only. This works everywhere.
+	 *     For H6, the "root key" will also be used as the "fw key".
+	 *  2) FOR H6 ONLY: Provide the private "root key" and a separate
+	 *     private "fw key".
+	 *  3) FOR H6 ONLY: Provide the private "fw key" and a pre-existing
+	 *     "key item" containing the corresponding  public "fw key".
+	 *     In this case, the private "root key" can be kept offline. The
+	 *     "key item" can be extracted from a TOC0 image generated using
+	 *     method #2 above.
+	 *
+	 *  Note that until the ROTPK_HASH eFuse is programmed, any "root key"
+	 *  will be accepted by the BROM.
+	 */
+	if (params->keydir) {
+		if (asprintf(&fw_key_file, "%s/%s", params->keydir, fw_key_file) < 0)
+			return -ENOMEM;
+		if (asprintf(&key_item_file, "%s/%s", params->keydir, key_item_file) < 0)
+			return -ENOMEM;
+		if (asprintf(&root_key_file, "%s/%s", params->keydir, root_key_file) < 0)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int toc0_verify_header(unsigned char *buf, int image_size,
+			      struct image_tool_params *params)
+{
+	int ret = EXIT_FAILURE;
+	RSA *root_key = NULL;
+	FILE *fp;
+
+	/* A root public key is optional. */
+	fp = fopen(root_key_file, "rb");
+	if (fp) {
+		pr_info("Verifying image with existing root key\n");
+		root_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
+		if (!root_key)
+			root_key = PEM_read_RSAPublicKey(fp, NULL, NULL, NULL);
+		fclose(fp);
+		if (!root_key) {
+			pr_err("Failed to read public key from '%s'\n",
+			       root_key_file);
+			goto err;
+		}
+	}
+
+	ret = toc0_verify(buf, image_size, root_key);
+
+err:
+	RSA_free(root_key);
+
+	return ret;
+}
+
+static const char *toc0_item_name(uint32_t name)
+{
+	if (name == TOC0_ITEM_INFO_NAME_CERT)
+		return "Certificate";
+	if (name == TOC0_ITEM_INFO_NAME_FIRMWARE)
+		return "Firmware";
+	if (name == TOC0_ITEM_INFO_NAME_KEY)
+		return "Key";
+	return "(unknown)";
+}
+
+static void toc0_print_header(const void *buf)
+{
+	const struct toc0_main_info *main_info = buf;
+	const struct toc0_item_info *item_info = (void *)(main_info + 1);
+	uint32_t head_length, main_length, num_items;
+	uint32_t item_offset, item_length, item_name;
+	int load_addr = -1;
+	int i;
+
+	num_items   = le32_to_cpu(main_info->num_items);
+	head_length = sizeof(*main_info) + num_items * sizeof(*item_info);
+	main_length = le32_to_cpu(main_info->length);
+
+	printf("Allwinner TOC0 Image\n"
+	       "Size: %d bytes\n"
+	       "Contents: %d items\n"
+	       " 00000000:%08x Headers\n",
+	       main_length, num_items, head_length);
+
+	for (i = 0; i < num_items; ++i, ++item_info) {
+		item_offset = le32_to_cpu(item_info->offset);
+		item_length = le32_to_cpu(item_info->length);
+		item_name   = le32_to_cpu(item_info->name);
+
+		if (item_name == TOC0_ITEM_INFO_NAME_FIRMWARE)
+			load_addr = le32_to_cpu(item_info->load_addr);
+
+		printf(" %08x:%08x %s\n",
+		       item_offset, item_length,
+		       toc0_item_name(item_name));
+	}
+
+	if (num_items && item_offset + item_length < main_length) {
+		item_offset = item_offset + item_length;
+		item_length = main_length - item_offset;
+
+		printf(" %08x:%08x Padding\n",
+		       item_offset, item_length);
+	}
+
+	if (load_addr != -1)
+		printf("Load address: 0x%08x\n", load_addr);
+}
+
+static void toc0_set_header(void *buf, struct stat *sbuf, int ifd,
+			    struct image_tool_params *params)
+{
+	uint32_t key_item_len = 0;
+	uint8_t *key_item = NULL;
+	int ret = EXIT_FAILURE;
+	RSA *root_key = NULL;
+	RSA *fw_key = NULL;
+	FILE *fp;
+
+	/* Either a key item or the root private key is required. */
+	fp = fopen(key_item_file, "rb");
+	if (fp) {
+		pr_info("Creating image using existing key item\n");
+		key_item_len = sizeof(struct toc0_key_item);
+		key_item = OPENSSL_malloc(key_item_len);
+		if (!key_item || fread(key_item, key_item_len, 1, fp) != 1) {
+			pr_err("Failed to read key item from '%s'\n",
+			       root_key_file);
+			goto err;
+		}
+		fclose(fp);
+		fp = NULL;
+	}
+
+	fp = fopen(root_key_file, "rb");
+	if (fp) {
+		root_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
+		if (!root_key)
+			root_key = PEM_read_RSAPublicKey(fp, NULL, NULL, NULL);
+		fclose(fp);
+		fp = NULL;
+	}
+
+	/* When using an existing key item, the root key is optional. */
+	if (!key_item && (!root_key || !RSA_get0_d(root_key))) {
+		pr_err("Failed to read private key from '%s'\n",
+		       root_key_file);
+		pr_info("Try 'openssl genrsa -out root_key.pem'\n");
+		goto err;
+	}
+
+	/* The certificate/firmware private key is always required. */
+	fp = fopen(fw_key_file, "rb");
+	if (fp) {
+		fw_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
+		fclose(fp);
+		fp = NULL;
+	}
+	if (!fw_key) {
+		/* If the root key is a private key, it can be used instead. */
+		if (root_key && RSA_get0_d(root_key)) {
+			pr_info("Using root key as firmware key\n");
+			fw_key = root_key;
+		} else {
+			pr_err("Failed to read private key from '%s'\n",
+			       fw_key_file);
+			goto err;
+		}
+	}
+
+	/* Warn about potential compatibility issues. */
+	if (key_item || fw_key != root_key)
+		pr_warn("Only H6 supports separate root and firmware keys\n");
+
+	ret = toc0_create(buf, params->file_size, root_key, fw_key,
+			  key_item, key_item_len,
+			  buf + TOC0_DEFAULT_HEADER_LEN,
+			  params->orig_file_size, params->addr);
+
+err:
+	OPENSSL_free(key_item);
+	OPENSSL_free(root_key);
+	if (fw_key != root_key)
+		OPENSSL_free(fw_key);
+	if (fp)
+		fclose(fp);
+
+	if (ret != EXIT_SUCCESS)
+		exit(ret);
+}
+
+static int toc0_check_image_type(uint8_t type)
+{
+	return type == IH_TYPE_SUNXI_TOC0 ? 0 : 1;
+}
+
+static int toc0_vrec_header(struct image_tool_params *params,
+			    struct image_type_params *tparams)
+{
+	tparams->hdr = calloc(tparams->header_size, 1);
+
+	/* Save off the unpadded data size for SHA256 calculation. */
+	params->orig_file_size = params->file_size - TOC0_DEFAULT_HEADER_LEN;
+
+	/* Return padding to 8K blocks. */
+	return ALIGN(params->file_size, PAD_SIZE) - params->file_size;
+}
+
+U_BOOT_IMAGE_TYPE(
+	sunxi_toc0,
+	"Allwinner TOC0 Boot Image support",
+	TOC0_DEFAULT_HEADER_LEN,
+	NULL,
+	toc0_check_params,
+	toc0_verify_header,
+	toc0_print_header,
+	toc0_set_header,
+	NULL,
+	toc0_check_image_type,
+	NULL,
+	toc0_vrec_header
+);
-- 
2.32.0

Re: [PATCH v4 2/4] tools: mkimage: Add Allwinner TOC0 support

[Andre Przywara]

On Tue, 19 Oct 2021 21:44:52 -0500
Samuel Holland <samuel@sholland.org> wrote:

> Most Allwinner sunxi SoCs have separate boot ROMs in non-secure and
> secure mode. The "non-secure" or "normal" boot ROM (NBROM) uses the
> existing sunxi_egon image type. The secure boot ROM (SBROM) uses a
> completely different image type, known as TOC0.
> 
> A TOC0 image is composed of a header and two or more items. One item
> is the firmware binary. The others form a chain linking the firmware
> signature to the root-of-trust public key (ROTPK), which has its hash
> burned in the SoC's eFuses. Signatures are made using RSA-2048 + SHA256.
> 
> The pseudo-ASN.1 structure is manually assembled; this is done to work
> around bugs/quirks in the boot ROM, which vary between SoCs. This TOC0
> implementation has been verified to work with the A50, A64, H5, H6,
> and H616 SBROMs, and it may work with other SoCs.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

So without understanding anything from the crypto stuff (just knowing
that it's all slightly non-standard thanks to Allwinner), this looks
good to me. Tested on a Remix Mini PC (which ships with the secure fuse
burnt).
Thanks for the changes!

Acked-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
> (no changes since v3)
> 
> Changes in v3:
>  - Removed TOOLS_LIBCRYPTO selection for sunxi, since most boards
>    do not need it
>  - Added __packed to all new "ABI" structs
>  - Added entry to MAINTAINERS for sunxi tools
> 
> Changes in v2:
>  - Moved certificate and key item structures out of sunxi_image.h
>  - Renamed "main" and "item" variables for clarity
>  - Improved error messages, and added a hint about key generation
>  - Added a comment explaining the purpose of the various key files
>  - Mentioned testing this code on A50 in the commit message
> 
>  MAINTAINERS           |   1 +
>  common/image.c        |   1 +
>  include/image.h       |   1 +
>  include/sunxi_image.h |  37 ++
>  tools/Makefile        |   3 +-
>  tools/sunxi_toc0.c    | 907 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 949 insertions(+), 1 deletion(-)
>  create mode 100644 tools/sunxi_toc0.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71f468c00a..0d62829f51 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -475,6 +475,7 @@ F:	board/sunxi/
>  F:	drivers/clk/sunxi/
>  F:	drivers/phy/allwinner/
>  F:	drivers/video/sunxi/
> +F:	tools/sunxi*
>  
>  ARM TEGRA
>  M:	Tom Warren <twarren@nvidia.com>
> diff --git a/common/image.c b/common/image.c
> index 3fa60b5827..d15b47ebbe 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -185,6 +185,7 @@ static const table_entry_t uimage_type[] = {
>  	{	IH_TYPE_MTKIMAGE,   "mtk_image",   "MediaTek BootROM loadable Image" },
>  	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
>  	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
> +	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
>  	{	-1,		    "",		  "",			},
>  };
>  
> diff --git a/include/image.h b/include/image.h
> index 34d13ada84..1547246ec8 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -227,6 +227,7 @@ enum {
>  	IH_TYPE_IMX8IMAGE,		/* Freescale IMX8Boot Image	*/
>  	IH_TYPE_COPRO,			/* Coprocessor Image for remoteproc*/
>  	IH_TYPE_SUNXI_EGON,		/* Allwinner eGON Boot Image */
> +	IH_TYPE_SUNXI_TOC0,		/* Allwinner TOC0 Boot Image */
>  
>  	IH_TYPE_COUNT,			/* Number of image types */
>  };
> diff --git a/include/sunxi_image.h b/include/sunxi_image.h
> index 5b2055c0af..379ca9196e 100644
> --- a/include/sunxi_image.h
> +++ b/include/sunxi_image.h
> @@ -9,9 +9,13 @@
>   *
>   * Shared between mkimage and the SPL.
>   */
> +
>  #ifndef	SUNXI_IMAGE_H
>  #define	SUNXI_IMAGE_H
>  
> +#include <linux/compiler_attributes.h>
> +#include <linux/types.h>
> +
>  #define BOOT0_MAGIC		"eGON.BT0"
>  #define BROM_STAMP_VALUE	0x5f0a6c39
>  #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
> @@ -79,4 +83,37 @@ struct boot_file_head {
>  /* Compile time check to assure proper alignment of structure */
>  typedef char boot_file_head_not_multiple_of_32[1 - 2*(sizeof(struct boot_file_head) % 32)];
>  
> +struct __packed toc0_main_info {
> +	uint8_t	name[8];
> +	__le32	magic;
> +	__le32	checksum;
> +	__le32	serial;
> +	__le32	status;
> +	__le32	num_items;
> +	__le32	length;
> +	uint8_t	platform[4];
> +	uint8_t	reserved[8];
> +	uint8_t	end[4];
> +};
> +
> +#define TOC0_MAIN_INFO_NAME		"TOC0.GLH"
> +#define TOC0_MAIN_INFO_MAGIC		0x89119800
> +#define TOC0_MAIN_INFO_END		"MIE;"
> +
> +struct __packed toc0_item_info {
> +	__le32	name;
> +	__le32	offset;
> +	__le32	length;
> +	__le32	status;
> +	__le32	type;
> +	__le32	load_addr;
> +	uint8_t	reserved[4];
> +	uint8_t	end[4];
> +};
> +
> +#define TOC0_ITEM_INFO_NAME_CERT	0x00010101
> +#define TOC0_ITEM_INFO_NAME_FIRMWARE	0x00010202
> +#define TOC0_ITEM_INFO_NAME_KEY		0x00010303
> +#define TOC0_ITEM_INFO_END		"IIE;"
> +
>  #endif
> diff --git a/tools/Makefile b/tools/Makefile
> index a9b3d982d8..e2aeb097aa 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -98,7 +98,8 @@ AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \
>  LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \
>  			lib/fdt-libcrypto.o \
>  			kwbimage.o \
> -			mxsimage.o
> +			mxsimage.o \
> +			sunxi_toc0.o
>  
>  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
>  
> diff --git a/tools/sunxi_toc0.c b/tools/sunxi_toc0.c
> new file mode 100644
> index 0000000000..58a6e7a0a1
> --- /dev/null
> +++ b/tools/sunxi_toc0.c
> @@ -0,0 +1,907 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2018 Arm Ltd.
> + * (C) Copyright 2020-2021 Samuel Holland <samuel@sholland.org>
> + */
> +
> +#include <assert.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <openssl/asn1t.h>
> +#include <openssl/pem.h>
> +#include <openssl/rsa.h>
> +
> +#include <image.h>
> +#include <sunxi_image.h>
> +
> +#include "imagetool.h"
> +#include "mkimage.h"
> +
> +/*
> + * NAND requires 8K padding. For other devices, BROM requires only
> + * 512B padding, but let's use the larger padding to cover everything.
> + */
> +#define PAD_SIZE		8192
> +
> +#define pr_fmt(fmt)		"mkimage (TOC0): %s: " fmt
> +#define pr_err(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "error", ##args)
> +#define pr_warn(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "warning", ##args)
> +#define pr_info(fmt, args...)	fprintf(stderr, pr_fmt(fmt), "info", ##args)
> +
> +struct __packed toc0_key_item {
> +	__le32  vendor_id;
> +	__le32  key0_n_len;
> +	__le32  key0_e_len;
> +	__le32  key1_n_len;
> +	__le32  key1_e_len;
> +	__le32  sig_len;
> +	uint8_t key0[512];
> +	uint8_t key1[512];
> +	uint8_t reserved[32];
> +	uint8_t sig[256];
> +};
> +
> +/*
> + * This looks somewhat like an X.509 certificate, but it is not valid BER.
> + *
> + * Some differences:
> + *  - Some X.509 certificate fields are missing or rearranged.
> + *  - Some sequences have the wrong tag.
> + *  - Zero-length sequences are accepted.
> + *  - Large strings and integers must be an even number of bytes long.
> + *  - Positive integers are not zero-extended to maintain their sign.
> + *
> + * See https://linux-sunxi.org/TOC0 for more information.
> + */
> +struct __packed toc0_small_tag {
> +	uint8_t tag;
> +	uint8_t length;
> +};
> +
> +typedef struct toc0_small_tag toc0_small_int;
> +typedef struct toc0_small_tag toc0_small_oct;
> +typedef struct toc0_small_tag toc0_small_seq;
> +typedef struct toc0_small_tag toc0_small_exp;
> +
> +#define TOC0_SMALL_INT(len) { 0x02, (len) }
> +#define TOC0_SMALL_SEQ(len) { 0x30, (len) }
> +#define TOC0_SMALL_EXP(tag, len) { 0xa0 | (tag), len }
> +
> +struct __packed toc0_large_tag {
> +	uint8_t tag;
> +	uint8_t prefix;
> +	uint8_t length_hi;
> +	uint8_t length_lo;
> +};
> +
> +typedef struct toc0_large_tag toc0_large_int;
> +typedef struct toc0_large_tag toc0_large_bit;
> +typedef struct toc0_large_tag toc0_large_seq;
> +
> +#define TOC0_LARGE_INT(len) { 0x02, 0x82, (len) >> 8, (len) & 0xff }
> +#define TOC0_LARGE_BIT(len) { 0x03, 0x82, (len) >> 8, (len) & 0xff }
> +#define TOC0_LARGE_SEQ(len) { 0x30, 0x82, (len) >> 8, (len) & 0xff }
> +
> +struct __packed toc0_cert_item {
> +	toc0_large_seq tag_totalSequence;
> +	struct __packed toc0_totalSequence {
> +		toc0_large_seq tag_mainSequence;
> +		struct __packed toc0_mainSequence {
> +			toc0_small_exp tag_explicit0;
> +			struct __packed toc0_explicit0 {
> +				toc0_small_int tag_version;
> +				uint8_t version;
> +			} explicit0;
> +			toc0_small_int tag_serialNumber;
> +			uint8_t serialNumber;
> +			toc0_small_seq tag_signature;
> +			toc0_small_seq tag_issuer;
> +			toc0_small_seq tag_validity;
> +			toc0_small_seq tag_subject;
> +			toc0_large_seq tag_subjectPublicKeyInfo;
> +			struct __packed toc0_subjectPublicKeyInfo {
> +				toc0_small_seq tag_algorithm;
> +				toc0_large_seq tag_publicKey;
> +				struct __packed toc0_publicKey {
> +					toc0_large_int tag_n;
> +					uint8_t n[256];
> +					toc0_small_int tag_e;
> +					uint8_t e[3];
> +				} publicKey;
> +			} subjectPublicKeyInfo;
> +			toc0_small_exp tag_explicit3;
> +			struct __packed toc0_explicit3 {
> +				toc0_small_seq tag_extension;
> +				struct __packed toc0_extension {
> +					toc0_small_int tag_digest;
> +					uint8_t digest[32];
> +				} extension;
> +			} explicit3;
> +		} mainSequence;
> +		toc0_large_bit tag_sigSequence;
> +		struct __packed toc0_sigSequence {
> +			toc0_small_seq tag_algorithm;
> +			toc0_large_bit tag_signature;
> +			uint8_t signature[256];
> +		} sigSequence;
> +	} totalSequence;
> +};
> +
> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> +
> +static const struct toc0_cert_item cert_item_template = {
> +	TOC0_LARGE_SEQ(sizeof(struct toc0_totalSequence)),
> +	{
> +		TOC0_LARGE_SEQ(sizeof(struct toc0_mainSequence)),
> +		{
> +			TOC0_SMALL_EXP(0, sizeof(struct toc0_explicit0)),
> +			{
> +				TOC0_SMALL_INT(sizeof_field(struct toc0_explicit0, version)),
> +				0,
> +			},
> +			TOC0_SMALL_INT(sizeof_field(struct toc0_mainSequence, serialNumber)),
> +			0,
> +			TOC0_SMALL_SEQ(0),
> +			TOC0_SMALL_SEQ(0),
> +			TOC0_SMALL_SEQ(0),
> +			TOC0_SMALL_SEQ(0),
> +			TOC0_LARGE_SEQ(sizeof(struct toc0_subjectPublicKeyInfo)),
> +			{
> +				TOC0_SMALL_SEQ(0),
> +				TOC0_LARGE_SEQ(sizeof(struct toc0_publicKey)),
> +				{
> +					TOC0_LARGE_INT(sizeof_field(struct toc0_publicKey, n)),
> +					{},
> +					TOC0_SMALL_INT(sizeof_field(struct toc0_publicKey, e)),
> +					{},
> +				},
> +			},
> +			TOC0_SMALL_EXP(3, sizeof(struct toc0_explicit3)),
> +			{
> +				TOC0_SMALL_SEQ(sizeof(struct toc0_extension)),
> +				{
> +					TOC0_SMALL_INT(sizeof_field(struct toc0_extension, digest)),
> +					{},
> +				},
> +			},
> +		},
> +		TOC0_LARGE_BIT(sizeof(struct toc0_sigSequence)),
> +		{
> +			TOC0_SMALL_SEQ(0),
> +			TOC0_LARGE_BIT(sizeof_field(struct toc0_sigSequence, signature)),
> +			{},
> +		},
> +	},
> +};
> +
> +#define TOC0_DEFAULT_NUM_ITEMS		3
> +#define TOC0_DEFAULT_HEADER_LEN						  \
> +	ALIGN(								  \
> +		sizeof(struct toc0_main_info)				+ \
> +		sizeof(struct toc0_item_info) *	TOC0_DEFAULT_NUM_ITEMS	+ \
> +		sizeof(struct toc0_cert_item)				+ \
> +		sizeof(struct toc0_key_item),				  \
> +	32)
> +
> +static char *fw_key_file   = "fw_key.pem";
> +static char *key_item_file = "key_item.bin";
> +static char *root_key_file = "root_key.pem";
> +
> +/*
> + * Create a key item in @buf, containing the public keys @root_key and @fw_key,
> + * and signed by the RSA key @root_key.
> + */
> +static int toc0_create_key_item(uint8_t *buf, uint32_t *len,
> +				RSA *root_key, RSA *fw_key)
> +{
> +	struct toc0_key_item *key_item = (void *)buf;
> +	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	int ret = EXIT_FAILURE;
> +	unsigned int sig_len;
> +	int n_len, e_len;
> +
> +	/* Store key 0. */
> +	n_len = BN_bn2bin(RSA_get0_n(root_key), key_item->key0);
> +	e_len = BN_bn2bin(RSA_get0_e(root_key), key_item->key0 + n_len);
> +	if (n_len + e_len > sizeof(key_item->key0)) {
> +		pr_err("Root key is too big for key item\n");
> +		goto err;
> +	}
> +	key_item->key0_n_len = cpu_to_le32(n_len);
> +	key_item->key0_e_len = cpu_to_le32(e_len);
> +
> +	/* Store key 1. */
> +	n_len = BN_bn2bin(RSA_get0_n(fw_key), key_item->key1);
> +	e_len = BN_bn2bin(RSA_get0_e(fw_key), key_item->key1 + n_len);
> +	if (n_len + e_len > sizeof(key_item->key1)) {
> +		pr_err("Firmware key is too big for key item\n");
> +		goto err;
> +	}
> +	key_item->key1_n_len = cpu_to_le32(n_len);
> +	key_item->key1_e_len = cpu_to_le32(e_len);
> +
> +	/* Sign the key item. */
> +	key_item->sig_len = cpu_to_le32(RSA_size(root_key));
> +	SHA256(buf, key_item->sig - buf, digest);
> +	if (!RSA_sign(NID_sha256, digest, sizeof(digest),
> +		      key_item->sig, &sig_len, root_key)) {
> +		pr_err("Failed to sign key item\n");
> +		goto err;
> +	}
> +	if (sig_len != sizeof(key_item->sig)) {
> +		pr_err("Bad key item signature length\n");
> +		goto err;
> +	}
> +
> +	*len = sizeof(*key_item);
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	return ret;
> +}
> +
> +/*
> + * Verify the key item in @buf, containing two public keys @key0 and @key1,
> + * and signed by the RSA key @key0. If @root_key is provided, only signatures
> + * by that key will be accepted. @key1 is returned in @key.
> + */
> +static int toc0_verify_key_item(const uint8_t *buf, uint32_t len,
> +				RSA *root_key, RSA **fw_key)
> +{
> +	struct toc0_key_item *key_item = (void *)buf;
> +	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	int ret = EXIT_FAILURE;
> +	int n_len, e_len;
> +	RSA *key0 = NULL;
> +	RSA *key1 = NULL;
> +	BIGNUM *n, *e;
> +
> +	if (len < sizeof(*key_item))
> +		goto err;
> +
> +	/* Load key 0. */
> +	n_len = le32_to_cpu(key_item->key0_n_len);
> +	e_len = le32_to_cpu(key_item->key0_e_len);
> +	if (n_len + e_len > sizeof(key_item->key0)) {
> +		pr_err("Bad root key size in key item\n");
> +		goto err;
> +	}
> +	n = BN_bin2bn(key_item->key0, n_len, NULL);
> +	e = BN_bin2bn(key_item->key0 + n_len, e_len, NULL);
> +	key0 = RSA_new();
> +	if (!key0)
> +		goto err;
> +	if (!RSA_set0_key(key0, n, e, NULL))
> +		goto err;
> +
> +	/* If a root key was provided, compare it to key 0. */
> +	if (root_key && (BN_cmp(n, RSA_get0_n(root_key)) ||
> +			 BN_cmp(e, RSA_get0_e(root_key)))) {
> +		pr_err("Wrong root key in key item\n");
> +		goto err;
> +	}
> +
> +	/* Verify the key item signature. */
> +	SHA256(buf, key_item->sig - buf, digest);
> +	if (!RSA_verify(NID_sha256, digest, sizeof(digest),
> +			key_item->sig, le32_to_cpu(key_item->sig_len), key0)) {
> +		pr_err("Bad key item signature\n");
> +		goto err;
> +	}
> +
> +	if (fw_key) {
> +		/* Load key 1. */
> +		n_len = le32_to_cpu(key_item->key1_n_len);
> +		e_len = le32_to_cpu(key_item->key1_e_len);
> +		if (n_len + e_len > sizeof(key_item->key1)) {
> +			pr_err("Bad firmware key size in key item\n");
> +			goto err;
> +		}
> +		n = BN_bin2bn(key_item->key1, n_len, NULL);
> +		e = BN_bin2bn(key_item->key1 + n_len, e_len, NULL);
> +		key1 = RSA_new();
> +		if (!key1)
> +			goto err;
> +		if (!RSA_set0_key(key1, n, e, NULL))
> +			goto err;
> +
> +		if (*fw_key) {
> +			/* If a FW key was provided, compare it to key 1. */
> +			if (BN_cmp(n, RSA_get0_n(*fw_key)) ||
> +			    BN_cmp(e, RSA_get0_e(*fw_key))) {
> +				pr_err("Wrong firmware key in key item\n");
> +				goto err;
> +			}
> +		} else {
> +			/* Otherwise, send key1 back to the caller. */
> +			*fw_key = key1;
> +			key1 = NULL;
> +		}
> +	}
> +
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	RSA_free(key0);
> +	RSA_free(key1);
> +
> +	return ret;
> +}
> +
> +/*
> + * Create a certificate in @buf, describing the firmware with SHA256 digest
> + * @digest, and signed by the RSA key @fw_key.
> + */
> +static int toc0_create_cert_item(uint8_t *buf, uint32_t *len, RSA *fw_key,
> +				 uint8_t digest[static SHA256_DIGEST_LENGTH])
> +{
> +	struct toc0_cert_item *cert_item = (void *)buf;
> +	uint8_t cert_digest[SHA256_DIGEST_LENGTH];
> +	struct toc0_totalSequence *totalSequence;
> +	struct toc0_sigSequence *sigSequence;
> +	struct toc0_extension *extension;
> +	struct toc0_publicKey *publicKey;
> +	int ret = EXIT_FAILURE;
> +	unsigned int sig_len;
> +
> +	memcpy(cert_item, &cert_item_template, sizeof(*cert_item));
> +	*len = sizeof(*cert_item);
> +
> +	/*
> +	 * Fill in the public key.
> +	 *
> +	 * Only 2048-bit RSA keys are supported. Since this uses a fixed-size
> +	 * structure, it may fail for non-standard exponents.
> +	 */
> +	totalSequence = &cert_item->totalSequence;
> +	publicKey = &totalSequence->mainSequence.subjectPublicKeyInfo.publicKey;
> +	if (BN_bn2binpad(RSA_get0_n(fw_key), publicKey->n, sizeof(publicKey->n)) < 0 ||
> +	    BN_bn2binpad(RSA_get0_e(fw_key), publicKey->e, sizeof(publicKey->e)) < 0) {
> +		pr_err("Firmware key is too big for certificate\n");
> +		goto err;
> +	}
> +
> +	/* Fill in the firmware digest. */
> +	extension = &totalSequence->mainSequence.explicit3.extension;
> +	memcpy(&extension->digest, digest, SHA256_DIGEST_LENGTH);
> +
> +	/*
> +	 * Sign the certificate.
> +	 *
> +	 * In older SBROM versions (and by default in newer versions),
> +	 * the last 4 bytes of the certificate are not signed.
> +	 *
> +	 * (The buffer passed to SHA256 starts at tag_mainSequence, but
> +	 *  the buffer size does not include the length of that tag.)
> +	 */
> +	SHA256((uint8_t *)totalSequence, sizeof(struct toc0_mainSequence), cert_digest);
> +	sigSequence = &totalSequence->sigSequence;
> +	if (!RSA_sign(NID_sha256, cert_digest, SHA256_DIGEST_LENGTH,
> +		      sigSequence->signature, &sig_len, fw_key)) {
> +		pr_err("Failed to sign certificate\n");
> +		goto err;
> +	}
> +	if (sig_len != sizeof(sigSequence->signature)) {
> +		pr_err("Bad certificate signature length\n");
> +		goto err;
> +	}
> +
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	return ret;
> +}
> +
> +/*
> + * Verify the certificate in @buf, describing the firmware with SHA256 digest
> + * @digest, and signed by the RSA key contained within. If @fw_key is provided,
> + * only that key will be accepted.
> + *
> + * This function is only expected to work with images created by mkimage.
> + */
> +static int toc0_verify_cert_item(const uint8_t *buf, uint32_t len, RSA *fw_key,
> +				 uint8_t digest[static SHA256_DIGEST_LENGTH])
> +{
> +	const struct toc0_cert_item *cert_item = (const void *)buf;
> +	uint8_t cert_digest[SHA256_DIGEST_LENGTH];
> +	const struct toc0_totalSequence *totalSequence;
> +	const struct toc0_sigSequence *sigSequence;
> +	const struct toc0_extension *extension;
> +	const struct toc0_publicKey *publicKey;
> +	int ret = EXIT_FAILURE;
> +	RSA *key = NULL;
> +	BIGNUM *n, *e;
> +
> +	/* Extract the public key from the certificate. */
> +	totalSequence = &cert_item->totalSequence;
> +	publicKey = &totalSequence->mainSequence.subjectPublicKeyInfo.publicKey;
> +	n = BN_bin2bn(publicKey->n, sizeof(publicKey->n), NULL);
> +	e = BN_bin2bn(publicKey->e, sizeof(publicKey->e), NULL);
> +	key = RSA_new();
> +	if (!key)
> +		goto err;
> +	if (!RSA_set0_key(key, n, e, NULL))
> +		goto err;
> +
> +	/* If a key was provided, compare it to the embedded key. */
> +	if (fw_key && (BN_cmp(RSA_get0_n(key), RSA_get0_n(fw_key)) ||
> +		       BN_cmp(RSA_get0_e(key), RSA_get0_e(fw_key)))) {
> +		pr_err("Wrong firmware key in certificate\n");
> +		goto err;
> +	}
> +
> +	/* If a digest was provided, compare it to the embedded digest. */
> +	extension = &totalSequence->mainSequence.explicit3.extension;
> +	if (digest && memcmp(&extension->digest, digest, SHA256_DIGEST_LENGTH)) {
> +		pr_err("Wrong firmware digest in certificate\n");
> +		goto err;
> +	}
> +
> +	/* Verify the certificate's signature. See the comment above. */
> +	SHA256((uint8_t *)totalSequence, sizeof(struct toc0_mainSequence), cert_digest);
> +	sigSequence = &totalSequence->sigSequence;
> +	if (!RSA_verify(NID_sha256, cert_digest, SHA256_DIGEST_LENGTH,
> +			sigSequence->signature,
> +			sizeof(sigSequence->signature), key)) {
> +		pr_err("Bad certificate signature\n");
> +		goto err;
> +	}
> +
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	RSA_free(key);
> +
> +	return ret;
> +}
> +
> +/*
> + * Always create a TOC0 containing 3 items. The extra item will be ignored on
> + * SoCs which do not support it.
> + */
> +static int toc0_create(uint8_t *buf, uint32_t len, RSA *root_key, RSA *fw_key,
> +		       uint8_t *key_item, uint32_t key_item_len,
> +		       uint8_t *fw_item, uint32_t fw_item_len, uint32_t fw_addr)
> +{
> +	struct toc0_main_info *main_info = (void *)buf;
> +	struct toc0_item_info *item_info = (void *)(main_info + 1);
> +	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	uint32_t *buf32 = (void *)buf;
> +	RSA *orig_fw_key = fw_key;
> +	int ret = EXIT_FAILURE;
> +	uint32_t checksum = 0;
> +	uint32_t item_offset;
> +	uint32_t item_length;
> +	int i;
> +
> +	/* Hash the firmware for inclusion in the certificate. */
> +	SHA256(fw_item, fw_item_len, digest);
> +
> +	/* Create the main TOC0 header, containing three items. */
> +	memcpy(main_info->name, TOC0_MAIN_INFO_NAME, sizeof(main_info->name));
> +	main_info->magic	= cpu_to_le32(TOC0_MAIN_INFO_MAGIC);
> +	main_info->checksum	= cpu_to_le32(BROM_STAMP_VALUE);
> +	main_info->num_items	= cpu_to_le32(TOC0_DEFAULT_NUM_ITEMS);
> +	memcpy(main_info->end, TOC0_MAIN_INFO_END, sizeof(main_info->end));
> +
> +	/* The first item links the ROTPK to the signing key. */
> +	item_offset = sizeof(*main_info) +
> +		      sizeof(*item_info) * TOC0_DEFAULT_NUM_ITEMS;
> +	/* Using an existing key item avoids needing the root private key. */
> +	if (key_item) {
> +		item_length = sizeof(*key_item);
> +		if (toc0_verify_key_item(key_item, item_length,
> +					 root_key, &fw_key))
> +			goto err;
> +		memcpy(buf + item_offset, key_item, item_length);
> +	} else if (toc0_create_key_item(buf + item_offset, &item_length,
> +					root_key, fw_key)) {
> +		goto err;
> +	}
> +
> +	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_KEY);
> +	item_info->offset	= cpu_to_le32(item_offset);
> +	item_info->length	= cpu_to_le32(item_length);
> +	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
> +
> +	/* The second item contains a certificate signed by the firmware key. */
> +	item_offset = item_offset + item_length;
> +	if (toc0_create_cert_item(buf + item_offset, &item_length,
> +				  fw_key, digest))
> +		goto err;
> +
> +	item_info++;
> +	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_CERT);
> +	item_info->offset	= cpu_to_le32(item_offset);
> +	item_info->length	= cpu_to_le32(item_length);
> +	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
> +
> +	/* The third item contains the actual boot code. */
> +	item_offset = ALIGN(item_offset + item_length, 32);
> +	item_length = fw_item_len;
> +	if (buf + item_offset != fw_item)
> +		memmove(buf + item_offset, fw_item, item_length);
> +
> +	item_info++;
> +	item_info->name		= cpu_to_le32(TOC0_ITEM_INFO_NAME_FIRMWARE);
> +	item_info->offset	= cpu_to_le32(item_offset);
> +	item_info->length	= cpu_to_le32(item_length);
> +	item_info->load_addr	= cpu_to_le32(fw_addr);
> +	memcpy(item_info->end, TOC0_ITEM_INFO_END, sizeof(item_info->end));
> +
> +	/* Pad to the required block size with 0xff to be flash-friendly. */
> +	item_offset = item_offset + item_length;
> +	item_length = ALIGN(item_offset, PAD_SIZE) - item_offset;
> +	memset(buf + item_offset, 0xff, item_length);
> +
> +	/* Fill in the total padded file length. */
> +	item_offset = item_offset + item_length;
> +	main_info->length = cpu_to_le32(item_offset);
> +
> +	/* Verify enough space was provided when creating the image. */
> +	assert(len >= item_offset);
> +
> +	/* Calculate the checksum. Yes, it's that simple. */
> +	for (i = 0; i < item_offset / 4; ++i)
> +		checksum += le32_to_cpu(buf32[i]);
> +	main_info->checksum = cpu_to_le32(checksum);
> +
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	if (fw_key != orig_fw_key)
> +		RSA_free(fw_key);
> +
> +	return ret;
> +}
> +
> +static const struct toc0_item_info *
> +toc0_find_item(const struct toc0_main_info *main_info, uint32_t name,
> +	       uint32_t *offset, uint32_t *length)
> +{
> +	const struct toc0_item_info *item_info = (void *)(main_info + 1);
> +	uint32_t item_offset, item_length;
> +	uint32_t num_items, main_length;
> +	int i;
> +
> +	num_items   = le32_to_cpu(main_info->num_items);
> +	main_length = le32_to_cpu(main_info->length);
> +
> +	for (i = 0; i < num_items; ++i, ++item_info) {
> +		if (le32_to_cpu(item_info->name) != name)
> +			continue;
> +
> +		item_offset = le32_to_cpu(item_info->offset);
> +		item_length = le32_to_cpu(item_info->length);
> +
> +		if (item_offset > main_length ||
> +		    item_length > main_length - item_offset)
> +			continue;
> +
> +		*offset = item_offset;
> +		*length = item_length;
> +
> +		return item_info;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int toc0_verify(const uint8_t *buf, uint32_t len, RSA *root_key)
> +{
> +	const struct toc0_main_info *main_info = (void *)buf;
> +	const struct toc0_item_info *item_info;
> +	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	uint32_t main_length = le32_to_cpu(main_info->length);
> +	uint32_t checksum = BROM_STAMP_VALUE;
> +	uint32_t *buf32 = (void *)buf;
> +	uint32_t length, offset;
> +	int ret = EXIT_FAILURE;
> +	RSA *fw_key = NULL;
> +	int i;
> +
> +	if (len < main_length)
> +		goto err;
> +
> +	/* Verify the main header. */
> +	if (memcmp(main_info->name, TOC0_MAIN_INFO_NAME, sizeof(main_info->name)))
> +		goto err;
> +	if (le32_to_cpu(main_info->magic) != TOC0_MAIN_INFO_MAGIC)
> +		goto err;
> +	/* Verify the checksum without modifying the buffer. */
> +	for (i = 0; i < main_length / 4; ++i)
> +		checksum += le32_to_cpu(buf32[i]);
> +	if (checksum != 2 * le32_to_cpu(main_info->checksum))
> +		goto err;
> +	/* The length must be at least 512 byte aligned. */
> +	if (main_length % 512)
> +		goto err;
> +	if (memcmp(main_info->end, TOC0_MAIN_INFO_END, sizeof(main_info->end)))
> +		goto err;
> +
> +	/* Verify the key item if present (it is optional). */
> +	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_KEY,
> +				   &offset, &length);
> +	if (!item_info)
> +		fw_key = root_key;
> +	else if (toc0_verify_key_item(buf + offset, length, root_key, &fw_key))
> +		goto err;
> +
> +	/* Hash the firmware to compare with the certificate. */
> +	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_FIRMWARE,
> +				   &offset, &length);
> +	if (!item_info) {
> +		pr_err("Missing firmware item\n");
> +		goto err;
> +	}
> +	SHA256(buf + offset, length, digest);
> +
> +	/* Verify the certificate item. */
> +	item_info = toc0_find_item(main_info, TOC0_ITEM_INFO_NAME_CERT,
> +				   &offset, &length);
> +	if (!item_info) {
> +		pr_err("Missing certificate item\n");
> +		goto err;
> +	}
> +	if (toc0_verify_cert_item(buf + offset, length, fw_key, digest))
> +		goto err;
> +
> +	ret = EXIT_SUCCESS;
> +
> +err:
> +	if (fw_key != root_key)
> +		RSA_free(fw_key);
> +
> +	return ret;
> +}
> +
> +static int toc0_check_params(struct image_tool_params *params)
> +{
> +	if (!params->dflag)
> +		return -EINVAL;
> +
> +	/*
> +	 * If a key directory was provided, look for key files there.
> +	 * Otherwise, look for them in the current directory. The key files are
> +	 * the "quoted" terms in the description below.
> +	 *
> +	 * A summary of the chain of trust on most SoCs:
> +	 *  1) eFuse contains a SHA256 digest of the public "root key".
> +	 *  2) Private "root key" signs the certificate item (generated here).
> +	 *  3) Certificate item contains a SHA256 digest of the firmware item.
> +	 *
> +	 * A summary of the chain of trust on the H6 (by default; a bit in the
> +	 * BROM_CONFIG eFuse makes it work like above):
> +	 *  1) eFuse contains a SHA256 digest of the public "root key".
> +	 *  2) Private "root key" signs the "key item" (generated here).
> +	 *  3) "Key item" contains the public "root key" and public "fw key".
> +	 *  4) Private "fw key" signs the certificate item (generated here).
> +	 *  5) Certificate item contains a SHA256 digest of the firmware item.
> +	 *
> +	 * This means there are three valid ways to generate a TOC0:
> +	 *  1) Provide the private "root key" only. This works everywhere.
> +	 *     For H6, the "root key" will also be used as the "fw key".
> +	 *  2) FOR H6 ONLY: Provide the private "root key" and a separate
> +	 *     private "fw key".
> +	 *  3) FOR H6 ONLY: Provide the private "fw key" and a pre-existing
> +	 *     "key item" containing the corresponding  public "fw key".
> +	 *     In this case, the private "root key" can be kept offline. The
> +	 *     "key item" can be extracted from a TOC0 image generated using
> +	 *     method #2 above.
> +	 *
> +	 *  Note that until the ROTPK_HASH eFuse is programmed, any "root key"
> +	 *  will be accepted by the BROM.
> +	 */
> +	if (params->keydir) {
> +		if (asprintf(&fw_key_file, "%s/%s", params->keydir, fw_key_file) < 0)
> +			return -ENOMEM;
> +		if (asprintf(&key_item_file, "%s/%s", params->keydir, key_item_file) < 0)
> +			return -ENOMEM;
> +		if (asprintf(&root_key_file, "%s/%s", params->keydir, root_key_file) < 0)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int toc0_verify_header(unsigned char *buf, int image_size,
> +			      struct image_tool_params *params)
> +{
> +	int ret = EXIT_FAILURE;
> +	RSA *root_key = NULL;
> +	FILE *fp;
> +
> +	/* A root public key is optional. */
> +	fp = fopen(root_key_file, "rb");
> +	if (fp) {
> +		pr_info("Verifying image with existing root key\n");
> +		root_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
> +		if (!root_key)
> +			root_key = PEM_read_RSAPublicKey(fp, NULL, NULL, NULL);
> +		fclose(fp);
> +		if (!root_key) {
> +			pr_err("Failed to read public key from '%s'\n",
> +			       root_key_file);
> +			goto err;
> +		}
> +	}
> +
> +	ret = toc0_verify(buf, image_size, root_key);
> +
> +err:
> +	RSA_free(root_key);
> +
> +	return ret;
> +}
> +
> +static const char *toc0_item_name(uint32_t name)
> +{
> +	if (name == TOC0_ITEM_INFO_NAME_CERT)
> +		return "Certificate";
> +	if (name == TOC0_ITEM_INFO_NAME_FIRMWARE)
> +		return "Firmware";
> +	if (name == TOC0_ITEM_INFO_NAME_KEY)
> +		return "Key";
> +	return "(unknown)";
> +}
> +
> +static void toc0_print_header(const void *buf)
> +{
> +	const struct toc0_main_info *main_info = buf;
> +	const struct toc0_item_info *item_info = (void *)(main_info + 1);
> +	uint32_t head_length, main_length, num_items;
> +	uint32_t item_offset, item_length, item_name;
> +	int load_addr = -1;
> +	int i;
> +
> +	num_items   = le32_to_cpu(main_info->num_items);
> +	head_length = sizeof(*main_info) + num_items * sizeof(*item_info);
> +	main_length = le32_to_cpu(main_info->length);
> +
> +	printf("Allwinner TOC0 Image\n"
> +	       "Size: %d bytes\n"
> +	       "Contents: %d items\n"
> +	       " 00000000:%08x Headers\n",
> +	       main_length, num_items, head_length);
> +
> +	for (i = 0; i < num_items; ++i, ++item_info) {
> +		item_offset = le32_to_cpu(item_info->offset);
> +		item_length = le32_to_cpu(item_info->length);
> +		item_name   = le32_to_cpu(item_info->name);
> +
> +		if (item_name == TOC0_ITEM_INFO_NAME_FIRMWARE)
> +			load_addr = le32_to_cpu(item_info->load_addr);
> +
> +		printf(" %08x:%08x %s\n",
> +		       item_offset, item_length,
> +		       toc0_item_name(item_name));
> +	}
> +
> +	if (num_items && item_offset + item_length < main_length) {
> +		item_offset = item_offset + item_length;
> +		item_length = main_length - item_offset;
> +
> +		printf(" %08x:%08x Padding\n",
> +		       item_offset, item_length);
> +	}
> +
> +	if (load_addr != -1)
> +		printf("Load address: 0x%08x\n", load_addr);
> +}
> +
> +static void toc0_set_header(void *buf, struct stat *sbuf, int ifd,
> +			    struct image_tool_params *params)
> +{
> +	uint32_t key_item_len = 0;
> +	uint8_t *key_item = NULL;
> +	int ret = EXIT_FAILURE;
> +	RSA *root_key = NULL;
> +	RSA *fw_key = NULL;
> +	FILE *fp;
> +
> +	/* Either a key item or the root private key is required. */
> +	fp = fopen(key_item_file, "rb");
> +	if (fp) {
> +		pr_info("Creating image using existing key item\n");
> +		key_item_len = sizeof(struct toc0_key_item);
> +		key_item = OPENSSL_malloc(key_item_len);
> +		if (!key_item || fread(key_item, key_item_len, 1, fp) != 1) {
> +			pr_err("Failed to read key item from '%s'\n",
> +			       root_key_file);
> +			goto err;
> +		}
> +		fclose(fp);
> +		fp = NULL;
> +	}
> +
> +	fp = fopen(root_key_file, "rb");
> +	if (fp) {
> +		root_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
> +		if (!root_key)
> +			root_key = PEM_read_RSAPublicKey(fp, NULL, NULL, NULL);
> +		fclose(fp);
> +		fp = NULL;
> +	}
> +
> +	/* When using an existing key item, the root key is optional. */
> +	if (!key_item && (!root_key || !RSA_get0_d(root_key))) {
> +		pr_err("Failed to read private key from '%s'\n",
> +		       root_key_file);
> +		pr_info("Try 'openssl genrsa -out root_key.pem'\n");
> +		goto err;
> +	}
> +
> +	/* The certificate/firmware private key is always required. */
> +	fp = fopen(fw_key_file, "rb");
> +	if (fp) {
> +		fw_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
> +		fclose(fp);
> +		fp = NULL;
> +	}
> +	if (!fw_key) {
> +		/* If the root key is a private key, it can be used instead. */
> +		if (root_key && RSA_get0_d(root_key)) {
> +			pr_info("Using root key as firmware key\n");
> +			fw_key = root_key;
> +		} else {
> +			pr_err("Failed to read private key from '%s'\n",
> +			       fw_key_file);
> +			goto err;
> +		}
> +	}
> +
> +	/* Warn about potential compatibility issues. */
> +	if (key_item || fw_key != root_key)
> +		pr_warn("Only H6 supports separate root and firmware keys\n");
> +
> +	ret = toc0_create(buf, params->file_size, root_key, fw_key,
> +			  key_item, key_item_len,
> +			  buf + TOC0_DEFAULT_HEADER_LEN,
> +			  params->orig_file_size, params->addr);
> +
> +err:
> +	OPENSSL_free(key_item);
> +	OPENSSL_free(root_key);
> +	if (fw_key != root_key)
> +		OPENSSL_free(fw_key);
> +	if (fp)
> +		fclose(fp);
> +
> +	if (ret != EXIT_SUCCESS)
> +		exit(ret);
> +}
> +
> +static int toc0_check_image_type(uint8_t type)
> +{
> +	return type == IH_TYPE_SUNXI_TOC0 ? 0 : 1;
> +}
> +
> +static int toc0_vrec_header(struct image_tool_params *params,
> +			    struct image_type_params *tparams)
> +{
> +	tparams->hdr = calloc(tparams->header_size, 1);
> +
> +	/* Save off the unpadded data size for SHA256 calculation. */
> +	params->orig_file_size = params->file_size - TOC0_DEFAULT_HEADER_LEN;
> +
> +	/* Return padding to 8K blocks. */
> +	return ALIGN(params->file_size, PAD_SIZE) - params->file_size;
> +}
> +
> +U_BOOT_IMAGE_TYPE(
> +	sunxi_toc0,
> +	"Allwinner TOC0 Boot Image support",
> +	TOC0_DEFAULT_HEADER_LEN,
> +	NULL,
> +	toc0_check_params,
> +	toc0_verify_header,
> +	toc0_print_header,
> +	toc0_set_header,
> +	NULL,
> +	toc0_check_image_type,
> +	NULL,
> +	toc0_vrec_header
> +);

[PATCH v4 3/4] sunxi: Support SPL in both eGON and TOC0 images

[Samuel Holland]

SPL uses the image header to detect the boot device and to find the
offset of the next U-Boot stage. Since this information is stored
differently in the eGON and TOC0 image headers, add code to find the
correct value based on the image type currently in use.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v3)

Changes in v3:
 - Fixed offset of magic passed to memcmp
 - Refactored functions to not return pointers (fixes ambiguous NULL)

Changes in v2:
 - Moved SPL header signature checks out of sunxi_image.h
 - Refactored SPL header signature checks to use fewer casts

 arch/arm/include/asm/arch-sunxi/spl.h |  2 --
 arch/arm/mach-sunxi/board.c           | 34 ++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
index 58cdf806d9..157b11e489 100644
--- a/arch/arm/include/asm/arch-sunxi/spl.h
+++ b/arch/arm/include/asm/arch-sunxi/spl.h
@@ -19,8 +19,6 @@
 #define SUNXI_BOOTED_FROM_MMC0_HIGH	0x10
 #define SUNXI_BOOTED_FROM_MMC2_HIGH	0x12
 
-#define is_boot0_magic(addr)	(memcmp((void *)(addr), BOOT0_MAGIC, 8) == 0)
-
 uint32_t sunxi_get_boot_device(void);
 
 #endif
diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index b4ba2a72c4..b2cd64bb3f 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -243,12 +243,28 @@ void s_init(void)
 
 #define SUNXI_INVALID_BOOT_SOURCE	-1
 
+static int sunxi_egon_valid(struct boot_file_head *egon_head)
+{
+	return !memcmp(egon_head->magic, BOOT0_MAGIC, 8); /* eGON.BT0 */
+}
+
+static int sunxi_toc0_valid(struct toc0_main_info *toc0_info)
+{
+	return !memcmp(toc0_info->name, TOC0_MAIN_INFO_NAME, 8); /* TOC0.GLH */
+}
+
 static int sunxi_get_boot_source(void)
 {
-	if (!is_boot0_magic(SPL_ADDR + 4)) /* eGON.BT0 */
-		return SUNXI_INVALID_BOOT_SOURCE;
+	struct boot_file_head *egon_head = (void *)SPL_ADDR;
+	struct toc0_main_info *toc0_info = (void *)SPL_ADDR;
+
+	if (sunxi_egon_valid(egon_head))
+		return readb(&egon_head->boot_media);
+	if (sunxi_toc0_valid(toc0_info))
+		return readb(&toc0_info->platform[0]);
 
-	return readb(SPL_ADDR + 0x28);
+	/* Not a valid image, so we must have been booted via FEL. */
+	return SUNXI_INVALID_BOOT_SOURCE;
 }
 
 /* The sunxi internal brom will try to loader external bootloader
@@ -296,10 +312,16 @@ uint32_t sunxi_get_boot_device(void)
 #ifdef CONFIG_SPL_BUILD
 static u32 sunxi_get_spl_size(void)
 {
-	if (!is_boot0_magic(SPL_ADDR + 4)) /* eGON.BT0 */
-		return 0;
+	struct boot_file_head *egon_head = (void *)SPL_ADDR;
+	struct toc0_main_info *toc0_info = (void *)SPL_ADDR;
 
-	return readl(SPL_ADDR + 0x10);
+	if (sunxi_egon_valid(egon_head))
+		return readl(&egon_head->length);
+	if (sunxi_toc0_valid(toc0_info))
+		return readl(&toc0_info->length);
+
+	/* Not a valid image, so use the default U-Boot offset. */
+	return 0;
 }
 
 /*
-- 
2.32.0

Re: [PATCH v4 3/4] sunxi: Support SPL in both eGON and TOC0 images

[Andre Przywara]

On Tue, 19 Oct 2021 21:44:53 -0500
Samuel Holland <samuel@sholland.org> wrote:

> SPL uses the image header to detect the boot device and to find the
> offset of the next U-Boot stage. Since this information is stored
> differently in the eGON and TOC0 image headers, add code to find the
> correct value based on the image type currently in use.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Many thanks for fixing the issues, actually in a more elegant way than
I did here internally.
Tested for regressions on A20, H3, A64, H616, all non-secure (those
were the ones broken in v2).

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
> (no changes since v3)
> 
> Changes in v3:
>  - Fixed offset of magic passed to memcmp
>  - Refactored functions to not return pointers (fixes ambiguous NULL)
> 
> Changes in v2:
>  - Moved SPL header signature checks out of sunxi_image.h
>  - Refactored SPL header signature checks to use fewer casts
> 
>  arch/arm/include/asm/arch-sunxi/spl.h |  2 --
>  arch/arm/mach-sunxi/board.c           | 34 ++++++++++++++++++++++-----
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index 58cdf806d9..157b11e489 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -19,8 +19,6 @@
>  #define SUNXI_BOOTED_FROM_MMC0_HIGH	0x10
>  #define SUNXI_BOOTED_FROM_MMC2_HIGH	0x12
>  
> -#define is_boot0_magic(addr)	(memcmp((void *)(addr), BOOT0_MAGIC, 8) == 0)
> -
>  uint32_t sunxi_get_boot_device(void);
>  
>  #endif
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index b4ba2a72c4..b2cd64bb3f 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -243,12 +243,28 @@ void s_init(void)
>  
>  #define SUNXI_INVALID_BOOT_SOURCE	-1
>  
> +static int sunxi_egon_valid(struct boot_file_head *egon_head)
> +{
> +	return !memcmp(egon_head->magic, BOOT0_MAGIC, 8); /* eGON.BT0 */
> +}
> +
> +static int sunxi_toc0_valid(struct toc0_main_info *toc0_info)
> +{
> +	return !memcmp(toc0_info->name, TOC0_MAIN_INFO_NAME, 8); /* TOC0.GLH */
> +}
> +
>  static int sunxi_get_boot_source(void)
>  {
> -	if (!is_boot0_magic(SPL_ADDR + 4)) /* eGON.BT0 */
> -		return SUNXI_INVALID_BOOT_SOURCE;
> +	struct boot_file_head *egon_head = (void *)SPL_ADDR;
> +	struct toc0_main_info *toc0_info = (void *)SPL_ADDR;
> +
> +	if (sunxi_egon_valid(egon_head))
> +		return readb(&egon_head->boot_media);
> +	if (sunxi_toc0_valid(toc0_info))
> +		return readb(&toc0_info->platform[0]);
>  
> -	return readb(SPL_ADDR + 0x28);
> +	/* Not a valid image, so we must have been booted via FEL. */
> +	return SUNXI_INVALID_BOOT_SOURCE;
>  }
>  
>  /* The sunxi internal brom will try to loader external bootloader
> @@ -296,10 +312,16 @@ uint32_t sunxi_get_boot_device(void)
>  #ifdef CONFIG_SPL_BUILD
>  static u32 sunxi_get_spl_size(void)
>  {
> -	if (!is_boot0_magic(SPL_ADDR + 4)) /* eGON.BT0 */
> -		return 0;
> +	struct boot_file_head *egon_head = (void *)SPL_ADDR;
> +	struct toc0_main_info *toc0_info = (void *)SPL_ADDR;
>  
> -	return readl(SPL_ADDR + 0x10);
> +	if (sunxi_egon_valid(egon_head))
> +		return readl(&egon_head->length);
> +	if (sunxi_toc0_valid(toc0_info))
> +		return readl(&toc0_info->length);
> +
> +	/* Not a valid image, so use the default U-Boot offset. */
> +	return 0;
>  }
>  
>  /*

[PATCH v4 4/4] sunxi: Support building a SPL as a TOC0 image

[Samuel Holland]

Now that mkimage can generate TOC0 images, and the SPL can interpret
them, hook up the build infrastructure so the user can choose which
image type to build. Since the absolute load address is stored in the
TOC0 header, that information must be passed to mkimage.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v2)

Changes in v2:
 - Rebase on top of Icenowy's RISC-V support series
 - Rename Kconfig symbols to include the full image type name

 arch/arm/mach-sunxi/Kconfig |  2 ++
 board/sunxi/Kconfig         | 24 ++++++++++++++++++++++++
 scripts/Makefile.spl        |  5 ++++-
 3 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 board/sunxi/Kconfig

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index 2c18cf02d1..879efb9f61 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -1050,6 +1050,8 @@ config BLUETOOTH_DT_DEVICE_FIXUP
 	  The used address is "bdaddr" if set, and "ethaddr" with the LSB
 	  flipped elsewise.
 
+source "board/sunxi/Kconfig"
+
 endif
 
 config CHIP_DIP_SCAN
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
new file mode 100644
index 0000000000..084a8b0c6c
--- /dev/null
+++ b/board/sunxi/Kconfig
@@ -0,0 +1,24 @@
+choice
+	prompt "SPL Image Type"
+	default SPL_IMAGE_TYPE_SUNXI_EGON
+
+config SPL_IMAGE_TYPE_SUNXI_EGON
+	bool "eGON (normal)"
+	help
+	  Select this option to embed the SPL binary in an eGON.BT0 image,
+	  which is compatible with the normal boot ROM (NBROM).
+
+	  This is usually the correct option to choose.
+
+config SPL_IMAGE_TYPE_SUNXI_TOC0
+	bool "TOC0 (secure)"
+	help
+	  Select this option to embed the SPL binary in a TOC0 image,
+	  which is compatible with the secure boot ROM (SBROM).
+
+endchoice
+
+config SPL_IMAGE_TYPE
+	string
+	default "sunxi_egon" if SPL_IMAGE_TYPE_SUNXI_EGON
+	default "sunxi_toc0" if SPL_IMAGE_TYPE_SUNXI_TOC0
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
index 4cc23799db..635fa14cb9 100644
--- a/scripts/Makefile.spl
+++ b/scripts/Makefile.spl
@@ -411,7 +411,10 @@ endif
 $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
 	$(call if_changed,mkimage)
 
-MKIMAGEFLAGS_sunxi-spl.bin = -A $(ARCH) -T sunxi_egon \
+MKIMAGEFLAGS_sunxi-spl.bin = \
+	-A $(ARCH) \
+	-T $(CONFIG_SPL_IMAGE_TYPE) \
+	-a $(CONFIG_SPL_TEXT_BASE) \
 	-n $(CONFIG_DEFAULT_DEVICE_TREE)
 
 OBJCOPYFLAGS_u-boot-spl-dtb.hex := -I binary -O ihex --change-address=$(CONFIG_SPL_TEXT_BASE)
-- 
2.32.0

Re: [PATCH v4 4/4] sunxi: Support building a SPL as a TOC0 image

[Andre Przywara]

On Tue, 19 Oct 2021 21:44:54 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> Now that mkimage can generate TOC0 images, and the SPL can interpret
> them, hook up the build infrastructure so the user can choose which
> image type to build. Since the absolute load address is stored in the
> TOC0 header, that information must be passed to mkimage.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Nice, that allows to build TOC0 images very easily!
Tested for regressions on a bunch of boards, and on a Remix Mini PC,
selecting SPL_IMAGE_TYPE_SUNXI_TOC0: boots straight through!
Will double check the DT I made, then send it later, so that we get a
user in the tree.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
> (no changes since v2)
> 
> Changes in v2:
>  - Rebase on top of Icenowy's RISC-V support series
>  - Rename Kconfig symbols to include the full image type name
> 
>  arch/arm/mach-sunxi/Kconfig |  2 ++
>  board/sunxi/Kconfig         | 24 ++++++++++++++++++++++++
>  scripts/Makefile.spl        |  5 ++++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100644 board/sunxi/Kconfig
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 2c18cf02d1..879efb9f61 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -1050,6 +1050,8 @@ config BLUETOOTH_DT_DEVICE_FIXUP
>  	  The used address is "bdaddr" if set, and "ethaddr" with the LSB
>  	  flipped elsewise.
>  
> +source "board/sunxi/Kconfig"
> +
>  endif
>  
>  config CHIP_DIP_SCAN
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> new file mode 100644
> index 0000000000..084a8b0c6c
> --- /dev/null
> +++ b/board/sunxi/Kconfig
> @@ -0,0 +1,24 @@
> +choice
> +	prompt "SPL Image Type"
> +	default SPL_IMAGE_TYPE_SUNXI_EGON
> +
> +config SPL_IMAGE_TYPE_SUNXI_EGON
> +	bool "eGON (normal)"
> +	help
> +	  Select this option to embed the SPL binary in an eGON.BT0 image,
> +	  which is compatible with the normal boot ROM (NBROM).
> +
> +	  This is usually the correct option to choose.
> +
> +config SPL_IMAGE_TYPE_SUNXI_TOC0
> +	bool "TOC0 (secure)"
> +	help
> +	  Select this option to embed the SPL binary in a TOC0 image,
> +	  which is compatible with the secure boot ROM (SBROM).
> +
> +endchoice
> +
> +config SPL_IMAGE_TYPE
> +	string
> +	default "sunxi_egon" if SPL_IMAGE_TYPE_SUNXI_EGON
> +	default "sunxi_toc0" if SPL_IMAGE_TYPE_SUNXI_TOC0
> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
> index 4cc23799db..635fa14cb9 100644
> --- a/scripts/Makefile.spl
> +++ b/scripts/Makefile.spl
> @@ -411,7 +411,10 @@ endif
>  $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
>  	$(call if_changed,mkimage)
>  
> -MKIMAGEFLAGS_sunxi-spl.bin = -A $(ARCH) -T sunxi_egon \
> +MKIMAGEFLAGS_sunxi-spl.bin = \
> +	-A $(ARCH) \
> +	-T $(CONFIG_SPL_IMAGE_TYPE) \
> +	-a $(CONFIG_SPL_TEXT_BASE) \
>  	-n $(CONFIG_DEFAULT_DEVICE_TREE)
>  
>  OBJCOPYFLAGS_u-boot-spl-dtb.hex := -I binary -O ihex --change-address=$(CONFIG_SPL_TEXT_BASE)