boot: bootutil: fix out of bounds sector reads in swap using scratch - #2854
Open
ThePo1es wants to merge 1 commit into
Open
boot: bootutil: fix out of bounds sector reads in swap using scratch#2854ThePo1es wants to merge 1 commit into
ThePo1es wants to merge 1 commit into
Conversation
boot_slots_compatible() and app_max_size() match the sectors of the primary slot against the sectors of the secondary slot. The loop runs while either slot has sectors left, but indexes the sector table of a slot without checking that this slot has any left. With a different number of sectors in the two slots it reads past the entries filled in by boot_initialize_area(): zeros from the static buffer on a target, whatever is on the stack in the simulator. With MCUBOOT_DECOMPRESS_IMAGES that can make the slots pass as compatible. Compare the slot sizes first, where they have to be equal, and stop the loop when the slot about to be read has no sectors left. This also replaces the DECOMPRESS_IMAGES check for a zero-sized sector, which relied on the unwritten entries being zero. Add a sim test booting a device with slots of different sizes in a swap using scratch build; the upgrade has to be refused and the flash left alone. The test also passes on the old code, where the stale reads happen to end in the same verdict, it pins down the behaviour now that it no longer depends on them. Fixes mcu-tools#2697 Assisted-by: Claude:fable-5 Signed-off-by: ThePo1es <choijinbeom9@gmail.com>
ThePo1es
force-pushed
the
fix-swap-scratch-slots-compatible
branch
from
September 7, 2026 07:21
bd441a4 to
65f4a60
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #2697
boot_slots_compatible() and app_max_size() in swap_scratch.c index the sector table of a slot without checking that the slot still has sectors left, so with slots of a different number of sectors the loop reads past what boot_initialize_area() wrote. Changing the loop condition to && (as first suggested in the issue) breaks layouts where one sector of a slot covers several sectors of the other, e.g. the stm32f4SpiFlash sim device, so instead the slot sizes are compared with flash_area_get_size() first and the loop stops when the slot it is about to read has run out. The DECOMPRESS_IMAGES check for a zero-sized sector is dropped, it relied on the unwritten entries being zero, which is not the case in the sim (context_boot_go() keeps the buffer on the stack).
In a DECOMPRESS_IMAGES build a secondary slot larger than the primary, which the old code let through by way of those stale reads, is still accepted; the check is not made stricter there than it was. There is no decompress feature in the sim, so that path was only traced by hand.
The sim gets an unequal_slots_rejected test: the Nrf52840UnequalSlots* devices get a scratch area so a scratch build can boot them, and the test checks that the upgrade is refused and the flash unchanged. The test also passes on the current code, the stale reads happen to end in the same "not compatible" verdict, it pins down the behaviour now that it no longer depends on what the unwritten entries contain.
This does not conflict with #2792, which deprecates swap using scratch; nothing changes for equal-sized slots.
cargo test in sim/: default, overwrite-only, swap-move all pass.