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

[Andre Przywara <andre.przywara@arm.com>]

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

Hi Samuel,

> On 10/19/21 5:41 AM, Andre Przywara wrote:
> > On Mon, 18 Oct 2021 09:09:04 -0500
> > "Alex G." <mr.nuke.me@gmail.com> wrote:
> > 
> > Hi,
> >   
> >> On 10/14/21 10:19 PM, 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.
> >>>
> >>> When building for platforms which use those image types, automatically
> >>> select TOOLS_LIBCRYPTO, since it is required for the build to complete.
> >>>
> >>> Signed-off-by: Samuel Holland <samuel@sholland.org>    
> >>
> >> NAK.
> >>
> >> The intent, as detailed in tools/Makefile, is to _NOT_ to conflate 
> >> target options with tools options.  
> > 
> > I am a bit undecided, because I think the intent was more for *just*
> > building mkimage (tools-only_defconfig, for the u-boot-tools distro
> > package, for instance). (Which doesn't seem to work, btw, with or without
> > this patch.)  
> 
> TOOLS_LIBCRYPTO=n works for me with this patch, and I just
> double-checked and verified that mkimage is compiled/linked without OpenSSL.

For tools-only_defconfig and "make tools"?
I see that the SSL linking errors vanish, but I still get this instead:
$ make -s tools
/usr/bin/ld: tools/image-host.o: in function `fit_image_setup_sig':
image-host.c:(.text+0xec): undefined reference to `image_get_checksum_algo'
/usr/bin/ld: image-host.c:(.text+0xfc): undefined reference to `image_get_crypto_algo'
/usr/bin/ld: image-host.c:(.text+0x108): undefined reference to `image_get_padding_algo'
/usr/bin/ld: tools/image-host.o: in function `fit_config_add_verification_data':
image-host.c:(.text+0x7b0): undefined reference to `fit_region_make_list'
/usr/bin/ld: tools/image-host.o: in function `fit_check_sign':
image-host.c:(.text+0x1548): undefined reference to `fit_config_verify'
/usr/bin/ld: tools/common/image-fit.o: in function `fit_image_verify_with_data':
image-fit.c:(.text+0x17c0): undefined reference to `fit_image_verify_required_sigs'
/usr/bin/ld: image-fit.c:(.text+0x19e0): undefined reference to `fit_image_check_sig'
/usr/bin/ld: tools/common/image-fit.o: in function `fit_image_load':
image-fit.c:(.text+0x27ec): undefined reference to `fit_config_verify'
collect2: error: ld returned 1 exit status
make[1]: *** [scripts/Makefile.host:104: tools/dumpimage] Error 1
make: *** [Makefile:1800: tools] Error 2
(On Ubuntu 20.04 arm64)

> 
> > However just building mkimage because it's needed to create a certain
> > board firmware is a different story, I think, and including OpenSSL (if
> > the platform requires that) is hardly a user's choice at this point.
> > 
> > But anyway: Samuel, what is the actual problem this patch is solving?  
> 
> The actual problem is that TOOLS_LIBCRYPTO=n is broken, and would be
> further broken by adding sunxi_toc0.o to dumpimage-mkimage-objs. Fixing
> that requires moving objects that depend on OpenSSL to LIBCRYPTO_OBJS,
> and adding sunxi_toc0.o there in patch 2.

Right, I was just confused because it is still broken for me (see above).

> > TOOLS_LIBCRYPTO is default y, so normally (make foo_defconfig; make)
> > everything should be fine? And it only breaks if a user deliberately and
> > manually deselects it, between "make foo_defconfig" and "make"?
> > 
> > So this patch is somewhat optional, at least for the purpose of TOC0
> > support?  
> 
> The Makefile changes are needed for TOC0 support. The Kconfig changes
> are not.

Right, I see.

> And I think the only controversial part of this patch is the
> "select TOOLS_LIBCRYPTO" lines. So I suggest omitting all of the Kconfig
> changes from this patch (and removing those lines from the commit
> message). I can send v4 or you can fix it up.

You should probably send a v4, so that we get Alex' OK for that.
I need to test on actual non-secure hardware tonight, to verify that it
still works (which I strongly assume now, from looking at the code). Then
I would be eager to merge it, since it booted my Remix Mini PC beautifully.

Cheers,
Andre

> >   
> >> Disabling openssl libs is purely at the user's discretion. If platforms 
> >> can't build a usable image, I suggest just printing a loud warning 
> >> instead of overriding the user.
> >>
> >> Alex
> >>  
> >>> ---
> >>>
> >>> 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
> >>>
> >>>   arch/arm/Kconfig              |  3 +++
> >>>   arch/arm/mach-imx/mxs/Kconfig |  2 ++
> >>>   scripts/config_whitelist.txt  |  1 -
> >>>   tools/Makefile                | 19 +++++--------------
> >>>   tools/mxsimage.c              |  3 ---
> >>>   5 files changed, 10 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >>> index d8c041a877..380ad4f670 100644
> >>> --- a/arch/arm/Kconfig
> >>> +++ b/arch/arm/Kconfig
> >>> @@ -566,6 +566,7 @@ config ARCH_KIRKWOOD
> >>>   	select BOARD_EARLY_INIT_F
> >>>   	select CPU_ARM926EJS
> >>>   	select GPIO_EXTRA_HEADER
> >>> +	select TOOLS_LIBCRYPTO
> >>>   
> >>>   config ARCH_MVEBU
> >>>   	bool "Marvell MVEBU family (Armada XP/375/38x/3700/7K/8K)"
> >>> @@ -580,12 +581,14 @@ config ARCH_MVEBU
> >>>   	select OF_CONTROL
> >>>   	select OF_SEPARATE
> >>>   	select SPI
> >>> +	select TOOLS_LIBCRYPTO
> >>>   	imply CMD_DM
> >>>   
> >>>   config ARCH_ORION5X
> >>>   	bool "Marvell Orion"
> >>>   	select CPU_ARM926EJS
> >>>   	select GPIO_EXTRA_HEADER
> >>> +	select TOOLS_LIBCRYPTO
> >>>   
> >>>   config TARGET_STV0991
> >>>   	bool "Support stv0991"
> >>> diff --git a/arch/arm/mach-imx/mxs/Kconfig b/arch/arm/mach-imx/mxs/Kconfig
> >>> index b2026a3758..6f138d25e9 100644
> >>> --- a/arch/arm/mach-imx/mxs/Kconfig
> >>> +++ b/arch/arm/mach-imx/mxs/Kconfig
> >>> @@ -3,6 +3,7 @@ if ARCH_MX23
> >>>   config MX23
> >>>   	bool
> >>>   	default y
> >>> +	select TOOLS_LIBCRYPTO
> >>>   
> >>>   choice
> >>>   	prompt "MX23 board select"
> >>> @@ -34,6 +35,7 @@ if ARCH_MX28
> >>>   config MX28
> >>>   	bool
> >>>   	default y
> >>> +	select TOOLS_LIBCRYPTO
> >>>   
> >>>   choice
> >>>   	prompt "MX28 board select"
> >>> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> >>> index 3a6865dc70..bea6b6f83b 100644
> >>> --- a/scripts/config_whitelist.txt
> >>> +++ b/scripts/config_whitelist.txt
> >>> @@ -838,7 +838,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
> >>>     
> >   
>