Fix plugging alignment holes for encrypted binaries (#227)

Use new `segment_from_section` function, as virtual addresses are not unique per segment in encrypted binaries

Fixes #224

Author: will-v-pi

.github/workflows/test.yml

@@ -71,3 +71,33 @@ jobs:
           picotool info -a blink.uf2
           picotool info -a hello_world.uf2
           picotool info -a flash_nuke.uf2
+
+  test-examples:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+      - name: Install dependencies
+        run: sudo apt install cmake ninja-build python3 build-essential gcc-arm-none-eabi libnewlib-arm-none-eabi libstdc++-arm-none-eabi-newlib libusb-1.0-0-dev
+      - name: Checkout Pico SDK
+        uses: actions/checkout@v4
+        with:
+          repository: raspberrypi/pico-sdk
+          ref: develop
+          path: pico-sdk
+          submodules: 'recursive'
+      - name: Build and Install
+        run: |
+          cmake -S . -B build -G "Ninja" -D PICO_SDK_PATH="${{ github.workspace }}/pico-sdk"
+          cmake --build build
+          sudo cmake --install build
+      - name: Checkout Pico Examples
+        uses: actions/checkout@v4
+        with:
+          repository: raspberrypi/pico-examples
+          ref: develop
+          path: pico-examples
+      - name: Build Pico Examples
+        run: |
+          cmake -S pico-examples -B build-examples -G "Ninja" -D PICO_SDK_PATH="${{ github.workspace }}/pico-sdk" -D PICO_BOARD=pico2_w
+          cmake --build build-examples

elf/elf_file.cpp

@@ -255,24 +255,27 @@ void elf_file::read_sh(void) {
 // This is necessary to ensure the whole segment contains data defined in sections, otherwise you end up
 // signing/hashing/encrypting data that may not be written, as many tools write in sections not segments
 void elf_file::remove_sh_holes(void) {
+    bool found_hole = false;
     for (int i=0; i+1 < sh_entries.size(); i++) {
         auto sh0 = &(sh_entries[i]);
         elf32_sh_entry sh1 = sh_entries[i+1];
         if (
             (sh0->type == SHT_PROGBITS && sh1.type == SHT_PROGBITS)
             && (sh0->size && sh1.size)
             && (sh0->addr + sh0->size < sh1.addr)
-            && (segment_from_virtual_address(sh0->addr) == segment_from_virtual_address(sh1.addr))
-            && segment_from_virtual_address(sh0->addr) != NULL
+            && (segment_from_section(*sh0) == segment_from_section(sh1))
+            && segment_from_section(*sh0) != NULL
         ) {
             uint32_t gap = sh1.addr - sh0->addr - sh0->size;
             if (gap > sh1.addralign) {
-                fail(ERROR_INCOMPATIBLE, "Cannot plug gap greater than alignment - gap %d, alignment %d", gap, sh1.addralign);
+                fail(ERROR_INCOMPATIBLE, "Section %d: Cannot plug gap greater than alignment - gap %d, alignment %d", i, gap, sh1.addralign);
             }
             if (verbose) printf("Section %d: Moving end from 0x%08x to 0x%08x to plug gap\n", i, sh0->addr + sh0->size, sh1.addr);
             sh0->size = sh1.addr - sh0->addr;
+            found_hole = true;
         }
     }
+    if (found_hole) read_sh_data();
 }
 
 // Read the section data from the internal byte array into discrete sections.
@@ -290,7 +293,7 @@ void elf_file::read_sh_data(void) {
 }
 
 const std::string elf_file::section_name(uint32_t sh_name) const {
-    if (!eh.sh_str_index || eh.sh_str_index > eh.sh_num)
+    if (!eh.sh_str_index || eh.sh_str_index > eh.sh_num || eh.sh_str_index >= sh_data.size())
         return "";
 
     if (sh_name > sh_data[eh.sh_str_index].size())
@@ -394,9 +397,6 @@ int elf_file::read_file(std::shared_ptr<std::iostream> file) {
         if (!rc) {
             read_ph();
             read_sh();
-
-            // Remove any holes in the ELF file, as these cause issues when signing/hashing/encrypting
-            remove_sh_holes();
         }
         read_sh_data();
     }
@@ -475,6 +475,16 @@ const elf32_ph_entry* elf_file::segment_from_virtual_address(uint32_t vaddr) {
     return NULL;
 }
 
+const elf32_ph_entry* elf_file::segment_from_section(const elf32_sh_entry &sh) {
+    for (int i = 0; i < eh.ph_num; i++) {
+        if (sh.offset >= ph_entries[i].offset && sh.offset < ph_entries[i].offset + ph_entries[i].filez) {
+            if (verbose) printf("segment %d contains section %s\n", i, section_name(sh.name).c_str());
+            return &ph_entries[i];
+        }
+    }
+    return nullptr;
+}
+
 // Appends a new segment and section - filled with zeros
 // Use content to replace the content
 const elf32_ph_entry& elf_file::append_segment(uint32_t vaddr, uint32_t paddr, uint32_t size, const std::string &name) {

elf/elf_file.h

@@ -43,9 +43,11 @@ class elf_file {
     const std::string section_name(uint32_t sh_name) const;
     const elf32_ph_entry* segment_from_physical_address(uint32_t paddr);
     const elf32_ph_entry* segment_from_virtual_address(uint32_t vaddr);
+    const elf32_ph_entry* segment_from_section(const elf32_sh_entry &sh);
     void dump(void) const;
 
     void move_all(int dist);
+    void remove_sh_holes(void);
 
     static std::vector<uint8_t> read_binfile(std::shared_ptr<std::iostream> file);
 
@@ -54,7 +56,6 @@ class elf_file {
     int read_header(void);
     void read_ph(void);
     void read_sh(void);
-    void remove_sh_holes(void);
     void read_sh_data(void);
     void read_bytes(unsigned offset, unsigned length, void *dest);
     uint32_t append_section_name(const std::string &sh_name_str);

main.cpp

@@ -4827,6 +4827,8 @@ bool encrypt_command::execute(device_map &devices) {
         elf_file source_file(settings.verbose);
         elf_file *elf = &source_file;
         elf->read_file(get_file(ios::in|ios::binary));
+        // Remove any holes in the ELF file, as these cause issues when encrypting
+        elf->remove_sh_holes();
 
         std::unique_ptr<block> first_block = find_first_block(elf);
         if (!first_block) {
@@ -5063,6 +5065,8 @@ bool seal_command::execute(device_map &devices) {
         elf_file source_file(settings.verbose);
         elf_file *elf = &source_file;
         elf->read_file(get_file(ios::in|ios::binary));
+        // Remove any holes in the ELF file, as these cause issues when signing/hashing
+        elf->remove_sh_holes();
         sign_guts_elf(elf, private_key, public_key);
 
         auto out = get_file_idx(ios::out|ios::binary, 1);