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

bootutil: Add support for devices without erase #2114

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions boot/boot_serial/src/boot_serial.c
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ static off_t erase_range(const struct flash_area *fap, off_t start, off_t end)
BOOT_LOG_DBG("Erasing range 0x%jx:0x%jx", (intmax_t)start,
(intmax_t)(start + size - 1));

rc = flash_area_erase(fap, start, size);
rc = boot_erase_region(fap, start, size);
if (rc != 0) {
BOOT_LOG_ERR("Error %d while erasing range", rc);
return -EINVAL;
Expand Down Expand Up @@ -895,7 +895,7 @@ bs_upload(char *buf, int len)
/* Non-progressive erase erases entire image slot when first chunk of
* an image is received.
*/
rc = flash_area_erase(fap, 0, area_size);
rc = boot_erase_region(fap, 0, area_size);
if (rc) {
goto out_invalid_data;
}
Expand Down
2 changes: 1 addition & 1 deletion boot/boot_serial/src/boot_serial_encryption.c
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ decrypt_region_inplace(struct boot_loader_state *state,
(off + bytes_copied + idx) - hdr->ih_hdr_size, blk_sz,
blk_off, &buf[idx]);
}
rc = flash_area_erase(fap, off + bytes_copied, chunk_sz);
rc = boot_erase_region(fap, off + bytes_copied, chunk_sz);
if (rc != 0) {
return BOOT_EFLASH;
}
Expand Down
8 changes: 8 additions & 0 deletions boot/bootutil/src/bootutil_priv.h
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,15 @@ int boot_copy_region(struct boot_loader_state *state,
const struct flash_area *fap_src,
const struct flash_area *fap_dst,
uint32_t off_src, uint32_t off_dst, uint32_t sz);
/* Prepare for write device that requires erase prior to write. This will
* do nothing on devices without erase requirement.
*/
int boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz);
/* SImilar to boot_erase_region but will alwasy remove data */
Copy link
Collaborator

Choose a reason for hiding this comment

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

-I+i, *always

int boot_scramble_region(const struct flash_area *fap, uint32_t off, uint32_t sz);
/* Makes slot unbootable, either by scrambling header magic, header sector
* or entire slot, depending on settings */
int boot_scramble_slot(const struct flash_area *fap);
bool boot_status_is_reset(const struct boot_status *bs);

#ifdef MCUBOOT_ENC_IMAGES
Expand Down
4 changes: 4 additions & 0 deletions boot/bootutil/src/bootutil_public.c
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,11 @@ boot_set_next(const struct flash_area *fa, bool active, bool confirm)
/* The image slot is corrupt. There is no way to recover, so erase the
* slot to allow future upgrades.
*/
#if !defined(CONFIG_MCUBOOT) && defined(__ZEPHYR__)
flash_area_flatten(fa, 0, flash_area_get_size(fa));
#else
Comment on lines +551 to +553
Copy link
Collaborator

Choose a reason for hiding this comment

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

think it was mentioned in a previous PR that OS specific code should not be present in here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can fix that although MCUboot has significant problem where we are coupled with Flash Map API which means that this has to be replaced with a function that is implemented for every supported system. I will figure way around.

flash_area_erase(fa, 0, flash_area_get_size(fa));
#endif
rc = BOOT_EBADIMAGE;
}
break;
Expand Down
105 changes: 96 additions & 9 deletions boot/bootutil/src/loader.c
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,7 @@ boot_validate_slot(struct boot_loader_state *state, int slot,
&boot_img_hdr(state, BOOT_PRIMARY_SLOT)->ih_ver);
if (rc < 0 && boot_check_header_erased(state, BOOT_PRIMARY_SLOT)) {
BOOT_LOG_ERR("insufficient version in secondary slot");
flash_area_erase(fap, 0, flash_area_get_size(fap));
boot_scramble_slot(fap);
/* Image in the secondary slot does not satisfy version requirement.
* Erase the image and continue booting from the primary slot.
*/
Expand All @@ -1040,7 +1040,7 @@ boot_validate_slot(struct boot_loader_state *state, int slot,
}
if (FIH_NOT_EQ(fih_rc, FIH_SUCCESS)) {
if ((slot != BOOT_PRIMARY_SLOT) || ARE_SLOTS_EQUIVALENT()) {
flash_area_erase(fap, 0, flash_area_get_size(fap));
boot_scramble_slot(fap);
/* Image is invalid, erase it to prevent further unnecessary
* attempts to validate and boot it.
*/
Expand Down Expand Up @@ -1081,7 +1081,7 @@ boot_validate_slot(struct boot_loader_state *state, int slot,
*
* Erase the image and continue booting from the primary slot.
*/
flash_area_erase(fap, 0, fap->fa_size);
boot_scramble_slot(fap);
fih_rc = FIH_NO_BOOTABLE_IMAGE;
goto out;
}
Expand Down Expand Up @@ -1174,20 +1174,108 @@ boot_validated_swap_type(struct boot_loader_state *state,
}
#endif

/* Helper macro to avoid compile errors with systems that do not
* provide function to check device type.
* Note: it used to be inline, but somehow compiler would not
* optimize out branches that were impossible when this evaluated to
* just "true".
*/
#ifdef MCUBOOT_DEV_WITHOUT_ERASE_THEN_NO_ERASE
#define device_requires_erase(fa) (flash_area_erase_required(fa))
#else
#define device_requires_erase(fa) (true)
#endif

/**
* Erases a region of flash.
* Erases a region of device that requires erase prior to write; does
* nothing on devices without erase.
*
* @param flash_area The flash_area containing the region to erase.
* @param fap The flash_area containing the region to erase.
* @param off The offset within the flash area to start the
* erase.
* erase.
* @param sz The number of bytes to erase.
*
* @return 0 on success; nonzero on failure.
*/
int
boot_erase_region(const struct flash_area *fap, uint32_t off, uint32_t sz)
{
return flash_area_erase(fap, off, sz);
if (device_requires_erase(fap)) {
return flash_area_erase(fap, off, sz);
}
return 0;
}

/**
* Removes data from specified region either by writing 0x00 in place of
* data or by doing erase, if device has such hardware requirement.
*
* @param fa The flash_area containing the region to erase.
* @param off The offset within the flash area to start the
* erase.
* @param size The number of bytes to erase.
*
* @return 0 on success; nonzero on failure.
*/
int
boot_scramble_region(const struct flash_area *fa, uint32_t off, uint32_t size)
{
int ret = 0;

if (device_requires_erase(fa)) {
return flash_area_erase(fa, off, size);
} else {
uint8_t buf[BOOT_MAX_ALIGN];
size_t size_done = 0;

memset(buf, flash_area_erased_val(fa), sizeof(buf));

while (size_done < size) {
ret = flash_area_write(fa, size_done + off, buf, sizeof(buf));
if (ret != 0) {
break;
}
size_done += sizeof(buf);
}
Comment on lines +1228 to +1239
Copy link
Collaborator

Choose a reason for hiding this comment

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

this just assumes max align will not overflow the size but makes no checks to ensure that, it should reduces buffer on final write to ensure it does not overflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was intentional as I have assumes that MCUboot should write at BOOT_MAX_ALIGN pace anyway, but maybe that should be fixed.

}
return ret;
}

/**
* Remove enough data from slot to mark is as unused
*
* @param fa Pointer to flash area object for slot
*
* @return 0 on success; nonzero on failure.
*/
int
boot_scramble_slot(const struct flash_area *fa)
{
size_t size;

/* Whether device with erase or not, without minimal scramble
* removing deata in entire slot.
*/
#if !defined(MCUBOOT_MINIMAL_SCRAMBLE)
size = flash_area_get_size(fa);
#else
if (device_requires_erase(fa)) {
int ret = 0;
struct flash_sector header;

ret = flash_area_get_sector(fa, 0, &header);
size = flash_sector_get_size(&header);

if (ret != 0) {
return ret;
}
} else {
size = MAX(sizeof(((struct image_header *)0)->ih_magic),
BOOT_MAX_ALIGN);
size = (size + BOOT_MAX_ALIGN - 1) & ~(BOOT_MAX_ALIGN - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should do the same for footer data because it might contain swap status or mcuboot flags for swapping images and those should always be cleared

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah right, header and slot trailer are verified separately.

}
#endif
return boot_scramble_region(fa, 0, size);
}

#if !defined(MCUBOOT_DIRECT_XIP) && !defined(MCUBOOT_RAM_LOAD)
Expand Down Expand Up @@ -2112,8 +2200,7 @@ check_downgrade_prevention(struct boot_loader_state *state)
if (rc < 0) {
/* Image in slot 0 prevents downgrade, delete image in slot 1 */
BOOT_LOG_INF("Image %d in slot 1 erased due to downgrade prevention", BOOT_CURR_IMG(state));
flash_area_erase(BOOT_IMG(state, 1).area, 0,
flash_area_get_size(BOOT_IMG(state, 1).area));
boot_scramble_slot(BOOT_IMG(state, 1).area);
} else {
rc = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion boot/bootutil/src/ram_load.c
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ boot_remove_image_from_flash(struct boot_loader_state *state, uint32_t slot)
area_id = flash_area_id_from_multi_image_slot(BOOT_CURR_IMG(state), slot);
rc = flash_area_open(area_id, &fap);
if (rc == 0) {
flash_area_erase(fap, 0, flash_area_get_size(fap));
boot_scramble_slot(fap);
flash_area_close(fap);
}

Expand Down
2 changes: 1 addition & 1 deletion boot/bootutil/src/swap_misc.c
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ swap_erase_trailer_sectors(const struct boot_loader_state *state,
do {
sz = boot_img_sector_size(state, slot, sector);
off = boot_img_sector_off(state, slot, sector);
rc = boot_erase_region(fap, off, sz);
rc = boot_scramble_region(fap, off, sz);
assert(rc == 0);

sector--;
Expand Down
30 changes: 30 additions & 0 deletions boot/zephyr/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,36 @@ endif # BOOT_DECOMPRESSION_SUPPORT

endmenu

config MCUBOOT_STORAGE_NO_UNNEEDED_ERASE
bool "Erase is performed only on devices that require it by design"
depends on FLASH_HAS_NO_EXPLICIT_ERASE
help
Not all devices require erase before write and, depending on driver,
may emulate erase by write, as a way to scramble data rather then
by hardware requirement. This unfortunately means that code that
does erase before write, when not needed, will write device twice
which not only reduces device life time but also doubles time
of any write operation (one write for erase and one write for actual
write).
When this option is enabled, MCUboot will check for type of device
and will avoid erase where not needed.

config MCUBOOT_STORAGE_MINIMAL_SCRAMBLE
bool "Do minimal required work to remove data"
help
In some cases MCUboot has to remove data, which usually means make
it non-viable for MCUboot rather then completely destroyed.
For example when MCUboot does not want to bother with broken image
in some slot it will remove it.
The same can be achieved with just removal of header, leaving the
rest of image untouched, as without header MCUboot will not be able
to recognize image in slot as bootable.
Comment on lines +856 to +858
Copy link
Collaborator

Choose a reason for hiding this comment

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

include footer

When this option is enabled, MCUboot will not attempt to erase
entire slot or image, instead it will just remove enough of
data from slot to not recognize it as image anymore.
Depending on type of device this may be done by erase of minimal
number of pages or overwrite of part of image.

config MCUBOOT_DEVICE_SETTINGS
# Hidden selector for device-specific settings
bool
Expand Down
2 changes: 1 addition & 1 deletion boot/zephyr/boot_serial_extension_zephyr_basic.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static int bs_custom_storage_erase(const struct nmgr_hdr *hdr,
if (rc < 0) {
BOOT_LOG_ERR("failed to open flash area");
} else {
rc = flash_area_erase(fa, 0, flash_area_get_size(fa));
rc = flash_area_flatten(fa, 0, flash_area_get_size(fa));
if (rc < 0) {
BOOT_LOG_ERR("failed to erase flash area");
}
Expand Down
18 changes: 18 additions & 0 deletions boot/zephyr/include/flash_map_backend/flash_map_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,24 @@ static inline uint32_t flash_sector_get_size(const struct flash_sector *fs)
int flash_area_get_sector(const struct flash_area *fa, off_t off,
struct flash_sector *fs);


#if defined(CONFIG_MCUBOOT)
static inline bool flash_area_erase_required(const struct flash_area *fa)
{
#if defined(CONFIG_FLASH_HAS_EXPLICIT_ERASE) && defined(CONFIG_FLASH_HAS_NO_EXPLICIT_ERASE)
const struct flash_parameters *fp = flash_get_parameters(flash_area_get_device(fa));

return flash_params_get_erase_cap(flash_get_parameters(flash_area_get_device(fa))) & FLASH_ERASE_C_EXPLICIT;
#else
#if defined(CONFIG_FLASH_HAS_EXPLICIT_ERASE)
return true;
#else
return false;
#endif
#endif
}
#endif

#ifdef __cplusplus
}
#endif
Expand Down
19 changes: 19 additions & 0 deletions boot/zephyr/include/mcuboot_config/mcuboot_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,25 @@
#define MCUBOOT_ERASE_PROGRESSIVELY
#endif

/*
* Devices that do not require erase should not call the function to
* avoid emulation of erase by additional write.
* The emulation is also taking time which doubles required write time
* for such devices.
*/
#ifdef CONFIG_MCUBOOT_STORAGE_NO_UNNEEDED_ERASE
#define MCUBOOT_DEV_WITHOUT_ERASE_THEN_NO_ERASE
#endif

/*
* MCUboot often calls erase on device just to remove data or make application
* image not recognizable. In such instances it may be faster to just remove
* portion of data to make image unrecognizable.
*/
#ifdef CONFIG_MCUBOOT_STORAGE_MINIMAL_SCRAMBLE
#define MCUBOOT_MINIMAL_SCRAMBLE
#endif

/*
* Enabling this option uses newer flash map APIs. This saves RAM and
* avoids deprecated API usage.
Expand Down
Loading