boot: cypress: bound flash access by area size#2803
Open
d3zd3z wants to merge 1 commit into
Open
Conversation
flash_area_read(), flash_area_write() and flash_area_erase() checked the requested offset against fa_off, the partition's absolute base address, instead of fa_size, its length. As off is relative to the start of the area while fa_off is a large device address, the check accepts offsets far past the end of the partition and rejects only absurdly large ones, so an out-of-range access is turned into an absolute address and followed. The checks were also assert()s, which compile out entirely under NDEBUG. Replace them with an unconditional range check against fa_size, ordered so that the arithmetic cannot wrap, returning -1 like the other error paths of these functions. The row alignment asserts express a separate caller contract and are left as they are. Assisted-by: Claude:opus-5 Signed-off-by: David Brown <david.brown@linaro.org>
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
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.
The Cypress flash map backend bounds every flash access against the wrong
field of
struct flash_area.flash_area_read(),flash_area_write()andflash_area_erase()all checkthe requested offset against
fa->fa_off, the partition's absolute baseaddress, rather than
fa->fa_size, its length. Sinceoffis relative tothe start of the area and
fa_offis a large device address, the checkaccepts offsets well past the end of the partition and rejects only
absurdly large ones. The offset is then converted to
fa->fa_off + offandused, so an out-of-range request is followed instead of refused.
The checks are also
assert()s, so they compile out entirely underNDEBUG.This matters because the bootutil core does not range-check image offsets
itself; it relies on
flash_area_read()returning an error for anythingoutside the area, which is what the other backends (Zephyr, mynewt, sim) do.
Replace all three with an unconditional range check against
fa_size,written as
so the arithmetic cannot wrap (
off,lenandfa_sizeare alluint32_t).-1is what these functions already return from theirbad-device-id path. The row alignment assert in
flash_area_erase()expresses a separate caller contract and is left alone.
Inspection only:
boot/cypress/is not built by the simulator and has no CIworkflow, so this has not been compiled. A review from someone who can build
MCUBootApp for PSoC6 would be welcome.