Skip to content

fix(runtime): pass a reset type, not a reason, to reset_system - #36

Merged
srpatcha merged 1 commit into
embeddedos-org:masterfrom
sunnyrishy:fix/stack-chk-reset-enum-confusion
Aug 27, 2026
Merged

fix(runtime): pass a reset type, not a reason, to reset_system#36
srpatcha merged 1 commit into
embeddedos-org:masterfrom
sunnyrishy:fix/stack-chk-reset-enum-confusion

Conversation

@sunnyrishy

@sunnyrishy sunnyrishy commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

__stack_chk_fail, the handler that runs when the stack protector detects
memory corruption, asks the system to halt rather than to reset. It does
this by passing a constant from the wrong enum. The comment directly above the
call says "Trigger immediate system reset", so the code does the opposite of
its stated intent.

Type of Change

  • fix — Bug fix
  • build — Build system or dependency changes

Changes

  • core/runtime_services.c — pass EOS_RESET_COLD instead of
    EOS_RESET_SOFTWARE from __stack_chk_fail, and document why the two enums
    must not be mixed.
  • CMakeLists.txt — promote -Wenum-conversion to -Werror=enum-conversion
    on GCC/Clang so this class of mistake cannot come back.

The defect

There are two reset enums, and they overlap numerically:

Enum Meaning Value 2
eos_reset_reason_t (include/eos_types.h) why a reset already happened EOS_RESET_SOFTWARE
eos_reset_type_t (include/eos_runtime_svc.h) which reset to perform EOS_RESET_HALT

eos_rtsvc_reset_system() takes an eos_reset_type_t. The call passed
EOS_RESET_SOFTWARE, a reason. Because both are 2, it silently becomes a
request to halt.

For a secure bootloader this inverts the security response: detecting a
stack-smashing attempt should reset into a controlled boot path, but halting
leaves the device wedged until someone power-cycles it — a detected attack
becomes a denial of service.

GCC had been reporting this the whole time; it was one warning among the ~9 the
native build emits, so it went unread:

core/runtime_services.c:138:28: warning: implicit conversion from
'enum <anonymous>' to 'eos_reset_type_t' [-Wenum-conversion]

Approach

The one-line correction is passing the right constant. The more useful half is
making the mistake impossible to repeat: -Werror=enum-conversion turns any
future cross-enum pass into a hard build failure.

I checked before enabling it — enum-conversion had exactly one occurrence
in the entire tree, the one fixed here, so the flag is clean everywhere else and
does not force unrelated churn.

The flag is scoped to native GCC/Clang builds (NOT CMAKE_CROSSCOMPILING),
deliberately. CI cross-compiles for ARM Cortex-M4, and I have no ARM toolchain
locally, so I am not willing to promote warnings to errors in board code I have
not actually compiled — that is how a guard turns into a broken pipeline. Native
builds cover core/ and hal/, the platform-agnostic code where this class of
mistake arises and where this defect lived, so the guard still catches exactly
what it needs to. MSVC has no equivalent flag, so the MSVC job is unaffected.

If maintainers with the cross toolchains in hand confirm the board tree is
clean, widening this to cross builds is a one-word change.

Testing

  • Unit tests pass (ctest --test-dir build --output-on-failure)
  • Manual testing performed

Native build, GCC 15.2 (MinGW-w64), -DEBLDR_BUILD_TESTS=ON:

  • Before: 1 enum-conversion warning. After: 0.
  • ctest: 13/14 pass, unchanged by this PR. The single failure is
    test_recovery, which fails to link (undefined reference to eos_boot_log_get_head / eos_boot_log_append). That is pre-existing and
    unrelated — it is already addressed by fix: link eboot_stage1 into test_recovery #35.

Verifying the guard actually guards — reintroducing the old constant now breaks
the build rather than warning:

core/runtime_services.c:143:28: error: implicit conversion from
'enum <anonymous>' to 'eos_reset_type_t' [-Werror=enum-conversion]
cc1.exe: some warnings being treated as errors

Considerations and limitations

  • The bug is latent today, not live. eos_rtsvc_reset_system() currently
    does (void)type; and ignores its argument on every platform, so nothing
    misbehaves at runtime right now. I want to be precise about that rather than
    overstate the impact. It goes live the moment any platform honors the
    parameter — which the enum's own COLD/WARM/HALT values clearly
    anticipate — and it would then be a silent, security-relevant wrong action in
    the hardest place to debug one.
  • I did not implement type handling. Making eos_rtsvc_reset_system()
    actually distinguish cold/warm/halt is per-platform work that cannot be
    validated on a host build, so it belongs in its own change. Happy to open a
    follow-up issue if that is wanted.
  • Two enums sharing the EOS_RESET_* prefix for different concepts remains a
    latent trap.
    A rename (e.g. EOS_RESET_REASON_*) would remove the ambiguity
    at the source, but it is a public-header API break, so I kept this PR to the
    defect plus the compiler guard rather than bundling it.

Pre-Submission Checklist

  • Code compiles without warnings — this removes the only enum-conversion
    warning in the tree. Other pre-existing warnings (unused variables in
    ed25519_verify.c, unused parameters in boot_menu.c) are untouched and
    out of scope here.
  • All existing tests pass — 13/14, unchanged by this PR. test_recovery
    fails to link on master and is already addressed by fix: link eboot_stage1 into test_recovery #35.
  • New tests added for new functionality — the -Werror guard is the
    regression test; reintroducing the defect now fails the build.
  • Documentation updated if API changed — n/a, no API change
  • Commit message follows <type>(<scope>): <description>
  • Branch is rebased on latest master (0 commits behind)

Related Issues

None. test_recovery's link failure seen during validation is unrelated and
already covered by #35.

…_fail

__stack_chk_fail called eos_rtsvc_reset_system(EOS_RESET_SOFTWARE). That
constant belongs to eos_reset_reason_t, which reports why a reset already
happened, while the parameter is an eos_reset_type_t, which selects the reset
to perform. The two enums overlap numerically: EOS_RESET_SOFTWARE is 2, and 2
is EOS_RESET_HALT in the parameter's enum.

The call is therefore a request to halt, not to reset, which is the opposite of
what the comment above it states. eos_rtsvc_reset_system currently discards its
type argument, so the defect is latent today. It becomes live as soon as any
platform honors the parameter, and would then turn stack-smashing detection
into a permanent halt instead of a recoverable reset.

Pass EOS_RESET_COLD, and promote -Wenum-conversion to an error on GCC/Clang so
the mistake cannot be reintroduced. That warning had exactly one occurrence in
the tree, this one, so the flag is clean everywhere else.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sunnyrishy sunnyrishy changed the title fix(runtime): pass a reset type, not a reset reason, from __stack_chk… fix(runtime): pass a reset type, not a reason, to reset_system Aug 26, 2026
@srpatcha
srpatcha merged commit 8da1f3c into embeddedos-org:master Aug 27, 2026
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