Harden shared image slot boundary validation

[Mohammed18-19]

Summary

• Add a shared eos_image_fits_slot() helper for complete image boundary validation.
• Validate the image header, payload, and TLV area against the configured flash slot.
• Use overflow-safe sequential size checks.
• Apply the shared validation across secure boot, slot management, recovery, and application loading paths.
• Add regression coverage for images and TLVs crossing the slot boundary.

Compatibility

eos_secure_boot_config_t now requires slot_size to be set to the flash capacity available to the image.

Existing in-tree callers have been updated accordingly. Out-of-tree integrations that zero-initialize eos_secure_boot_config_t must also set slot_size; otherwise secure boot will fail closed with EOS_SBOOT_ERR_BAD_HEADER.

Testing

Full host test execution is currently blocked by a pre-existing duplicate eboot_test_fdt_loader / test_fdt_loader target in tests/CMakeLists.txt on master.

The duplicate target causes CMake configuration to fail before the project can be configured and built with this PR, so the new tests have not yet been verified by a clean configure/build/ctest run.

The duplicate CMake target is unrelated to this change and should be removed in a separate PR against master.

Previously observed host build failures in core/ed25519_verify.c and core/sha512.c are also pre-existing baseline issues and are not modified by this PR.

[srpatcha]

Review — eBoot#109 "Harden shared image slot boundary validation"

head: d693147 author: Mohammed18-19 ci: fail

Verdict: Consolidates four copies of the slot-bounds check into one eos_image_fits_slot() helper and extends it to cover tlv_len — the right direction for §5.1's "minimal and auditable" TCB — but it adds a required field to a public config struct without updating the in-tree callers that zero-initialise it, so eos_secure_boot() now fails closed for every caller that has not been recompiled, and the PR's testing claim cannot have covered the diff.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	include/eos_secure_boot.h:31, core/secure_boot.c:90	slot_size is a new required field of the public eos_secure_boot_config_t. Callers using the documented memset(&cfg, 0, sizeof(cfg)) pattern get slot_size == 0; eos_image_fits_slot() then returns false for any header with hdr_size > 0, so eos_secure_boot() returns EOS_SBOOT_ERR_BAD_HEADER where it previously returned EOS_SBOOT_OK. tests/unit/test_secure_boot_policy.c:183,203,222,259 all zero-init and never set the field; three of those tests assert EOS_SBOOT_OK and will now fail. The PR body says nothing about compatibility impact (brief §8) — silence about a breaking change to a public struct is itself the finding.	Set cfg.slot_size in every in-tree caller, starting with the four sites in test_secure_boot_policy.c. State the compatibility impact and migration step in the PR body: any out-of-tree board integration that zero-initialises this struct stops booting until it sets the field. Failing closed is correct; doing it silently is not.
2	High	PR body, "Testing"	The body claims ctest --test-dir build --output-on-failure and "19/19 existing tests passed" while the same body states that full CMake reconfiguration is blocked. A build tree that was never reconfigured cannot contain core/image_slot_bounds.c or the two new tests, so that run did not exercise this diff. Per .ai/reviewer.md, an unsupported "verified" is the finding.	Remove the claim or replace it with output from a configure+build+ctest --no-tests=error run that actually includes the new sources. Until finding 3 is cleared, state plainly that the new tests have not run.
3	High	tests/CMakeLists.txt:187-191 and :201-205 (on origin/master)	Six required checks are red — Host Build & Tests, Build & Test (Linux x86_64), CI Gate, Cross-compile STM32F4, Fuzz Harness Build, Analyze (C/C++). Root cause is a pre-existing duplicate block on master: add_executable(eboot_test_fdt_loader ...) / add_test(NAME test_fdt_loader ...) appear twice, so CMake configure aborts with add_executable cannot create target "eboot_test_fdt_loader" because another target with the same name already exists before anything compiles. Not introduced here — the author correctly identified it — but it means nothing in this PR has been verified by CI either.	Delete the duplicate block (lines 201-205 plus its comment) on master in its own PR; this PR then rebases and gets real CI. No open eBoot PR currently covers this.
4	Medium	tests/unit/test_jump_app_bounds.c:184, :41	Assertion weakened: ASSERT(payload_bytes_read == image_size) became >=, and the accounting window at line 41 was simultaneously widened from addr < SLOT_B_ADDR to addr < SLOT_A_ADDR + SLOT_A_SIZE. The test no longer pins how much of the payload is read; it now passes for any over-read that stays inside slot A. A loosened assertion is a finding regardless of the reason given (.ai/reviewer.md).	If the intent is to also count TLV reads, assert the exact expected total (image_size + hdr.tlv_len) rather than relaxing the comparison.
5	Medium	tests/unit/test_secure_boot.c:202, :226	test_tlv_beyond_slot_is_rejected is a plain static void invoked directly from main(), not declared with the file's TEST() macro. Consequences: it never increments tests_run/tests_passed, so the suite's own return tests_passed == tests_run gate does not cover it; and it never calls eos_hal_init(&sim_ops), so it only works because a preceding run_test_* left the HAL registered — write_image() calls eos_crc32(), which needs a live HAL. Reordering or removing an earlier test silently breaks it. This is the exact failure the file's own closing comment was written to prevent.	Declare it as TEST(test_tlv_beyond_slot_is_rejected) and call run_test_tlv_beyond_slot_is_rejected() from main().
6	Low	core/secure_boot.c:90	This is the only one of four call sites without a preceding slot_size == 0 guard (core/recovery.c:318, core/slot_manager.c:54, stage1/jump_app.c:38 all have one). eos_image_fits_slot() returns true for an all-zero header against slot_size == 0, so the helper alone does not make the guard redundant.	Either add `cfg->slot_size == 0
7	Low	core/image_slot_bounds.c:1	Missing the // SPDX-License-Identifier: MIT / copyright / standards header that every other file in core/ carries (e.g. core/slot_manager.c:1-3). Relevant to the SPDX SBOM commitment in .github/STANDARDS.md.	Add the three-line header used by the sibling files.

Architecture conformance

Conforms. Master design §5.1: the new code sits in eBoot/core and eBoot/include, includes only eos_image.h, and adds no dependency pointing up a tier — core/image_slot_bounds.c is reachable from stage1/ and core/ only. Per .ai/architect.md's target shape, core/ is the correct home for shared boot logic and include/ for the contract. §5.1's "eBoot keeps the trusted computing base minimal and auditable" is served rather than harmed: replacing four hand-rolled copies of the same arithmetic with one non-overflowing helper is a net reduction in auditable surface, and extending it to tlv_len closes a real gap — the previous checks bounded header+payload only, so a manifest could declare a TLV region running past the slot. §8.1 ("Verify Manifest → Verify Image") is unaffected in ordering; the new gate runs before integrity streaming, which is the correct position. No tier or repository-placement question arises (§21: eBoot is Tier 1).

Proposed changes

Smallest sequence that keeps everything building:

1. Separate PR against master, nothing else in it — delete the duplicated test_fdt_loader block:

   --- a/tests/CMakeLists.txt
   @@ -199,7 +199,3 @@
   -# --- test_fdt_loader: device tree parsing against malformed blobs ---
   -
   -add_executable(eboot_test_fdt_loader unit/test_fdt_loader.c)
   -target_link_libraries(eboot_test_fdt_loader PRIVATE eboot_core)
   -add_test(NAME test_fdt_loader COMMAND eboot_test_fdt_loader)

   Verify with cmake -B build/host -DEBLDR_BUILD_TESTS=ON then ctest --test-dir build/host --output-on-failure --no-tests=error.
2. Rebase this PR on that, then set cfg.slot_size in all four test_secure_boot_policy.c fixtures (FLASH_BASE slot capacity, matching stage_bootable_image()).
3. Convert the new test at tests/unit/test_secure_boot.c:202 to the TEST() macro and call the generated run_test_* from main().
4. Restore an exact assertion at test_jump_app_bounds.c:184.
5. Re-run configure + build + ctest --no-tests=error and replace the PR body's Testing section with that output. Add the compatibility note for the eos_secure_boot_config_t field.

Items 6 and 7 are one-line cleanups that can ride along.

Not checked

• No build or test was run locally. The duplicate-target defect makes cmake configure fail before compilation, and this pipeline does not check out PR branches into the user's tree. Findings 1, 4, 5, 6 are from reading the head revision d693147, not from observed test output. Finding 1's specific claim — that the three EOS_SBOOT_OK assertions in test_secure_boot_policy.c now fail — is inferred from the code path (memset → slot_size == 0 → hdr_size > 0 → false), not observed.
• Cross-compile behaviour on STM32F4 and the ARM Cortex-M4 target: those jobs are red or skipped for the same configure failure, so no target-side evidence exists.
• Fuzz coverage for the new tlv_len path — .ai/security.md requires fuzz, not only unit tests, for externally reachable parsers. The Fuzz Harness Build job is red, so whether any harness reaches eos_image_fits_slot() is unknown.
• Out-of-tree consumers of eos_secure_boot_config_t. Only this repository was searched; board integrations living elsewhere were not.
• Whether eos_image_parse_header() can return EOS_OK for a header with hdr_size == 0, which is what would make finding 6 reachable in practice.

---

Automated architecture review of d693147bccc2 — 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.

[srpatcha]

Review — eBoot#109 "Harden shared image slot boundary validation"

head: 33fce87 author: Mohammed18-19 ci: fail (6 required checks red — all inherited from master, see below)

Verdict: Follow-up. Two new commits address five of the seven previous findings, and
I was able to run the tests this time: on a scratch merge of this PR onto #115, with
#111's duplicate target removed, the full suite is 100% tests passed, 0 tests failed out of 31, including all three test_jump_app_bounds cases and test_tlv_beyond_slot_is_rejected.
The code is ready. What is left is sequencing: this PR cannot go green until #111 and #115
land, and it carries a merge conflict with #115 that must be resolved in #115's favour.

Previous findings

#	Was	Now	Evidence
1	High — slot_size added as a required field of a public struct, in-tree callers zero-init it, breaking change undocumented	Resolved	33fce87 sets cfg.slot_size = FLASH_SIZE at all four fixtures (test_secure_boot_policy.c:184,205,225,263). The PR body now carries a ## Compatibility section naming the out-of-tree impact and the fail-closed mode. Verified by execution: test_secure_boot_policy 7/7 PASS.
2	High — PR body claimed a ctest run that could not have covered the diff	Resolved	The body's ## Testing section now states plainly that configure is blocked and "the new tests have not yet been verified by a clean configure/build/ctest run". That is the honest form.
3	High — 6 required checks red from a duplicate eboot_test_fdt_loader target on master	Still open, not this author's to fix; now known to be two defects deep	See finding 1 below.
4	Medium — assertion weakened from == to >=	Resolved	test_jump_app_bounds.c:186 is back to ASSERT(payload_bytes_read == image_size). Verified: the test passes with the exact comparison.
5	Medium — test_tlv_beyond_slot_is_rejected was a bare static void, outside the suite's own pass gate and with no eos_hal_init	Resolved	Now TEST(test_tlv_beyond_slot_is_rejected) at test_secure_boot.c:202, invoked as run_test_tlv_beyond_slot_is_rejected() from main(). Verified: it reports [PASS] inside suite 5's own tally.
6	Low — core/secure_boot.c:90 was the only call site without a slot_size == 0 guard	Resolved	core/secure_boot.c:90-91 is now if (cfg->slot_size == 0 || !eos_image_fits_slot(...)), matching recovery.c:318, slot_manager.c:54, jump_app.c:38.
7	Low — missing SPDX/standards header	Resolved	core/image_slot_bounds.c:1-3 now carries the three-line header used by its siblings.

One correction to the earlier report: it described the accounting window in sim_flash_read
as widened. It was narrowed — SLOT_A_ADDR + SLOT_A_SIZE is 0xC000, SLOT_B_ADDR is
0x14000. The new window is exactly slot A, which is what the test means. No action.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	tests/CMakeLists.txt:202-206; core/sha512.c:90,96,98,120,121; core/ed25519_verify.c:303	master does not build, and the duplicate CMake target is only the first of two blockers. Removing the duplicate eboot_test_fdt_loader block makes configure succeed — then compilation fails on eos_sha512_ctx_t has no member named 'count' (5 sites) and redefinition of 'scalarbase'. I reproduced the identical error set from a clean origin/master worktree with only the duplicate removed, so none of it is attributable to this PR — neither file is in files.txt. This is why all six checks are red.	Nothing to do here. #111 removes the duplicate; #115 repairs sha512.c and ed25519_verify.c. This PR needs both, then a rebase.
2	Medium	tests/unit/test_jump_app_bounds.c:214 (main())	Merge conflict with #115, and the naive resolution silently undoes #115's fix. #115 replaces the hand-maintained tests_run = N; with tests_run++ inside the TEST macro and adds tests/unit/test_suite_bookkeeping.py to keep it that way. This PR bumps the hardcode tests_run = 2 → 3. git merge of the two conflicts on exactly this line. Resolving it in this PR's favour restores a counter that has to be edited by hand every time a test is added — the bookkeeping defect #115 exists to remove.	Whichever lands second: drop the tests_run = 3; line entirely and keep #115's macro-driven counter. I verified this resolution builds and that the suite still reports 3/3 tests passed with the real count.
3	Low	tests/unit/test_jump_app_bounds.c:39	uint32_t payload_start is computed on every sim_flash_read call and used once. Harmless in a test double, and it does read better than the inlined expression, but it is unrelated churn in a security PR — it is the kind of line that makes a reviewer re-derive whether the window changed (it did: SLOT_B_ADDR → SLOT_A_ADDR + SLOT_A_SIZE). The window change is correct and intended; it just is not mentioned in the PR body.	Add one line to the PR summary: the payload-accounting window now ends at the slot A boundary rather than at slot B, so the test measures slot A only. No code change needed.

Verification performed for this review

Run in a detached scratch worktree under .ai/autoreview/state/verify/. The user's
eBoot checkout was not touched, nothing was committed to a tracked branch, nothing pushed.

What	Result
cmake -B build/host -DEBLDR_BUILD_TESTS=ON on PR head, duplicate target removed	PASS — configure completes, eBootloader v3.0.2, Tests: ON
cmake --build build/host on PR head (same tree)	FAIL — sha512.c ×5, ed25519_verify.c ×1
Same build from clean origin/master, duplicate removed, PR not applied	FAIL — identical error set (this is what proves the failure is inherited)
cmake --build on PR head merged onto #115	PASS — no errors
ctest --test-dir build/host --output-on-failure --no-tests=error on that merged tree	PASS — 100%, 31/31
ctest -R "test_jump_app_bounds|test_secure_boot" verbose	PASS — test_jump_app_bounds 3/3 incl. test_tlv_beyond_slot_rejected_before_payload_read; test_secure_boot incl. test_tlv_beyond_slot_is_rejected; test_secure_boot_policy 7/7

So the substance of this PR is verified working. The red CI is sequencing, not defect.

Architecture conformance

Conforms; unchanged from the last review and confirmed against the current head. §5.1:
core/image_slot_bounds.c and the include/eos_image.h declaration add no edge pointing
up a tier — the helper includes only eos_image.h and is reached from core/ and
stage1/ alone. Per .ai/architect.md's target shape, core/ is the right home for
shared boot logic and include/ for the contract. §5.1's "eBoot keeps the trusted
computing base minimal and auditable" is served: four hand-rolled copies of the same
bounds arithmetic collapse to one overflow-safe helper, and extending it to tlv_len
closes a real hole — previously a manifest could declare a TLV region running past the
slot. §8.1's "Verify Manifest → Verify Image" ordering is preserved; the gate runs before
integrity streaming, which is the correct position. §21: eBoot is Tier 1, no placement
question arises.

Blocked status and what unblocks it

Blocked on two other PRs, in this order:

1. #111 — removes the duplicate eboot_test_fdt_loader target. Unblocks CMake configure.
2. #115 — repairs core/sha512.c and core/ed25519_verify.c. Unblocks compilation.
3. Rebase this PR; resolve test_jump_app_bounds.c:main() by dropping tests_run = 3;.
4. Replace the PR body's ## Testing section with the real ctest output, which will
   then be obtainable.

Not checked

• Fuzz coverage for the new tlv_len path. .ai/security.md requires fuzz, not only
  unit tests, for externally reachable parsers, and the manifest header is one. The Fuzz
  Harness Build check is red; I did not build the fuzz harnesses locally and do not know
  whether any of them reach eos_image_fits_slot(). This is the one substantive gap left.
• Cross-compile targets. STM32F4 and Cortex-M4 were not built. My verification was
  host-only x86-64 with the system GCC; the cross toolchains were not exercised, so
  nothing here says the change compiles for a target.
• Out-of-tree consumers of eos_secure_boot_config_t. Only this repository was
  searched. Board integrations living in other repos or downstream forks that
  zero-initialise this struct will fail closed at first boot after upgrading. The PR body
  now documents this, which is the right treatment, but the blast radius is unmeasured.
• Overflow behaviour of eos_image_fits_slot() itself was read, not fuzzed or
  property-tested. The sequential-addition form looks correct for the uint32_t inputs,
  but no test drives it with values near UINT32_MAX.
• I did not verify that eos_image_parse_header() can return EOS_OK for a header with
  hdr_size == 0, which is what would make the slot_size == 0 guard reachable in
  practice. The guard is correct defensively either way.

---

Automated architecture review of 33fce87c9113 — 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.

[srpatcha]

Thanks Mohammed — consolidating the three copies of the bounds check into eos_image_fits_slot() and making it cover tlv_len is a good change; the helper's arithmetic at core/image_slot_bounds.c:7-26 is correct and overflow-safe, and the jump_app test genuinely proves the payload is never read. I built the branch rebased on #115 and it passes 31/31.

Three things I'd like addressed before merge:

1. API break, undocumented — include/eos_secure_boot.h:31 adds slot_size, and core/secure_boot.c:90 refuses to boot when it is 0. Every existing caller that zero-inits the config now fails closed on every boot. That may be the right default, but it has to be stated in the PR, the struct doc and CHANGELOG.md. Alternatively derive the slot from cfg->image_addr through the HAL, as rollback.c's slot_remain_from() already does at step 5b — no API change, and no way for a caller to pass a too-large slot_size that disables the check.
2. 2. tests/unit/test_secure_boot.c:202 doesn't exercise the new code — with the new block removed the test still passes, because step 5b already returns BAD_HEADER for that header. The new property is ordering; please assert no payload bytes were read (as test_jump_app_bounds.c does) or that the attest log has a single step-2 entry.
3. 3. Rebase on #115 — master doesn't build; tests/unit/test_jump_app_bounds.c:216 (tests_run = 3;) conflicts with #115's macro counting and should just be dropped.
      Requesting changes for those; happy to re-review quickly.

[Kartikey1306]

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

master (221325c) does not configure: tests/CMakeLists.txt declares
add_executable(eboot_test_fdt_loader …) twice, so CMake stops at

CMake Error at tests/CMakeLists.txt:203 (add_executable):
  add_executable cannot create target "eboot_test_fdt_loader" because another
  target with the same name already exists.

before anything is compiled — which is why every build-side check fails
identically, and why CI Gate (correctly) reports that. Behind it, master's
core/sha512.c also uses a count member that eos_sha512_ctx_t no longer
has, so even with the duplicate removed the core library does not compile.

Both are repaired by #115 (fix: repair master after the 09-07 batch merge),
which is open, green and on top of current master.

Verified by building the merge result locally rather than inferring it:

master alone	fails to configure (duplicate target)
this PR merged with #115	build OK, ctest 31/31, and test_jump_app_bounds reports 3/3

One thing you will hit when you rebase: a one-hunk conflict in
tests/unit/test_jump_app_bounds.c. This PR adds a third test and sets
tests_run = 3; by hand; #115 removes that hardcoded line and counts runs in
the RUN_TEST macro instead (tests_run++). Resolve by dropping the
tests_run = 3; line — your new test is counted automatically, and the
result above (3/3) is with that resolution. Nothing else conflicts.