| Message ID | 20250119180926.41739-1-marex@denx.de |
|---|---|
| State | Accepted, archived |
| Commit | 259bfa86f384206f0d0a96a5b84887186c5f689e |
| Headers | show |
| Series | [v5] u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled | expand |
Hi Marek, Thank you for fixing this bug. I think this v5 looks really nice. Not sure how to add my Reviewed-by: Adrian Freihofer <adrian.freihofer@siemens.com> but if via E-Mail is fine, here it is. Regards, Adrian > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#210018): > https://lists.openembedded.org/g/openembedded-core/message/210018 > Mute This Topic: https://lists.openembedded.org/mt/110702086/4454582 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: > https://lists.openembedded.org/g/openembedded-core/unsub [ > adrian.freihofer@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Sun, 2025-01-19 at 19:08 +0100, Marek Vasut 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 > V5: - Add missing trailing backslash after unused.itb > - Replace ${UBOOT_MKIMAGE_MODE} with $UBOOT_MKIMAGE_MODE > - Move -d /dev/null to the end of mkimage invocation > --- > .../kernel-fitimage-config.bbclass | 50 +++++++++++++++++ > meta/classes-recipe/kernel-fitimage.bbclass | 54 +------------------ > meta/classes-recipe/uboot-sign.bbclass | 27 +++++----- > 3 files changed, 65 insertions(+), 66 deletions(-) > create mode 100644 meta/classes-recipe/kernel-fitimage-config.bbclass This looks great and I'm nearly ready to merge it but I do have one further request. Instead of creating kernel-fitimage-config.bbclass, could you create something like meta/conf/image-fitimage.conf? There is already precedent for this with image-uefi.conf and you can include with require ../conf/image-fitimage.conf. I'd like to see config variables being in conf files rather than class files. Adding some comments at the top of the new file explaining which config settings are there and what it is for would also be good. Thanks, Richard
On 1/21/25 1:34 PM, Richard Purdie wrote: > On Sun, 2025-01-19 at 19:08 +0100, Marek Vasut 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 >> V5: - Add missing trailing backslash after unused.itb >> - Replace ${UBOOT_MKIMAGE_MODE} with $UBOOT_MKIMAGE_MODE >> - Move -d /dev/null to the end of mkimage invocation >> --- >> .../kernel-fitimage-config.bbclass | 50 +++++++++++++++++ >> meta/classes-recipe/kernel-fitimage.bbclass | 54 +------------------ >> meta/classes-recipe/uboot-sign.bbclass | 27 +++++----- >> 3 files changed, 65 insertions(+), 66 deletions(-) >> create mode 100644 meta/classes-recipe/kernel-fitimage-config.bbclass > > This looks great and I'm nearly ready to merge it but I do have one > further request. Instead of creating kernel-fitimage-config.bbclass, > could you create something like meta/conf/image-fitimage.conf? > > There is already precedent for this with image-uefi.conf and you can > include with require ../conf/image-fitimage.conf. I'd like to see > config variables being in conf files rather than class files. > > Adding some comments at the top of the new file explaining which config > settings are there and what it is for would also be good. I hope they are all addressed in V6, thanks.
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..fafa7866d16 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,26 @@ 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 \ + -k "${UBOOT_SIGN_KEYDIR}" \ + -o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \ + -g "${UBOOT_SIGN_IMG_KEYNAME}" \ -K "${UBOOT_DTB_BINARY}" \ - -r ${B}/fitImage-linux \ + -d /dev/null \ + -r ${B}/unused.itb \ ${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 +356,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
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 V5: - Add missing trailing backslash after unused.itb - Replace ${UBOOT_MKIMAGE_MODE} with $UBOOT_MKIMAGE_MODE - Move -d /dev/null to the end of mkimage invocation --- .../kernel-fitimage-config.bbclass | 50 +++++++++++++++++ meta/classes-recipe/kernel-fitimage.bbclass | 54 +------------------ meta/classes-recipe/uboot-sign.bbclass | 27 +++++----- 3 files changed, 65 insertions(+), 66 deletions(-) create mode 100644 meta/classes-recipe/kernel-fitimage-config.bbclass