diff mbox series

[v1,2/2] uboot-sign: Make SPL DTB public key injection optional

Message ID 20250606071915.2680182-3-jamin_lin@aspeedtech.com
State New
Headers show
Series uboot-sign: Make SPL DTB public key injection optional | expand

Commit Message

Jamin Lin June 6, 2025, 7:19 a.m. UTC 
Introduce SPL_SIGN_ADD_PUBKEY to control whether the public key is
added into the SPL device tree and whether FIT signature verification is
performed after signing.

Key changes:
- Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
- Conditionally apply '-K <dtb>' to mkimage only if adding key is enabled
- Skip fit_check_sign when public key injection is disabled
- Suppress concat_spl_dtb() warnings if key adding is turned off

This allows U-Boot FIT images to be signed without modifying the SPL DTB,
useful in scenarios where public key management is handled elsewhere or
post-processing will be done separately.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

AdrianF June 12, 2025, 10:58 a.m. UTC | #1 
On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via lists.openembedded.org
wrote:
> Introduce SPL_SIGN_ADD_PUBKEY to control whether the public key is
> added into the SPL device tree and whether FIT signature verification
> is
> performed after signing.
> 
> Key changes:
> - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> - Conditionally apply '-K <dtb>' to mkimage only if adding key is
> enabled
> - Skip fit_check_sign when public key injection is disabled
> - Suppress concat_spl_dtb() warnings if key adding is turned off
> 
> This allows U-Boot FIT images to be signed without modifying the SPL
> DTB,
> useful in scenarios where public key management is handled elsewhere
> or
> post-processing will be done separately.

Thank you for the patch.

Introducing a new variable is one way to solve your issue, but it
increases complexity and requires additional test coverage and
documentation. So I would like to ask if this is extra complexity is
really needed to achieve your goal.

For a similar use case, we use this approach:

 * bitbake always signs with a development key (no build variant
   needed, no extra test matrix)
 * Then we test the firmware
 * For release, we use a script to replace the development signature
   with a production signature (without rebuilding with bitbake)

This seems similar to your approach, but I don't understand why
injecting the development public key is problematic for your workflow.
Would it be better to have a post-processing step that simply replaces
the public key, rather than requiring an empty DTB without any public
key? That would mean that this patch would not be needed at all.

Adrian

> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-
> recipe/uboot-sign.bbclass
> index 6ee1874cd6..3bcf47dd33 100644
> --- a/meta/classes-recipe/uboot-sign.bbclass
> +++ b/meta/classes-recipe/uboot-sign.bbclass
> @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
>  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE = "1"
>  SPL_SIGN_ENABLE ?= "0"
>  
> +# Whether to add (embed) the public key into the SPL Device Tree
> (.dtb).
> +# If set to "1", the key will be inserted into the /signature node
> of the DTB
> +# and fit_check_sign will be used to verify the signature.
> +# If set to "0", only signing will be performed, without modifying
> the DTB.
> +SPL_SIGN_ADD_PUBKEY ?= "1"
> +
>  # Default value for deployment filenames.
>  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
>  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> @@ -245,7 +251,9 @@ concat_spl_dtb() {
>   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
>   cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED} >
> "${SPL_BINARY}"
>   else
> - bbwarn "Failure while adding public key to spl binary. Verified U-
> Boot boot won't be available."
> + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> + bbwarn "Failure while adding public key to spl binary. Verified U-
> Boot boot won't be available."
> + fi
>   fi
>  }
>  
> @@ -474,15 +482,17 @@ EOF
>   ${UBOOT_MKIMAGE_SIGN} \
>   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}')
> else ''} \
>   -F -k "${SPL_SIGN_KEYDIR}" \
> - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
>   -r ${UBOOT_FITIMAGE_BINARY} \
>   ${SPL_MKIMAGE_SIGN_ARGS}
>   #
>   # Verify the U-boot FIT image and SPL dtb
>   #
> - ${UBOOT_FIT_CHECK_SIGN} \
> - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> - -f ${UBOOT_FITIMAGE_BINARY}
> + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> + ${UBOOT_FIT_CHECK_SIGN} \
> + -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> + -f ${UBOOT_FITIMAGE_BINARY}
> + fi
>   fi
>  
>   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#218114):
> https://lists.openembedded.org/g/openembedded-core/message/218114
> Mute This Topic: https://lists.openembedded.org/mt/113499585/3616858
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe:
> https://lists.openembedded.org/g/openembedded-core/unsub [
> adrian.freihofer@siemens.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Jamin Lin June 13, 2025, 2:08 a.m. UTC | #2 
Hi Adrian, 

> Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB public key
> injection optional
> 
> On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via lists.openembedded.org
> wrote:
> > Introduce SPL_SIGN_ADD_PUBKEY to control whether the public key is
> > added into the SPL device tree and whether FIT signature verification
> > is performed after signing.
> >
> > Key changes:
> > - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> > - Conditionally apply '-K <dtb>' to mkimage only if adding key is
> > enabled
> > - Skip fit_check_sign when public key injection is disabled
> > - Suppress concat_spl_dtb() warnings if key adding is turned off
> >
> > This allows U-Boot FIT images to be signed without modifying the SPL
> > DTB, useful in scenarios where public key management is handled
> > elsewhere or post-processing will be done separately.
> 
> Thank you for the patch.
> 
> Introducing a new variable is one way to solve your issue, but it increases
> complexity and requires additional test coverage and documentation. So I
> would like to ask if this is extra complexity is really needed to achieve your
> goal.
> 
> For a similar use case, we use this approach:
> 
>  * bitbake always signs with a development key (no build variant
>    needed, no extra test matrix)
>  * Then we test the firmware
>  * For release, we use a script to replace the development signature
>    with a production signature (without rebuilding with bitbake)
> 
> This seems similar to your approach, but I don't understand why injecting the
> development public key is problematic for your workflow.
> Would it be better to have a post-processing step that simply replaces the
> public key, rather than requiring an empty DTB without any public key? That
> would mean that this patch would not be needed at all.
> 
> Adrian

Thanks for your reply.
Let me clarify the motivation behind this patch.

In our project, we only use "U-Boot and do not have an SPL". Our goal is to sign the U-Boot FIT image only,
without modifying or involving any SPL DTB. However, the current design in uboot-sign.bbclass assumes that
SPL is always present and forces the public key to be added into the SPL DTB, followed by a verification step
involving that DTB.

Here’s what I ran into:
We set SPL_SIGN_ENABLE = "1" to enable signing, which correctly generates the ITS with a signature node for U-Boot.
But enabling SPL_SIGN_ENABLE also triggers these additional steps in uboot-sign.bbclass:
a. The call to mkimage with -K ${SPL_DTB_BINARY} tries to inject the public key into the SPL DTB.
b. Then fit_check_sign tries to verify the FIT image using the SPL DTB.

This fails for us because:
1. We don't have ${SPL_DTB_BINARY} at all.
2. The build breaks during the verification step due to this missing SPL DTB.
3. Even setting an invalid SPL_SIGN_KEYDIR to skip actual signing does not bypass the verification step.

Instead of introducing a new variable like SPL_SIGN_ADD_PUBKEY, we could consider modifying the logic to gracefully handle the absence of SPL DTBs.
Here’s the idea:
1. Only concat SPL binary if both parts exist:
if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
    cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED} > "${SPL_BINARY}"
else
    if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
        bbwarn "Failure while adding public key to spl binary. Verified U-Boot boot won't be available."
    fi
fi

2. Conditionally apply -K only if SPL DTB exists:
${UBOOT_MKIMAGE_SIGN} \
    ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
    -F -k "${SPL_SIGN_KEYDIR}" \
    ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
    -r ${UBOOT_FITIMAGE_BINARY} \
    ${SPL_MKIMAGE_SIGN_ARGS}

3. Only run fit_check_sign if SPL DTB exists:
if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
    ${UBOOT_FIT_CHECK_SIGN} \
        -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
        -f ${UBOOT_FITIMAGE_BINARY}
fi

Could you please give me any suggestion?
Thanks-Jamin

> 
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-
> > recipe/uboot-sign.bbclass index 6ee1874cd6..3bcf47dd33 100644
> > --- a/meta/classes-recipe/uboot-sign.bbclass
> > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
> >  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE = "1"
> >  SPL_SIGN_ENABLE ?= "0"
> >
> > +# Whether to add (embed) the public key into the SPL Device Tree
> > (.dtb).
> > +# If set to "1", the key will be inserted into the /signature node
> > of the DTB
> > +# and fit_check_sign will be used to verify the signature.
> > +# If set to "0", only signing will be performed, without modifying
> > the DTB.
> > +SPL_SIGN_ADD_PUBKEY ?= "1"
> > +
> >  # Default value for deployment filenames.
> >  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
> >  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> > @@ -245,7 +251,9 @@ concat_spl_dtb() {
> >   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
> >   cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED} >
> > "${SPL_BINARY}"
> >   else
> > - bbwarn "Failure while adding public key to spl binary. Verified U-
> > Boot boot won't be available."
> > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then bbwarn "Failure while
> > + adding public key to spl binary. Verified U-
> > Boot boot won't be available."
> > + fi
> >   fi
> >  }
> >
> > @@ -474,15 +482,17 @@ EOF
> >   ${UBOOT_MKIMAGE_SIGN} \
> >   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}')
> > else ''} \
> >   -F -k "${SPL_SIGN_KEYDIR}" \
> > - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> >   -r ${UBOOT_FITIMAGE_BINARY} \
> >   ${SPL_MKIMAGE_SIGN_ARGS}
> >   #
> >   # Verify the U-boot FIT image and SPL dtb
> >   #
> > - ${UBOOT_FIT_CHECK_SIGN} \
> > - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > - -f ${UBOOT_FITIMAGE_BINARY}
> > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then ${UBOOT_FIT_CHECK_SIGN}
> > + \ -k "${SPL_DIR}/${SPL_DTB_BINARY}" \ -f ${UBOOT_FITIMAGE_BINARY} fi
> >   fi
> >
> >   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> >
> > -=-=-=-=-=-=-=-=-=-=-=-
> > Links: You receive all messages sent to this group.
> > View/Reply Online (#218114):
> > https://lists.openembedded.org/g/openembedded-core/message/218114
> > Mute This Topic: https://lists.openembedded.org/mt/113499585/3616858
> > Group Owner: openembedded-core+owner@lists.openembedded.org
> > Unsubscribe:
> > https://lists.openembedded.org/g/openembedded-core/unsub [
> > adrian.freihofer@siemens.com]
> > -=-=-=-=-=-=-=-=-=-=-=-
> >
AdrianF June 13, 2025, 12:36 p.m. UTC | #3 
On Fri, 2025-06-13 at 02:08 +0000, Jamin Lin wrote:
> Hi Adrian,
>
> > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > public key
> > injection optional
> >
> > On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via
> > lists.openembedded.org
> > wrote:
> > > Introduce SPL_SIGN_ADD_PUBKEY to control whether the public key
> > > is
> > > added into the SPL device tree and whether FIT signature
> > > verification
> > > is performed after signing.
> > >
> > > Key changes:
> > > - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> > > - Conditionally apply '-K <dtb>' to mkimage only if adding key is
> > > enabled
> > > - Skip fit_check_sign when public key injection is disabled
> > > - Suppress concat_spl_dtb() warnings if key adding is turned off
> > >
> > > This allows U-Boot FIT images to be signed without modifying the
> > > SPL
> > > DTB, useful in scenarios where public key management is handled
> > > elsewhere or post-processing will be done separately.
> >
> > Thank you for the patch.
> >
> > Introducing a new variable is one way to solve your issue, but it
> > increases
> > complexity and requires additional test coverage and documentation.
> > So I
> > would like to ask if this is extra complexity is really needed to
> > achieve your
> > goal.
> >
> > For a similar use case, we use this approach:
> >
> >  * bitbake always signs with a development key (no build variant
> >    needed, no extra test matrix)
> >  * Then we test the firmware
> >  * For release, we use a script to replace the development
> > signature
> >    with a production signature (without rebuilding with bitbake)
> >
> > This seems similar to your approach, but I don't understand why
> > injecting the
> > development public key is problematic for your workflow.
> > Would it be better to have a post-processing step that simply
> > replaces the
> > public key, rather than requiring an empty DTB without any public
> > key? That
> > would mean that this patch would not be needed at all.
> >
> > Adrian
>
> Thanks for your reply.
> Let me clarify the motivation behind this patch.
>
> In our project, we only use "U-Boot and do not have an SPL". Our goal
> is to sign the U-Boot FIT image only,
> without modifying or involving any SPL DTB. However, the current
> design in uboot-sign.bbclass assumes that
> SPL is always present and forces the public key to be added into the
> SPL DTB, followed by a verification step
> involving that DTB.
>
> Here’s what I ran into:
> We set SPL_SIGN_ENABLE = "1" to enable signing, which correctly
> generates the ITS with a signature node for U-Boot.
> But enabling SPL_SIGN_ENABLE also triggers these additional steps in
> uboot-sign.bbclass:
> a. The call to mkimage with -K ${SPL_DTB_BINARY} tries to inject the
> public key into the SPL DTB.
> b. Then fit_check_sign tries to verify the FIT image using the SPL
> DTB.

The explanation has helped me to understand the actual use case. But we
need to capture this understanding in the code, the documentation and
the commit message. (The updated commit message looks already good to
me).

I suggest to explicitly cover your use case in the code. The real
scenario isn't that you don't want to sign the SPL or inject the key
into the SPL's devicetree - it's that you don't compile an SPL at all.
That means that the implementation and the documentation should
transparently handle this use case. Introducing a variable
SPL_SIGN_ADD_PUBKEY looks like working around that in a misleading way
to me. It is simply confusing (for me) that we compile and sign an SPL
but do not add the public key to the DTB.

What about setting the SPL_DTB_BINARY (or some of the SPL* variables)
to an empty string? The source code could then check for [ -z
"${SPL_DTB_BINARY}" ] or [ -n "${SPL_DTB_BINARY}" ] and fail if
SPL_DTB_BINARY is expected but missing.

This would also allow us to document the use case by extending the
SPL_DTB_BINARY variable documentation with something like:
"If SPL_DTB_BINARY is set to empty string, the uboot-sign class assumes
that no SPL is built and all the related post-processing steps are
skipped."

>
> This fails for us because:
> 1. We don't have ${SPL_DTB_BINARY} at all.
> 2. The build breaks during the verification step due to this missing
> SPL DTB.
> 3. Even setting an invalid SPL_SIGN_KEYDIR to skip actual signing
> does not bypass the verification step.
>
> Instead of introducing a new variable like SPL_SIGN_ADD_PUBKEY, we
> could consider modifying the logic to gracefully handle the absence
> of SPL DTBs.
> Here’s the idea:
> 1. Only concat SPL binary if both parts exist:
> if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
>     cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED} >
> "${SPL_BINARY}"
> else
>     if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
>         bbwarn "Failure while adding public key to spl binary.
> Verified U-Boot boot won't be available."
>     fi
> fi
>
> 2. Conditionally apply -K only if SPL DTB exists:
> ${UBOOT_MKIMAGE_SIGN} \
>     ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}')
> else ''} \
>     -F -k "${SPL_SIGN_KEYDIR}" \
>     ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
>     -r ${UBOOT_FITIMAGE_BINARY} \
>     ${SPL_MKIMAGE_SIGN_ARGS}
>
> 3. Only run fit_check_sign if SPL DTB exists:
> if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
>     ${UBOOT_FIT_CHECK_SIGN} \
>         -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
>         -f ${UBOOT_FITIMAGE_BINARY}
> fi
>
> Could you please give me any suggestion?

"Gracefully handle the absence of SPL DTBs" is fragile. A file might be
missing due to various bugs or build issues. If the code fails
immediately when a file is expected but missing, related bugs would
likely be caught by the Yocto autobuilder and problematic patches could
be immediately rejected.

Depending on the details, such graceful file checks can also become
very problematic when it comes to builds from sstate-cache versus full
builds. When the build comes from the sstate-cache, there is simply no
build folder at all - only the sysroots and the deploy folder are
restored from the cache. This creates another possibility for various
corner cases which we should avoid by design.

Therefore, I prefer an implementation that relies on if statements
checking BitBake variables (-z, -n) over an implementation that relies
on if statements checking the state of a potentially broken or absent
build directory (-f, -e, -x, -d).

Is it possible to come up with an implementation which does, for
example:

if [ -n "${SPL_DTB_BINARY}" ]; then

instead of

if [ -f "${SPL_DTB_BINARY}" ]; then

Thanks for the clarification
Adrian



> Thanks-Jamin
>
> >
> > >
> > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > ---
> > >  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-----
> > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/meta/classes-recipe/uboot-sign.bbclass
> > > b/meta/classes-
> > > recipe/uboot-sign.bbclass index 6ee1874cd6..3bcf47dd33 100644
> > > --- a/meta/classes-recipe/uboot-sign.bbclass
> > > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > > @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
> > >  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE =
> > > "1"
> > >  SPL_SIGN_ENABLE ?= "0"
> > >
> > > +# Whether to add (embed) the public key into the SPL Device Tree
> > > (.dtb).
> > > +# If set to "1", the key will be inserted into the /signature
> > > node
> > > of the DTB
> > > +# and fit_check_sign will be used to verify the signature.
> > > +# If set to "0", only signing will be performed, without
> > > modifying
> > > the DTB.
> > > +SPL_SIGN_ADD_PUBKEY ?= "1"
> > > +
> > >  # Default value for deployment filenames.
> > >  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
> > >  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> > > @@ -245,7 +251,9 @@ concat_spl_dtb() {
> > >   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
> > >   cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED}
> > > >
> > > "${SPL_BINARY}"
> > >   else
> > > - bbwarn "Failure while adding public key to spl binary. Verified
> > > U-
> > > Boot boot won't be available."
> > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then bbwarn "Failure
> > > while
> > > + adding public key to spl binary. Verified U-
> > > Boot boot won't be available."
> > > + fi
> > >   fi
> > >  }
> > >
> > > @@ -474,15 +482,17 @@ EOF
> > >   ${UBOOT_MKIMAGE_SIGN} \
> > >   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > len('${SPL_MKIMAGE_DTCOPTS}')
> > > else ''} \
> > >   -F -k "${SPL_SIGN_KEYDIR}" \
> > > - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > >   -r ${UBOOT_FITIMAGE_BINARY} \
> > >   ${SPL_MKIMAGE_SIGN_ARGS}
> > >   #
> > >   # Verify the U-boot FIT image and SPL dtb
> > >   #
> > > - ${UBOOT_FIT_CHECK_SIGN} \
> > > - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > - -f ${UBOOT_FITIMAGE_BINARY}
> > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> > > ${UBOOT_FIT_CHECK_SIGN}
> > > + \ -k "${SPL_DIR}/${SPL_DTB_BINARY}" \ -f
> > > ${UBOOT_FITIMAGE_BINARY} fi
> > >   fi
> > >
> > >   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > >
> > > -=-=-=-=-=-=-=-=-=-=-=-
> > > Links: You receive all messages sent to this group.
> > > View/Reply Online (#218114):
> > > https://lists.openembedded.org/g/openembedded-core/message/218114
> > > Mute This Topic:
> > > https://lists.openembedded.org/mt/113499585/3616858
> > > Group Owner: openembedded-core+owner@lists.openembedded.org
> > > Unsubscribe:
> > > https://lists.openembedded.org/g/openembedded-core/unsub
> > >  [
> > > adrian.freihofer@siemens.com]
> > > -=-=-=-=-=-=-=-=-=-=-=-
> > >
>
Jamin Lin June 16, 2025, 3:50 a.m. UTC | #4 
Hi Adrian, 

> Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB public key
> injection optional
> 
> On Fri, 2025-06-13 at 02:08 +0000, Jamin Lin wrote:
> > Hi Adrian,
> >
> > > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > > public key injection optional
> > >
> > > On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via
> > > lists.openembedded.org
> > > wrote:
> > > > Introduce SPL_SIGN_ADD_PUBKEY to control whether the public key is
> > > > added into the SPL device tree and whether FIT signature
> > > > verification is performed after signing.
> > > >
> > > > Key changes:
> > > > - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> > > > - Conditionally apply '-K <dtb>' to mkimage only if adding key is
> > > > enabled
> > > > - Skip fit_check_sign when public key injection is disabled
> > > > - Suppress concat_spl_dtb() warnings if key adding is turned off
> > > >
> > > > This allows U-Boot FIT images to be signed without modifying the
> > > > SPL DTB, useful in scenarios where public key management is
> > > > handled elsewhere or post-processing will be done separately.
> > >
> > > Thank you for the patch.
> > >
> > > Introducing a new variable is one way to solve your issue, but it
> > > increases complexity and requires additional test coverage and
> > > documentation.
> > > So I
> > > would like to ask if this is extra complexity is really needed to
> > > achieve your goal.
> > >
> > > For a similar use case, we use this approach:
> > >
> > >  * bitbake always signs with a development key (no build variant
> > >    needed, no extra test matrix)
> > >  * Then we test the firmware
> > >  * For release, we use a script to replace the development signature
> > >    with a production signature (without rebuilding with bitbake)
> > >
> > > This seems similar to your approach, but I don't understand why
> > > injecting the development public key is problematic for your
> > > workflow.
> > > Would it be better to have a post-processing step that simply
> > > replaces the public key, rather than requiring an empty DTB without
> > > any public key? That would mean that this patch would not be needed
> > > at all.
> > >
> > > Adrian
> >
> > Thanks for your reply.
> > Let me clarify the motivation behind this patch.
> >
> > In our project, we only use "U-Boot and do not have an SPL". Our goal
> > is to sign the U-Boot FIT image only, without modifying or involving
> > any SPL DTB. However, the current design in uboot-sign.bbclass assumes
> > that SPL is always present and forces the public key to be added into
> > the SPL DTB, followed by a verification step involving that DTB.
> >
> > Here’s what I ran into:
> > We set SPL_SIGN_ENABLE = "1" to enable signing, which correctly
> > generates the ITS with a signature node for U-Boot.
> > But enabling SPL_SIGN_ENABLE also triggers these additional steps in
> > uboot-sign.bbclass:
> > a. The call to mkimage with -K ${SPL_DTB_BINARY} tries to inject the
> > public key into the SPL DTB.
> > b. Then fit_check_sign tries to verify the FIT image using the SPL
> > DTB.
> 
> The explanation has helped me to understand the actual use case. But we need
> to capture this understanding in the code, the documentation and the commit
> message. (The updated commit message looks already good to me).
> 
> I suggest to explicitly cover your use case in the code. The real scenario isn't
> that you don't want to sign the SPL or inject the key into the SPL's devicetree -
> it's that you don't compile an SPL at all.
> That means that the implementation and the documentation should
> transparently handle this use case. Introducing a variable
> SPL_SIGN_ADD_PUBKEY looks like working around that in a misleading way to
> me. It is simply confusing (for me) that we compile and sign an SPL but do not
> add the public key to the DTB.
> 
> What about setting the SPL_DTB_BINARY (or some of the SPL* variables) to an
> empty string? The source code could then check for [ -z "${SPL_DTB_BINARY}" ]
> or [ -n "${SPL_DTB_BINARY}" ] and fail if SPL_DTB_BINARY is expected but
> missing.
> 
> This would also allow us to document the use case by extending the
> SPL_DTB_BINARY variable documentation with something like:
> "If SPL_DTB_BINARY is set to empty string, the uboot-sign class assumes that
> no SPL is built and all the related post-processing steps are skipped."
> 
> >
> > This fails for us because:
> > 1. We don't have ${SPL_DTB_BINARY} at all.
> > 2. The build breaks during the verification step due to this missing
> > SPL DTB.
> > 3. Even setting an invalid SPL_SIGN_KEYDIR to skip actual signing does
> > not bypass the verification step.
> >
> > Instead of introducing a new variable like SPL_SIGN_ADD_PUBKEY, we
> > could consider modifying the logic to gracefully handle the absence of
> > SPL DTBs.
> > Here’s the idea:
> > 1. Only concat SPL binary if both parts exist:
> > if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> >     cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED}
> >
> > "${SPL_BINARY}"
> > else
> >     if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> >         bbwarn "Failure while adding public key to spl binary.
> > Verified U-Boot boot won't be available."
> >     fi
> > fi
> >
> > 2. Conditionally apply -K only if SPL DTB exists:
> > ${UBOOT_MKIMAGE_SIGN} \
> >     ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}')
> > else ''} \
> >     -F -k "${SPL_SIGN_KEYDIR}" \
> >     ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> >     -r ${UBOOT_FITIMAGE_BINARY} \
> >     ${SPL_MKIMAGE_SIGN_ARGS}
> >
> > 3. Only run fit_check_sign if SPL DTB exists:
> > if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> >     ${UBOOT_FIT_CHECK_SIGN} \
> >         -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> >         -f ${UBOOT_FITIMAGE_BINARY}
> > fi
> >
> > Could you please give me any suggestion?
> 
> "Gracefully handle the absence of SPL DTBs" is fragile. A file might be missing
> due to various bugs or build issues. If the code fails immediately when a file is
> expected but missing, related bugs would likely be caught by the Yocto
> autobuilder and problematic patches could be immediately rejected.
> 
> Depending on the details, such graceful file checks can also become very
> problematic when it comes to builds from sstate-cache versus full builds. When
> the build comes from the sstate-cache, there is simply no build folder at all -
> only the sysroots and the deploy folder are restored from the cache. This
> creates another possibility for various corner cases which we should avoid by
> design.
> 
> Therefore, I prefer an implementation that relies on if statements checking
> BitBake variables (-z, -n) over an implementation that relies on if statements
> checking the state of a potentially broken or absent build directory (-f, -e, -x,
> -d).
> 
> Is it possible to come up with an implementation which does, for
> example:
> 
> if [ -n "${SPL_DTB_BINARY}" ]; then
> 
> instead of
> 
> if [ -f "${SPL_DTB_BINARY}" ]; then
> 
> Thanks for the clarification
> Adrian

> 
Thank you very much for your suggestion and detailed feedback.
Based on your advice, I have updated the code and the related variables accordingly. Here’s a summary of what I changed and my rationale:
Updated Variables
I set the variables like this to clearly represent the case where we do not build an SPL:

SPL_SIGN_ENABLE ?= "1"
SPL_SIGN_KEYDIR ?= "${STAGING_DATADIR_NATIVE}/aspeed-secure-config/keys"
SPL_DTB_BINARY = ""
UBOOT_FITIMAGE_ENABLE = "1"

SPL_DTB_BINARY is explicitly set to an empty string, which means no SPL is built.
UBOOT_FITIMAGE_ENABLE is still 1 to ensure that the U-Boot FIT image is generated as usual.
SPL_SIGN_ENABLE controls whether the FIT image gets signed.

Code Changes
1. Removed redundant SPL_DTB_BINARY checks for FIT image generation and deployment
I dropped the checks for -n ${SPL_DTB_BINARY} in the FIT image assembly and deploy helpers because the FIT image generation should proceed as long as UBOOT_FITIMAGE_ENABLE is 1,
regardless of whether the SPL exists or not.

- if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ]; then
+ if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then

2. Simplified deploy condition
Similarly, I updated the deploy helper to deploy the ITS and FIT image when UBOOT_FITIMAGE_ENABLE is enabled.
-       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ]; then
+       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
                if [ -n "${type}" ]; then
                        uboot_its_image="u-boot-its-${type}-${PV}-${PR}"
                        uboot_fitimage_image="u-boot-fitImage-${type}-${PV}-${PR}"

3. Adjusted SPL DTB deploy condition
I unified the check to reuse SPL_DTB_BINARY instead of having an extra SPL_DTB_SIGNED variable. So if SPL_DTB_BINARY is empty, we skip deploying the SPL DTB.
-       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ; then
+       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ; then
                deploy_spl_dtb $type
        fi

4. Improved the sign logic
Now the sign step checks if SPL_DTB_BINARY is empty:
If present, it signs the FIT image and injects the public key into the SPL DTB, then verifies both.
If empty, it only signs the FIT image and generates the ITS with the signature node, but does not attempt to verify or add the key to a non-existent SPL DTB.

Key Behavior Explained
If SPL_DTB_BINARY is empty, we assume there is no SPL.
If UBOOT_FITIMAGE_ENABLE=1, we always create the FIT image and ITS.
If SPL_SIGN_ENABLE=1, we always sign the FIT image, but only inject the key into the SPL DTB if it exists.


Please let me know if this approach aligns with what you had in mind, or if you have any further suggestions to refine it!
Thanks again for your valuable input.
Thanks-Jamin

diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
index 73e9ce3f11..67f14ac92f 100644
--- a/meta/classes-recipe/uboot-sign.bbclass
+++ b/meta/classes-recipe/uboot-sign.bbclass
@@ -466,21 +466,29 @@ EOF
                ${UBOOT_FITIMAGE_BINARY}

        if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
-               #
-               # Sign the U-boot FIT image and add public key to SPL dtb
-               #
-               ${UBOOT_MKIMAGE_SIGN} \
-                       ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
-                       -F -k "${SPL_SIGN_KEYDIR}" \
-                       -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
-                       -r ${UBOOT_FITIMAGE_BINARY} \
-                       ${SPL_MKIMAGE_SIGN_ARGS}
-               #
-               # Verify the U-boot FIT image and SPL dtb
-               #
-               ${UBOOT_FIT_CHECK_SIGN} \
-                       -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
-                       -f ${UBOOT_FITIMAGE_BINARY}
+               if [ -n "${SPL_DTB_BINARY}" ] ; then
+                       #
+                       # Sign the U-boot FIT image and add public key to SPL dtb
+                       #
+                       ${UBOOT_MKIMAGE_SIGN} \
+                               ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
+                               -F -k "${SPL_SIGN_KEYDIR}" \
+                               -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
+                               -r ${UBOOT_FITIMAGE_BINARY} \
+                               ${SPL_MKIMAGE_SIGN_ARGS}
+
+                       # Verify the U-boot FIT image and SPL dtb
+                       ${UBOOT_FIT_CHECK_SIGN} \
+                               -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
+                               -f ${UBOOT_FITIMAGE_BINARY}
+               else
+                       # Sign the U-boot FIT image
+                       ${UBOOT_MKIMAGE_SIGN} \
+                               ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
+                               -F -k "${SPL_SIGN_KEYDIR}" \
+                               -r ${UBOOT_FITIMAGE_BINARY} \
+                               ${SPL_MKIMAGE_SIGN_ARGS}
+               fi
        fi

        if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
@@ -496,7 +504,7 @@ uboot_assemble_fitimage_helper() {
                concat_dtb "$type" "$binary"
        fi

-       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ]; then
+       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
                uboot_fitimage_assemble
        fi

@@ -543,7 +551,7 @@ deploy_helper() {
                deploy_dtb $type
        fi

-       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ]; then
+       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
                if [ -n "${type}" ]; then
                        uboot_its_image="u-boot-its-${type}-${PV}-${PR}"
                        uboot_fitimage_image="u-boot-fitImage-${type}-${PV}-${PR}"
@@ -561,7 +569,7 @@ deploy_helper() {
                fi
        fi

-       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ; then
+       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ; then
                deploy_spl_dtb $type
        fi
 }

> 
> 
> > Thanks-Jamin
> >
> > >
> > > >
> > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > > ---
> > > >  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-----
> > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/meta/classes-recipe/uboot-sign.bbclass
> > > > b/meta/classes-
> > > > recipe/uboot-sign.bbclass index 6ee1874cd6..3bcf47dd33 100644
> > > > --- a/meta/classes-recipe/uboot-sign.bbclass
> > > > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > > > @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
> > > >  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE =
> > > > "1"
> > > >  SPL_SIGN_ENABLE ?= "0"
> > > >
> > > > +# Whether to add (embed) the public key into the SPL Device Tree
> > > > (.dtb).
> > > > +# If set to "1", the key will be inserted into the /signature
> > > > node
> > > > of the DTB
> > > > +# and fit_check_sign will be used to verify the signature.
> > > > +# If set to "0", only signing will be performed, without
> > > > modifying
> > > > the DTB.
> > > > +SPL_SIGN_ADD_PUBKEY ?= "1"
> > > > +
> > > >  # Default value for deployment filenames.
> > > >  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
> > > >  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> > > > @@ -245,7 +251,9 @@ concat_spl_dtb() {
> > > >   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
> > > >   cat ${SPL_DIR}/${SPL_NODTB_BINARY}
> ${SPL_DIR}/${SPL_DTB_SIGNED}
> > > > >
> > > > "${SPL_BINARY}"
> > > >   else
> > > > - bbwarn "Failure while adding public key to spl binary. Verified
> > > > U-
> > > > Boot boot won't be available."
> > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then bbwarn "Failure
> > > > while
> > > > + adding public key to spl binary. Verified U-
> > > > Boot boot won't be available."
> > > > + fi
> > > >   fi
> > > >  }
> > > >
> > > > @@ -474,15 +482,17 @@ EOF
> > > >   ${UBOOT_MKIMAGE_SIGN} \
> > > >   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > > len('${SPL_MKIMAGE_DTCOPTS}')
> > > > else ''} \
> > > >   -F -k "${SPL_SIGN_KEYDIR}" \
> > > > - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > > >   -r ${UBOOT_FITIMAGE_BINARY} \
> > > >   ${SPL_MKIMAGE_SIGN_ARGS}
> > > >   #
> > > >   # Verify the U-boot FIT image and SPL dtb
> > > >   #
> > > > - ${UBOOT_FIT_CHECK_SIGN} \
> > > > - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > - -f ${UBOOT_FITIMAGE_BINARY}
> > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> > > > ${UBOOT_FIT_CHECK_SIGN}
> > > > + \ -k "${SPL_DIR}/${SPL_DTB_BINARY}" \ -f
> > > > ${UBOOT_FITIMAGE_BINARY} fi
> > > >   fi
> > > >
> > > >   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > > >
> > > > -=-=-=-=-=-=-=-=-=-=-=-
> > > > Links: You receive all messages sent to this group.
> > > > View/Reply Online (#218114):
> > > > https://lists.openembedded.org/g/openembedded-core/message/218114
> > > > Mute This Topic:
> > > > https://lists.openembedded.org/mt/113499585/3616858
> > > > Group Owner: openembedded-core+owner@lists.openembedded.org
> > > > Unsubscribe:
> > > > https://lists.openembedded.org/g/openembedded-core/unsub
> > > >  [
> > > > adrian.freihofer@siemens.com]
> > > > -=-=-=-=-=-=-=-=-=-=-=-
> > > >
> >
AdrianF June 16, 2025, 9:16 a.m. UTC | #5 
Hi Jamin

Thank you for these improvements. To me it looks much more
understandable, robust and also test-able now.

Regarding test-able, would it be possible to add a test case to oe-
selftest for this use case? E.g. here:
https://git.yoctoproject.org/poky/tree/meta/lib/oeqa/selftest/cases/fitimage.py#n1125
it should be relatively straight forward to add  something like this:


    def test_sign_uboot_fit_without_spl(self):
        """
        Summary:     Test building and signing u-boot without SPL
        ...
        """
        config = """
# There's no U-boot defconfig with CONFIG_FIT_SIGNATURE yet, so we need
at
# least CONFIG_SPL_LOAD_FIT and CONFIG_SPL_OF_CONTROL set
MACHINE = "qemuarm"
UBOOT_MACHINE = "am57xx_evm_defconfig"
SPL_BINARY = ""
# Enable creation and signing of the U-Boot fitImage
UBOOT_FITIMAGE_ENABLE = "1"
SPL_SIGN_ENABLE = "1"
SPL_SIGN_KEYNAME = "spl-oe-selftest"
SPL_SIGN_KEYDIR = "${TOPDIR}/signing-keys"
...
"""
        self.write_config(config)
        bb_vars = self._fit_get_bb_vars()
        self._test_fitimage(bb_vars)



You can run the oe-selftest suite with a local.conf file from the
template (without modifications) and this additional line:

SANITY_TESTED_DISTROS = ""

Then you can execute the test by
oe-selftest -v -K -r
fitimage.UBootFitImageTests.test_sign_uboot_fit_without_spl

With -v the log file in build/tmp/log/oe-selftest-results.log shows a
log of details which is helpful for understanding how the test suite
works.


One more detail: shellcheck complains against lines like this:

if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ];

We should change such lines to become:

if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ] && [ -n "${SPL_DTB_BINARY}" ];

if we need to touch them anyway.


Thank you and regards,
Adrian


On Mon, 2025-06-16 at 03:50 +0000, Jamin Lin wrote:
> Hi Adrian,
>
> > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > public key
> > injection optional
> >
> > On Fri, 2025-06-13 at 02:08 +0000, Jamin Lin wrote:
> > > Hi Adrian,
> > >
> > > > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > > > public key injection optional
> > > >
> > > > On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via
> > > > lists.openembedded.org
> > > > wrote:
> > > > > Introduce SPL_SIGN_ADD_PUBKEY to control whether the public
> > > > > key is
> > > > > added into the SPL device tree and whether FIT signature
> > > > > verification is performed after signing.
> > > > >
> > > > > Key changes:
> > > > > - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> > > > > - Conditionally apply '-K <dtb>' to mkimage only if adding
> > > > > key is
> > > > > enabled
> > > > > - Skip fit_check_sign when public key injection is disabled
> > > > > - Suppress concat_spl_dtb() warnings if key adding is turned
> > > > > off
> > > > >
> > > > > This allows U-Boot FIT images to be signed without modifying
> > > > > the
> > > > > SPL DTB, useful in scenarios where public key management is
> > > > > handled elsewhere or post-processing will be done separately.
> > > >
> > > > Thank you for the patch.
> > > >
> > > > Introducing a new variable is one way to solve your issue, but
> > > > it
> > > > increases complexity and requires additional test coverage and
> > > > documentation.
> > > > So I
> > > > would like to ask if this is extra complexity is really needed
> > > > to
> > > > achieve your goal.
> > > >
> > > > For a similar use case, we use this approach:
> > > >
> > > >  * bitbake always signs with a development key (no build
> > > > variant
> > > >    needed, no extra test matrix)
> > > >  * Then we test the firmware
> > > >  * For release, we use a script to replace the development
> > > > signature
> > > >    with a production signature (without rebuilding with
> > > > bitbake)
> > > >
> > > > This seems similar to your approach, but I don't understand why
> > > > injecting the development public key is problematic for your
> > > > workflow.
> > > > Would it be better to have a post-processing step that simply
> > > > replaces the public key, rather than requiring an empty DTB
> > > > without
> > > > any public key? That would mean that this patch would not be
> > > > needed
> > > > at all.
> > > >
> > > > Adrian
> > >
> > > Thanks for your reply.
> > > Let me clarify the motivation behind this patch.
> > >
> > > In our project, we only use "U-Boot and do not have an SPL". Our
> > > goal
> > > is to sign the U-Boot FIT image only, without modifying or
> > > involving
> > > any SPL DTB. However, the current design in uboot-sign.bbclass
> > > assumes
> > > that SPL is always present and forces the public key to be added
> > > into
> > > the SPL DTB, followed by a verification step involving that DTB.
> > >
> > > Here’s what I ran into:
> > > We set SPL_SIGN_ENABLE = "1" to enable signing, which correctly
> > > generates the ITS with a signature node for U-Boot.
> > > But enabling SPL_SIGN_ENABLE also triggers these additional steps
> > > in
> > > uboot-sign.bbclass:
> > > a. The call to mkimage with -K ${SPL_DTB_BINARY} tries to inject
> > > the
> > > public key into the SPL DTB.
> > > b. Then fit_check_sign tries to verify the FIT image using the
> > > SPL
> > > DTB.
> >
> > The explanation has helped me to understand the actual use case.
> > But we need
> > to capture this understanding in the code, the documentation and
> > the commit
> > message. (The updated commit message looks already good to me).
> >
> > I suggest to explicitly cover your use case in the code. The real
> > scenario isn't
> > that you don't want to sign the SPL or inject the key into the
> > SPL's devicetree -
> > it's that you don't compile an SPL at all.
> > That means that the implementation and the documentation should
> > transparently handle this use case. Introducing a variable
> > SPL_SIGN_ADD_PUBKEY looks like working around that in a misleading
> > way to
> > me. It is simply confusing (for me) that we compile and sign an SPL
> > but do not
> > add the public key to the DTB.
> >
> > What about setting the SPL_DTB_BINARY (or some of the SPL*
> > variables) to an
> > empty string? The source code could then check for [ -z
> > "${SPL_DTB_BINARY}" ]
> > or [ -n "${SPL_DTB_BINARY}" ] and fail if SPL_DTB_BINARY is
> > expected but
> > missing.
> >
> > This would also allow us to document the use case by extending the
> > SPL_DTB_BINARY variable documentation with something like:
> > "If SPL_DTB_BINARY is set to empty string, the uboot-sign class
> > assumes that
> > no SPL is built and all the related post-processing steps are
> > skipped."
> >
> > >
> > > This fails for us because:
> > > 1. We don't have ${SPL_DTB_BINARY} at all.
> > > 2. The build breaks during the verification step due to this
> > > missing
> > > SPL DTB.
> > > 3. Even setting an invalid SPL_SIGN_KEYDIR to skip actual signing
> > > does
> > > not bypass the verification step.
> > >
> > > Instead of introducing a new variable like SPL_SIGN_ADD_PUBKEY,
> > > we
> > > could consider modifying the logic to gracefully handle the
> > > absence of
> > > SPL DTBs.
> > > Here’s the idea:
> > > 1. Only concat SPL binary if both parts exist:
> > > if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > >     cat ${SPL_DIR}/${SPL_NODTB_BINARY}
> > > ${SPL_DIR}/${SPL_DTB_SIGNED}
> > >
> > > "${SPL_BINARY}"
> > > else
> > >     if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> > >         bbwarn "Failure while adding public key to spl binary.
> > > Verified U-Boot boot won't be available."
> > >     fi
> > > fi
> > >
> > > 2. Conditionally apply -K only if SPL DTB exists:
> > > ${UBOOT_MKIMAGE_SIGN} \
> > >     ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > len('${SPL_MKIMAGE_DTCOPTS}')
> > > else ''} \
> > >     -F -k "${SPL_SIGN_KEYDIR}" \
> > >     ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > >     -r ${UBOOT_FITIMAGE_BINARY} \
> > >     ${SPL_MKIMAGE_SIGN_ARGS}
> > >
> > > 3. Only run fit_check_sign if SPL DTB exists:
> > > if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> > >     ${UBOOT_FIT_CHECK_SIGN} \
> > >         -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > >         -f ${UBOOT_FITIMAGE_BINARY}
> > > fi
> > >
> > > Could you please give me any suggestion?
> >
> > "Gracefully handle the absence of SPL DTBs" is fragile. A file
> > might be missing
> > due to various bugs or build issues. If the code fails immediately
> > when a file is
> > expected but missing, related bugs would likely be caught by the
> > Yocto
> > autobuilder and problematic patches could be immediately rejected.
> >
> > Depending on the details, such graceful file checks can also become
> > very
> > problematic when it comes to builds from sstate-cache versus full
> > builds. When
> > the build comes from the sstate-cache, there is simply no build
> > folder at all -
> > only the sysroots and the deploy folder are restored from the
> > cache. This
> > creates another possibility for various corner cases which we
> > should avoid by
> > design.
> >
> > Therefore, I prefer an implementation that relies on if statements
> > checking
> > BitBake variables (-z, -n) over an implementation that relies on if
> > statements
> > checking the state of a potentially broken or absent build
> > directory (-f, -e, -x,
> > -d).
> >
> > Is it possible to come up with an implementation which does, for
> > example:
> >
> > if [ -n "${SPL_DTB_BINARY}" ]; then
> >
> > instead of
> >
> > if [ -f "${SPL_DTB_BINARY}" ]; then
> >
> > Thanks for the clarification
> > Adrian
>
> >
> Thank you very much for your suggestion and detailed feedback.
> Based on your advice, I have updated the code and the related
> variables accordingly. Here’s a summary of what I changed and my
> rationale:
> Updated Variables
> I set the variables like this to clearly represent the case where we
> do not build an SPL:
>
> SPL_SIGN_ENABLE ?= "1"
> SPL_SIGN_KEYDIR ?= "${STAGING_DATADIR_NATIVE}/aspeed-secure-
> config/keys"
> SPL_DTB_BINARY = ""
> UBOOT_FITIMAGE_ENABLE = "1"
>
> SPL_DTB_BINARY is explicitly set to an empty string, which means no
> SPL is built.
> UBOOT_FITIMAGE_ENABLE is still 1 to ensure that the U-Boot FIT image
> is generated as usual.
> SPL_SIGN_ENABLE controls whether the FIT image gets signed.
>
> Code Changes
> 1. Removed redundant SPL_DTB_BINARY checks for FIT image generation
> and deployment
> I dropped the checks for -n ${SPL_DTB_BINARY} in the FIT image
> assembly and deploy helpers because the FIT image generation should
> proceed as long as UBOOT_FITIMAGE_ENABLE is 1,
> regardless of whether the SPL exists or not.
>
> - if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ];
> then
> + if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
>
> 2. Simplified deploy condition
> Similarly, I updated the deploy helper to deploy the ITS and FIT
> image when UBOOT_FITIMAGE_ENABLE is enabled.
> -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> "${SPL_DTB_BINARY}" ]; then
> +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
>                 if [ -n "${type}" ]; then
>                         uboot_its_image="u-boot-its-${type}-${PV}-
> ${PR}"
>                         uboot_fitimage_image="u-boot-fitImage-
> ${type}-${PV}-${PR}"
>
> 3. Adjusted SPL DTB deploy condition
> I unified the check to reuse SPL_DTB_BINARY instead of having an
> extra SPL_DTB_SIGNED variable. So if SPL_DTB_BINARY is empty, we skip
> deploying the SPL DTB.
> -       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ;
> then
> +       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ;
> then
>                 deploy_spl_dtb $type
>         fi
>
> 4. Improved the sign logic
> Now the sign step checks if SPL_DTB_BINARY is empty:
> If present, it signs the FIT image and injects the public key into
> the SPL DTB, then verifies both.
> If empty, it only signs the FIT image and generates the ITS with the
> signature node, but does not attempt to verify or add the key to a
> non-existent SPL DTB.
>
> Key Behavior Explained
> If SPL_DTB_BINARY is empty, we assume there is no SPL.
> If UBOOT_FITIMAGE_ENABLE=1, we always create the FIT image and ITS.
> If SPL_SIGN_ENABLE=1, we always sign the FIT image, but only inject
> the key into the SPL DTB if it exists.
>
>
> Please let me know if this approach aligns with what you had in mind,
> or if you have any further suggestions to refine it!
> Thanks again for your valuable input.
> Thanks-Jamin
>
> diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-
> recipe/uboot-sign.bbclass
> index 73e9ce3f11..67f14ac92f 100644
> --- a/meta/classes-recipe/uboot-sign.bbclass
> +++ b/meta/classes-recipe/uboot-sign.bbclass
> @@ -466,21 +466,29 @@ EOF
>                 ${UBOOT_FITIMAGE_BINARY}
>
>         if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
> -               #
> -               # Sign the U-boot FIT image and add public key to SPL
> dtb
> -               #
> -               ${UBOOT_MKIMAGE_SIGN} \
> -                       ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> -                       -F -k "${SPL_SIGN_KEYDIR}" \
> -                       -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> -                       -r ${UBOOT_FITIMAGE_BINARY} \
> -                       ${SPL_MKIMAGE_SIGN_ARGS}
> -               #
> -               # Verify the U-boot FIT image and SPL dtb
> -               #
> -               ${UBOOT_FIT_CHECK_SIGN} \
> -                       -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> -                       -f ${UBOOT_FITIMAGE_BINARY}
> +               if [ -n "${SPL_DTB_BINARY}" ] ; then
> +                       #
> +                       # Sign the U-boot FIT image and add public
> key to SPL dtb
> +                       #
> +                       ${UBOOT_MKIMAGE_SIGN} \
> +                               ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> +                               -F -k "${SPL_SIGN_KEYDIR}" \
> +                               -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> +                               -r ${UBOOT_FITIMAGE_BINARY} \
> +                               ${SPL_MKIMAGE_SIGN_ARGS}
> +
> +                       # Verify the U-boot FIT image and SPL dtb
> +                       ${UBOOT_FIT_CHECK_SIGN} \
> +                               -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> +                               -f ${UBOOT_FITIMAGE_BINARY}
> +               else
> +                       # Sign the U-boot FIT image
> +                       ${UBOOT_MKIMAGE_SIGN} \
> +                               ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> +                               -F -k "${SPL_SIGN_KEYDIR}" \
> +                               -r ${UBOOT_FITIMAGE_BINARY} \
> +                               ${SPL_MKIMAGE_SIGN_ARGS}
> +               fi
>         fi
>
>         if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> @@ -496,7 +504,7 @@ uboot_assemble_fitimage_helper() {
>                 concat_dtb "$type" "$binary"
>         fi
>
> -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> "${SPL_DTB_BINARY}" ]; then
> +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
>                 uboot_fitimage_assemble
>         fi
>
> @@ -543,7 +551,7 @@ deploy_helper() {
>                 deploy_dtb $type
>         fi
>
> -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> "${SPL_DTB_BINARY}" ]; then
> +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
>                 if [ -n "${type}" ]; then
>                         uboot_its_image="u-boot-its-${type}-${PV}-
> ${PR}"
>                         uboot_fitimage_image="u-boot-fitImage-
> ${type}-${PV}-${PR}"
> @@ -561,7 +569,7 @@ deploy_helper() {
>                 fi
>         fi
>
> -       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ;
> then
> +       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ;
> then
>                 deploy_spl_dtb $type
>         fi
>  }
>
> >
> >
> > > Thanks-Jamin
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > > > ---
> > > > >  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-
> > > > > ----
> > > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/meta/classes-recipe/uboot-sign.bbclass
> > > > > b/meta/classes-
> > > > > recipe/uboot-sign.bbclass index 6ee1874cd6..3bcf47dd33 100644
> > > > > --- a/meta/classes-recipe/uboot-sign.bbclass
> > > > > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > > > > @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
> > > > >  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE
> > > > > =
> > > > > "1"
> > > > >  SPL_SIGN_ENABLE ?= "0"
> > > > >
> > > > > +# Whether to add (embed) the public key into the SPL Device
> > > > > Tree
> > > > > (.dtb).
> > > > > +# If set to "1", the key will be inserted into the
> > > > > /signature
> > > > > node
> > > > > of the DTB
> > > > > +# and fit_check_sign will be used to verify the signature.
> > > > > +# If set to "0", only signing will be performed, without
> > > > > modifying
> > > > > the DTB.
> > > > > +SPL_SIGN_ADD_PUBKEY ?= "1"
> > > > > +
> > > > >  # Default value for deployment filenames.
> > > > >  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
> > > > >  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> > > > > @@ -245,7 +251,9 @@ concat_spl_dtb() {
> > > > >   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > > > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
> > > > >   cat ${SPL_DIR}/${SPL_NODTB_BINARY}
> > ${SPL_DIR}/${SPL_DTB_SIGNED}
> > > > > >
> > > > > "${SPL_BINARY}"
> > > > >   else
> > > > > - bbwarn "Failure while adding public key to spl binary.
> > > > > Verified
> > > > > U-
> > > > > Boot boot won't be available."
> > > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then bbwarn "Failure
> > > > > while
> > > > > + adding public key to spl binary. Verified U-
> > > > > Boot boot won't be available."
> > > > > + fi
> > > > >   fi
> > > > >  }
> > > > >
> > > > > @@ -474,15 +482,17 @@ EOF
> > > > >   ${UBOOT_MKIMAGE_SIGN} \
> > > > >   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > > > len('${SPL_MKIMAGE_DTCOPTS}')
> > > > > else ''} \
> > > > >   -F -k "${SPL_SIGN_KEYDIR}" \
> > > > > - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > > + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > > > >   -r ${UBOOT_FITIMAGE_BINARY} \
> > > > >   ${SPL_MKIMAGE_SIGN_ARGS}
> > > > >   #
> > > > >   # Verify the U-boot FIT image and SPL dtb
> > > > >   #
> > > > > - ${UBOOT_FIT_CHECK_SIGN} \
> > > > > - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > > - -f ${UBOOT_FITIMAGE_BINARY}
> > > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> > > > > ${UBOOT_FIT_CHECK_SIGN}
> > > > > + \ -k "${SPL_DIR}/${SPL_DTB_BINARY}" \ -f
> > > > > ${UBOOT_FITIMAGE_BINARY} fi
> > > > >   fi
> > > > >
> > > > >   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > > > >
> > > > > -=-=-=-=-=-=-=-=-=-=-=-
> > > > > Links: You receive all messages sent to this group.
> > > > > View/Reply Online (#218114):
> > > > > https://lists.openembedded.org/g/openembedded-core/message/218114
> > > > > Mute This Topic:
> > > > > https://lists.openembedded.org/mt/113499585/3616858
> > > > > Group Owner: openembedded-core+owner@lists.openembedded.org
> > > > > Unsubscribe:
> > > > > https://lists.openembedded.org/g/openembedded-core/unsub
> > > > >  [
> > > > > adrian.freihofer@siemens.com]
> > > > > -=-=-=-=-=-=-=-=-=-=-=-
> > > > >
> > >
>
Jamin Lin June 17, 2025, 8:17 a.m. UTC | #6 
Hi Adrian,

> Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB public key
> injection optional
> 
> Hi Jamin
> 
> Thank you for these improvements. To me it looks much more understandable,
> robust and also test-able now.
> 
> Regarding test-able, would it be possible to add a test case to oe- selftest for
> this use case? E.g. here:
> https://git.yoctoproject.org/poky/tree/meta/lib/oeqa/selftest/cases/fitimage.p
> y#n1125
> it should be relatively straight forward to add  something like this:
> 
> 
>     def test_sign_uboot_fit_without_spl(self):
>         """
>         Summary:     Test building and signing u-boot without SPL
>         ...
>         """
>         config = """
> # There's no U-boot defconfig with CONFIG_FIT_SIGNATURE yet, so we need at
> # least CONFIG_SPL_LOAD_FIT and CONFIG_SPL_OF_CONTROL set MACHINE =
> "qemuarm"
> UBOOT_MACHINE = "am57xx_evm_defconfig"
> SPL_BINARY = ""
> # Enable creation and signing of the U-Boot fitImage
> UBOOT_FITIMAGE_ENABLE = "1"
> SPL_SIGN_ENABLE = "1"
> SPL_SIGN_KEYNAME = "spl-oe-selftest"
> SPL_SIGN_KEYDIR = "${TOPDIR}/signing-keys"
> ...
> """
>         self.write_config(config)
>         bb_vars = self._fit_get_bb_vars()
>         self._test_fitimage(bb_vars)
> 
> 
> 
> You can run the oe-selftest suite with a local.conf file from the template
> (without modifications) and this additional line:
> 
> SANITY_TESTED_DISTROS = ""
> 
> Then you can execute the test by
> oe-selftest -v -K -r
> fitimage.UBootFitImageTests.test_sign_uboot_fit_without_spl
> 
> With -v the log file in build/tmp/log/oe-selftest-results.log shows a log of
> details which is helpful for understanding how the test suite works.
> 
> 
> One more detail: shellcheck complains against lines like this:
> 
> if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ];
> 
> We should change such lines to become:
> 
> if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ] && [ -n "${SPL_DTB_BINARY}" ];
> 
> if we need to touch them anyway.
> 
Thanks for the suggestion and review.
I re-send v2 here, 
https://patchwork.yoctoproject.org/project/oe-core/list/?series=35521
https://patchwork.yoctoproject.org/project/oe-core/cover/20250617081052.3087995-1-jamin_lin@aspeedtech.com/
Thanks-Jamin

> 
> Thank you and regards,
> Adrian
> 
> 
> On Mon, 2025-06-16 at 03:50 +0000, Jamin Lin wrote:
> > Hi Adrian,
> >
> > > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > > public key injection optional
> > >
> > > On Fri, 2025-06-13 at 02:08 +0000, Jamin Lin wrote:
> > > > Hi Adrian,
> > > >
> > > > > Subject: Re: [OE-core] [PATCH v1 2/2] uboot-sign: Make SPL DTB
> > > > > public key injection optional
> > > > >
> > > > > On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via
> > > > > lists.openembedded.org
> > > > > wrote:
> > > > > > Introduce SPL_SIGN_ADD_PUBKEY to control whether the public
> > > > > > key is added into the SPL device tree and whether FIT
> > > > > > signature verification is performed after signing.
> > > > > >
> > > > > > Key changes:
> > > > > > - Added SPL_SIGN_ADD_PUBKEY variable (default = "1")
> > > > > > - Conditionally apply '-K <dtb>' to mkimage only if adding key
> > > > > > is enabled
> > > > > > - Skip fit_check_sign when public key injection is disabled
> > > > > > - Suppress concat_spl_dtb() warnings if key adding is turned
> > > > > > off
> > > > > >
> > > > > > This allows U-Boot FIT images to be signed without modifying
> > > > > > the SPL DTB, useful in scenarios where public key management
> > > > > > is handled elsewhere or post-processing will be done
> > > > > > separately.
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > Introducing a new variable is one way to solve your issue, but
> > > > > it increases complexity and requires additional test coverage
> > > > > and documentation.
> > > > > So I
> > > > > would like to ask if this is extra complexity is really needed
> > > > > to achieve your goal.
> > > > >
> > > > > For a similar use case, we use this approach:
> > > > >
> > > > >  * bitbake always signs with a development key (no build variant
> > > > >    needed, no extra test matrix)
> > > > >  * Then we test the firmware
> > > > >  * For release, we use a script to replace the development
> > > > > signature
> > > > >    with a production signature (without rebuilding with
> > > > > bitbake)
> > > > >
> > > > > This seems similar to your approach, but I don't understand why
> > > > > injecting the development public key is problematic for your
> > > > > workflow.
> > > > > Would it be better to have a post-processing step that simply
> > > > > replaces the public key, rather than requiring an empty DTB
> > > > > without any public key? That would mean that this patch would
> > > > > not be needed at all.
> > > > >
> > > > > Adrian
> > > >
> > > > Thanks for your reply.
> > > > Let me clarify the motivation behind this patch.
> > > >
> > > > In our project, we only use "U-Boot and do not have an SPL". Our
> > > > goal is to sign the U-Boot FIT image only, without modifying or
> > > > involving any SPL DTB. However, the current design in
> > > > uboot-sign.bbclass assumes that SPL is always present and forces
> > > > the public key to be added into the SPL DTB, followed by a
> > > > verification step involving that DTB.
> > > >
> > > > Here’s what I ran into:
> > > > We set SPL_SIGN_ENABLE = "1" to enable signing, which correctly
> > > > generates the ITS with a signature node for U-Boot.
> > > > But enabling SPL_SIGN_ENABLE also triggers these additional steps
> > > > in
> > > > uboot-sign.bbclass:
> > > > a. The call to mkimage with -K ${SPL_DTB_BINARY} tries to inject
> > > > the public key into the SPL DTB.
> > > > b. Then fit_check_sign tries to verify the FIT image using the SPL
> > > > DTB.
> > >
> > > The explanation has helped me to understand the actual use case.
> > > But we need
> > > to capture this understanding in the code, the documentation and the
> > > commit message. (The updated commit message looks already good to
> > > me).
> > >
> > > I suggest to explicitly cover your use case in the code. The real
> > > scenario isn't that you don't want to sign the SPL or inject the key
> > > into the SPL's devicetree - it's that you don't compile an SPL at
> > > all.
> > > That means that the implementation and the documentation should
> > > transparently handle this use case. Introducing a variable
> > > SPL_SIGN_ADD_PUBKEY looks like working around that in a misleading
> > > way to me. It is simply confusing (for me) that we compile and sign
> > > an SPL but do not add the public key to the DTB.
> > >
> > > What about setting the SPL_DTB_BINARY (or some of the SPL*
> > > variables) to an
> > > empty string? The source code could then check for [ -z
> > > "${SPL_DTB_BINARY}" ] or [ -n "${SPL_DTB_BINARY}" ] and fail if
> > > SPL_DTB_BINARY is expected but missing.
> > >
> > > This would also allow us to document the use case by extending the
> > > SPL_DTB_BINARY variable documentation with something like:
> > > "If SPL_DTB_BINARY is set to empty string, the uboot-sign class
> > > assumes that no SPL is built and all the related post-processing
> > > steps are skipped."
> > >
> > > >
> > > > This fails for us because:
> > > > 1. We don't have ${SPL_DTB_BINARY} at all.
> > > > 2. The build breaks during the verification step due to this
> > > > missing SPL DTB.
> > > > 3. Even setting an invalid SPL_SIGN_KEYDIR to skip actual signing
> > > > does not bypass the verification step.
> > > >
> > > > Instead of introducing a new variable like SPL_SIGN_ADD_PUBKEY, we
> > > > could consider modifying the logic to gracefully handle the
> > > > absence of SPL DTBs.
> > > > Here’s the idea:
> > > > 1. Only concat SPL binary if both parts exist:
> > > > if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > > "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > > >     cat ${SPL_DIR}/${SPL_NODTB_BINARY}
> > > > ${SPL_DIR}/${SPL_DTB_SIGNED}
> > > >
> > > > "${SPL_BINARY}"
> > > > else
> > > >     if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> > > >         bbwarn "Failure while adding public key to spl binary.
> > > > Verified U-Boot boot won't be available."
> > > >     fi
> > > > fi
> > > >
> > > > 2. Conditionally apply -K only if SPL DTB exists:
> > > > ${UBOOT_MKIMAGE_SIGN} \
> > > >     ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > > len('${SPL_MKIMAGE_DTCOPTS}')
> > > > else ''} \
> > > >     -F -k "${SPL_SIGN_KEYDIR}" \
> > > >     ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > > >     -r ${UBOOT_FITIMAGE_BINARY} \
> > > >     ${SPL_MKIMAGE_SIGN_ARGS}
> > > >
> > > > 3. Only run fit_check_sign if SPL DTB exists:
> > > > if [ -f ${SPL_DIR}/${SPL_DTB_BINARY} ]; then
> > > >     ${UBOOT_FIT_CHECK_SIGN} \
> > > >         -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > >         -f ${UBOOT_FITIMAGE_BINARY} fi
> > > >
> > > > Could you please give me any suggestion?
> > >
> > > "Gracefully handle the absence of SPL DTBs" is fragile. A file might
> > > be missing due to various bugs or build issues. If the code fails
> > > immediately when a file is expected but missing, related bugs would
> > > likely be caught by the Yocto autobuilder and problematic patches
> > > could be immediately rejected.
> > >
> > > Depending on the details, such graceful file checks can also become
> > > very problematic when it comes to builds from sstate-cache versus
> > > full builds. When the build comes from the sstate-cache, there is
> > > simply no build folder at all - only the sysroots and the deploy
> > > folder are restored from the cache. This creates another possibility
> > > for various corner cases which we should avoid by design.
> > >
> > > Therefore, I prefer an implementation that relies on if statements
> > > checking BitBake variables (-z, -n) over an implementation that
> > > relies on if statements checking the state of a potentially broken
> > > or absent build directory (-f, -e, -x, -d).
> > >
> > > Is it possible to come up with an implementation which does, for
> > > example:
> > >
> > > if [ -n "${SPL_DTB_BINARY}" ]; then
> > >
> > > instead of
> > >
> > > if [ -f "${SPL_DTB_BINARY}" ]; then
> > >
> > > Thanks for the clarification
> > > Adrian
> >
> > >
> > Thank you very much for your suggestion and detailed feedback.
> > Based on your advice, I have updated the code and the related
> > variables accordingly. Here’s a summary of what I changed and my
> > rationale:
> > Updated Variables
> > I set the variables like this to clearly represent the case where we
> > do not build an SPL:
> >
> > SPL_SIGN_ENABLE ?= "1"
> > SPL_SIGN_KEYDIR ?= "${STAGING_DATADIR_NATIVE}/aspeed-secure-
> > config/keys"
> > SPL_DTB_BINARY = ""
> > UBOOT_FITIMAGE_ENABLE = "1"
> >
> > SPL_DTB_BINARY is explicitly set to an empty string, which means no
> > SPL is built.
> > UBOOT_FITIMAGE_ENABLE is still 1 to ensure that the U-Boot FIT image
> > is generated as usual.
> > SPL_SIGN_ENABLE controls whether the FIT image gets signed.
> >
> > Code Changes
> > 1. Removed redundant SPL_DTB_BINARY checks for FIT image generation
> > and deployment I dropped the checks for -n ${SPL_DTB_BINARY} in the
> > FIT image assembly and deploy helpers because the FIT image generation
> > should proceed as long as UBOOT_FITIMAGE_ENABLE is 1, regardless of
> > whether the SPL exists or not.
> >
> > - if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ];
> > then
> > + if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
> >
> > 2. Simplified deploy condition
> > Similarly, I updated the deploy helper to deploy the ITS and FIT image
> > when UBOOT_FITIMAGE_ENABLE is enabled.
> > -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> > "${SPL_DTB_BINARY}" ]; then
> > +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
> >                 if [ -n "${type}" ]; then
> >                         uboot_its_image="u-boot-its-${type}-${PV}-
> > ${PR}"
> >                         uboot_fitimage_image="u-boot-fitImage-
> > ${type}-${PV}-${PR}"
> >
> > 3. Adjusted SPL DTB deploy condition
> > I unified the check to reuse SPL_DTB_BINARY instead of having an extra
> > SPL_DTB_SIGNED variable. So if SPL_DTB_BINARY is empty, we skip
> > deploying the SPL DTB.
> > -       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ;
> > then
> > +       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ;
> > then
> >                 deploy_spl_dtb $type
> >         fi
> >
> > 4. Improved the sign logic
> > Now the sign step checks if SPL_DTB_BINARY is empty:
> > If present, it signs the FIT image and injects the public key into the
> > SPL DTB, then verifies both.
> > If empty, it only signs the FIT image and generates the ITS with the
> > signature node, but does not attempt to verify or add the key to a
> > non-existent SPL DTB.
> >
> > Key Behavior Explained
> > If SPL_DTB_BINARY is empty, we assume there is no SPL.
> > If UBOOT_FITIMAGE_ENABLE=1, we always create the FIT image and ITS.
> > If SPL_SIGN_ENABLE=1, we always sign the FIT image, but only inject
> > the key into the SPL DTB if it exists.
> >
> >
> > Please let me know if this approach aligns with what you had in mind,
> > or if you have any further suggestions to refine it!
> > Thanks again for your valuable input.
> > Thanks-Jamin
> >
> > diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-
> > recipe/uboot-sign.bbclass index 73e9ce3f11..67f14ac92f 100644
> > --- a/meta/classes-recipe/uboot-sign.bbclass
> > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > @@ -466,21 +466,29 @@ EOF
> >                 ${UBOOT_FITIMAGE_BINARY}
> >
> >         if [ "${SPL_SIGN_ENABLE}" = "1" ] ; then
> > -               #
> > -               # Sign the U-boot FIT image and add public key to SPL
> > dtb
> > -               #
> > -               ${UBOOT_MKIMAGE_SIGN} \
> > -                       ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> > -                       -F -k "${SPL_SIGN_KEYDIR}" \
> > -                       -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > -                       -r ${UBOOT_FITIMAGE_BINARY} \
> > -                       ${SPL_MKIMAGE_SIGN_ARGS}
> > -               #
> > -               # Verify the U-boot FIT image and SPL dtb
> > -               #
> > -               ${UBOOT_FIT_CHECK_SIGN} \
> > -                       -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > -                       -f ${UBOOT_FITIMAGE_BINARY}
> > +               if [ -n "${SPL_DTB_BINARY}" ] ; then
> > +                       #
> > +                       # Sign the U-boot FIT image and add public
> > key to SPL dtb
> > +                       #
> > +                       ${UBOOT_MKIMAGE_SIGN} \
> > +                               ${@'-D
> "${SPL_MKIMAGE_DTCOPTS}"' if
> > len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> > +                               -F -k "${SPL_SIGN_KEYDIR}" \
> > +                               -K "${SPL_DIR}/${SPL_DTB_BINARY}"
> \
> > +                               -r ${UBOOT_FITIMAGE_BINARY} \
> > +                               ${SPL_MKIMAGE_SIGN_ARGS}
> > +
> > +                       # Verify the U-boot FIT image and SPL dtb
> > +                       ${UBOOT_FIT_CHECK_SIGN} \
> > +                               -k "${SPL_DIR}/${SPL_DTB_BINARY}"
> \
> > +                               -f ${UBOOT_FITIMAGE_BINARY}
> > +               else
> > +                       # Sign the U-boot FIT image
> > +                       ${UBOOT_MKIMAGE_SIGN} \
> > +                               ${@'-D
> "${SPL_MKIMAGE_DTCOPTS}"' if
> > len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
> > +                               -F -k "${SPL_SIGN_KEYDIR}" \
> > +                               -r ${UBOOT_FITIMAGE_BINARY} \
> > +                               ${SPL_MKIMAGE_SIGN_ARGS}
> > +               fi
> >         fi
> >
> >         if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then @@ -496,7
> > +504,7 @@ uboot_assemble_fitimage_helper() {
> >                 concat_dtb "$type" "$binary"
> >         fi
> >
> > -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> > "${SPL_DTB_BINARY}" ]; then
> > +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
> >                 uboot_fitimage_assemble
> >         fi
> >
> > @@ -543,7 +551,7 @@ deploy_helper() {
> >                 deploy_dtb $type
> >         fi
> >
> > -       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" -a -n
> > "${SPL_DTB_BINARY}" ]; then
> > +       if [ "${UBOOT_FITIMAGE_ENABLE}" = "1" ]; then
> >                 if [ -n "${type}" ]; then
> >                         uboot_its_image="u-boot-its-${type}-${PV}-
> > ${PR}"
> >                         uboot_fitimage_image="u-boot-fitImage-
> > ${type}-${PV}-${PR}"
> > @@ -561,7 +569,7 @@ deploy_helper() {
> >                 fi
> >         fi
> >
> > -       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_SIGNED}" ] ;
> > then
> > +       if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ;
> > then
> >                 deploy_spl_dtb $type
> >         fi
> >  }
> >
> > >
> > >
> > > > Thanks-Jamin
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > > > > > ---
> > > > > >  meta/classes-recipe/uboot-sign.bbclass | 20 +++++++++++++++-
> > > > > > ----
> > > > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/meta/classes-recipe/uboot-sign.bbclass
> > > > > > b/meta/classes-
> > > > > > recipe/uboot-sign.bbclass index 6ee1874cd6..3bcf47dd33 100644
> > > > > > --- a/meta/classes-recipe/uboot-sign.bbclass
> > > > > > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > > > > > @@ -34,6 +34,12 @@ UBOOT_FITIMAGE_ENABLE ?= "0"
> > > > > >  # Signature activation - this requires UBOOT_FITIMAGE_ENABLE
> > > > > > = "1"
> > > > > >  SPL_SIGN_ENABLE ?= "0"
> > > > > >
> > > > > > +# Whether to add (embed) the public key into the SPL Device
> > > > > > Tree
> > > > > > (.dtb).
> > > > > > +# If set to "1", the key will be inserted into the
> > > > > > /signature
> > > > > > node
> > > > > > of the DTB
> > > > > > +# and fit_check_sign will be used to verify the signature.
> > > > > > +# If set to "0", only signing will be performed, without
> > > > > > modifying
> > > > > > the DTB.
> > > > > > +SPL_SIGN_ADD_PUBKEY ?= "1"
> > > > > > +
> > > > > >  # Default value for deployment filenames.
> > > > > >  UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
> > > > > >  UBOOT_DTB_BINARY ?= "u-boot.dtb"
> > > > > > @@ -245,7 +251,9 @@ concat_spl_dtb() {
> > > > > >   if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e
> > > > > > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
> > > > > >   cat ${SPL_DIR}/${SPL_NODTB_BINARY}
> > > ${SPL_DIR}/${SPL_DTB_SIGNED}
> > > > > > >
> > > > > > "${SPL_BINARY}"
> > > > > >   else
> > > > > > - bbwarn "Failure while adding public key to spl binary.
> > > > > > Verified
> > > > > > U-
> > > > > > Boot boot won't be available."
> > > > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then bbwarn "Failure
> > > > > > while
> > > > > > + adding public key to spl binary. Verified U-
> > > > > > Boot boot won't be available."
> > > > > > + fi
> > > > > >   fi
> > > > > >  }
> > > > > >
> > > > > > @@ -474,15 +482,17 @@ EOF
> > > > > >   ${UBOOT_MKIMAGE_SIGN} \
> > > > > >   ${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if
> > > > > len('${SPL_MKIMAGE_DTCOPTS}')
> > > > > > else ''} \
> > > > > >   -F -k "${SPL_SIGN_KEYDIR}" \
> > > > > > - -K "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > > > + ${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if
> > > > > > d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
> > > > > >   -r ${UBOOT_FITIMAGE_BINARY} \
> > > > > >   ${SPL_MKIMAGE_SIGN_ARGS}
> > > > > >   #
> > > > > >   # Verify the U-boot FIT image and SPL dtb
> > > > > >   #
> > > > > > - ${UBOOT_FIT_CHECK_SIGN} \
> > > > > > - -k "${SPL_DIR}/${SPL_DTB_BINARY}" \
> > > > > > - -f ${UBOOT_FITIMAGE_BINARY}
> > > > > > + if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
> > > > > > ${UBOOT_FIT_CHECK_SIGN}
> > > > > > + \ -k "${SPL_DIR}/${SPL_DTB_BINARY}" \ -f
> > > > > > ${UBOOT_FITIMAGE_BINARY} fi
> > > > > >   fi
> > > > > >
> > > > > >   if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then
> > > > > >
> > > > > >
> > > > > >
> > > >
> >
AdrianF June 18, 2025, 10:03 a.m. UTC | #7 
Hi Jamin

Thank you for the improvements. It looks good to me now.

Adrian
diff mbox series

Patch

diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
index 6ee1874cd6..3bcf47dd33 100644
--- a/meta/classes-recipe/uboot-sign.bbclass
+++ b/meta/classes-recipe/uboot-sign.bbclass
@@ -34,6 +34,12 @@  UBOOT_FITIMAGE_ENABLE ?= "0"
 # Signature activation - this requires UBOOT_FITIMAGE_ENABLE = "1"
 SPL_SIGN_ENABLE ?= "0"
 
+# Whether to add (embed) the public key into the SPL Device Tree (.dtb).
+# If set to "1", the key will be inserted into the /signature node of the DTB
+# and fit_check_sign will be used to verify the signature.
+# If set to "0", only signing will be performed, without modifying the DTB.
+SPL_SIGN_ADD_PUBKEY ?= "1"
+
 # Default value for deployment filenames.
 UBOOT_DTB_IMAGE ?= "u-boot-${MACHINE}-${PV}-${PR}.dtb"
 UBOOT_DTB_BINARY ?= "u-boot.dtb"
@@ -245,7 +251,9 @@  concat_spl_dtb() {
 	if [ -e "${SPL_DIR}/${SPL_NODTB_BINARY}" -a -e "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then
 		cat ${SPL_DIR}/${SPL_NODTB_BINARY} ${SPL_DIR}/${SPL_DTB_SIGNED} > "${SPL_BINARY}"
 	else
-		bbwarn "Failure while adding public key to spl binary. Verified U-Boot boot won't be available."
+		if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
+			bbwarn "Failure while adding public key to spl binary. Verified U-Boot boot won't be available."
+		fi
 	fi
 }
 
@@ -474,15 +482,17 @@  EOF
 		${UBOOT_MKIMAGE_SIGN} \
 			${@'-D "${SPL_MKIMAGE_DTCOPTS}"' if len('${SPL_MKIMAGE_DTCOPTS}') else ''} \
 			-F -k "${SPL_SIGN_KEYDIR}" \
-			-K "${SPL_DIR}/${SPL_DTB_BINARY}" \
+			${@'-K "${SPL_DIR}/${SPL_DTB_BINARY}"' if d.getVar("SPL_SIGN_ADD_PUBKEY") == "1" else ''} \
 			-r ${UBOOT_FITIMAGE_BINARY} \
 			${SPL_MKIMAGE_SIGN_ARGS}
 		#
 		# Verify the U-boot FIT image and SPL dtb
 		#
-		${UBOOT_FIT_CHECK_SIGN} \
-			-k "${SPL_DIR}/${SPL_DTB_BINARY}" \
-			-f ${UBOOT_FITIMAGE_BINARY}
+		if [ "${SPL_SIGN_ADD_PUBKEY}" = "1" ]; then
+			${UBOOT_FIT_CHECK_SIGN} \
+				-k "${SPL_DIR}/${SPL_DTB_BINARY}" \
+				-f ${UBOOT_FITIMAGE_BINARY}
+		fi
 	fi
 
 	if [ -e "${SPL_DIR}/${SPL_DTB_BINARY}" ]; then