Hi Eva,
On 2026-09-25T07:27:39, Eva Crystal <[email protected]> wrote:
> image-fit-sig: fail when the control FDT has no /signature node
>
> fit_image_verify_required_sigs() looks up the "/signature" node of the
> U-Boot control devicetree to work out which keys are marked required.
> When that node is absent it sets *no_sigsp and returns 0, so every
> caller treats the image as not requiring signatures. Nothing downstream
> can fail it after that: a FIT whose signatures are absent, malformed or
> invalid verifies successfully, and the only indication is a debug()
> call, which is compiled out of a normal build.
>
> Against a control devicetree holding no "/signature" node at all, and a
> FIT whose configuration signature carries no "value", an unpatched
> build reports both images as verified:
>
> ## Checking hash(es) for FIT Image at 10400100 ...
> Hash(es) for Image 0 (kernel): sha256+ OK
> Hash(es) for Image 1 (fdt-1): sha256+ OK
>
> That '+' is fit_image_verify_with_data() reporting the signature as
> verified, on a board holding no keys whatsoever.
>
> Add FIT_REQUIRE_IMAGE_SIGS, which turns a missing "/signature" node
> into a verification failure on this gate, and report the missing node
> with log_warning() so that the condition is no longer silent. With
> CONFIG_FIT_REQUIRE_IMAGE_SIGS=y the same image is refused:
>
> ## Checking hash(es) for FIT Image at 10400100 ...
> Hash(es) for Image 0 (kernel): fit_image_verify_required_sigs() No
> signature node in control FDT: FDT_ERR_NOTFOUND
> error!
> Unable to verify required signature for '' hash node in 'kernel' image node
> Bad hash in FIT image!
>
> The adjacent configuration gate in fit_config_verify_required_keys() reports
> the same missing node with debug(), after Jonas Karlman objected that a
> verbose error is wrong on a Rockchip board which enables FIT_SIGNATURE for
> image hashes alone and has no /signature node by design. This gate keeps
> log_warning() for a reason specific to the image side. Under bootm the
> refusal surfaces as Bad Data Hash, indistinguishable from a corrupt image,
> and nothing in this function can tell an intentional hash-only configuration
> from a control FDT that lost its /signature node, so a silent return
> withholds the signal from exactly the case this patch exists for. The
> message fires once per boot rather than once per image, the level is
> LOGL_WARNING rather than an error, and CONFIG_LOGLEVEL suppresses it for
> boards which do not want it. On evb-rk3568_defconfig the shipped
> CONFIG_SPL_LOGLEVEL is 4, so on that board at its default configuration
> this message appears once in SPL.
Not quite. CONFIG_LOGLEVEL only filters output at runtime when
CONFIG_LOG is enabled. Without it, as with evb-rk3568_defconfig in
both SPL and U-Boot proper, log() in include/log.h prints anything at
or below LOGL_INFO with a plain printf(), whatever CONFIG_LOGLEVEL
says. So the Rockchip boards Jonas mentioned would see this message in
SPL and again in U-Boot proper, and cannot turn it off without
enabling LOG. The help text makes the same claim, so please can you
fix both.
> Measured on sandbox with test/py, running the vboot tests:
The commit message is very long. The test counts, the 'Not tested'
list and the Fixes: discussion are useful for reviewers but belong
below the --- line or in the cover letter. The paragraph answering
Jonas could be cut to a couple of sentences. Please can you trim it to
the problem, the new option and the reason for the warning?
> [...]
>
> boot/Kconfig | 34 ++++++++++++++++++++++++++++++++++
> boot/image-fit-sig.c | 15 +++++++++++++--
> tools/mkimage.h | 1 +
> 3 files changed, 48 insertions(+), 2 deletions(-)
> diff --git a/boot/Kconfig b/boot/Kconfig
> @@ -259,6 +285,14 @@ config SPL_FIT_SIGNATURE_MAX_SIZE
> +config SPL_FIT_REQUIRE_IMAGE_SIGS
> + bool "Fail image verification when the SPL control FDT has no
> /signature node"
> + depends on SPL_FIT_SIGNATURE
> + select FIT_REQUIRE_IMAGE_SIGS
The select seems wrong. In SPL,
CONFIG_IS_ENABLED(FIT_REQUIRE_IMAGE_SIGS) checks
CONFIG_SPL_FIT_REQUIRE_IMAGE_SIGS, so the SPL option already works by
itself. The select only forces enforcement on in U-Boot proper as
well, so a board cannot enforce in SPL alone. Please drop it so the
two options are independent, as with the other SPL_ variants.
> diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
> @@ -201,8 +202,18 @@ int fit_image_verify_required_sigs(const void *fit, int
> image_noffset,
> + if (!reported) {
> + reported = true;
> + log_warning("No signature node in control FDT: %s\n",
> + fdt_strerror(key_node));
> + }
> + if (CONFIG_IS_ENABLED(FIT_REQUIRE_IMAGE_SIGS))
> + return -EPERM;
> + return 0;
I'm not keen on the warning when the option is off. Without
FIT_REQUIRE_IMAGE_SIGS, a missing node is the documented hash-only
setup, which this function cannot tell apart from a misconfigured
board, so the warning is noise for every such board, as Jonas said for
the configuration gate. Also, the static 'reported' flag adds state to
library code just to rate-limit one message, and when enforcement is
on, the second and later images fail with only 'error!' and no reason.
I suggest log_err() on the enforcing path, with no rate-limiting since
it is fatal anyway, and debug() otherwise to match
fit_config_verify_required_keys(). That also removes the need for the
mkimage.h shim. If you want people to know about the fail-open case,
the Kconfig help and the docs are a better place for that. What do you
think?
Regards,
Simon
