Skip to content

Fix read_priv crash on a lookup miss and reject truncated AVM packs - #2340

Merged
bettio merged 3 commits into
atomvm:release-0.7from
bettio:fix-read_priv
Oct 26, 2026
Merged

bettio merged 3 commits into
atomvm:release-0.7from
bettio:fix-read_priv

Conversation

@bettio

@bettio bettio commented Jun 22, 2026 •

Copy link
Copy Markdown
Collaborator

The packbeam (.avm) scanners were unbounded and trusted every section
size. They stopped only on a header with flags 0, so when the end marker
was not inside the mapped region (an image truncated or overflowed by
its partition, or a corrupt header) a lookup miss, e.g.
atomvm:read_priv/2 for a file that is not packed, walked into erased
flash, took 0xFFFFFFFF for a section size and crashed the VM
(LoadProhibited on ESP32). The scanners also reported the whole section
size as the payload size, so code:get_object_code/1 returned the module
followed by the next section's header and name, and read_priv trusted
the length prefix of a priv entry, so a corrupt entry produced a binary
extending past the pack.

Bound every scan with the pack size, validate every section header,
return the payload size, and reject truncated or incomplete packs when
they are loaded. No on-disk format change: packs produced by
atomvm_packbeam load unchanged.

Highlights:

  • Keep the pack size in AVMPackData and bound/validate every section
    header in find_by_name/_by_flag/fold; a miss on a damaged pack ends
    the walk instead of leaving it
  • Return the section payload size, so code:get_object_code/1 returns
    exactly the module bytes
  • Reject truncated packs at load: a pack of known size (files,
    in-memory binaries, the embedded escript pack) must end with the
    terminator, checked from its tail so memory-mapped packs stay lazily
    paged; a pack in a region (partitions, rp2/stm32 flash) is walked to
    its terminator to find its real size; sizes above 32 bits and data
    after the terminator are rejected
  • Bound read_priv's length prefix (read unaligned) and raise invalid_avm
    on a corrupt entry; a missing resource still returns undefined
  • Add tests/test-avmpack.c, run it in CI and extend
    test-jit_stream_flash

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

@petermm

This comment was marked as outdated.

@petermm

This comment was marked as outdated.

@bettio
bettio force-pushed the fix-read_priv branch 2 times, most recently from 22d6864 to 2a6c79b Compare July 22, 2026 11:41
@petermm

This comment was marked as outdated.

@petermm

This comment was marked as outdated.

@bettio bettio changed the title Fix atomvm:read_priv and detect truncated packs Bound avmpack scanning and detect truncated packs Oct 25, 2026
@bettio

bettio commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Going to push the updated PR, that address the previous review comments.

JIT mistakes ordinary data sections for the end marker
Fixed.

A malformed zero-sized marker can trigger an out-of-bounds string read
Fixed.

avmpack_is_complete() accepts a disconnected tail marker
Not Fixed. This is deliberate. avmpack_is_complete() reads the header and the last 16 bytes only, so that a memory-mapped pack stays lazily paged: generic_unix mmaps the .avm and must not touch the whole file at startup.

Exact file sizes are narrowed before validation
Fixed.

Compare pack addresses as integers in JIT
Fixed.

@petermm

petermm commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

lgtm, AMP found one issue, I made it do test and fix:

Review: AtomVM PR #2340

Findings, line numbers, and proposed diffs refer to the reviewed head above.

P1 — ESP32/LX6 rejection releases DBUS while retaining a stale executable translation

Location: src/platforms/esp32/components/avm_sys/sys.c:458–467.

The new invalid-pack and allocation-failure paths call spi_flash_munmap(part_handle), but this is only the data-bus (DBUS) mapping. On Xtensa with AVM_NO_JIT undefined, esp32_sys_mmap_partition() has already created an instruction-bus (IBUS) mapping and stored its DBUS→IBUS translation in s_xtensa_part_mappings (sys.c:382–402). Neither that translation nor its IBUS handle is removed on rejection.

sys_map_native_code() searches these records oldest-first and uses the first containing range (sys.c:951–957). Consequently, releasing the DBUS window without removing its translation makes the following sequence unsafe:

  1. Map partition A. Its IBUS mapping and translation are registered.
  2. Reject A as corrupt/truncated, or fail to allocate its pack wrapper. The new cleanup releases A's DBUS mapping only.
  3. Map a different, valid partition B into the released DBUS window.
  4. Resolve B's native code. The oldest matching translation points into A's instruction mapping, not B's.

Impact: On classic ESP32/LX6, DBUS and IBUS use separate allocation regions, so retaining A's IBUS mapping does not keep its DBUS window occupied. If that DBUS window is reused, native execution can jump into the wrong partition and crash or execute incorrect code. Independently of address reuse, rejection retains the IBUS mapping and translation record. Do not generalize this allocation explanation to every Xtensa chip: ESP32-S3 has a combined executable/readable region, and SDK configuration matters.

Evidence boundary: The mapping registration, incomplete cleanup, and oldest-first lookup are established by source inspection. Actual MMU-window reuse and wrong-code execution were not reproduced on ESP32 hardware. The failure scenario is conditional on that reuse. The successful-close destructor has a related pre-existing lifecycle issue; the new rejection cleanup introduces another path that releases DBUS while retaining the translation.

Suggested fix

Associate each ordinary translation with the exact partition mmap wrapper handle that registered it. Forget that wrapper's translation before releasing DBUS, under s_xtensa_part_mutex. Do not delete records merely because their DBUS addresses match: separate wrappers may share an address.

Preserve IBUS mappings for VM lifetime. In the inspected ESP-IDF implementations, enclosed duplicate mappings can return a borrowed virtual address without acquiring an independent MMU reference. Releasing the original IBUS mapping could invalidate another wrapper's borrowed address, including the JIT cache's permanent globals. The proposed fix therefore removes stale translations without adding IBUS unmaps; it is not a complete mapping-lifetime repair.

Relevant ESP-IDF v5.5.5 source:

The two patches below together contain the complete local fix and regression test
against the reviewed head: esp32_sys.h, sys.c, and test_main.c. No additional
source files, CMake changes, or partition-table changes are needed. The separate
non-blocking documentation suggestion later in this review is not part of this fix.

Implementation patch

This includes the declaration, handle association, helper implementation,
rejection/destructor call sites, and explicit initialization of ownerless fallback
records:

--- a/src/platforms/esp32/components/avm_sys/include/esp32_sys.h
+++ b/src/platforms/esp32/components/avm_sys/include/esp32_sys.h
@@ -101,4 +101,6 @@
 const void *esp32_sys_mmap_partition(
     const char *partition_name, spi_flash_mmap_handle_t *handle, int *size);
 
+void esp32_sys_munmap_partition(spi_flash_mmap_handle_t handle);
+
 #endif
--- a/src/platforms/esp32/components/avm_sys/sys.c
+++ b/src/platforms/esp32/components/avm_sys/sys.c
@@ -342,6 +342,9 @@
     uintptr_t ibus_base;
     size_t size;
     spi_flash_mmap_handle_t ibus_handle;
+    // Associates this translation with a wrapper, not an independent MMU reference.
+    bool has_partition_handle;
+    spi_flash_mmap_handle_t partition_handle;
 };
 static struct xtensa_part_mapping *s_xtensa_part_mappings = NULL;
 static int s_xtensa_part_count = 0;
@@ -398,6 +401,8 @@
                 s_xtensa_part_mappings[s_xtensa_part_count].ibus_base = (uintptr_t) ibus_ptr;
                 s_xtensa_part_mappings[s_xtensa_part_count].size = partition->size;
                 s_xtensa_part_mappings[s_xtensa_part_count].ibus_handle = ibus_handle;
+                s_xtensa_part_mappings[s_xtensa_part_count].has_partition_handle = true;
+                s_xtensa_part_mappings[s_xtensa_part_count].partition_handle = *handle;
                 s_xtensa_part_count++;
                 pthread_mutex_unlock(&s_xtensa_part_mutex);
             }
@@ -411,6 +416,29 @@
     return mapped_memory;
 }
 
+void esp32_sys_munmap_partition(spi_flash_mmap_handle_t handle)
+{
+#if !defined(CONFIG_IDF_TARGET_ARCH_RISCV) && !defined(AVM_NO_JIT)
+    pthread_mutex_lock(&s_xtensa_part_mutex);
+    for (int i = 0; i < s_xtensa_part_count; i++) {
+        struct xtensa_part_mapping *mapping = &s_xtensa_part_mappings[i];
+        if (mapping->has_partition_handle && mapping->partition_handle == handle) {
+            // Forget this wrapper's translation before its DBUS address can be reused.
+            // Keep IBUS handles alive as before: another mmap wrapper (including the
+            // JIT cache) may borrow the same MMU block without retaining a reference.
+            memmove(mapping, mapping + 1,
+                (s_xtensa_part_count - i - 1) * sizeof(*mapping));
+            s_xtensa_part_count--;
+            break;
+        }
+    }
+    spi_flash_munmap(handle);
+    pthread_mutex_unlock(&s_xtensa_part_mutex);
+#else
+    spi_flash_munmap(handle);
+#endif
+}
+
 struct ESP32PartAVMPack
 {
     struct AVMPackData base;
@@ -428,7 +456,7 @@
     UNUSED(global);
 
     struct ESP32PartAVMPack *part_avm = CONTAINER_OF(obj, struct ESP32PartAVMPack, base);
-    spi_flash_munmap(part_avm->part_handle);
+    esp32_sys_munmap_partition(part_avm->part_handle);
     free(obj);
 }
 
@@ -457,13 +485,13 @@
         }
         uint32_t avm_size;
         if (UNLIKELY(!avmpack_compute_size(part_data, size, &avm_size))) {
-            spi_flash_munmap(part_handle);
+            esp32_sys_munmap_partition(part_handle);
             return AVM_OPEN_INVALID;
         }
 
         struct ESP32PartAVMPack *part_avm = malloc(sizeof(struct ESP32PartAVMPack));
         if (IS_NULL_PTR(part_avm)) {
-            spi_flash_munmap(part_handle);
+            esp32_sys_munmap_partition(part_handle);
             return AVM_OPEN_FAILED_ALLOC;
         }
         avmpack_data_init(&part_avm->base, &esp32_part_avm_pack_info, avm_size);
@@ -994,6 +1022,8 @@
         s_xtensa_part_mappings[s_xtensa_part_count].ibus_base = (uintptr_t) ibus_ptr;
         s_xtensa_part_mappings[s_xtensa_part_count].size = map_size;
         s_xtensa_part_mappings[s_xtensa_part_count].ibus_handle = ibus_handle;
+        s_xtensa_part_mappings[s_xtensa_part_count].has_partition_handle = false;
+        s_xtensa_part_mappings[s_xtensa_part_count].partition_handle = 0;
         s_xtensa_part_count++;
         pthread_mutex_unlock(&s_xtensa_part_mutex);
         addr = (uintptr_t) ibus_ptr + offset_in_page;

Regression-test patch

The existing Unity runner discovers this test automatically. It uses the scratch
data and lib.avm partitions already present in partitions-test.csv; it erases
their first flash sectors and must only run with the test partition layout. Both
checks require actual DBUS reuse, so a simulator that does not reproduce reuse
cannot silently report success. The physical-address assertion fails on the
reviewed implementation and passes with the implementation patch above.

diff --git a/src/platforms/esp32/test/main/test_main.c b/src/platforms/esp32/test/main/test_main.c
--- a/src/platforms/esp32/test/main/test_main.c
+++ b/src/platforms/esp32/test/main/test_main.c
@@ -20,6 +20,7 @@
 
 #include "unity.h"
 #include <errno.h>
+#include <inttypes.h>
 #include <stdio.h>
 #include <string.h>
 
@@ -200,6 +201,62 @@
 }
 
 #ifndef AVM_NO_JIT
+#if CONFIG_IDF_TARGET_ESP32
+TEST_CASE("test_partition_native_mapping_reuse", "[test_run]")
+{
+    // These are scratch partitions in partitions-test.csv, not the JIT cache.
+    const esp_partition_t *rejected = esp_partition_find_first(
+        ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_ANY, "data");
+    const esp_partition_t *valid = esp_partition_find_first(
+        ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_ANY, "lib.avm");
+    TEST_ASSERT_NOT_NULL(rejected);
+    TEST_ASSERT_NOT_NULL(valid);
+    TEST_ESP_OK(esp_partition_erase_range(rejected, 0, SPI_FLASH_SEC_SIZE));
+    TEST_ESP_OK(esp_partition_erase_range(valid, 0, SPI_FLASH_SEC_SIZE));
+    static const uint8_t empty_pack[] = {
+        '#', '!', '/', 'u', 's', 'r', '/', 'b', 'i', 'n', '/', 'e', 'n', 'v', ' ', 'A',
+        't', 'o', 'm', 'V', 'M', '\n', 0, 0,
+        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 'e', 'n', 'd', 0
+    };
+    TEST_ESP_OK(esp_partition_write(valid, 0, empty_pack, sizeof(empty_pack)));
+
+    // Observe A's DBUS address without registering an AtomVM translation.
+    const void *rejected_dbus;
+    spi_flash_mmap_handle_t probe_handle;
+    TEST_ESP_OK(esp_partition_mmap(rejected, 0, rejected->size,
+        SPI_FLASH_MMAP_DATA, &rejected_dbus, &probe_handle));
+    spi_flash_munmap(probe_handle);
+
+    struct AVMPackData *pack = NULL;
+    TEST_ASSERT_EQUAL(AVM_OPEN_INVALID,
+        sys_open_avm_from_file(NULL, "/dev/partition/by-name/data", &pack));
+    TEST_ASSERT_EQUAL(AVM_OPEN_OK,
+        sys_open_avm_from_file(NULL, "/dev/partition/by-name/lib.avm", &pack));
+    TEST_ASSERT_NOT_NULL(pack);
+    bool reused = pack->data == rejected_dbus;
+    ModuleNativeEntryPoint entry = sys_map_native_code(pack->data, 4);
+    size_t physical = spi_flash_cache2phys((const void *) entry);
+    printf("Rejected partition: DBUS=%p; replacement: DBUS=%p native=%p physical=0x%x expected=0x%" PRIx32 "\n",
+        rejected_dbus, pack->data, (void *) entry, (unsigned) physical, valid->address);
+    avmpack_data_destroy(pack, NULL);
+    TEST_ASSERT_TRUE_MESSAGE(reused, "Test must exercise actual DBUS window reuse");
+    TEST_ASSERT_EQUAL_HEX32(valid->address, physical);
+
+    // Closing a valid pack must also forget its translation before DBUS reuse.
+    TEST_ESP_OK(esp_partition_write(rejected, 0, empty_pack, sizeof(empty_pack)));
+    TEST_ASSERT_EQUAL(AVM_OPEN_OK,
+        sys_open_avm_from_file(NULL, "/dev/partition/by-name/data", &pack));
+    reused = pack->data == rejected_dbus;
+    entry = sys_map_native_code(pack->data, 4);
+    physical = spi_flash_cache2phys((const void *) entry);
+    printf("After close: DBUS=%p native=%p physical=0x%x expected=0x%" PRIx32 "\n",
+        pack->data, (void *) entry, (unsigned) physical, rejected->address);
+    avmpack_data_destroy(pack, NULL);
+    TEST_ASSERT_TRUE_MESSAGE(reused, "Test must exercise DBUS reuse after close");
+    TEST_ASSERT_EQUAL_HEX32(rejected->address, physical);
+}
+#endif
+
 TEST_CASE("test_jit_compile", "[test_run]")
 {
     prepare_event_queue();

Test configuration: Build src/platforms/esp32/test for target esp32 with
CONFIG_JIT_ENABLED=y, CONFIG_ESPTOOLPY_FLASHSIZE_8MB=y, and
CONFIG_AVM_RTC_SLOW_MAX_SIZE=1024, retaining the existing test partition table.
The regression is intentionally excluded from interpreter-only and other-chip
builds. Use sim_boards/diagram.esp32.json with the existing Wokwi workflow in
src/platforms/esp32/test/README.md. These settings describe the tested local
configuration; they do not change the project's default JIT setting.

Scope and limitations:

  • IBUS handles/mappings intentionally remain allocated for VM lifetime, including those installed for rejected packs. Removing a translation discards its stored IBUS handle without unmapping it. This preserves existing retention behavior; it does not fix the residual IBUS leak.
  • Handle association is bookkeeping, not reference counting. Existing DBUS borrowed-mapping ownership hazards remain; the helper retains the current raw DBUS-unmap behavior.
  • Ownerless fallback/embedded registrations remain untouched. Rejected packs have not reached native-module loading, so this covers the new rejection path; it does not resolve every pre-existing fallback/close lifecycle.
  • Startup stays pinned. Leave main.c and jit_stream_flash_platform.c unchanged. In particular, the JIT platform's temporary raw DBUS unmap must not use this helper: that wrapper did not register an ordinary partition translation.
  • Release each handle once, with no concurrent use of the released pack. The helper does not support double-unmap or repair use-after-close races.
  • These patches are applied locally and remain uncommitted. Full ESP-IDF compilation and Wokwi validation are recorded in the follow-up below; physical-hardware validation remains outstanding.

Regression needed: Reject A, force B to reuse A's DBUS window, and verify that B's native entry point resolves to B's physical flash. Cover both validation failure and wrapper allocation failure; two registrations sharing a DBUS base but having different handles; ownerless fallback records; record-order preservation; and JIT-disabled/RISC-V controls. No new IBUS unmaps should occur. A host stub can check bookkeeping but cannot establish actual MMU reuse, SDK borrowed-address lifetimes, or native execution correctness.

Non-blocking — Describe tail-only validation accurately

avmpack_is_complete() intentionally checks the magic, size/alignment, and final marker without walking the section chain (src/libAtomVM/avmpack.c:237–257). The test at tests/test-avmpack.c:288–301 explicitly requires acceptance of a corrupt chain with a valid final marker.

This preserves lazy paging and is not another memory-safety blocker: bounded scanners stop at corrupt headers, while region loaders and JIT terminator discovery walk the chain. However, the PR description's claim that every pack is validated by walking the chain at load does not match exact-size loaders. The header's unconditional promise to reject trailing bytes is also stronger than the implementation.

A local probe using the public parser API produced:

valid: complete=1 walk=1 walk_size=40
trailing terminator: complete=1 walk=1 walk_size=40 exact_size=56
disconnected tail: complete=1 walk=0
oversized: complete=0

The second case is a valid 40-byte empty pack followed by another 16-byte terminator. The tail predicate accepts all 56 bytes, although the chain ends at byte 40. The third case has an oversized first section and an unreachable valid tail marker.

Preserve the chosen lazy-loading behavior, but narrow the documentation and PR description. For example:

--- a/src/libAtomVM/avmpack.h
+++ b/src/libAtomVM/avmpack.h
@@
- * known (a file size, an in-memory binary). Trailing bytes after the terminator and a size above
- * \c UINT32_MAX, which 32-bit section sizes cannot describe, are rejected.
+ * known (a file size, an in-memory binary). A missing final marker or a size above
+ * \c UINT32_MAX is rejected. This tail-only check does not prove that the final marker is
+ * reachable, or that no earlier terminator precedes trailing data.

Suggested PR-description wording:

Check exact-size files and binaries for a valid header and final terminator, preserving lazy paging. Walk and validate the section chain when only a containing region's upper bound is known. Bound every subsequent scan so corrupt section headers stop traversal safely.

Other safety checks in this push

Behavior Assessment
JIT terminator discovery Uses structural avmpack_find_terminator() and refuses append positioning when the walk fails. Current reference-packer data files use flag 4; flags-0 coverage is compatibility/hardening coverage, not an observed current-packer corruption case.
Terminator-name bounds Recognition checks exactly four bounded bytes, including the terminating NUL.
Exact-size pack validation Deliberately accepts a disconnected final marker; the documentation suggestion above makes this limitation explicit.
File/binary size validation Protected Unix, embedded, and binary ingress paths pass size_t to the validator, which rejects values above UINT32_MAX before narrowing. ESP32 filesystem st_size narrowing is pre-existing and outside this finding.

Verification performed

Run against the reviewed head in an isolated worktree:

cmake -S . -B /tmp/pr2340-build
cmake --build /tmp/pr2340-build --target test-avmpack test-jit_stream_flash -j4
/tmp/pr2340-build/tests/test-avmpack
/tmp/pr2340-build/tests/test-jit_stream_flash
git diff --check d4d4db553d41f5a83ae549b57e2cf26611eb80f6..HEAD
  • test-avmpack: All avmpack tests passed!
  • test-jit_stream_flash: All tests passed!, including a pack without a terminator and a data section before the terminator.
  • Diff whitespace check passed.
  • The Unix AtomVM executable also built successfully.
  • The temporary parser probe above confirmed the deliberate tail-only behavior and oversized-size rejection; its source and binary were removed afterward.
  • The FreeBSD 13.5 CI failure occurred during package installation: the FreeBSD package repository returned Not Found errors. Compilation had not started. Other CI jobs were still pending at inspection time; this is not an all-green CI claim.
  • The embedded implementation diff passes git apply --check against the reviewed head; it was not applied to the source checkout.
  • The exact helper body extracted from this document was compiled in a temporary host stub harness using cc -std=c11 -Wall -Wextra -Werror, in Xtensa, RISC-V, and JIT-disabled configurations. All three printed PASS: helper bookkeeping and DBUS-only release. Checks covered same-address/different-handle registrations, an ownerless record with a matching numeric handle, middle/last-record compaction, surviving-record order, missing handles, an empty table, and removal before the raw DBUS release. The harness issued no IBUS unmaps and was removed afterward.
  • These are helper bookkeeping checks, not ESP-IDF integration tests. No full ESP-IDF build of the proposed patch or ESP32 hardware/MMU lifecycle test was run.

Additional useful coverage, not additional blockers: read_priv/2 empty/exact-fit/short payload boundaries with in_use = false, and the payload-size correction through the user-visible code:get_object_code/1 result.

Delivery

At the time of the review above, the diffs were suggestions only. No GitHub review comments have been posted.

Local Wokwi follow-up — 2026-10-25

The proposed ESP32 translation cleanup is now applied locally, with a regression
in src/platforms/esp32/test/main/test_main.c named
test_partition_native_mapping_reuse. These changes are uncommitted and unpushed.
The reviewed upstream head itself is unchanged.

Built the full ESP32 test firmware using ESP-IDF v5.5.5 with
CONFIG_JIT_ENABLED=y, then ran it on Wokwi's classic ESP32/LX6 model. No host
MMU stubs were used. The regression opens partitions through
sys_open_avm_from_file, translates with sys_map_native_code, and independently
checks the SDK's spi_flash_cache2phys result against the partition table.

  • Before: A (data, physical 0x5d0000) was rejected. B (lib.avm, physical
    0x510000) reused A's actual DBUS address 0x3f610000, but translation returned
    A's physical 0x5d0000. The regression failed with expected 0x00510000, actual
    0x005D0000. Reconfirmed after removing the source fix with the final test config.
  • After: The same DBUS reuse translated to B's correct physical 0x510000.
    After closing B, reopening valid A reused that window and correctly translated
    to 0x5d0000. The regression passed on repeated simulator boots.
  • Controls: Existing test_jit_compile passed before and after. Host
    test-avmpack and test-jit_stream_flash passed. The changed ESP32 source and
    test also compiled with AVM_NO_JIT defined.
  • Broader suite: Twenty tests passed before the run stopped at
    test_wifi_example: the test image cannot resolve network.beam. Confirmed
    the same Wi-Fi failure without the mapping fix; not an all-green suite claim.
    An earlier scratch configuration used RTC capacity 4096 instead of the suite's
    required 1024; corrected that config and confirmed the RTC test passes.

Evidence and the local rerun script are in .amp/in/artifacts/pr2340-wokwi/:
before-control.serial.log, after-full.serial.log, final.serial.log,
host-tests.log, and run-smoke.sh. The smoke script stops after the mapping
regression and existing JIT execution control, before the unrelated Wi-Fi failure.
The build/configuration remain under .amp/in/pr2340-wokwi/ for reruns.

Evidence boundary: Actual DBUS reuse and incorrect/correct physical native
translation are now reproduced in simulation with the real SDK. This does not
execute native instructions from the replacement empty pack, test physical
hardware, inject wrapper-allocation failure, or validate RISC-V at runtime.
IBUS mappings intentionally remain retained, as described in the suggested fix.

@bettio bettio changed the title Bound avmpack scanning and detect truncated packs Fix read_priv crash on a lookup miss and reject truncated AVM packs Oct 26, 2026
@bettio

bettio commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

I realized that while addressing the issues I was finding the PR scope was expanding. I pushed again, less commits and smaller scope. There will be a follow up PR after this.

@petermm petermm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, AMP only had this extreme nitpick - do ship!

Small non-blocking documentation nit: the tail-only check cannot rule out an earlier terminator followed by trailing data ending in another valid marker. No behavior change needed:

--- a/src/libAtomVM/avmpack.h
+++ b/src/libAtomVM/avmpack.h
@@
  * @details Reads the header and the last 16 bytes only, so a memory-mapped pack stays lazily
- * paged. Trailing bytes after the terminator and sizes above \c UINT32_MAX are rejected.
+ * paged. A missing final marker or a size above \c UINT32_MAX is rejected. This does not
+ * validate the section chain or rule out an earlier terminator followed by trailing data.

The packbeam scanners were unbounded and trusted every section size,
stopping only on a header with flags 0. When the end marker was not in
the mapped region (an image truncated or overflowed by its partition,
or a corrupt header) a lookup miss, e.g. read_priv for a file that is
not packed, walked into erased flash, took 0xFFFFFFFF for a section
size and crashed.

Keep the pack size in AVMPackData, bound every scan with it and
validate each header, so a miss on a damaged pack ends the walk. The
end marker is matched by its zero size and "end" name in any case, as
the JIT flash cache rewrites it to "END" in place. On rp2 the bound is
the 16 MiB XIP window rather than XIP_SRAM_BASE, which spans past the
flash. A pack whose marker is not found no longer provides the JIT
cache with an append position.

The scanners also reported the whole section size as the payload size,
so code:get_object_code/1 returned the module followed by the next
header. They now return the payload size.

Signed-off-by: Davide Bettio <davide@uninstall.it>
Only the 24-byte magic header was checked at load, so a pack larger
than the region holding it, e.g. an image flashed to a too-small
partition, loaded and failed late: every lookup past the truncation
point missed, so modules failed to load and read_priv returned
undefined, and the JIT flash cache, finding no end marker, refused to
append native code.

Reject such packs when they are loaded. When the exact size is known
(a file, a binary, the escript pack) the pack must end with the end
marker; only the tail is read, so a memory-mapped pack stays lazily
paged. When only an upper bound is known (partitions, rp2 and stm32
flash) the section chain is walked to the marker, which also gives the
scanners the real size. Trailing data and sizes above 32 bits are
rejected.

Signed-off-by: Davide Bettio <davide@uninstall.it>
A priv entry is <<Size:32, Data>> and read_priv trusted Size, so a
corrupt prefix produced a binary extending past the entry. Check it
against the section payload and raise invalid_avm when it does not
fit; a resource that is not in any pack still returns undefined.

The prefix is read unaligned: a pack kept in place from a const
binary, such as a module literal, may sit at any byte offset.

Signed-off-by: Davide Bettio <davide@uninstall.it>
@bettio
bettio merged commit 7138b3b into atomvm:release-0.7 Oct 26, 2026
174 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants