Scatter/gather ELF files according to segments `paddr` #562

danielinux · 2025-04-07T10:25:03Z

This PR adds the option ELF_SCATTERED, which allows for an ELF file to be stored in different partitions on the target flash memory.
Before starting, wolfboot will check all the scattered fragments for integreity, by calculating the digest to match the one in the header.

Also added: simulator config files to test. Still unable to stage the scattered image in the simulator, but the current wolfboot.elf application shows the working scattered store/gathered digest procedures before do_boot().

…ER-GAPS.md and video

Copilot

Copilot reviewed 9 out of 16 changed files in this pull request and generated 1 comment.

Files not reviewed (7)

CMakeLists.txt: Language not supported
arch.mk: Language not supported
config/examples/sim-elf-scattered.config: Language not supported
config/examples/sim32-elf-scattered.config: Language not supported
options.mk: Language not supported
test-app/Makefile: Language not supported
tools/elf-parser/Makefile: Language not supported

src/elf.c

Spotted by copilot Co-authored-by: Copilot <[email protected]>

Copilot

Copilot reviewed 9 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (7)

CMakeLists.txt: Language not supported
arch.mk: Language not supported
config/examples/sim-elf-scattered.config: Language not supported
config/examples/sim32-elf-scattered.config: Language not supported
options.mk: Language not supported
test-app/Makefile: Language not supported
tools/elf-parser/Makefile: Language not supported

Comments suppressed due to low confidence (2)

include/wolfboot/wolfboot.h:313

The constant IMG_STATE_ELF_LOADING is defined here and again later. Please remove the duplicate definition to avoid potential conflicts.

/* ELF loading state - only valid on boot partition so doesn't conflict with

include/wolfboot/wolfboot.h:325

Duplicate definition of IMG_STATE_ELF_LOADING; consider removing one instance to make the code cleaner.

#define IMG_STATE_ELF_LOADING 0x70

bigbrett

Great stuff so far. In addition to my review comments, did you have integration into the signing keytool that wasn't committed? You must have in order to be able to generate images right?

options.mk

src/elf.c

test-app/Makefile

src/elf.c

include/wolfboot/wolfboot.h

src/update_flash.c

danielinux · 2025-04-09T12:10:50Z

Great comments, I'll work on these ASAP.

Regarding your question about sign, sign has not been modified, as there is no change in the header. The elf is 'dynamically rehashed' at runtime using the stored elf header + the scattered fragments, and eventually calculates the same digest as the full image. In fact this is also used to detect if there is no elf image at all in the fragments locations, or if the images have been only partially stored before a power failure event.

- ARCH_FLASH_OFFSET only used in simulation (dynamic 'base' address) - Fixed the DISABLE_BACKUP case - renamed define to WOLFBOOT_ELF_SCATTERED

- Added missing `test-app/app_sim_scattered.c` - Added missing `test-app/sim_scattered.ld` - Fixed comments at the end of define blocks for consistency - Removed unused constants as indicated

danielinux · 2025-04-11T14:30:13Z

@bigbrett all the remarks have been addressed. I'm available to discuss further on optimizing the update time. I think it's a valid point but I'd like to prioritize design simplicity in this case. I'm open to better approaches if we find one.

I've also added a simple build-verify test case using the simulator since your last review.

bigbrett

This is great work Daniele. I wish we had a better way to actually verify the boot, but given the dynamic library shenanigans we would need to wrangle to get this running in the simulator, I think seeing the hash verify is good enough for me to get this in.

I think we should ideally figure out a way to test the actual boot in CI at some point, whether it be on renode or on QEMU. ~~Theoretically we should be able to load any of the elfs curently used for testing on renode via this mechanism, and they should still boot.~~ Edit: JK no we can't, since it is linked to run from the boot partition

dgarske

All tests out here! Nice work!

config/examples/sim-elf-scattered.config

bigbrett

@danielinux I just spotted something. This new feature breaks wolfBoot_erase_bootloader(), which assumes the bootloader occupies all memory between ARCH_FLASH_OFFSET and WOLFBOOT_PARTITION_BOOT_ADDRESS. This is no longer an appropriate assumption when scattered elf loading is turned on, as this could be perfectly valid memory to load to. Not sure if we have a good way currently to get just the size of wolfBoot itself....would need to think about this. Thoughts? Maybe we need to introduce an additional "scatter region" or maybe just a dedicated partition region for wolfBoot itself?

Edit: At least internally, this seems to only be used by self-update. I guess that would mean the self update could potentially destroy the "active image" e.g. scattered elf, however I believe that would just cause wolfBoot to re-scatter the elf in the boot partition on the next boot?

bigbrett

also found a few lingering printfs

bigbrett · 2025-04-11T20:30:29Z

src/elf.c

+            paddr = (unsigned long)ph[i].paddr;
+            offset = (unsigned long)ph[i].offset;
+            filesz = (unsigned long)ph[i].file_size;
+            printf("Writing section at address %lx offset %lx\n", paddr, offset);


Suggested change

printf("Writing section at address %lx offset %lx\n", paddr, offset);

wolfBoot_printf("Writing section at address %lx offset %lx\n", paddr, offset);

bigbrett · 2025-04-11T20:30:49Z

src/elf.c

+            paddr = (unsigned long)ph[i].paddr;
+            offset = (unsigned long)ph[i].offset;
+            filesz = (unsigned long)ph[i].file_size;
+            printf("Writing section at address %lx offset %lx\n", paddr, offset);


Suggested change

printf("Writing section at address %lx offset %lx\n", paddr, offset);

wolfBoot_printf("Writing section at address %lx offset %lx\n", paddr, offset);

billphipps · 2025-04-12T16:19:08Z

Couple of questions:

I think I'm missing some code: elf_check_image_scattered? This is where I assumed there would be a loop of hashes against each of the individual sections. It was unclear to me if each section would have a stored hash or if a single hash would be updated and only finalized at the end.
Is there a reason that scattered support isn't default within the ELF support? Seems like an elf file that has a single segment should measure the same when scattered support is enabled.
I think the simulator should be able test this. I'll take a quick swing at pinning memory maps and see if that can make static linkage work.

danielinux · 2025-04-14T16:09:36Z

@danielinux I just spotted something. This new feature breaks wolfBoot_erase_bootloader(),

It would make sense to me to make the two features incompatible

danielinux · 2025-04-14T16:17:07Z

Couple of questions:

1. I think I'm missing some code: elf_check_image_scattered?  This is where I assumed there would be a loop of hashes against each of the individual sections.  It was unclear to me if each section would have a stored hash or if a single hash would be updated and only finalized at the end.

Yes. The latter. Basically it assumes that a copy of the complete image is still available, to retrieve the image header + elf header, then the digest calculation is performed on the chunks to ensure that they correspond to the selected stored image.

2. Is there a reason that scattered support isn't default within the ELF support?  Seems like an elf file that has a single segment should measure the same when scattered support is enabled.

This is a good point. Elf support is different on other platforms though, and base address for loading may vary for relocatable images. It could be an improvement for those cases where the elf is loaded (manually) into RAM after verification.

3. I think the simulator should be able test this.  I'll take a quick swing at pinning memory maps and see if that can make static linkage work.

Thanks, that would be great. hal/sim.c uses exec with an actual executable in the 'normal' use case. I think it's mostly a matter of crafting the right linker script for the test-application, but it was becoming too much time-consuming

- Removed stray printfs from elf.c - Updated test-configs.yml to include build tests for config files

bigbrett · 2025-04-15T16:20:53Z

@danielinux @dgarske I'm going to be making some changes to this functionality as I integrate and test on TC3xx (it wont quite work written as-is at the moment). That said, since this is working in Sim, should we go ahead an merge and I will add additional changes in a subsequent PR? Or would you prefer we close this PR and I'll add additional commits on top of @danielinux 's work and create a new PR once working on TC3xx? Slight preference to just merging the work as-is, since it isn't in use by anyone yet, but up to you

bigbrett

@danielinux I found a few other issues the more I look at this....do you want to fix/work on these or hand it over to me? If the latter, thoughts on closing the PR vs merging work as-is even if it doesn't work on AURIX?

bigbrett · 2025-04-11T20:35:16Z

src/elf.c

+    const unsigned char *image;
+    int is_elf32;
+    unsigned short entry_count;
+    unsigned short entry_size;


unused but set variable (compiler warning)

bigbrett · 2025-04-15T20:57:19Z

src/image.c

+    wolfBoot_hash_t ctx;
+    if (wolfBoot_open_image(&boot, part) < 0)
+        return -1;
+    p = get_img_hdr(&boot);


this wont work if the image is in external flash, as get_img_hdr() returns a pointer to the RAM buffer if the partition is external. Later we do things like elf_h = p + IMAGE_HEADER_SIZE which will break if p is the RAM buffer.

bigbrett · 2025-04-15T21:05:00Z

src/elf.c

+            if (ext_flash) {
+                ext_flash_unlock();
+                ext_flash_erase(paddr + BASE_OFF, filesz);
+                ext_flash_write(paddr + BASE_OFF, image + offset, filesz);
+                ext_flash_lock();
+            }
+            else
+#endif
+            {
+                hal_flash_unlock();
+                hal_flash_erase(paddr + BASE_OFF, filesz);
+                hal_flash_write(paddr + BASE_OFF, image + offset, filesz);
+                hal_flash_lock();
+            }


This assumes that the HAL implementation of {ext,hal}_flash_write() can handle reading from flash as well, which I'm not sure all of our HALs do (at least TC3xx does not). Normally when ext flash is enabled we do this via wolfBoot_copy_sector() which copies between flash and swap through a RAM buffer.

bigbrett and others added 9 commits April 3, 2025 09:53

AI WIP

9510360

WIP: first stab at elf loading impl

4f6921d

formatting

7caf579

WIP: structure is there, but some major issues. See WOLFBOOT-ELF-LOAD…

fb6ca43

…ER-GAPS.md and video

Moved elf parsing/scattering to elf.c, WIP sim

e7cd340

ELF_SCATTERED: added sha check (WIP)

1109194

Fix scatter-gather ELF

d9104ec

Added example config files for sim, sim32 + ELF sg

50725b3

Fix regressions detected by github workflows

386a172

danielinux force-pushed the update-flash-elf branch from 660d832 to 386a172 Compare April 7, 2025 15:11

Fixed 'backup disabled' case without ELF_SCATTERED

d142426

danielinux requested a review from Copilot April 8, 2025 06:44

danielinux assigned bigbrett Apr 8, 2025

Copilot AI reviewed Apr 8, 2025

View reviewed changes

src/elf.c Outdated Show resolved Hide resolved

fixed misleading comment

9d7084c

Spotted by copilot Co-authored-by: Copilot <[email protected]>

danielinux requested a review from Copilot April 8, 2025 07:12

Copilot AI reviewed Apr 8, 2025

View reviewed changes

bigbrett requested changes Apr 8, 2025

View reviewed changes

danielinux assigned danielinux and unassigned bigbrett Apr 9, 2025

danielinux added 3 commits April 11, 2025 15:55

Some of the fixes requested during review:

9c3e862

- ARCH_FLASH_OFFSET only used in simulation (dynamic 'base' address) - Fixed the DISABLE_BACKUP case - renamed define to WOLFBOOT_ELF_SCATTERED

Added missing files + cosmetics as per review

ea0b4fb

- Added missing `test-app/app_sim_scattered.c` - Added missing `test-app/sim_scattered.ld` - Fixed comments at the end of define blocks for consistency - Removed unused constants as indicated

Added test for scattered ELF + validation via sim

5f8b79a

danielinux force-pushed the update-flash-elf branch from d6cce06 to 9bf813c Compare April 11, 2025 14:16

Fixed warnings in test build

c841113

danielinux force-pushed the update-flash-elf branch from 9bf813c to c841113 Compare April 11, 2025 14:19

danielinux requested a review from bigbrett April 11, 2025 14:27

danielinux assigned bigbrett and unassigned danielinux Apr 11, 2025

bigbrett approved these changes Apr 11, 2025

View reviewed changes

bigbrett assigned dgarske and unassigned dgarske and bigbrett Apr 11, 2025

dgarske approved these changes Apr 11, 2025

View reviewed changes

dgarske requested changes Apr 11, 2025

View reviewed changes

config/examples/sim-elf-scattered.config Show resolved Hide resolved

dgarske assigned danielinux and unassigned dgarske Apr 11, 2025

bigbrett self-requested a review April 11, 2025 19:37

bigbrett requested changes Apr 11, 2025

View reviewed changes

danielinux added 3 commits April 14, 2025 18:33

Reviewers requests

71f2424

- Removed stray printfs from elf.c - Updated test-configs.yml to include build tests for config files

Removed elf32bit build test

11b8a68

Attempt to update renode base images

02c2152

dgarske approved these changes Apr 15, 2025

View reviewed changes

bigbrett reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scatter/gather ELF files according to segments `paddr` #562

Scatter/gather ELF files according to segments `paddr` #562

danielinux commented Apr 7, 2025

Copilot AI left a comment

Copilot AI left a comment

bigbrett left a comment

danielinux commented Apr 9, 2025

danielinux commented Apr 11, 2025

bigbrett left a comment •

edited

Loading

dgarske left a comment

bigbrett left a comment •

edited

Loading

bigbrett left a comment

bigbrett Apr 11, 2025

bigbrett Apr 11, 2025

billphipps commented Apr 12, 2025

danielinux commented Apr 14, 2025

danielinux commented Apr 14, 2025

bigbrett commented Apr 15, 2025

bigbrett left a comment

bigbrett Apr 11, 2025

bigbrett Apr 15, 2025

bigbrett Apr 15, 2025

	printf("Writing section at address %lx offset %lx\n", paddr, offset);
	wolfBoot_printf("Writing section at address %lx offset %lx\n", paddr, offset);

Scatter/gather ELF files according to segments paddr #562

Are you sure you want to change the base?

Scatter/gather ELF files according to segments paddr #562

Conversation

danielinux commented Apr 7, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

danielinux commented Apr 9, 2025

danielinux commented Apr 11, 2025

bigbrett left a comment • edited Loading

Choose a reason for hiding this comment

dgarske left a comment

Choose a reason for hiding this comment

bigbrett left a comment • edited Loading

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

bigbrett Apr 11, 2025

Choose a reason for hiding this comment

bigbrett Apr 11, 2025

Choose a reason for hiding this comment

billphipps commented Apr 12, 2025

danielinux commented Apr 14, 2025

danielinux commented Apr 14, 2025

bigbrett commented Apr 15, 2025

bigbrett left a comment

Choose a reason for hiding this comment

bigbrett Apr 11, 2025

Choose a reason for hiding this comment

bigbrett Apr 15, 2025

Choose a reason for hiding this comment

bigbrett Apr 15, 2025

Choose a reason for hiding this comment

Scatter/gather ELF files according to segments `paddr` #562

Scatter/gather ELF files according to segments `paddr` #562

bigbrett left a comment •

edited

Loading

bigbrett left a comment •

edited

Loading