Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boot: bootutil: Fix swap-scratch when the trailer is larger than the last sector #2206

Merged

Conversation

taltenbach
Copy link
Contributor

@taltenbach taltenbach commented Feb 16, 2025

When swap-scratch is used and the trailer is larger than the last sector of the primary and/or the secondary slot, the upgrade would fail and a potential flash memory corruption could occur due to an underflow in case a sector contains both part of the firmware image and part of the trailer.

This issue was not shown by the simulator since when max-align-32 is not selected, the trailer fit in a single sector in all tested configuration, and when max-align-32 is selected, the firmware image size is chosen so that no sector contain both part of the firmware image and part of the trailer. So, perhaps that's a known limitation and it is expected the users ensure their firmware images are small enough so that this issue doesn't occur, but since I didn't find any documentation mentioning that point and that using a too large image could lead to potentially dangerous underflow, I've considered that as being a bug and attempted to fix it.

The first commit modifies the simulator to compute the maximum firmware image size the same way, irrespective of whether or not max-align-32 is selected, allowing in both case the sector containing the first part of the trailer (or the whole trailer if it is small enough) to also contain part of the firmware image. As expected, this leads to the tests to fail in the configuration where the trailer doesn't fit in a single sector.

The following commits fixes the issue in the swap-scratch implementation.


Example of configuration where the issue occurs:

  • Primary slot: 680 KiB -> 170 * 4096-byte sectors
  • Secondary slot: 680 KiB -> 85 * 8192-byte sectors
  • Scratch area: 8 KiB -> 1 * 8192-byte sector
  • Flash write size: 32 bytes

Therefore:

  • Slot trailer size: (3 * 85 * 32) + (32 * 5) = 8320 bytes
  • Scratch trailer size: (3 * 1 * 32) + (32 * 5) = 256 bytes

This leads to:

         PRIMARY                               SECONDARY                              SCRATCH
|          ...          |              |          ...          |
+-----------------------+              +-----------------------+             +-----------------------+
| Firmware (4096 bytes) | Sector 166   | Firmware (8064 bytes) | Sector 83   | Firmware (7936 bytes) |
|           .           |              |           .           |             |           .           |
+-----------------------+              |           .           |             |           .           |
| Firmware (3968 bytes) | Sector 167   |           .           |             |  Trailer (256 bytes)  |
|  Trailer (128 bytes)  |              | Trailer  (128 bytes)  |             |           .           |
+-----------------------+              +-----------------------+             +-----------------------+
|  Trailer (4096 bytes) | Sector 168   | Trailer  (8192 bytes) | Sector 84
|           .           |              |           .           |
+-----------------------+              |           .           |
|  Trailer (4096 bytes) | Sector 169   |           .           |
|           .           |              |           .           |
+-----------------------+              +-----------------------+

The first operation performed by the swap-scratch algorithm is to swap the last sector containing part of the firmware image. Assuming the image is larger than 672 KiB, this means swapping sectors 166+167 (primary) and 83 (secondary) in this example. This operation is performed by the boot_swap_sectors routine with sz == 8192. Before the fixes introduced by this MR:

  • copy_sz was computed as sz - trailer_sz = 8192 - 8320 = -128, so copy_sz == 4294967167 after wrap around.
  • copy_sz was computed without taking the size of the scratch area into account. In this example, it is clear that if the whole 8064 bytes of firmware data are copied from sector 83 (secondary) to the scratch area, the trailer of the scratch area would be overwritten. copy_sz must therefore be adjusted to ensure the scratch trailer won't be corrupted.
  • Only sectors 166+167 (primary) and 83 (secondary) were erased, causing in particular issues when attempting to rewrite the trailer in the primary slot.

…align-32

When testing the swap-scratch upgrade strategy with 32-byte flash write
size, the largest image size was computed assuming the trailer starts at
the beginning of a sector. This assumption is not mentionned in the
MCUboot's user documentation and a user would therefore expect that
swap-scratch is working even if a sector contains both firmware and
trailer data.

Also, this assumption is not made for flash write sizes lower than 32
bytes. This is probably due to the fact that when 'max-align-32' is not
selected, the trailer fits into a single sector for all tested
configuration. However, when 'max-align-32' is enabled, the trailer is
larger than a single sector in some tested configurations, which makes
harder to compute the maximal image size since the size of the trailer
in the scratch area has to be taken into account.

This commit modifies in the simulator the routine computing the size
that must be reserved for the trailer to properly handle the case of
trailers too large to fit into a single sector. Unfortunately, this
reveals that the swap-scratch implementation doesn't properly handle
this case, which will be fixed in the following commits.

Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
When using swap-scratch, the last sector of a slot able to contain
firmware data might also contain part of the trailer (or the whole
trailer, if the latter is small enough). When the trailer is large, a
single sector it might not fit in a single sector and that last firmware
sector might therefore not be the last sector of the slot.

When that happens, and unless the trailer starts exactly at the
beginning of a sector, an underflow could occur when computing the
number of bytes that must be copied from the last firmware sector.
Indeed, when the trailer is large, its size can be larger than that
sector and, depending on the size of the sratch area, 'copy_sz' can at
worst equal to the size of this sector.

If this underflow occurs, 'copy_sz' would end up containing a very large
value, that would probably cause the upgrade to fail and could lead to a
corruption of a large part of the flash memory if no bound check is
performed in the flash driver.

Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
…arge

When using swap-scratch and the image trailer doesn't fit in a single
sector, some padding might be necessary between the end of the firmware
data and the beginning of the image trailer. Indeed, when the trailer
fit in a single sector, it is guaranteed that when copying the firmware
data from this sector to the scratch area, it won't overwrite the
trailer in the scratch trailer since that trailer is always smaller the
image trailer.

However, when the trailer is larger than a single sector, the sector
containing the last part of the firmware data might only contain a very
small part of the trailer. There is no more guarantee that the scratch
trailer won't get overwritten when copying that sector to the sratch
area. Therefore, a check must be added to handle that case.

Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
When using swap-scratch and the trailer is too large to fit in a single
sector, might not be fully erased in both the primary and the secondary
slot if the last sector containing firmware data also contains part of
the trailer. Indeed, in that case, it might happen that only the first
part of the trailer was erased, causing issues e.g. when attempting to
rewrite the trailer in the primary slot during the upgrade.

Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
…image size

When using swap-scratch and the trailer is larger than a single sector,
the sector containing the last part of the firmware data might only
contain a very small part of the trailer and some padding might be
necessary between the end of the firmware image and the beginning of the
trailer to ensure the scratch trailer won't be overwritten when copying
that sector to the scratch area.

This commit updates the 'bootutil_max_image_size' routine to take that
padding into account.

Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
@taltenbach taltenbach force-pushed the fix/swap-scratch-multi-trailer-sectors branch from 155b802 to 08985c9 Compare February 21, 2025 00:15
@davidvincze davidvincze added 2.2.0 Release v2.2.0 bug labels Feb 27, 2025
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting the work into figuring this out. I'm curious if this is enough to fix swap-scratch or swap-offset for things like the STM32F4, with just 3 128k sectors available.

@taltenbach
Copy link
Contributor Author

taltenbach commented Mar 3, 2025

Thanks for putting the work into figuring this out. I'm curious if this is enough to fix swap-scratch or swap-offset for things like the STM32F4, with just 3 128k sectors available.

@d3zd3z You're welcome :) Are you referring to #2052? I'm might be able to put some time into this issue in the following weeks.

@de-nordic de-nordic requested a review from nordicjm March 4, 2025 08:44
@nordicjm nordicjm merged commit d00b11d into mcu-tools:main Mar 7, 2025
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.2.0 Release v2.2.0 bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants