Re: [Buildroot] [PATCH v2 1/1] boot/arm-trusted-firmware add Trusted Board Boot option

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

Thanks for your patch, and sorry for the delay in getting back to
you. We reviewed your patch (as part of the Buildroot Developers
Meeting) and we have a number of questions/comments. See below.

On Mon, Jan 06, 2025 at 03:47:10PM +0100, Jean-Christophe Bauduin via buildroot wrote:

> +config BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOARD_BOOT
> +	bool "Enable Trusted Board boot"
> +	help
> +	  Enables trusted board boot.
> +	  More info:
> +	  https://wiki.st.com/stm32mpu/wiki/TF-A_BL2_Trusted_Board_Boot
> +
> +endif
> +
> +if BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOARD_BOOT
> +
> +config BR2_TARGET_ARM_TRUSTED_FIRMWARE_ROT_KEY
> +	string "Path to the ROT private key"

Why is the private key needed, and not just the public key?

> +	help
> +	  The path to the private ROT key used by ATF.
> +	  The private key must be in PEM format.

Is it possible to add more details on how to generate the key in the
expected format, or is that platform/vendor dependent?

> +config BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION
> +	string "The version number (ex 2.8) needed for the secure boot"

This description is not good, it should be:

	string "MbedTLS version"

> +	help

And then here explain why it is needed, and perhaps how to figure out
which one should be used.

> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> index 172a930b5c..dfd4480ba6 100644
> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> @@ -18,6 +18,25 @@ else
>  # Handle stable official ATF versions
>  ARM_TRUSTED_FIRMWARE_SITE = https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git
>  ARM_TRUSTED_FIRMWARE_SITE_METHOD = git
> +
> +ifneq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION),)

This won't work, as BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION
being a string option, it's never empty. When no value is specified in
menuconfig, the value of
BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION will be "". So you need to do:

ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION = $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION))

and then you can use $(ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION).

> +ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE = mbedtls-$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION))
> +
> +ARM_TRUSTED_FIRMWARE_MBEDTLS_SITE = https://codeload.github.com/Mbed-TLS/mbedtls/tar.gz/refs/tags/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE)
> +
> +define ARM_TRUSTED_FIRMWARE_MBEDTLS_EXTRACT_CMDS
> +	$(WGET) -O $(DL_DIR)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE) $(ARM_TRUSTED_FIRMWARE_MBEDTLS_SITE)

This is not good at all: you are downloading during the extract
step. By doing this, you break offline build, you break hash integrity
checking, and you don't account for the license of mbedtls in the
license information.

Instead, use EXTRA_DOWNLOADS. Make sure to update the license
information as well.

> +	mkdir -p $(@D)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE)
> +	$(TAR) -xvf $(DL_DIR)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE) -C $(@D)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE) --strip-components=1
> +endef
> +
> +define ARM_TRUSTED_FIRMWARE_MBEDTLS_BUILD_CMDS
> +	$(MAKE) -C $(@D)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE) \
> +	CFLAGS="$(TARGET_CFLAGS)" LDFLAGS="$(TARGET_LDFLAGS)" lib

Second line should be indented with one more tab (minor detail).

> +endef
> +ARM_TRUSTED_FIRMWARE_POST_EXTRACT_HOOKS += ARM_TRUSTED_FIRMWARE_MBEDTLS_EXTRACT_CMDS
> +endif
> +
>  # The licensing of custom or from-git versions is unknown.
>  # This is valid only for the latest (i.e. known) version.
>  ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_LATEST_VERSION)$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_LATEST_LTS_2_10_VERSION)$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_LATEST_LTS_2_8_VERSION),y)
> @@ -190,12 +209,24 @@ define ARM_TRUSTED_FIRMWARE_BL31_UBOOT_INSTALL_ELF
>  endef
>  endif
>  
> +ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOARD_BOOT),y)
> +ARM_TRUSTED_FIRMWARE_TRUSTED_BOOT_ROT_KEY=$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_ROT_KEY))

Spaces around = sign (minor detail).

> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += \
> +	ROT_KEY=$(ARM_TRUSTED_FIRMWARE_TRUSTED_BOOT_ROT_KEY) \
> +	MBEDTLS_DIR=$(@D)/$(ARM_TRUSTED_FIRMWARE_MBEDTLS_SOURCE) \
> +	GENERATE_COT=1 \
> +	TRUSTED_BOARD_BOOT=1
> +endif
> +
>  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS += \
>  	$(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_ADDITIONAL_TARGETS))
>  
>  ARM_TRUSTED_FIRMWARE_CUSTOM_DTS_PATH = $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_CUSTOM_DTS_PATH))
>  
>  define ARM_TRUSTED_FIRMWARE_BUILD_CMDS
> +	$(if $(BR2_TARGET_ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION),

Use ARM_TRUSTED_FIRMWARE_MBEDTLS_VERSION. Or perhaps better, use
BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOARD_BOOT. Indeed, as soon as
BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOARD_BOOT is y we need to
build mbedtls, and if the user has forgotten to set the MbedTLS
version, a clear build failure would be better than skipping the
MbedTLS build.

Could you have a look at those issues and submit a v3?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot