[PATCH v1 4/5] tools: mkimage: detect overlapping load regions in FIT configurations

[Aristo Chen]

Yannic Moog <Y.Moog at phytec.de> 於 2025年7月31日 週四 下午7:43寫道：
>
> Am Dienstag, dem 29.07.2025 um 12:48 +0000 schrieb Aristo Chen:
> > This patch adds a validation step in mkimage to detect memory region
> > overlaps between images specified in the same configuration of a
> > FIT image. If any overlaps are found, the tool prints an error and
> > aborts the build.
> >
> > This helps prevent runtime memory corruption caused by conflicting
> > load addresses between images.
> >
> > Signed-off-by: Aristo Chen <aristo.chen at canonical.com>
> > ---
> >  tools/fit_image.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/mkimage.c   |  3 +-
> >  2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/fit_image.c b/tools/fit_image.c
> > index 331be5ae71d..d0061adf051 100644
> > --- a/tools/fit_image.c
> > +++ b/tools/fit_image.c
> > @@ -22,6 +22,22 @@
> >  #include <version.h>
> >  #include <u-boot/crc.h>
> >
> > +#define MAX_IMAGES_PER_CONFIG 16
>
> How did you get this number?
>
> If this is arbitrary, add to the error message below that if someone needs more
> images, they can increase the size.
Yes, it is an arbitrary number, I am planning to dynamically allocate
the memory to solve this concern. Otherwise,
we may need to add a new variable to allow the user to modify the
value of the variable?
>
> > +
> > +struct fit_region {
> > +     ulong load;
> > +     ulong size;
> > +     const char *name;
> > +};
> > +
> > +static int regions_overlap(struct fit_region *a, struct fit_region *b)
>
> Nit: can you constify these?
>
> > +{
> > +     ulong a_end = a->load + a->size;
> > +     ulong b_end = b->load + b->size;
> > +
> > +     return !(a_end <= b->load || b_end <= a->load);
> > +}
> > +
> >  static struct legacy_img_hdr header;
> >
> >  static int fit_estimate_hash_sig_size(struct image_tool_params *params, const
> > char *fname)
> > @@ -775,6 +791,8 @@ static int fit_import_data(struct image_tool_params
> > *params, const char *fname)
> >       }
> >
> >       fdt_for_each_subnode(node, fdt, confs) {
> > +             struct fit_region regions[MAX_IMAGES_PER_CONFIG];
>
> init to {0}?
>
> > +             int img_count = 0;
>
> Nit: use unsigned
>
> >               const char *conf_name = fdt_get_name(fdt, node, NULL);
> >
> >               for (int i = 0; i < ARRAY_SIZE(props); i++) {
> > @@ -798,6 +816,66 @@ static int fit_import_data(struct image_tool_params
> > *params, const char *fname)
> >                                       ret = FDT_ERR_NOTFOUND;
> >                                       goto err_munmap;
> >                               }
> > +
> > +                             ulong img_load = 0;
> > +                             int img_size = 0;
> > +
> > +                             if (fit_image_get_load(fdt, img, &img_load))
> > {
> > +                                     fprintf(stderr,
> > +                                             "Warning: not able to get
> > `load` of node '%s'\n",
> > +                                             img_name);
> > +                                     // Skip checking the components that
> > do not have a
> > +                                     // definition for `load`
> > +                                     continue;
> > +                             }
> > +                             const char *img_data = fdt_getprop(fdt, img,
> > +
> > FIT_DATA_PROP,
> > +
> > &img_size);
> > +
> > +                             if (!img_data || img_size == 0)
>
> be consistent, use !img_size
>
> > +                                     continue;
> > +
> > +                             // Check if we've already added this image to
> > avoid duplicates
> > +                             bool already_added = false;
> > +
> > +                             for (int k = 0; k < img_count; k++) {
> > +                                     if (strcmp(regions[k].name, img_name)
> > == 0) {
>
> same here, !strcmp should do, right?
>
> > +                                             already_added = true;
>
> You can simplify and do continue; here?
> Avoids the already_added variable.
>
> > +                                             break;
> > +                                     }
> > +                             }
> > +                             if (already_added)
> > +                                     continue;
> > +
> > +                             if (img_count >= MAX_IMAGES_PER_CONFIG) {
> > +                                     fprintf(stderr, "Too many images in
> > config %s\n",
> > +                                             fdt_get_name(fdt, node,
> > NULL));
> > +                                     break;
> > +                             }
> > +
> > +                             regions[img_count].load = img_load;
> > +                             regions[img_count].size = img_size;
> > +                             regions[img_count].name = img_name;
> > +                             img_count++;
> > +                     }
> > +             }
> > +
> > +             // Check for overlap within this config only
> > +             for (int i = 0; i < img_count; i++) {
> > +                     for (int j = i + 1; j < img_count; j++) {
>
> Nit: unsigned here as well
>
> Yannic
>
> > +                             if (regions_overlap(&regions[i],
> > &regions[j])) {
> > +                                     fprintf(stderr,
> > +                                             "[Config: %s] Error: Overlap
> > detected:\n"
> > +                                             "  - %s: [0x%lx - 0x%lx]\n"
> > +                                             "  - %s: [0x%lx - 0x%lx]\n",
> > +                                             fdt_get_name(fdt, node,
> > NULL),
> > +                                             regions[i].name,
> > regions[i].load,
> > +                                             regions[i].load +
> > regions[i].size,
> > +                                             regions[j].name,
> > regions[j].load,
> > +                                             regions[j].load +
> > regions[j].size);
> > +                                     ret = FDT_ERR_BADSTRUCTURE;
> > +                                     goto err_munmap;
> > +                             }
> >                       }
> >               }
> >       }
> > diff --git a/tools/mkimage.c b/tools/mkimage.c
> > index 361711c53b2..3f28918f5cf 100644
> > --- a/tools/mkimage.c
> > +++ b/tools/mkimage.c
> > @@ -520,7 +520,8 @@ int main(int argc, char **argv)
> >                       retval = tparams->fflag_handle(&params);
> >
> >               if (retval != EXIT_SUCCESS) {
> > -                     if (retval == FDT_ERR_NOTFOUND) {
> > +                     if (retval == FDT_ERR_NOTFOUND ||
> > +                         retval == FDT_ERR_BADSTRUCTURE) {
> >                               // Already printed error, exit cleanly
> >                               exit(EXIT_FAILURE);
> >                       }