[PATCH v4 4/8] tools: mkimage: add dm-verity Merkle-tree generation

[Simon Glass]

Hi Daniel,

On 2026-05-13T02:08:46, Daniel Golle <daniel at makrotopia.org> wrote:
> tools: mkimage: add dm-verity Merkle-tree generation
>
> When mkimage encounters a dm-verity subnode inside a component image
> node it now automatically invokes veritysetup(8) with --no-superblock
> to generate the Merkle hash tree, screen-scrapes the Root hash and Salt
> from the tool output, and writes the computed properties back into the
> FIT blob.
>
> The user only needs to specify algorithm, data-block-size, and
> hash-block-size in the ITS; mkimage fills in digest, salt,
> num-data-blocks, and hash-start-block.  Because --no-superblock is
> used, hash-start-block equals num-data-blocks with no off-by-one.
>
> The image data property is replaced with the expanded content (original
> data followed directly by the hash tree) so that subsequent hash and
> signature subnodes operate on the complete image.
>
> fit_image_add_verification_data() is restructured into two passes:
> dm-verity first (may grow data), then hashes and signatures.
>
> [...]
>
> tools/fit_image.c  | 116 +++++++++++++++++-
>  tools/image-host.c | 349 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 455 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

Thoughts below

> diff --git a/tools/image-host.c b/tools/image-host.c
> @@ -626,6 +629,310 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> +static int fit_image_process_verity(void *fit, const char *image_name,
> +                                 int image_noffset, int verity_noffset,
> +                                 const void *data, size_t data_size,
> +                                 void **expanded_data, size_t *expanded_size)

Please drop image_noffset - it is unused.

> diff --git a/tools/image-host.c b/tools/image-host.c
> @@ -626,6 +629,310 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> +     {
> +             static const char * const valid_algos[] = {
> +                     'sha1', 'sha224', 'sha256', 'sha384', 'sha512',
> +                     'ripemd160', 'whirlpool',
> +                     'sha3-224', 'sha3-256', 'sha3-384', 'sha3-512',
> +                     'stribog256', 'stribog512',
> +                     'sm3',
> +                     /* blake2b/blake2s with explicit digest sizes */
> +                     'blake2b-160', 'blake2b-256', 'blake2b-384',
> +                     'blake2b-512',
> +                     'blake2s-128', 'blake2s-160', 'blake2s-224',
> +                     'blake2s-256',
> +                     NULL
> +             };

This list will drift out of sync with what veritysetup supports, and
users get a misleading 'Unsupported dm-verity hash algorithm' from
mkimage when veritysetup would accept it.

The only reason for valid_algos[] is shell-injection avoidance from
popen, right?. I'd much rather see fork()/execvp() used directly: pass
the algo as an argv[] element and the list disappears. What do you
think?

> diff --git a/tools/image-host.c b/tools/image-host.c
> @@ -626,6 +629,310 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> +     /*
> +      * Stash the temp-file path so that fit_extract_data() can use
> +      * the expanded content for the external-data section.
> +      */
> +     ret = fdt_setprop_string(fit, verity_noffset,
> +                              'verity-data-file', tmpfile);

Communicating a host-side tmpfile path through the FIT blob leaks a
transient filesystem detail into the FDT, and the in-memory expanded
buffer and on-disk copy of the same bytes coexist. Since you already
return expanded_data/expanded_size to the caller, can
fit_extract_data() consume that buffer directly? That removes both
verity-data-file and the second read in fit_copy_image_data().

> diff --git a/tools/image-host.c b/tools/image-host.c
> @@ -667,9 +976,12 @@ int fit_image_add_verification_data(const char *keydir, const char *keyfile,
> -             if (ret < 0)
> +             if (ret < 0) {
> +                     free(verity_data);
>                       return ret;
> +             }
>       }
>
> +     free(verity_data);
>       return 0;
>  }

The temp file is only unlinked by fit_copy_image_data() on the happy
path. If pass 2 fails, or mkimage aborts before fit_extract_data()
runs, the tmpfile is leaked in /tmp. Please also unlink() on the error
return path (or drop the tmpfile mechanism - see other comment). We
try to have a blank line before the final return in a function.

> diff --git a/tools/image-host.c b/tools/image-host.c
> @@ -626,6 +629,310 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> +     int data_block_size, hash_block_size;

The v3 changelog notes these moved to unsigned int on the runtime side
- please apply the same here for consistency, otherwise a malformed
.its with a huge u32 sneaks through fdt32_to_cpu() as a negative int
and the >= 512 check fires for the wrong reason.

Regards,
Simon