Hi, On Tue, 3 Mar 2026 at 01:09, Ahmad Fatoum <[email protected]> wrote: > > Hello Tom, > > On 3/2/26 23:09, Tom Rini wrote: > > There is a flaw in how U-Boot verifies and generates signatures for FIT > > images. To prevent mix and match style attacks, it is recommended to > > use signed configurations. How this is supposed to work is documented in > > doc/usage/fit/signature.rst. > > > > Crucially, the `hashed-nodes` property of the `signature` node contains > > which nodes of the FIT device tree were hashed as part of the signature > > and should be verified. However, this property itself is not part of the > > hash and can therefore be modified by an attacker. Furthermore, the > > signature only contains the name of each node and not the path in the > > device tree to the node. > > > > This patch reworks the code to address this specific oversight. > > Do I understand correctly that this is a breaking change > for FIT with signed configurations? > > - New U-Boot hashes more than intended for old FIT > - Old U-Boot hashes less than intended for new FIT
Yes, that's right. Reviewed-by: Simon Glass <[email protected]> I can see how this works. Please see nit below. > > Thanks, > Ahmad > > > > > Thanks to Apple Security Engineering and Architecture (SEAR) for > > reporting this issue and then coming up with a fix. > > > > Reported-by: Apple Security Engineering and Architecture (SEAR) > > Signed-off-by: Tom Rini <[email protected]> > > --- > > I assume one question will be about adding a test case. I have been > > asked to not share the testcase for a few releases yet, so plan to add > > that later in the year. > > --- > > boot/image-fit-sig.c | 31 +++++++++++++++++++++++-------- > > include/image.h | 12 ++++++++++++ > > tools/image-host.c | 9 +++++++-- > > 3 files changed, 42 insertions(+), 10 deletions(-) > > > > diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c > > index f23e9d5d0b0f..dce8bbe30e93 100644 > > --- a/boot/image-fit-sig.c > > +++ b/boot/image-fit-sig.c > > @@ -44,12 +44,6 @@ struct image_region *fit_region_make_list(const void > > *fit, > > debug("Hash regions:\n"); > > debug("%10s %10s\n", "Offset", "Size"); > > > > - /* > > - * Use malloc() except in SPL (to save code size). In SPL the caller > > - * must allocate the array. > > - */ > > - if (!IS_ENABLED(CONFIG_XPL_BUILD) && !region) > > - region = calloc(sizeof(*region), count); > > if (!region) > > return NULL; > > for (i = 0; i < count; i++) { > > @@ -62,6 +56,23 @@ struct image_region *fit_region_make_list(const void > > *fit, > > return region; > > } > > > > +int fit_region_add_hashed_nodes(struct image_region *region, int count, > > + const char* hashed_nodes, int > > hashed_nodes_len) That should line up with the previous line. > > +{ > > + /* Add the spacer to ensure the hashed strings and hashed-nodes > > cannot "overlap". */ > > + const char* HASHED_NODES_SPACER = "hashed-nodes"; > > + region[count].data = HASHED_NODES_SPACER; > > + region[count].size = strlen(HASHED_NODES_SPACER); > > + count++; > > + > > + region[count].data = hashed_nodes; > > + region[count].size = hashed_nodes_len; > > + count++; > > + > > + /* Now add the actual hashed-nodes value. */ > > + return count; > > +} > > + > > static int fit_image_setup_verify(struct image_sign_info *info, > > const void *fit, int noffset, > > const void *key_blob, int required_keynode, > > @@ -376,10 +387,14 @@ static int fit_config_check_sig(const void *fit, int > > noffset, int conf_noffset, > > count++; > > } > > > > - /* Allocate the region list on the stack */ > > - struct image_region region[count]; > > + /* Allocate the region list on the stack, +2 for the hashed-nodes */ > > + struct image_region region[count+2]; spaces around + > > > > fit_region_make_list(fit, fdt_regions, count, region); > > + > > + /* Add the hashed-nodes */ > > + count = fit_region_add_hashed_nodes(region, count, prop, prop_len); > > + > > if (info.crypto->verify(&info, region, count, fit_value, > > fit_value_len)) { > > *err_msgp = "Verification failed"; > > diff --git a/include/image.h b/include/image.h > > index 34efac6056dd..5b847527b586 100644 > > --- a/include/image.h > > +++ b/include/image.h > > @@ -1771,6 +1771,18 @@ struct image_region *fit_region_make_list(const void > > *fit, > > struct fdt_region *fdt_regions, int count, > > struct image_region *region); > > > > +/** > > + * fit_region_add_hashed_nodes() - Add a list of hashed nodes to the > > regions > > + * > > + * @region: List of regions to add to > > + * @count: Number of existing regions (Note: region needs to > > have space for an additional two regions) long line > > + * @hashed_nodes: List of hashed nodes > > + * @hashed_nodes_len: Length of list > > + * Return: The updated count value (i.e. count+2). > > + */ > > +int fit_region_add_hashed_nodes(struct image_region *region, int count, > > + const char* hashed_nodes, int hashed_nodes_len); > > + > > static inline int fit_image_check_target_arch(const void *fdt, int node) > > { > > #ifndef USE_HOSTCC > > diff --git a/tools/image-host.c b/tools/image-host.c > > index 48d69191c921..1e7c38a9a211 100644 > > --- a/tools/image-host.c > > +++ b/tools/image-host.c > > @@ -1018,13 +1018,15 @@ static int fit_config_get_regions(const void *fit, > > int conf_noffset, > > return -EINVAL; > > } > > > > - /* Build our list of data blocks */ > > - region = fit_region_make_list(fit, fdt_regions, count, NULL); > > + /* Allocate the region array, +2 for the hashed-nodes region. */ > > + region = calloc(count+2, sizeof(*region)); > > if (!region) { > > fprintf(stderr, "Out of memory hashing configuration > > '%s/%s'\n", > > conf_name, sig_name); > > return -ENOMEM; > > } > > + /* Build our list of data blocks */ > > + fit_region_make_list(fit, fdt_regions, count, region); > > > > /* Create a list of all hashed properties */ > > debug("Hash nodes:\n"); > > @@ -1043,6 +1045,9 @@ static int fit_config_get_regions(const void *fit, > > int conf_noffset, > > strcpy(region_prop + len, node_inc.strings[i]); > > strlist_free(&node_inc); > > > > + /* Add the hashed-nodes. */ > > + count = fit_region_add_hashed_nodes(region, count, region_prop, len); > > + > > *region_countp = count; > > *regionp = region; > > *region_propp = region_prop; Regards, Simon
