fix(sha512): drop a carry term that is always zero by Kartikey1306 · Pull Request #123 · embeddedos-org/eBoot

Kartikey1306 · 2026-09-14T19:43:52Z

Stacked on #115. Review e152d8e..46e1d89 (corrected from a276016..b828d02 at 12:20 IST 2026-09-15: #115 was rebased onto master 682d005 as e152d8e and this branch restacked onto it — one commit replayed clean, content unchanged; verified on the new head before the push: build OK, ctest 31/31, pytest 58 passed / 3 skipped): one commit, one file, 2 insertions / 1 deletion against blob d9aa57c.

Problem (review finding 2 on #115; eBoot #121)

core/sha512.c, eos_sha512_update():

ctx->bitlen[0] += (uint64_t)copy >> 61;

is always zero. copy is clamped to 128 - ctx->buffer_len, so copy <= 128 and copy >> 61 == 0 on every iteration. The real 128-bit carry is already handled by the old_low comparison two lines above. The digest is correct either way (RFC 8032 vectors and the SHA-512 KATs pass), but the line reads as overflow handling and does nothing — the kind of thing a future reader preserves because they assume it matters.

Kept out of #115 deliberately so that core/sha512.c there stays a byte-exact restoration of the previously reviewed blob.

Change

The line is replaced by a two-line comment giving the bound: copy is at most 128 - buffer_len, so copy << 3 fits the low word and can wrap it at most once per iteration; the old_low comparison above is that carry.

Verification

Verified, macOS/clang: Release build clean, ctest 31/31, eboot_test_ed25519 14/14 including test_sha512_known_answers and test_sha512_streaming_matches_one_shot (the streaming test drives eos_sha512_update() across chunk boundaries, which is where a wrong carry would show).

Closing issue

Fixes #121

codecov-commenter · 2026-09-14T19:45:50Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tests/unit/test_fw_update_test_sigs.py	93.33%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

srpatcha

Review — eBoot#123 "fix(sha512): drop a carry term that is always zero"

head: b828d02 author: Kartikey1306 ci: pass (26 green, 0 red; only Create GitHub Release skipped)

Verdict: Clean. One dead line removed from core/sha512.c and replaced with a comment
that states the invariant making it dead. The reasoning is correct, and I confirmed the change
is a behavioural no-op by hashing 360 inputs through the code before and after and diffing the
output: byte-identical, and both match the reference implementation. Nothing to fix.

Findings

#	Severity	File:line	Finding	Recommended fix
—	—	—	None.	—

This PR is one deleted line and one added comment. I verified it rather than waved it through
— evidence below — and there is nothing to report. Per the brief, that is the whole finding
table; I am not manufacturing a second opinion to fill it.

Disclosure, because it matters for who is checking whom: this deletion was recommended by
this pipeline's own earlier review of #115 (finding 2). .ai/reviewer.md's rule is that you do
not approve your own work; suggesting a change in a review is not implementing it, but it does
mean I had a prior answer in mind, so I verified this one harder than its size warrants rather
than confirming myself. The evidence below stands on its own and would have caught a wrong
deletion.

Why the line was dead

core/sha512.c:190 sets copy = 128 - ctx->buffer_len, then clamps it down to len.
buffer_len is 0 after eos_sha512_init() and is reset to 0 immediately after every
sha512_transform() (:214), so it is in [0,127] on entry to the loop body and copy is in
[1,128]. 128 >> 61 == 0, so (uint64_t)copy >> 61 was zero on every iteration the function
can ever execute — not "in practice", but for every reachable value. The real 128-bit carry is
the old_low comparison at :206-207: bits = copy << 3 <= 1024, so adding it to
bitlen[1] can wrap the low word at most once, and that single wrap is exactly what the
comparison detects and propagates into bitlen[0]. The replacement comment says this, which is
the useful outcome — the line's problem was that it looked like overflow handling, so a
future reader would preserve it on the assumption it mattered.

There is no test to add here and correctly none was added: a test that distinguishes the two
versions would have to hash more than 2⁶¹ bytes.

Verification performed for this review

Detached worktree at .ai/autoreview/state/scratch/eBoot-123 off refs/pull/123/head. The
user's eBoot checkout was not touched and is still clean; nothing was committed or pushed.

Check	Result
`cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug` → `cmake --build --parallel 4`	PASS
`ctest --test-dir build/host --output-on-failure --no-tests=error`	PASS — 31/31, 10.92s
`eboot_test_ed25519` verbose	PASS — 14/14, including `test_sha512_known_answers` and `test_sha512_streaming_matches_one_shot`. Ed25519 derives its challenge scalar from this hash, so the RFC 8032 vectors would fail on any deviation.
Differential test against a reference implementation. Built a harness linking `libeboot_core.a` and hashed 20 lengths chosen to straddle every buffer and block boundary — `0,1,55,56,63,64,111,112,113,127,128,129,200,255,256,257,1023,1024,1025,4096` — one-shot, and again streamed in 17 different chunk sizes (1…17) each	PASS — 360 digests, 0 mismatches. Every one-shot digest equals Python `hashlib.sha512` of the same input, and every chunked digest equals its own one-shot.
Is the deletion a no-op? Restored `core/sha512.c` from the parent commit `a276016`, rebuilt `eboot_core`, re-ran the same harness, diffed the two outputs	IDENTICAL — the pre-fix and post-fix trees produce byte-identical digests across all 20 lengths × 18 chunkings. This is the claim that matters and it is now measured, not argued. `core/sha512.c` was restored afterwards; `git diff --stat` clean.
Can `copy` be `0`, making the loop spin?	No — `buffer_len` is reset to `0` on reaching 128 (`:215`), so it is never 128 on entry and `copy >= 1`. Checked because the deletion drew my eye to the loop bounds.

Was anything weakened? No. Nothing else in the diff: no test disabled, no assertion
removed, no lint loosened. The change is -1/+2 lines in one file, and the two added lines are
a comment.

Architecture conformance

Conforms. §21: eBoot is Tier 1 Foundation and core/sha512.c is in the owning repo, in the
directory .ai/architect.md's target shape assigns to shared boot logic. No #include, link
line, CMake entry or manifest dependency changes, so §5.1's dependency direction is untouched
and nothing points up a tier. §14.1's "do not invent cryptographic primitives" is not engaged —
this removes a line, it does not alter the algorithm, and the differential test above
demonstrates that directly. §5.1's "eBoot keeps the trusted computing base minimal and
auditable" is served in the small: dead code that impersonates a security-relevant operation is
the opposite of auditable.

On keeping it out of #115. The right call, and worth recording. #115 restores
core/sha512.c to blob d9aa57c byte-for-byte, and that byte-identity is itself the evidence
for #115's central claim — that the file was not hand-written into the TCB but restored from a
previously-reviewed blob. Making this edit inside #115 would have destroyed the one property
that made #115 cheap to trust. Splitting it into a one-line PR stacked on top preserves both.

Proposed changes

None. Merge after #115 (it is stacked on a276016 and touches a file #115 restores).

Not checked

Cross-compile (STM32F4, Cortex-M4), fuzz-harness build, CodeQL, cppcheck/clang-tidy —
NOT RUN locally. CI reports all green on this head; I did not read the logs. My
differential harness is a host x86-64 build with gcc, so the result is not evidence about
a big-endian or 32-bit size_t target — though on such a target copy is smaller still, so
the deleted term is if anything more obviously zero.
pytest tests/ — NOT RUN at this head. The diff touches no Python. I ran it at #115's
head (80 passed) and expect it unchanged; I did not execute it here.
ASan/UBSan and Valgrind — NOT RUN. test_ed25519 is in EBLDR_UNIT_TESTS so CI's
Valgrind leg covers it there; I did not run Valgrind locally.
No behaviour at lengths above 2³² bytes was exercised. The 128-bit length accumulator's
high word bitlen[0] is only reached after 2⁶¹ bytes, which is not testable; the argument
that the carry is handled correctly rests on reading :205-207, not on execution. That was
equally true before this PR.

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

Kartikey1306 · 2026-09-15T06:19:39Z

Acknowledged — nothing to change. Head stays b828d02. Merge after #115, as stated: stacked on a276016 and it touches core/sha512.c, which #115 restores.

Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/[email protected] pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.

… -1.0.0 as an option test_out_of_range_version_is_rejected[-1.0.0] passed locally (Python 3.14) and failed in CI (ubuntu-22.04, Python 3.10) with argparse's own "expected one argument": the older negative-number matcher does not accept -1.0.0, so the token was read as an unknown option and imgpack.py's range check -- the thing under test -- never ran. The joined form is unambiguous on every interpreter and the test now reaches the tool's message.

tests/vectors/fw_update_test_sigs.h is the output of tools/gen_fw_update_test_sigs.py, committed because eBoot has no Ed25519 signer in C. Nothing checked that the two agree: a generator edit without a regeneration leaves test_fw_update and test_fw_transport verifying against stale signatures, failing with EOS_ERR_SIGNATURE and nothing to say why. tests/unit/test_fw_update_test_sigs.py runs the generator with the test's own interpreter and compares its stdout to the committed header byte for byte, so a line-ending change counts too. It follows the same dependency rule as test_eos_sign_payload_offset.py: with EOS_REQUIRE_SIGNING_TESTS set (the CI workflow sets it before the pytest step) a missing cryptography module fails the job instead of skipping. Negative control: one flipped hex byte in the header fails the test with a unified diff naming the line. build_image() and build_container() now carry a comment naming the coupling: the signed prefix is assembled both there and in the generator, and changing any field in it means changing the generator's copy and regenerating the header.

…epair embeddedos-org#103 and embeddedos-org#104 were both merged and disagree on whether the anti-rollback counter or the signature is checked first in eos_fw_update_finalize(). The master design orders boot as verify image, then version policy (section 8.1), but its update flow (section 15) never places the anti-rollback check, so the order the install path uses existed only in a PR body. ADR-020 records it: the signature over the signed header prefix is verified first, the TLV counter is read only after the prefix that binds it is authenticated, and an image that fails verification is refused as EOS_ERR_SIGNATURE without its counter being consulted. docs/adr/README.md is added in the shape of the eos repository's index; 020 avoids reusing 001 through 019. CHANGELOG.md gains the Unreleased entries for the repair: the configure, compile and link breakage after the 09-07 batch merge, the settled check ordering with the suites streaming signed images, the re-derived Valgrind list, fuzz-build in the CI gate, counted tests_run, the EoSim Windows legs running under bash, and the Scorecard action on its ghcr.io-hosted release.

The changelog said eleven suites had no Valgrind run. Eleven were missing from EBLDR_UNIT_TESTS, but four of those were named in the hand-written foreach and did run; seven had no run at all. Say which. ADR-020: its design-document citation now says where it comes from (the architecture review of embeddedos-org#115), and the sentence about the two PRs' bases is replaced with what the history shows -- embeddedos-org#103's commits predate embeddedos-org#104's merge, and embeddedos-org#104 was written without embeddedos-org#103's check in place.

The regeneration test's failure path -- the unified diff under the regenerate command -- ran only when the header was stale, so a green run never executed it and the coverage report said so. The report is now a helper the match asserts with, and a second test drives the helper with two byte strings that differ in one byte and checks the command and both sides of the changed line appear.

eos_sha512_update() added (uint64_t)copy >> 61 to bitlen[0] after the old_low comparison had already carried the low word's wrap. copy is bounded by 128 - ctx->buffer_len, so it is at most 128 and copy >> 61 is zero on every call. The term could only be nonzero for a single memcpy of 2^61 bytes or more, which the loop never issues because it feeds the 128-byte block buffer one block at a time. The message length still reaches bitlen[0] the way FIPS 180-4 requires: with copy << 3 at most 1024, the low word wraps at most once per iteration, and that wrap is exactly what the old_low comparison carries. A comment in place of the line states the bound. Kept out of the repair of master so core/sha512.c there stays a byte-exact restoration of the pre-merge file (blob d9aa57c).

srpatcha

Review — eBoot#123 "fix(sha512): drop a carry term that is always zero"

head: 46e1d89 author: Kartikey1306 ci: pass (26 green, 0 red; only Create GitHub Release skipped)

Verdict: The previous review of b828d02f found nothing, and there is still nothing. This
head is a pure re-stack onto #115's rebased tip — the single commit is byte-identical by
git patch-id — so no new work has been introduced to review. Re-verified green on the new
base rather than assumed. Merge after #115.

Status of the previous review (`eBoot-123-b828d02f.md`)

No findings were raised, so there is nothing to follow up. The differential evidence that
carried it — 360 digests identical before and after the deletion, and identical to
hashlib.sha512 — was gathered against parent a276016; core/sha512.c is unchanged between
that parent and this one, so the result carries. That is Inferred from patch-id rather than
re-measured; see "Not checked".

Findings

#	Severity	File:line	Finding	Recommended fix
—	—	—	None.	—

One re-stacked commit, -1/+2 lines in one file, two of which are a comment. Per the brief I
am not manufacturing a finding to justify commenting again.

Verification performed for this review

Detached worktree at .ai/autoreview/state/scratch/eBoot-123 off refs/pull/123/head. The
user's eBoot checkout was not touched; nothing was committed or pushed.

Check	Result
Is this head new work or a re-stack?	RE-STACK, confirmed. One own commit at each head; `git patch-id --stable` of `b828d02f` and `46e1d891` are identical. The parent moved `a276016` → `e152d8e`, which is #115's own rebase.
`cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug` → `cmake --build --parallel 4`	PASS, no errors
`ctest --test-dir build/host --output-on-failure --no-tests=error`	PASS — 31/31, 11.41s
`eboot_test_ed25519` verbose	PASS — 14/14, including `test_sha512_known_answers` and `test_sha512_streaming_matches_one_shot`. Ed25519 derives its challenge scalar from this hash, so the RFC 8032 vectors would fail on any deviation in it.
Did the new base change `core/sha512.c` under the PR?	No — the commit still applies to blob `d9aa57c`, which is what #115 restores; the diff header reads `index d9aa57c..89bdb99`.

Was anything weakened? No. There is no new source since the last review.

One stale detail, not a finding. The PR comment of 2026-09-15T06:19Z says "Head stays
b828d02. … stacked on a276016". Both are now out of date — the branch was re-stacked to
46e1d891 on e152d8e when #115 rebased. The content is unchanged, so nothing about the
comment's substance is wrong; noted only so the next reader does not try to resolve a SHA that
is no longer reachable. Ranges on a three-deep stack go stale on every re-stack and chasing
them is not worth anyone's time.

Architecture conformance

Conforms; re-checked, not recalled. §21: eBoot is Tier 1 Foundation and core/sha512.c sits in
the directory .ai/architect.md's target shape assigns to shared boot logic. No #include,
link line, CMake target_link_libraries entry or manifest dependency changes, so §5.1's
dependency direction is untouched and nothing points up a tier. §14.1's "do not invent
cryptographic primitives" is not engaged: this removes a line and adds a comment, and the
previous review's differential test demonstrated the algorithm is unchanged. §5.1's "eBoot keeps
the trusted computing base minimal and auditable" is served in the small — dead code that
impersonates a security-relevant operation is the opposite of auditable.

Proposed changes

None. Merge after #115.

Not checked

The differential harness was NOT re-run at this head. 360 digests × before/after was run at
b828d02f; the commit is patch-identical here and applies to the same blob, so the result
carries — but that is Inferred from patch-id, not re-observed. The passing
test_sha512_known_answers and test_sha512_streaming_matches_one_shot at this head are the
direct evidence I do have.
Cross-compile (STM32F4, Cortex-M4), fuzz-harness build, CodeQL, cppcheck/clang-tidy —
NOT RUN locally. CI green on this head; job logs not read. The local build is host x86-64
gcc, so nothing here is evidence about a big-endian or 32-bit size_t target.
pytest tests/ — NOT RUN at this head. The diff touches no Python.
ASan/UBSan and Valgrind — NOT RUN.
No behaviour above 2³² bytes was exercised. bitlen[0] is only reached after 2⁶¹ bytes,
which is not testable; the carry argument rests on reading core/sha512.c:205-208. That was
equally true before this PR.

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

Kartikey1306 · 2026-09-15T07:04:15Z

Ack for the 06:49Z re-review at 46e1d89: pure re-stack onto #115's e152d8e, single commit byte-identical, nothing open. Merge after #115.

Kartikey1306 requested review from hshanmug12 and srpatcha as code owners September 14, 2026 19:43

Kartikey1306 mentioned this pull request Sep 14, 2026

fix: repair master after the 09-07 batch merge #115

Open

Kartikey1306 force-pushed the fix/sha512-dead-carry-term branch from 89bc7f9 to b828d02 Compare September 14, 2026 20:03

srpatcha reviewed Sep 14, 2026

View reviewed changes

Kartikey1306 mentioned this pull request Sep 14, 2026

build: drop EBLDR_REQUIRE_SIGNATURES and EBLDR_RECOVERY_AUTH, switches wired to nothing #133

Open

Kartikey1306 added 7 commits September 15, 2026 11:56

Kartikey1306 force-pushed the fix/sha512-dead-carry-term branch from b828d02 to 46e1d89 Compare September 15, 2026 06:29

srpatcha reviewed Sep 15, 2026

View reviewed changes

This was referenced Sep 17, 2026

master does not configure, compile, link or pass tests after the 09-07 batch merge #118

Open

ci: pin every action to a Node 24 major, by commit SHA #144

Open

fix(multicore): an AMP core starts only on an image that verified #146

Open

Navigation Menu

fix(sha512): drop a carry term that is always zero - #123

fix(sha512): drop a carry term that is always zero#123
Kartikey1306 wants to merge 7 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/sha512-dead-carry-term

Kartikey1306 commented Sep 14, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 14, 2026 •

edited

Loading

Uh oh!

srpatcha left a comment

Uh oh!

Kartikey1306 commented Sep 15, 2026

Uh oh!

srpatcha left a comment

Uh oh!

Kartikey1306 commented Sep 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Kartikey1306 commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem (review finding 2 on #115; eBoot #121)

Change

Verification

Closing issue

Uh oh!

codecov-commenter commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#123 "fix(sha512): drop a carry term that is always zero"

Findings

Why the line was dead

Verification performed for this review

Architecture conformance

Proposed changes

Not checked

Uh oh!

Kartikey1306 commented Sep 15, 2026

Uh oh!

srpatcha left a comment

Choose a reason for hiding this comment

Review — eBoot#123 "fix(sha512): drop a carry term that is always zero"

Status of the previous review (eBoot-123-b828d02f.md)

Findings

Verification performed for this review

Architecture conformance

Proposed changes

Not checked

Uh oh!

Kartikey1306 commented Sep 15, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kartikey1306 commented Sep 14, 2026 •

edited

Loading

codecov-commenter commented Sep 14, 2026 •

edited

Loading

Status of the previous review (`eBoot-123-b828d02f.md`)