Skip to content

Fix eos_crc32 signature mismatch and duplicate variable in recovery write path - #65

Closed
Jayakrishnan9539 wants to merge 3 commits into
embeddedos-org:masterfrom
Jayakrishnan9539:fix/crc32-signature-and-duplicate-var
Closed

Fix eos_crc32 signature mismatch and duplicate variable in recovery write path#65
Jayakrishnan9539 wants to merge 3 commits into
embeddedos-org:masterfrom
Jayakrishnan9539:fix/crc32-signature-and-duplicate-var

Conversation

@Jayakrishnan9539

Copy link
Copy Markdown

Problem

Two independent bugs currently block the eBoot host build
(cmake -DEBLDR_BUILD_TESTS=ON) from compiling:

  1. include/eos_image.h declared eos_crc32() as
    int eos_crc32(uint32_t addr, size_t len, uint32_t *out);
    but this disagreed with both the function's own doc comment
    just above it (which describes a 2-argument function that
    returns the CRC32 value directly) and the actual implementation
    in core/image_verify.c (uint32_t eos_crc32(uint32_t addr, size_t len)). No caller anywhere in the codebase uses the
    3-argument form — this was a stale/copy-pasted prototype from
    the sibling function eos_crc32_checked().

  2. core/recovery.c's recovery_handle_write() declared
    uint32_t slot_size = eos_hal_slot_size(slot); twice in the
    same function — once near the top, and again immediately before
    a later bounds-check comment (apparently left over from when
    that bounds check was added). The redeclaration is a compile
    error under MSVC (and a redundant no-op under GCC).

Approach

  • Corrected the eos_crc32 prototype in eos_image.h to match its
    own documentation and the real implementation.
  • Removed the duplicate slot_size declaration in recovery.c,
    keeping the existing one and its later use in the bounds check
    unchanged.

Testing

Built with MSVC (Visual Studio 2026 Build Tools, cmake -B build_test
-DEBLDR_BUILD_TESTS=ON) and confirmed both core/image_verify.c and
core/recovery.c compile with zero errors and zero warnings.

Limitations / additional considerations

While verifying the fix, I found a separate, unrelated, pre-existing
bug in core/ed25519_verify.c: eos_ed25519_verify() contains what
appears to be leftover code from an incomplete merge — a duplicate,
inconsistent "compute k" block (referencing types/functions like
sha512_ctx_t and sc_reduce that don't exist elsewhere in the file),
and the function's actual signature-verification comparison (the
diff variable it returns) is never computed. Checking git blame,
this has been present since the file was first added, so there's no
earlier working version to restore. I did not attempt to fix this as
part of this PR: this is the codebase's signature-verification logic
for trusted boot, and a mistaken "fix" here would be far worse than
a build error, since incorrect signature verification can silently
accept unauthorized firmware. This deserves focused review by
someone who can verify the cryptographic correctness of the fix,
rather than a quick patch.

@srpatcha

Copy link
Copy Markdown
Member

Correct diagnosis — and you are the fourth person to find it

Both defects are real. I confirmed the first independently before seeing this PR:

core/image_verify.c:63: error: conflicting types for 'eos_crc32';
  have 'uint32_t(uint32_t, size_t)'

include/eos_image.h declares int eos_crc32(uint32_t, size_t, uint32_t *); core/image_verify.c defines uint32_t eos_crc32(uint32_t, size_t). Your reading is right that the header is the stale side — its own doc comment describes the two-argument form, and no caller uses the three-argument one. That came from #38 and #52 landing different API shapes without either rebasing on the other.

The overlap you should know about

Four PRs now fix this same breakage: #55, #57, #58 and this one. That is not a criticism of any of them — master has not compiled since 28 Aug, and four people independently doing the work is what happens when a repository stays red.

I have approved #58, and would like it to land first. It is the widest: besides eos_crc32 and the duplicate slot_size, it also catches that core/sha512.c and core/rollback.c were never added to CMakeLists.txt — two merged security features compiled into nothing — that the EBLDR_BOARD dispatch listed 83 boards twice with a stray FATAL_ERROR spliced into the kalimba branch, and that stage0/jump_stage1.c has never been built on any board because stage1_expected_hash is defined nowhere while EBLDR_VERIFY_STAGE1 defaults ON.

Both of your defects are in #58's set, so after it lands this branch reduces to nothing. I would rather say that plainly than leave it open looking like it still has work in it.

What I would rather have from you

If you want a follow-up in the same area, the thing #58 does not do is stop this recurring. core/ gains a file and nobody notices it was never added to CMakeLists.txt; a header and its implementation disagree and nobody notices until someone builds. Both are mechanically checkable:

  • a test asserting every core/*.c appears in CMakeLists.txtfix: restore host build and harden Ed25519 verification #57 has a tests/unit/test_cmake_core_sources.py doing exactly this, and it is the single most valuable file in that PR
  • building the host target in CI on every push, which would have caught the eos_crc32 mismatch the day it landed

Either would be worth more than this fix, because they prevent the next four PRs.

Verification

Confirmed master does not build, and that the eos_crc32 mismatch is one of two errors. Not merged this branch with #58 — they touch the same lines and #58 supersedes them.

Blocker outside this PR

eBoot sets required_signatures: true and commits here are unsigned, as are every contributor's. Nothing here is mergeable until that changes; raised with the maintainer.

@Jayakrishnan9539

Copy link
Copy Markdown
Author

Thanks for the clear explanation and detailed context, @srpatcha!

Makes complete sense to close this in favor of #58 since it covers
the broader set of fixes across the build system.

I'll look into setting up host-target CI compilation as a follow-up,
so header/implementation mismatches and missing CMake sources get
caught automatically on every push. I've also set up commit signing
(SSH) going forward.

Closing this branch in favor of #58.

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