Fix encryption when using clang, and with allocatable heap (#243)

* Fix encryption when using clang, and with allocatable heap

* Test build examples with clang too

* Delete empty PH segments when moving

These segments are not necessary, and will likely be moved to invalid memory addresses

Author: will-v-pi

.github/workflows/test.yml

@@ -76,12 +76,22 @@ jobs:
     # Prevent running twice for PRs from same repo
     if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name
     name: Test Build Examples
+    strategy:
+      fail-fast: false
+      matrix:
+        compiler: ["gcc", "clang"]
     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
+        run: sudo apt install cmake ninja-build python3 build-essential libusb-1.0-0-dev
+      - name: Install GCC
+        if: matrix.compiler == 'gcc'
+        run: sudo apt install gcc-arm-none-eabi libnewlib-arm-none-eabi libstdc++-arm-none-eabi-newlib
+      - name: Install Clang
+        if: matrix.compiler == 'clang'
+        uses: stellar-aria/llvm-embedded-toolchain-for-arm-action@latest
       - name: Checkout Pico SDK
         uses: actions/checkout@v4
         with:
@@ -102,5 +112,5 @@ jobs:
           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 -S pico-examples -B build-examples -G "Ninja" -D PICO_SDK_PATH="${{ github.workspace }}/pico-sdk" -D PICO_BOARD=pico2_w ${{ matrix.compiler == 'clang' && '-D PICO_COMPILER=pico_arm_clang' || '' }}
           cmake --build build-examples

bintool/bintool.cpp

@@ -263,6 +263,7 @@ block place_new_block(elf_file *elf, std::unique_ptr<block> &first_block, bool s
         // std::cout << &seg << " " << to_string(seg) << std::endl;
         const uint32_t paddr = seg.physical_address();
         const uint32_t psize = seg.physical_size();
+        if (psize == 0) continue;
         if (paddr >= 0x20000000 && paddr < 0x20080000) {
             highest_ram_address = std::max(paddr + psize, highest_ram_address);
         } else if (paddr >= 0x10000000 && paddr < 0x20000000) {
@@ -406,6 +407,9 @@ std::vector<std::unique_ptr<block>> get_all_blocks(std::vector<uint8_t> &bin, ui
                 next_item += size;
             }
         }
+        if (new_first_block == nullptr) {
+            fail(ERROR_UNKNOWN, "Block loop is not valid - incomplete block found at %08x\n", (int)(next_block_addr));
+        }
         if (new_first_block->physical_addr + new_first_block->next_block_rel == first_block->physical_addr) {
             DEBUG_LOG("Found last block in block loop\n");
             all_blocks.push_back(std::move(new_first_block));

elf/elf_file.cpp

@@ -251,7 +251,33 @@ void elf_file::read_sh(void) {
     }
 }
 
-// If there are holes between sections within segments, increase the section size to plug the hole
+// If there are holes between segments, increase the segment size to plug the hole
+// This is necessary to ensure that segments are contiguous, which is required for encrypting,
+// but this is not necessary for signing/hashing
+void elf_file::remove_ph_holes(void) {
+    for (int i=0; i+1 < ph_entries.size(); i++) {
+        auto ph0 = &(ph_entries[i]);
+        elf32_ph_entry ph1 = ph_entries[i+1];
+        if (
+            (ph0->type == PT_LOAD && ph1.type == PT_LOAD)
+            && (ph0->filez && ph1.filez)
+            && (ph0->paddr + ph0->filez < ph1.paddr)
+        ) {
+            uint32_t gap = ph1.paddr - ph0->paddr - ph0->filez;
+            if (gap > ph1.align) {
+                fail(ERROR_INCOMPATIBLE, "Segment %d: Cannot plug gap greater than alignment - gap %d, alignment %d", i, gap, ph1.align);
+            }
+            if (ph0->offset + ph0->filez + gap > ph1.offset) {
+                fail(ERROR_INCOMPATIBLE, "Segment %d: Cannot plug gap without space in file - gap %d", i, gap);
+            }
+            if (verbose) printf("Segment %d: Moving end from 0x%08x to 0x%08x to plug gap\n", i, ph0->paddr + ph0->filez, ph1.paddr);
+            ph0->filez = ph1.paddr - ph0->paddr;
+            ph0->memsz = ph0->filez;
+        }
+    }
+}
+
+// If there are holes within segments, increase the section size to plug the hole
 // 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) {
@@ -273,11 +299,31 @@ void elf_file::remove_sh_holes(void) {
             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;
+        } else if (
+            (sh0->type == SHT_PROGBITS)
+            && (segment_from_section(*sh0) != segment_from_section(sh1))
+            && segment_from_section(*sh0) != NULL
+            && (sh0->offset + sh0->size < segment_from_section(*sh0)->offset + segment_from_section(*sh0)->filez)
+        ) {
+            const elf32_ph_entry *seg = segment_from_section(*sh0);
+            uint32_t gap = seg->offset + seg->filez - sh0->offset - sh0->size;
+            if (verbose) printf("Section %d: Moving end from 0x%08x to 0x%08x to plug gap at end of segment\n", i, sh0->addr + sh0->size, seg->offset + seg->filez);
+            sh0->size = seg->offset + seg->filez - sh0->offset;
+            found_hole = true;
         }
     }
     if (found_hole) read_sh_data();
 }
 
+void elf_file::remove_empty_ph_entries(void) {
+    for (int i = 0; i < ph_entries.size(); i++) {
+        if (ph_entries[i].filez == 0) {
+            ph_entries.erase(ph_entries.begin() + i);
+            eh.ph_num--; i--;
+        }
+    }
+}
+
 // Read the section data from the internal byte array into discrete sections.
 // This is used after modifying segments but before inserting new segments
 void elf_file::read_sh_data(void) {
@@ -373,6 +419,8 @@ void elf_file::dump(void) const {
 
 void elf_file::move_all(int dist) {
     if (verbose) printf("Incrementing all paddr by %d\n", dist);
+    // Remove empty ph entries, as they will likely be moved to invalid addresses
+    remove_empty_ph_entries();
     for (auto &ph: ph_entries) {
         ph.paddr += dist;
     }

elf/elf_file.h

@@ -47,7 +47,9 @@ class elf_file {
     void dump(void) const;
 
     void move_all(int dist);
+    void remove_ph_holes(void);
     void remove_sh_holes(void);
+    void remove_empty_ph_entries(void);
 
     static std::vector<uint8_t> read_binfile(std::shared_ptr<std::iostream> file);
 

main.cpp

@@ -5218,6 +5218,7 @@ bool encrypt_command::execute(device_map &devices) {
         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_ph_holes();
         elf->remove_sh_holes();
 
         std::unique_ptr<block> first_block = find_first_block(elf);