[PATCH v3 4/4] iminfo: also verify signatures

[Simon Glass]

Hi Ludwig,

On 2026-05-07T12:06:22, Ludwig Nussel <ludwig.nussel at siemens.com> wrote:
> iminfo: also verify signatures
>
> The iminfo command already verifies hashes of images. This change also
> verifies signatures of configurations if enabled.
>
> Adjusts error output slighly to be on stderr
>
> Signed-off-by: Ludwig Nussel <ludwig.nussel at siemens.com>
>
> boot/image-fit.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  cmd/bootm.c      |  7 +++++++
>  include/image.h  |  8 ++++++++
>  3 files changed, 69 insertions(+)

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -1512,6 +1512,60 @@ int fit_all_image_verify(const void *fit)
> +/**
> + * fit_all_configurations_verify - verify signatures of all configurations
> + * @fit: pointer to the FIT format image header
> + *
> + * fit_all_configurations_verify() iterates over all configurations
> + * in the FIT and checks the signatures. Returns success if all
> + * configurations have valid signatures. See documentation at
> + * fit_config_verify_required_keys() or fit_config_verify_key().
> + *
> + * returns:
> + *     0, success
> + *     -ENOENT, no configurations present
> + *     <0, error

Please use kernel-doc style - 'Return:' rather than 'returns:' - also
'fit_all_configurations_verify' at the top should be
fit_all_configurations_verify()

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -1512,6 +1512,60 @@ int fit_all_image_verify(const void *fit)
> +#if FIT_IMAGE_ENABLE_VERIFY
> +int fit_all_configurations_verify(const void *fit)

The header in include/image.h declares this prototype under
CONFIG_IS_ENABLED(FIT_SIGNATURE) but here you guard the definition
with FIT_IMAGE_ENABLE_VERIFY. Please use one consistently -
CONFIG_IS_ENABLED(FIT_SIGNATURE) reads more naturally and matches the
neighbouring fit_config_verify()

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -1512,6 +1512,60 @@ int fit_all_image_verify(const void *fit)
> +             log_info("   %s ... ", fit_get_name(fit, noffset, NULL));
> +             /* fit_config_verify() prints results so we can be silent */
> +             ret = fit_config_verify(fit, noffset);
> +             if (ret) {
> +                     r = ret;
> +                     log_err('FAIL\n');
> +                     continue;
> +             }
> +             /* valid config found */
> +             if (r == -ENOENT)
> +                     r = 0;
> +
> +             log_info('OK\n');

The comment 'so we can be silent' contradicts the next lines, which
print FAIL/OK. Either drop it or rework - fit_config_verify() does
emit its own diagnostics on failure, which will interleave with the
FAIL line printed here.

> diff --git a/cmd/bootm.c b/cmd/bootm.c
> @@ -335,6 +335,13 @@ static int image_info(ulong addr)
> +             if (CONFIG_IS_ENABLED(FIT_SIGNATURE) &&
> +                 fit_all_configurations_verify(hdr) != 0) {
> +                     puts("Signature verification failed!\n");
> +                     unmap_sysmem(hdr);
> +                     return CONFIG_IS_ENABLED(FIT_SIGNATURE_REQUIRED) == 1;
> +             }

A FIT with no /configurations node (an images-only FIT, which is
valid) makes fit_all_configurations_verify() return -ENOENT and log
'Can't find configurations parent node' from inside the helper. That
ends up here as 'Signature verification failed!' which is misleading.
Please treat -ENOENT as 'nothing to do' in this caller, and don't
log_err inside the helper for that case.

Also, 'CONFIG_IS_ENABLED(FIT_SIGNATURE_REQUIRED) == 1' is odd - the
macro already evaluates to 0 or 1, so just return it directly. Better
yet, make the success/failure path explicit rather than encoding
policy in the return value.

> Adjusts error output slighly to be on stderr

Typo: slighly -> slightly. Worth also mentioning that, for a
verification failure, the iminfo exit status is gated on
FIT_SIGNATURE_REQUIRED — that is a non-obvious behaviour change.

Regards,
Simon