Re: [OE-core] [PATCH v4] u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled

[Adrian Freihofer <adrian.freihofer@gmail.com>]

Hi Marek

On Sun, 2025-01-19 at 15:03 +0100, Marek Vasut via
lists.openembedded.org wrote:
> In case both UBOOT_SIGN_ENABLE and UBOOT_ENV are enabled and
> kernel-fitimage.bbclass is in use to generate signed kernel
> fitImage, there is a circular dependency between uboot-sign
> and kernel-fitimage bbclasses . The loop looks like this:
> 
> kernel-fitimage.bbclass:
> - do_populate_sysroot depends on do_assemble_fitimage
>   - do_assemble_fitimage depends on
> virtual/bootloader:do_populate_sysroot
>     - virtual/bootloader:do_populate_sysroot depends on
> virtual/bootloader:do_install
>       => The virtual/bootloader:do_install installs and the
>          virtual/bootloader:do_populate_sysroot places into
>          sysroot an U-Boot environment script embedded into
>          kernel fitImage during do_assemble_fitimage run .
> 
> uboot-sign.bbclass:
> - DEPENDS on KERNEL_PN, which is really virtual/kernel. More
> accurately
>   - do_deploy depends on do_uboot_assemble_fitimage
>   - do_install depends on do_uboot_assemble_fitimage
>   - do_uboot_assemble_fitimage depends on
> virtual/kernel:do_populate_sysroot
>     => do_install depends on virtual/kernel:do_populate_sysroot
> 
> => virtual/bootloader:do_install depends on
> virtual/kernel:do_populate_sysroot
>    virtual/kernel:do_populate_sysroot depends on
> virtual/bootloader:do_install
> 
> Attempt to resolve the loop. Pull fitimage configuration options into
> separate
> new bbclass kernel-fitimage-config.bbclass so these configuration
> options can
> be shared by both uboot-sign.bbclass and kernel-fitimage.bbclass, and
> make use
> of mkimage -f auto-conf / mkimage -f auto option to insert /signature
> node key-*
> subnode into U-Boot control DT without depending on the layout of
> kernel fitImage
> itself. This is perfectly valid to do, because the U-Boot /signature
> node key-*
> subnodes 'required' property can contain either of two values, 'conf'
> or 'image'
> to authenticate either selected configuration or all of images when
> booting the
> fitImage.
> 
> For details of the U-Boot fitImage signing process, see:
> https://docs.u-boot.org/en/latest/usage/fit/signature.html
> For details of mkimage -f auto-conf and -f auto, see:
> https://manpages.debian.org/experimental/u-boot-tools/mkimage.1.en.html#EXAMPLES
> 
> Fixes: 5e12dc911d0c ("u-boot: Rework signing to remove
> interdependencies")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Adrian Freihofer <adrian.freihofer@siemens.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: Sean Anderson <sean.anderson@seco.com>
> ---
> V2: Take a different approach, split the kernel-fitimage.bbclass and
>     use it to generate dummy fitImage on demand
> V3: Use mkimage -f auto-conf and mkimage -f auto to break the loop,
>     the fitImage .its source is not even needed because the
> 'required'
>     property can only have two values, 'conf' or 'image' .
> V4: Restore CC list
> ---
>  .../kernel-fitimage-config.bbclass            | 50 +++++++++++++++++
>  meta/classes-recipe/kernel-fitimage.bbclass   | 54 +----------------
> --
>  meta/classes-recipe/uboot-sign.bbclass        | 26 ++++-----
>  3 files changed, 64 insertions(+), 66 deletions(-)
>  create mode 100644 meta/classes-recipe/kernel-fitimage-
> config.bbclass
> 
> diff --git a/meta/classes-recipe/kernel-fitimage-config.bbclass
> b/meta/classes-recipe/kernel-fitimage-config.bbclass
> new file mode 100644
> index 00000000000..1f665f7d47c
> --- /dev/null
> +++ b/meta/classes-recipe/kernel-fitimage-config.bbclass
> @@ -0,0 +1,50 @@
> +# Description string
> +FIT_DESC ?= "Kernel fitImage for ${DISTRO_NAME}/${PV}/${MACHINE}"
> +
> +# Kernel fitImage Hash Algo
> +FIT_HASH_ALG ?= "sha256"
> +
> +# Kernel fitImage Signature Algo
> +FIT_SIGN_ALG ?= "rsa2048"
> +
> +# Kernel / U-Boot fitImage Padding Algo
> +FIT_PAD_ALG ?= "pkcs-1.5"
> +
> +# Generate keys for signing Kernel fitImage
> +FIT_GENERATE_KEYS ?= "0"
> +
> +# Size of private keys in number of bits
> +FIT_SIGN_NUMBITS ?= "2048"
> +
> +# args to openssl genrsa (Default is just the public exponent)
> +FIT_KEY_GENRSA_ARGS ?= "-F4"
> +
> +# args to openssl req (Default is -batch for non interactive mode
> and
> +# -new for new certificate)
> +FIT_KEY_REQ_ARGS ?= "-batch -new"
> +
> +# Standard format for public key certificate
> +FIT_KEY_SIGN_PKCS ?= "-x509"
> +
> +# Sign individual images as well
> +FIT_SIGN_INDIVIDUAL ?= "0"
> +
> +FIT_CONF_PREFIX ?= "conf-"
> +FIT_CONF_PREFIX[doc] = "Prefix to use for FIT configuration node
> name"
> +
> +FIT_SUPPORTED_INITRAMFS_FSTYPES ?= "cpio.lz4 cpio.lzo cpio.lzma
> cpio.xz cpio.zst cpio.gz ext2.gz cpio"
> +
> +# Allow user to select the default DTB for FIT image when multiple
> dtb's exists.
> +FIT_CONF_DEFAULT_DTB ?= ""
> +
> +# length of address in number of <u32> cells
> +# ex: 1 32bits address, 2 64bits address
> +FIT_ADDRESS_CELLS ?= "1"
> +
> +# Keys used to sign individually image nodes.
> +# The keys to sign image nodes must be different from those used to
> sign
> +# configuration nodes, otherwise the "required" property, from
> +# UBOOT_DTB_BINARY, will be set to "conf", because "conf" prevails
> on "image".
> +# Then the images signature checking will not be mandatory and no
> error will be
> +# raised in case of failure.
> +# UBOOT_SIGN_IMG_KEYNAME = "dev2" # keys name in keydir (eg.
> "dev2.crt", "dev2.key")
> diff --git a/meta/classes-recipe/kernel-fitimage.bbclass
> b/meta/classes-recipe/kernel-fitimage.bbclass
> index 67c98adb232..33dae750672 100644
> --- a/meta/classes-recipe/kernel-fitimage.bbclass
> +++ b/meta/classes-recipe/kernel-fitimage.bbclass
> @@ -4,7 +4,7 @@
>  # SPDX-License-Identifier: MIT
>  #
>  
> -inherit kernel-uboot kernel-artifact-names uboot-config
> +inherit kernel-uboot kernel-artifact-names uboot-config kernel-
> fitimage-config
>  
>  def get_fit_replacement_type(d):
>      kerneltypes = d.getVar('KERNEL_IMAGETYPES') or ""
> @@ -52,58 +52,6 @@ python __anonymous () {
>          d.setVar('EXTERNAL_KERNEL_DEVICETREE',
> "${RECIPE_SYSROOT}/boot/devicetree")
>  }
>  
> -
> -# Description string
> -FIT_DESC ?= "Kernel fitImage for ${DISTRO_NAME}/${PV}/${MACHINE}"
> -
> -# Kernel fitImage Hash Algo
> -FIT_HASH_ALG ?= "sha256"
> -
> -# Kernel fitImage Signature Algo
> -FIT_SIGN_ALG ?= "rsa2048"
> -
> -# Kernel / U-Boot fitImage Padding Algo
> -FIT_PAD_ALG ?= "pkcs-1.5"
> -
> -# Generate keys for signing Kernel fitImage
> -FIT_GENERATE_KEYS ?= "0"
> -
> -# Size of private keys in number of bits
> -FIT_SIGN_NUMBITS ?= "2048"
> -
> -# args to openssl genrsa (Default is just the public exponent)
> -FIT_KEY_GENRSA_ARGS ?= "-F4"
> -
> -# args to openssl req (Default is -batch for non interactive mode
> and
> -# -new for new certificate)
> -FIT_KEY_REQ_ARGS ?= "-batch -new"
> -
> -# Standard format for public key certificate
> -FIT_KEY_SIGN_PKCS ?= "-x509"
> -
> -# Sign individual images as well
> -FIT_SIGN_INDIVIDUAL ?= "0"
> -
> -FIT_CONF_PREFIX ?= "conf-"
> -FIT_CONF_PREFIX[doc] = "Prefix to use for FIT configuration node
> name"
> -
> -FIT_SUPPORTED_INITRAMFS_FSTYPES ?= "cpio.lz4 cpio.lzo cpio.lzma
> cpio.xz cpio.zst cpio.gz ext2.gz cpio"
> -
> -# Allow user to select the default DTB for FIT image when multiple
> dtb's exists.
> -FIT_CONF_DEFAULT_DTB ?= ""
> -
> -# length of address in number of <u32> cells
> -# ex: 1 32bits address, 2 64bits address
> -FIT_ADDRESS_CELLS ?= "1"
> -
> -# Keys used to sign individually image nodes.
> -# The keys to sign image nodes must be different from those used to
> sign
> -# configuration nodes, otherwise the "required" property, from
> -# UBOOT_DTB_BINARY, will be set to "conf", because "conf" prevails
> on "image".
> -# Then the images signature checking will not be mandatory and no
> error will be
> -# raised in case of failure.
> -# UBOOT_SIGN_IMG_KEYNAME = "dev2" # keys name in keydir (eg.
> "dev2.crt", "dev2.key")
> -
>  #
>  # Emit the fitImage ITS header
>  #
> diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-
> recipe/uboot-sign.bbclass
> index a17be745cec..d23ae4d8d15 100644
> --- a/meta/classes-recipe/uboot-sign.bbclass
> +++ b/meta/classes-recipe/uboot-sign.bbclass
> @@ -25,7 +25,7 @@
>  # For more details on signature process, please refer to U-Boot
> documentation.
>  
>  # We need some variables from u-boot-config
> -inherit uboot-config
> +inherit uboot-config kernel-fitimage-config
>  
>  # Enable use of a U-Boot fitImage
>  UBOOT_FITIMAGE_ENABLE ?= "0"
> @@ -85,9 +85,6 @@ UBOOT_FIT_KEY_SIGN_PKCS ?= "-x509"
>  # ex: 1 32bits address, 2 64bits address
>  UBOOT_FIT_ADDRESS_CELLS ?= "1"
>  
> -# This is only necessary for determining the signing configuration
> -KERNEL_PN = "${PREFERRED_PROVIDER_virtual/kernel}"
> -
>  UBOOT_FIT_UBOOT_LOADADDRESS ?= "${UBOOT_LOADADDRESS}"
>  UBOOT_FIT_UBOOT_ENTRYPOINT ?= "${UBOOT_ENTRYPOINT}"
>  
> @@ -96,8 +93,6 @@ python() {
>      sign = d.getVar('UBOOT_SIGN_ENABLE') == '1'
>      if d.getVar('UBOOT_FITIMAGE_ENABLE') == '1' or sign:
>          d.appendVar('DEPENDS', " u-boot-tools-native dtc-native")
> -    if sign:
> -        d.appendVar('DEPENDS', " " + d.getVar('KERNEL_PN'))
>  }
>  
>  concat_dtb() {
> @@ -106,16 +101,25 @@ concat_dtb() {
>  
>  	if [ -e "${UBOOT_DTB_BINARY}" ]; then
>  		# Re-sign the kernel in order to add the keys to our
> dtb
> +		UBOOT_MKIMAGE_MODE="auto-conf"
> +		# Signing individual images is not recommended as
> that
> +		# makes fitImage susceptible to mix-and-match
> attack.
> +		if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then
> +			UBOOT_MKIMAGE_MODE="auto"
> +		fi
>  		${UBOOT_MKIMAGE_SIGN} \
>  			${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if
> len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
> -			-F -k "${UBOOT_SIGN_KEYDIR}" \
> +			-f ${UBOOT_MKIMAGE_MODE} -d /dev/null \

As UBOOT_MKIMAGE_MODE appears to be a shell variable and not a bitbake
variable, it should be used like $UBOOT_MKIMAGE_MODE and not like
${UBOOT_MKIMAGE_MODE}.

> +			-k "${UBOOT_SIGN_KEYDIR}" \
> +			-o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \
> +			-g "${UBOOT_SIGN_IMG_KEYNAME}" \
>  			-K "${UBOOT_DTB_BINARY}" \
> -			-r ${B}/fitImage-linux \
> +			-r ${B}/unused.itb

Here a \ is missing. The line should look like:
			-r ${B}/unused.itb \

This patch looks much simpler than v2 and is a good step towards
decoupling the kernel and u-boot. Now the dependencies between u-boot
and the kernel are primarily some shared variables. The task
dependencies are much less critical.

Thank you
Adrian

>  			${UBOOT_MKIMAGE_SIGN_ARGS}
>  		# Verify the kernel image and u-boot dtb
>  		${UBOOT_FIT_CHECK_SIGN} \
>  			-k "${UBOOT_DTB_BINARY}" \
> -			-f ${B}/fitImage-linux
> +			-f ${B}/unused.itb
>  		cp ${UBOOT_DTB_BINARY} ${UBOOT_DTB_SIGNED}
>  	fi
>  
> @@ -351,10 +355,6 @@ uboot_assemble_fitimage_helper() {
>  }
>  
>  do_uboot_assemble_fitimage() {
> -	if [ "${UBOOT_SIGN_ENABLE}" = "1" ] ; then
> -		cp "${STAGING_DIR_HOST}/sysroot-only/fitImage"
> "${B}/fitImage-linux"
> -	fi
> -
>  	if [ -n "${UBOOT_CONFIG}" ]; then
>  		unset i
>  		for config in ${UBOOT_MACHINE}; do
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#210008):
> https://lists.openembedded.org/g/openembedded-core/message/210008
> Mute This Topic: https://lists.openembedded.org/mt/110697929/4454582
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe:
> https://lists.openembedded.org/g/openembedded-core/unsub [
> adrian.freihofer@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>