Hi Daniel, On 2026-05-13T02:08:46, Daniel Golle <[email protected]> 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 <[email protected]> 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
