Re: [PATCH 1/7] spl: fit: Don't overwrite previous loadable if "load" is missing

Hi Alexandru,

On Wed, 10 Mar 2021 at 11:04, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

spl_load_fit_image() will try to load an image at the address given in the "load" property. Absent such property, it uses

    image_info->load_addr

Correct use of this is demonstrated in spl_fit_append_fdt(), which resets the 'load_addr' before each spl_load_fit_image() call.

On the other hand loading "loadables" loop in spl_load_simple_fit() completely ignores this. It re-uses the same structure, but doesn't reset load_addr. If loadable [i] does not have a "load" property, its load address defaults to load_addr, which still contains the address of loadable [i - 1].

A simple solution is to treat NULL as an invalid load address. The caller can set load_addr = 0 to request an abort if the "load" property is absent.

Another way would be to add a load_valid bool or flag to the struct.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/spl/spl_fit.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 75c8ff065b..43fc43cf2b 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -223,7 +223,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,

• @image_info: will be filled with information about the loaded image

•         If the FIT node does not contain a "load" (address) property,
  

•         the image gets loaded to the address pointed to by the
  

• •         load_addr member in this struct.
    

• •         load_addr member in this struct, if load_addr is not NULL
    

s/NULL/0/

This is not a pointer, but an address.

• Return: 0 on success or a negative error number.

*/ @@ -258,8 +258,14 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, debug("%s ", genimg_get_comp_name(image_comp)); }

•   if (fit_image_get_load(fit, node, &load_addr))
  

•   if (fit_image_get_load(fit, node, &load_addr)) {
  

•           if (!image_info->load_addr) {
  

•                   printf("Can't load %s: No load address and no buffer\n",
  

•                          fit_get_name(fit, node, NULL));
  

•                   return -ENOBUFS;
  

•           }
            load_addr = image_info->load_addr;
  

•   }
  
    if (!fit_image_get_data_position(fit, node, &offset)) {
            external_data = true;
  

@@ -697,6 +703,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (firmware_node == node) continue;

•           image_info.load_addr = 0;
            ret = spl_load_fit_image(info, sector, &ctx, node, &image_info);
            if (ret < 0) {
                    printf("%s: can't load image loadables index %d (ret = %d)\n",
  

-- 2.26.2

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon