find_last_sector_idx never returning

[lefebvresam]

After calling boot_read_image_header(&state, 0, &hdr, &status); there is a call find_swap_count(state, swap_size); and find_last_sector_idx(state, copy_size); with copy_size = 76040.

This code part is never converging and crashes (line 481 of swap_scratch.c):

    while (1) {
        if ((primary_slot_size < copy_size) ||
            (primary_slot_size < secondary_slot_size)) {
           primary_slot_size += boot_img_sector_size(state,
                                                     BOOT_PRIMARY_SLOT,
                                                     last_sector_idx);
        }
        if ((secondary_slot_size < copy_size) ||
            (secondary_slot_size < primary_slot_size)) {
           secondary_slot_size += boot_img_sector_size(state,
                                                       BOOT_SECONDARY_SLOT,
                                                       last_sector_idx);
        }
        if (primary_slot_size >= copy_size &&
                secondary_slot_size >= copy_size &&
                primary_slot_size == secondary_slot_size) {
            break;
        }
        last_sector_idx++;
    }

It should look more logical that this must be:

        if ((primary_slot_size >= copy_size &&
                secondary_slot_size >= copy_size) ||
                primary_slot_size == secondary_slot_size) {
            break;
        }

Then I have only one iteration:

[INFO][BL]: Booting firmware image at 0x8021000
[INFO][main]: Boot confirmed
boot_read_swap_size
swap_size32 67212
flash_area_close
rc 0
copy_size 67212
primary_slot_size 0
secondary_slot_size 0

primary_slot_size 0
secondary_slot_size 66224890
primary_slot_size 35651584
break
last_sector_idx 1
swap_count 1

The piece of code should better have a check to assert before crashing/hanging.

Reproduction:

git clone git@bitbucket.org:saleconix/mcuboot.git
git clone git@bitbucket.org:saleconix/mcuboot_app.git

[nordicjm]

This code works fine on zephyr and mynewt so you have a problem with your deprecated and unsupported mbedos code which is outside the scope of MCUboot

[lefebvresam]

swap_scratch.c is part of bootutil and has nothing to do with mbed.

@multiplemonomials asked to restart the mbed-ce support in pull/2152 becasue we have our community fork of mbed-os.

[nordicjm]

swap_scratch.c is part of bootutil and has nothing to do with mbed.

The code above adds sector sizes to a count value and that code is definitely coming from mbed, mcuboot has to query it somehow so your mbed code has an issue there (i.e. maybe it is returning 0 if you enter an infinite loop)

@multiplemonomials asked to restart the mbed-ce support in pull/2152 becasue we have our community port of mbed-os.

Interesting, if they're willing to maintain support for it and look into the issue then that's fine

[lefebvresam]

I do not see any mbed references in this code:

 * SPDX-License-Identifier: Apache-2.0
 *
 * Copyright (c) 2019 JUUL Labs

[nordicjm]

@multiplemonomials can you check if this can be closed?

[multiplemonomials]

@lefebvresam Do you have a project that reproduces this issue that is based on current master branch of mcuboot (i.e. one that uses the CMake build system)? If so I can give it a shot on my STM32U5 board and see if I can reproduce the error

[lefebvresam]

@multiplemonomials This was in the context of version extraction. In the beginning there was no initialization of state and boot status which led to unlogical decisions in the calling tree. After doing such initalization as proposed in the patch, this problem was not occuring anymore. However, I never went into the deep details of checking primary/secundary slot size in this piece of code and I still find it a strange construction which can potentially lead to hanging software. I should recommend to revise this code and put some additional checks or assertions to avoid hanging behavior.

[lefebvresam]

If you want to reproduce you can cherry pick from here I think. Taking into account the remarks above.