On Tue, 19 Oct 2021 08:28:44 -0500 Samuel Holland <[email protected]> wrote:
Hi Samuel, > On 10/19/21 5:41 AM, Andre Przywara wrote: > > On Mon, 18 Oct 2021 09:09:04 -0500 > > "Alex G." <[email protected]> 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 <[email protected]> > >> > >> 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 <[email protected]> > >>> */ > >>> > >>> -#ifdef CONFIG_MXS > >>> - > >>> #include <errno.h> > >>> #include <fcntl.h> > >>> #include <stdio.h> > >>> @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE( > >>> NULL, > >>> mxsimage_generate > >>> ); > >>> -#endif > >>> > > >
