Re: [PATCH] tools: Use a single target-independent config to enable OpenSSL

["Alex G." <mr.nuke.me@gmail.com>]

On 6/15/21 6:34 PM, AKASHI Takahiro wrote:
> A gentle ping.
> What is the current review status?
> Who will take care of this patch?

Patchwork automatically delegates this to a maintainer [1], but anyone 
is welcome to comment and review.

Alex

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20210524202317.1492578-1-mr.nuke.me@gmail.com/

> -Takahiro Akashi
> 
> On Mon, May 24, 2021 at 03:23:17PM -0500, Alexandru Gagniuc wrote:
>> Host tool features, such as mkimage's ability to sign FIT images were
>> enabled or disabled based on the target configuration. However, this
>> misses the point of a target-agnostic host tool.
>>
>> A target's ability to verify FIT signatures is independent of
>> mkimage's ability to create those signatures. In fact, u-boot's build
>> system doesn't sign images. The target code can be successfully built
>> without relying on any ability to sign such code.
>>
>> Conversely, mkimage's ability to sign images does not require that
>> those images will only work on targets which support FIT verification.
>> Linking mkimage cryptographic features to target support for FIT
>> verification is misguided.
>>
>> Without loss of generality, we can say that host features are and
>> should be independent of target features.
>>
>> While we prefer that a host tool always supports the same feature set,
>> we recognize the following
>>    - some users prefer to build u-boot without a dependency on OpenSSL.
>>    - some distros prefer to ship mkimage without linking to OpenSSL
>>
>> To allow these use cases, introduce a host-only Kconfig which is used
>> to select or deselect libcrypto support. Some mkimage features or some
>> host tools might not be available, but this shouldn't affect the
>> u-boot build.
>>
>> I also considered setting the default of this config based on
>> FIT_SIGNATURE. While it would preserve the old behaviour it's also
>> contrary to the goals of this change. I decided to enable it by
>> default, so that the default build yields the most feature-complete
>> mkimage.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>
>> This patch is designed to apply on top of
>>      [PATCH v2 00/18] image: Reduce #ifdef abuse in image code
>>
>>
>>
>>   tools/Kconfig  | 11 +++++++++++
>>   tools/Makefile | 46 ++++++++++++++++++++++++++++++----------------
>>   2 files changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/Kconfig b/tools/Kconfig
>> index b2f5012240..214932ae30 100644
>> --- a/tools/Kconfig
>> +++ b/tools/Kconfig
>> @@ -9,4 +9,15 @@ config MKIMAGE_DTC_PATH
>>   	  some cases the system dtc may not support all required features
>>   	  and the path to a different version should be given here.
>>   
>> +config TOOLS_USE_LIBCRYPTO
>> +	bool "Use OpenSSL's libcrypto library for host tools"
>> +	default y
>> +	help
>> +	  Cryptographic signature, verification, and encryption of images is
>> +	  provided by host tools using OpenSSL's libcrypto. Select 'n' here if
>> +	  you wish to build host tools without OpenSSL. mkimage will not have
>> +	  the ability to sign images.
>> +	  This selection does not affect target features, such as runtime FIT
>> +	  signature verification.
>> +
>>   endmenu
>> diff --git a/tools/Makefile b/tools/Makefile
>> index 722355e984..1f30a06cce 100644
>> --- a/tools/Makefile
>> +++ b/tools/Makefile
>> @@ -3,6 +3,25 @@
>>   # (C) Copyright 2000-2006
>>   # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>>   
>> +# A note on target vs host configuration:
>> +#
>> +# 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.
>> +#
>> +# Some linux distributions package these utilities as u-boot-tools, and it
>> +# would be unmaintainable to have a different tool variation for each
>> +# arch or configuration.
>> +#
>> +# A couple of simple rules:
>> +#
>> +# 1) Do not use target CONFIG_* options to enable or disable features in host
>> +#    tools. Only use the configs from tools/Kconfig
>> +# 2) It's okay to use target configs to disable building specific tools.
>> +#    That's as long as the features of those tools aren't modified.
>> +#
>> +
>>   # Enable all the config-independent tools
>>   ifneq ($(HOST_TOOLS_ALL),)
>>   CONFIG_ARCH_KIRKWOOD = y
>> @@ -53,30 +72,30 @@ hostprogs-y += mkenvimage
>>   mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
>>   
>>   hostprogs-y += dumpimage mkimage
>> -hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
>> +hostprogs-$(CONFIG_TOOLS_USE_LIBCRYPTO) += fit_info fit_check_sign
>>   
>>   hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
>>   
>>   FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o
>> -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o common/image-fit-sig.o
>> -FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
>> +FIT_SIG_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := image-sig-host.o common/image-fit-sig.o
>> +FIT_CIPHER_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := common/image-cipher.o
>>   
>>   # The following files are synced with upstream DTC.
>>   # Use synced versions from scripts/dtc/libfdt/.
>>   LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \
>>   		fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o)
>>   
>> -RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \
>> +RSA_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := $(addprefix lib/rsa/, \
>>   					rsa-sign.o rsa-verify.o \
>>   					rsa-mod-exp.o)
>>   
>> -ECDSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o)
>> +ECDSA_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o)
>>   
>> -AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \
>> +AES_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := $(addprefix lib/aes/, \
>>   					aes-encrypt.o aes-decrypt.o)
>>   
>>   # Cryptographic helpers that depend on openssl/libcrypto
>> -LIBCRYPTO_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/, \
>> +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_USE_LIBCRYPTO) := $(addprefix lib/, \
>>   					fdt-libcrypto.o)
>>   
>>   ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
>> @@ -136,22 +155,17 @@ 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_FIT_SIGNATURE),)
>> +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_USE_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_FIT_SIGNATURE
>> +ifdef CONFIG_TOOLS_USE_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=$(CONFIG_FIT_SIGNATURE_MAX_SIZE)
>> -endif
>> -
>> -ifdef CONFIG_FIT_CIPHER
>> -# This affects include/image.h, but including the board config file
>> -# is tricky, so manually define this options here.
>> +HOST_EXTRACFLAGS	+= -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff
>>   HOST_EXTRACFLAGS	+= -DCONFIG_FIT_CIPHER
>>   endif
>>   
>> @@ -164,7 +178,7 @@ HOSTCFLAGS_kwbimage.o += -DCONFIG_KWB_SECURE
>>   endif
>>   
>>   # MXSImage needs LibSSL
>> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),)
>> +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_USE_LIBCRYPTO),)
>>   HOSTCFLAGS_kwbimage.o += \
>>   	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
>>   HOSTLDLIBS_mkimage += \
>> -- 
>> 2.31.1
>>