security: harden application slot boundary validation

[Mohammed18-19]

Summary

Harden application slot boundary validation in eboot_jump_to_app().

The validation now ensures that:

• the actual slot size is non-zero.
• the image header fits within the slot.
• the image payload fits within the remaining slot space.
• payload integrity verification cannot read beyond the selected application slot.

Added a direct regression test for eboot_jump_to_app() covering both oversized and in-bounds images.

Also clarified the jump comment to note that entry_addr is authenticated as part of the signed image header.

Validation

• cmake --build build — passed
• ctest --test-dir build --output-on-failure — 19/19 passed
• git diff --check — passed

The existing duplicated bounds checks in core/slot_manager.c and core/recovery.c are intentionally left unchanged; extracting a shared helper can be handled separately to avoid unrelated refactoring in this security behavior change.

[srpatcha]

Review — eBoot#96 "security: harden application slot boundary validation"

head: 2acb082 author: Mohammed18-19 ci: fail

Verdict: Against current master the only new code here is the regression test plus a comment rewording — the slot-bounds check the title and body describe is already on master. The test itself is sound and passes locally (19/19), but the body reads as if unmerged hardening is being added, and the equivalent bound is still missing on the core/secure_boot.c entry point.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	core/secure_boot.c:82-134	The hardening this PR is titled for exists only on the stage1/jump_app.c path. eos_secure_boot_verify() parses the header at cfg->image_addr and then runs eos_image_verify_integrity() (line 90) and eos_rollback_read_image_counter() (line 134) with no slot-size bound at all. eos_image_verify_integrity() bounds image_size only against a flat 16 MiB cap (core/image_verify.c:95), not against the slot. So the same over-read the PR closes in jump_app.c is still reachable through secure boot.	Hoist the check into one helper (e.g. eos_image_fits_slot(const eos_image_header_t*, uint32_t slot_size)) and call it from jump_app.c, secure_boot.c, core/slot_manager.c and core/recovery.c. The PR body already notes the duplicated checks in the latter two — this is the fourth copy site, and the one with no check.
2	Medium	stage1/jump_app.c:34-41	The bound is hdr_size + image_size <= slot_size. On current master an image is header + payload + TLV area: core/rollback.c:69-96 reads the TLV at image_addr + hdr_size + image_size for hdr.tlv_len bytes. tlv_len is capped at EOS_TLV_MAX_SIZE (512, include/eos_image_tlv.h:50) and checked for wraparound, but never against the slot end, so a header can still place an authenticated-looking TLV read up to 512 bytes past the selected slot. Not reachable from jump_app.c today (that path calls eos_image_check_rollback(), which reads only the HW counter), but it is reachable from finding 1's path.	Add tlv_len to the arithmetic in the shared helper: reject when (uint32_t)hdr.tlv_len > slot_size - hdr.hdr_size - hdr.image_size. Add a test_jump_app_bounds.c case with tlv_len pointing past the slot end.
3	Medium	pr.json body / title	The body presents the four bounds properties as the change. Three of them are already on master: git diff origin/master..2acb082 -- stage1/jump_app.c is only the comment rewording at lines 80-86. The check itself landed via eBoot#76 (42c3f226), which is on this branch but reached master separately; the branch is 16 commits behind. A reviewer reading the body will believe they are approving unmerged hardening.	Retitle to what is actually new (test: cover jump-app slot boundary validation), rebase onto master, and reduce the body to the test plus the comment change.
4	Low	tests/CMakeLists.txt:45-47	target_link_libraries(eboot_test_jump_app_bounds PRIVATE eboot_core eboot_stage1) reintroduces the pattern master removed three commits ago with a recorded reason — master's tests/CMakeLists.txt now reads PRIVATE eboot_stage1 for test_recovery with the comment "eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on the link line twice … the linker reported the duplicate." Verified it links clean here with GNU ld 2.x on Linux, so this is a convention regression, not a break.	target_link_libraries(eboot_test_jump_app_bounds PRIVATE eboot_stage1).
5	Low	tests/unit/test_jump_app_bounds.c:96-98	sim_flash_read() attributes any read in [SLOT_A_ADDR + sizeof(hdr), SLOT_B_ADDR) to payload_bytes_read. That window is the whole gap between the slots, not slot A, and the exact-equality assert at line 240 (payload_bytes_read == image_size) additionally pins the CRC32 path to reading the payload in exactly one pass. Both make the test fail for reasons unrelated to the bound it exists to protect.	Bound the accounting window to [SLOT_A_ADDR + hdr_size, SLOT_A_ADDR + SLOT_A_SIZE) and relax line 240 to payload_bytes_read >= image_size — or better, assert the read never leaves the slot, which is the actual property.

CI

Four required checks are red: Host Build & Tests, Build & Test (Linux x86_64), Cross-compile STM32F4, Analyze (C/C++).

None of them are this PR's fault. origin/master (22d8f8b) does not compile. Verified by building it in a clean worktree:

• include/eos_image.h:135 and :142 assert on eos_image_header_t::reserved, which no longer exists — master replaced reserved[30] with tlv_len + tlv_hash[28] and left the two old asserts behind. → error: 'eos_image_header_t' has no member named 'reserved'
• core/ed25519_verify.c:281 and :338 both define point_is_identity, and key_has_prime_order / public_key_is_valid_subgroup are two copies of the same subgroup check. → error: redefinition of 'point_is_identity'
• tests/unit/test_ed25519.c:31,263 redefine run_test_ed25519_identity_key_forgery_rejected / test_ed25519_identity_key_forgery_rejected, and lines 299-300 reference k_low_order / messages that are not in scope.

All three are one bad merge of two competing subgroup-check PRs. eBoot#94 ("fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken") already targets exactly this, so no fix PR was opened from here. #96 cannot go green until #94 lands.

The PR head alone builds and tests clean — verified:

cmake -S . -B bt -DEBLDR_BUILD_TESTS=ON && cmake --build bt   # rc=0
ctest --test-dir bt --no-tests=error                          # 19/19 passed, test_jump_app_bounds included

Note for the body's validation block: ctest finds no tests at all without -DEBLDR_BUILD_TESTS=ON (default OFF, CMakeLists.txt:23) and still exits 0 on a plain cmake -B build. The quoted 19/19 is real, but only with that flag — state it, or the next person reproduces a green run that measured nothing.

Architecture conformance

Conforms. §8 / §8.1 — bounds-checking a slot before streaming payload bytes out of it is stage-1 "verify manifest → verify image → slot selection" work, and it stays in stage1/. No new dependency: stage1/jump_app.c gains only a call to eos_hal_slot_size(), which is a downward call into the HAL, and the test links eboot_stage1 → eboot_core → eboot_hal, the direction .ai/architect.md requires. §5.1 satisfied — nothing points up a tier. Tier 1 (§21), correct repo. The per-repo layout in .ai/architect.md is respected: boot logic in stage1/, test in tests/unit/.

The duplication in finding 1 is the architectural point worth acting on: four call sites, three with a hand-copied bound and one with none, is how the fourth site stays wrong. That is a shared-helper problem, not a per-PR problem.

Proposed changes

Smallest sequence that keeps everything building:

1. Land eBoot#94 first. Nothing here can be verified in CI until master compiles.
2. Rebase this branch onto master and retitle per finding 3. The rebase drops the already-merged 42c3f226 and leaves the test plus the comment.
3. tests/CMakeLists.txt: drop eboot_core from the new link line (finding 4).
4. Tighten the test's read accounting (finding 5).
5. Separate PR, not this one: extract eos_image_fits_slot() and call it from all four sites, tlv_len included (findings 1 and 2). This is a behaviour change on the secure-boot path and belongs on its own, with its own test.

Not checked

• Nothing was run on hardware. The 19/19 result is host x86-64 only; Cross-compile STM32F4 and Cross-compile ARM Cortex-M4 were not reproduced locally, so the change is unverified under arm-none-eabi.
• The merged result was not fully built. I merged 2acb082 into master locally (clean, no conflicts) but could not complete the build past master's three pre-existing compile errors, so CI green after #94 lands is unverified — in particular finding 4's link line was proven only at the PR head, not in the merge.
• Analyze (C/C++) and CodeQL findings were not reviewed; CodeQL reports skipping and the Analyze job failed at the build step, so neither produced results for this head.
• No fuzz coverage exists for eboot_jump_to_app(). .ai/security.md requires fuzzing for externally reachable parsers, and the image header is one. This PR adds two unit cases, not fuzz coverage; the gap is pre-existing and not assessed further.
• existing-comments.txt is empty for this head, so no prior reviewer point was checked for overlap beyond that.

---

Automated architecture review of 2acb082156a1 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[Mohammed18-19]

Thanks for the detailed review. I’ll follow the proposed sequence: wait for eBoot#94 to land first, then rebase #96 onto the updated master, retitle it to focus on the regression test, and address the test/linking cleanup. I’ll keep the shared eos_image_fits_slot() helper and secure-boot/TLV hardening as a separate PR. Le jeu. 3 sept. 2026 à 16:25, Srikanth Patchava ***@***.***> a écrit :

…

***@***.**** commented on this pull request. Review — eBoot#96 "security: harden application slot boundary validation" head: 2acb082 <2acb082> author: Mohammed18-19 ci: fail *Verdict:* Against current master the only new code here is the regression test plus a comment rewording — the slot-bounds check the title and body describe is already on master. The test itself is sound and passes locally (19/19), but the body reads as if unmerged hardening is being added, and the equivalent bound is still missing on the core/secure_boot.c entry point. Findings # Severity File:line Finding Recommended fix 1 Medium core/secure_boot.c:82-134 The hardening this PR is titled for exists only on the stage1/jump_app.c path. eos_secure_boot_verify() parses the header at cfg->image_addr and then runs eos_image_verify_integrity() (line 90) and eos_rollback_read_image_counter() (line 134) with *no* slot-size bound at all. eos_image_verify_integrity() bounds image_size only against a flat 16 MiB cap (core/image_verify.c:95), not against the slot. So the same over-read the PR closes in jump_app.c is still reachable through secure boot. Hoist the check into one helper (e.g. eos_image_fits_slot(const eos_image_header_t*, uint32_t slot_size)) and call it from jump_app.c, secure_boot.c, core/slot_manager.c and core/recovery.c. The PR body already notes the duplicated checks in the latter two — this is the fourth copy site, and the one with no check. 2 Medium stage1/jump_app.c:34-41 The bound is hdr_size + image_size <= slot_size. On current master an image is header + payload + *TLV area*: core/rollback.c:69-96 reads the TLV at image_addr + hdr_size + image_size for hdr.tlv_len bytes. tlv_len is capped at EOS_TLV_MAX_SIZE (512, include/eos_image_tlv.h:50) and checked for wraparound, but never against the slot end, so a header can still place an authenticated-looking TLV read up to 512 bytes past the selected slot. Not reachable from jump_app.c today (that path calls eos_image_check_rollback(), which reads only the HW counter), but it is reachable from finding 1's path. Add tlv_len to the arithmetic in the shared helper: reject when (uint32_t)hdr.tlv_len > slot_size - hdr.hdr_size - hdr.image_size. Add a test_jump_app_bounds.c case with tlv_len pointing past the slot end. 3 Medium pr.json body / title The body presents the four bounds properties as the change. Three of them are already on master: git diff origin/master..2acb082 -- stage1/jump_app.c is *only* the comment rewording at lines 80-86. The check itself landed via eBoot#76 (42c3f22), which is on this branch but reached master separately; the branch is 16 commits behind. A reviewer reading the body will believe they are approving unmerged hardening. Retitle to what is actually new (test: cover jump-app slot boundary validation), rebase onto master, and reduce the body to the test plus the comment change. 4 Low tests/CMakeLists.txt:45-47 target_link_libraries(eboot_test_jump_app_bounds PRIVATE eboot_core eboot_stage1) reintroduces the pattern master removed three commits ago with a recorded reason — master's tests/CMakeLists.txt now reads PRIVATE eboot_stage1 for test_recovery with the comment "eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on the link line twice … the linker reported the duplicate." Verified it links clean here with GNU ld 2.x on Linux, so this is a convention regression, not a break. target_link_libraries(eboot_test_jump_app_bounds PRIVATE eboot_stage1). 5 Low tests/unit/test_jump_app_bounds.c:96-98 sim_flash_read() attributes any read in [SLOT_A_ADDR + sizeof(hdr), SLOT_B_ADDR) to payload_bytes_read. That window is the whole gap between the slots, not slot A, and the exact-equality assert at line 240 (payload_bytes_read == image_size) additionally pins the CRC32 path to reading the payload in exactly one pass. Both make the test fail for reasons unrelated to the bound it exists to protect. Bound the accounting window to [SLOT_A_ADDR + hdr_size, SLOT_A_ADDR + SLOT_A_SIZE) and relax line 240 to payload_bytes_read >= image_size — or better, assert the read never leaves the slot, which is the actual property. CI Four required checks are red: Host Build & Tests, Build & Test (Linux x86_64), Cross-compile STM32F4, Analyze (C/C++). *None of them are this PR's fault.* origin/master (22d8f8b) does not compile. Verified by building it in a clean worktree: - include/eos_image.h:135 and :142 assert on eos_image_header_t::reserved, which no longer exists — master replaced reserved[30] with tlv_len + tlv_hash[28] and left the two old asserts behind. → error: 'eos_image_header_t' has no member named 'reserved' - core/ed25519_verify.c:281 and :338 both define point_is_identity, and key_has_prime_order / public_key_is_valid_subgroup are two copies of the same subgroup check. → error: redefinition of 'point_is_identity' - tests/unit/test_ed25519.c:31,263 redefine run_test_ed25519_identity_key_forgery_rejected / test_ed25519_identity_key_forgery_rejected, and lines 299-300 reference k_low_order / messages that are not in scope. All three are one bad merge of two competing subgroup-check PRs. *eBoot#94 ("fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken") already targets exactly this*, so no fix PR was opened from here. #96 <#96> cannot go green until #94 <#94> lands. The PR head *alone* builds and tests clean — verified: cmake -S . -B bt -DEBLDR_BUILD_TESTS=ON && cmake --build bt # rc=0 ctest --test-dir bt --no-tests=error # 19/19 passed, test_jump_app_bounds included Note for the body's validation block: ctest finds *no tests at all* without -DEBLDR_BUILD_TESTS=ON (default OFF, CMakeLists.txt:23) and still exits 0 on a plain cmake -B build. The quoted 19/19 is real, but only with that flag — state it, or the next person reproduces a green run that measured nothing. Architecture conformance Conforms. §8 / §8.1 — bounds-checking a slot before streaming payload bytes out of it is stage-1 "verify manifest → verify image → slot selection" work, and it stays in stage1/. No new dependency: stage1/jump_app.c gains only a call to eos_hal_slot_size(), which is a downward call into the HAL, and the test links eboot_stage1 → eboot_core → eboot_hal, the direction .ai/architect.md requires. §5.1 satisfied — nothing points up a tier. Tier 1 (§21), correct repo. The per-repo layout in .ai/architect.md is respected: boot logic in stage1/, test in tests/unit/. The duplication in finding 1 is the architectural point worth acting on: four call sites, three with a hand-copied bound and one with none, is how the fourth site stays wrong. That is a shared-helper problem, not a per-PR problem. Proposed changes Smallest sequence that keeps everything building: 1. Land eBoot#94 first. Nothing here can be verified in CI until master compiles. 2. Rebase this branch onto master and retitle per finding 3. The rebase drops the already-merged 42c3f22 and leaves the test plus the comment. 3. tests/CMakeLists.txt: drop eboot_core from the new link line (finding 4). 4. Tighten the test's read accounting (finding 5). 5. Separate PR, not this one: extract eos_image_fits_slot() and call it from all four sites, tlv_len included (findings 1 and 2). This is a behaviour change on the secure-boot path and belongs on its own, with its own test. Not checked - Nothing was run on hardware. The 19/19 result is host x86-64 only; Cross-compile STM32F4 and Cross-compile ARM Cortex-M4 were not reproduced locally, so the change is unverified under arm-none-eabi. - The merged result was not fully built. I merged 2acb082 into master locally (clean, no conflicts) but could not complete the build past master's three pre-existing compile errors, so *CI green after #94 <#94> lands is unverified* — in particular finding 4's link line was proven only at the PR head, not in the merge. - Analyze (C/C++) and CodeQL findings were not reviewed; CodeQL reports skipping and the Analyze job failed at the build step, so neither produced results for this head. - No fuzz coverage exists for eboot_jump_to_app(). .ai/security.md requires fuzzing for externally reachable parsers, and the image header is one. This PR adds two unit cases, not fuzz coverage; the gap is pre-existing and not assessed further. - existing-comments.txt is empty for this head, so no prior reviewer point was checked for overlap beyond that. ------------------------------ *Automated architecture review of 2acb082 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.* — Reply to this email directly, view it on GitHub <#96?email_source=notifications&email_token=BFUJGLU5RPL5FSZGIN7TS5T5NGEOXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM3TGNZTHAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5103737381>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFUJGLQWHGKMDUIXMJCR4OL5NGEOXAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJRGA4TONJQGE5US43TOVSTWNJTGM3TEMJTGMZDNILWAI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/BFUJGLREPBPDL3QGWUWZLN35NGEOXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM3TGNZTHAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/BFUJGLQEK7VBXFGIQCGUFID5NGEOXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM3TGNZTHAY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: <embeddedos-org/eBoot/pull/96/review/5103737381 ***@***.*** com>

[Kartikey1306]

All four red checks here are inherited from master, not caused by this PR.

master (22d8f8b) does not compile: include/eos_image.h:135 and :142
static-assert on offsetof(eos_image_header_t, reserved), and #93 replaced
reserved[30] with tlv_len + tlv_hash. Every job that builds eboot_core
fails on it, which is exactly the four you see.

Verified rather than assumed — I built the merge result locally:

master alone, host build	fails, no member named 'reserved' at :135 and :142
this PR merged with #94	build OK, ctest 22/22

The header defect is target-independent, so it is not an ARM or toolchain
issue: compiling eos_image.h on its own gives 2 errors on master and 0 on
#94, for both host and --target=aarch64-linux-gnu.

So nothing is needed from you. When #94 lands (it is open, green and mergeable;
#98 is a near-duplicate repair and also builds clean at 21/21), rebase or merge
master and these four go green. Your added suite is the reason the merge
result runs 22 tests rather than 21, and it passes.