boot: bootutil: Fix max image size computation for swap-move and swap-offset #2283
+102
−76
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
de-nordic
reviewed
Apr 29, 2025
boot/bootutil/src/bootutil_misc.c
Outdated
Comment on lines
478
to
500
| #elif defined(MCUBOOT_SWAP_USING_MOVE) | ||
| (void) fap; | ||
|
|
||
| struct flash_sector sector; | ||
| /* get the last sector offset */ | ||
| int rc = flash_area_get_sector(fap, boot_status_off(fap), §or); | ||
| if (rc) { | ||
| BOOT_LOG_ERR("Unable to determine flash sector of the image trailer"); | ||
| return 0; /* Returning of zero here should cause any check which uses | ||
| * this value to fail. | ||
| */ | ||
| } | ||
| return flash_sector_get_off(§or); | ||
| const struct flash_area *fap_pri = BOOT_IMG_AREA(state, BOOT_PRIMARY_SLOT); | ||
| assert(fap_pri != NULL); | ||
|
|
||
| size_t trailer_sz = boot_trailer_sz(BOOT_WRITE_SZ(state)); | ||
| size_t sector_sz = boot_img_sector_size(state, BOOT_PRIMARY_SLOT, 0); | ||
| size_t padding_sz = sector_sz; | ||
|
|
||
| /* The trailer size needs to be sector-aligned */ | ||
| trailer_sz = ALIGN_UP(trailer_sz, sector_sz); | ||
|
|
||
| return flash_area_get_size(fap_pri) - trailer_sz - padding_sz; | ||
| #elif defined(MCUBOOT_SWAP_USING_OFFSET) | ||
| (void) fap; | ||
|
|
||
| const struct flash_area *fap_sec = BOOT_IMG_AREA(state, BOOT_SECONDARY_SLOT); | ||
| assert(fap_sec != NULL); | ||
|
|
||
| size_t trailer_sz = boot_trailer_sz(BOOT_WRITE_SZ(state)); | ||
| size_t sector_sz = boot_img_sector_size(state, BOOT_SECONDARY_SLOT, 0); | ||
| size_t padding_sz = sector_sz; | ||
|
|
||
| /* The trailer size needs to be sector-aligned */ | ||
| trailer_sz = ALIGN_UP(trailer_sz, sector_sz); | ||
|
|
||
| return flash_area_get_size(fap_sec) - trailer_sz - padding_sz; | ||
| #elif defined(MCUBOOT_OVERWRITE_ONLY) |
There was a problem hiding this comment.
If you ifdef around variable assigned with BOOT_PRIMARY_SLOT or BOOT_SECONDARY_SLOT, and passed where these are accepted, then there is no difference between both blocks.
There was a problem hiding this comment.
@de-nordic You're right, I was anticipating #2247, which will need different logic for swap-move and swap-offset. But considering only this PR, it makes indeed more sense to merge the two blocks. I updated my changes accordingly
When computing the maximum image size in bootutil_max_image_size for swap-move or swap-offset strategy, the computation was using the size of the flash area provided as argument and was not taking into account the size of the padding sector. This was causing an incorrect size to be returned in some cases, for example when the two slots have the same size or when the slots haven't the same size but the routine is called for the slot containing the padding sector. For example, let's imagine swap-move is being used on a device having a sector size S and two slots of N bytes. This is valid configuration and the maximum image size is N - S - T, T being the size of the trailer rounded up to the next multiple of S. When calling bootutil_max_image_size with either the primary or secondary slot, the size N - T is returned, which is incorrect. This commit fixes the issue by computing always the maximum image using the size of the slot containing the padding and substracting the size of the padding and of the aligned trailer. Signed-off-by: Thomas Altenbach <[email protected]>
When checking the size of an image in bootutil_img_validate, the offset to the end of the TLV area was used as the image size in all cases. However, when using swap-offset, the upgrade image is written in the secondary with an offset. This offset is not part of the image and must therefore not be taken into account in the image size. Signed-off-by: Thomas Altenbach <[email protected]>
For the swap-move and swap-offset strategies, the computation of the largest image size was not taking taking into account the padding that is needed when using those strategies. Due to this limitation, the simulator is currently using hardcoded image sizes, smaller than the maximum possible size, when running tests for the swap-move or swap-offset strategies. This commit fixes the maximum image size computation for those strategies. Signed-off-by: Thomas Altenbach <[email protected]>
The simulator was testing the upgrade with the largest image possible for all strategies, except for overwrite-only, swap-move and swap-offset because some tests were failing when the maximum image size was used. For overwrite-only, this was due to an incorrect trailer size computation. This has been fixed by 88294be. For swap-move and swap-offset, this was due to the simulator not taking into account the padding needed by those strategies in the primary or secondary slot, but also to incorrect computation of the maximum image size in some cases by the MCUboot library. Both issues have been fixed by the previous commits. Since all those issues have been fixed, the simulator can now be configured to test upgrade with the largest possible image for all strategies. Note that logic needed to generate image of a given image is kept even if not useful anymore at the moment, since that might be needed when test will be added to ensure proper behavior when images of different sizes are used. Signed-off-by: Thomas Altenbach <[email protected]>
taltenbach
force-pushed
the
fix/swap-move-swap-offset-max-img-size
branch
from
a494479 to
52ba928
Compare
d3zd3z
approved these changes
May 1, 2025
nordicjm
approved these changes
May 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When computing the maximum image size in
bootutil_max_image_sizefor the swap-move or swap-offset strategy, the computation was using the size of the flash area provided as argument and was not taking into account the size of the padding sector. This was causing an incorrect (too large) size to be returned in the following cases:For example, let's imagine swap-move is being used on a device having a sector size
Sand two slots ofNbytes. This is valid configuration and the maximum image size isN - S - T,Tbeing the size of the trailer rounded up to the next multiple ofS. When callingbootutil_max_image_sizewith either the primary or secondary slot, the sizeN - Tis returned, which is incorrect.This was not detected by the tests since, due to limitations of the simulator, the swap-move and swap-offset strategies were not tested with the largest possible images. In addition to fixing the issue in
bootutil_max_image_size, this PR modifies the simulator to allow the simulator to use largest possible images for all strategies and therefore detect those kind of issues.