Skip to content

fix(recovery): reject out-of-range UART writes via the existing helper - #64

Merged
srpatcha merged 2 commits into
embeddedos-org:masterfrom
ShrenikMensinkai:fix/recovery-write-range
Aug 30, 2026
Merged

fix(recovery): reject out-of-range UART writes via the existing helper#64
srpatcha merged 2 commits into
embeddedos-org:masterfrom
ShrenikMensinkai:fix/recovery-write-range

Conversation

@ShrenikMensinkai

Copy link
Copy Markdown
Contributor

Summary

UART recovery WRITE takes offset and length from the serial packet. The write handler declared slot size twice (invalid C) and did not call eos_recovery_write_in_range, so wrap and zero-base checks were skipped. This PR uses that helper once, exposes it in a public header, and adds host tests for those cases.

This addresses:

  • Issue: merge leftover in the write handler; wire-controlled offset could miss range checks
  • Approach: single slot-size variable; NACK unless the existing helper accepts the write
  • Testing: new helper unit tests plus the existing out-of-bounds WRITE protocol test
  • Limits: WRITE path only; VERIFY still has no slot-capacity check; no hardware run on this machine

Type of Change

  • feat — New feature
  • fix — Bug fix
  • docs — Documentation only
  • style — Formatting, no code change
  • refactor — Code restructuring without behavior change
  • test — Add or fix tests
  • build — Build system or dependency changes
  • ci — CI/CD pipeline changes
  • perf — Performance improvement

Changes

  • Removed the duplicate slot-size declaration in the recovery write handler
  • Route WRITE through eos_recovery_write_in_range before receiving payload or programming flash
  • Added include/eos_recovery.h so tests can call the helper and enter recovery
  • Added host tests for zero base, zero length, past-slot offset, base+offset wrap, and a valid in-slot write

Testing

  • Unit tests pass (ctest --test-dir build --output-on-failure)
  • Integration tests pass
  • Manual testing performed
  • New tests added for new functionality

CMake and a C compiler were not available on the Windows host used for this change. Please run a native build with EBLDR_BUILD_TESTS=ON and ctest -R test_recovery before merge.

Pre-Submission Checklist

  • Code compiles without warnings (-Wall -Wextra -Werror for C)
  • All existing tests pass
  • New tests added for new functionality
  • Documentation updated if API changed (new public header)
  • Commit messages follow <type>(<scope>): <description> convention
  • Branch is rebased on latest master

Related Issues

None. Found while reviewing the recovery write path after a recent pull.

Screenshots / Logs

Not applicable. Host unit tests only; no board or UART capture.

Additional Notes

  • Recovery VERIFY still does not apply the slot-capacity check that the slot manager already has. That can be a follow-up.
  • Recovery authentication and RESET-without-auth are unchanged.
  • Tests use simulated flash and a scripted UART, not a real probe.
  • The CRC fail-open path is already fixed upstream and is not part of this PR.

The write handler redeclared slot size and skipped eos_recovery_write_in_range,
so a wire-controlled offset could miss wrap and zero-base checks. Use the
helper once and cover those cases in the host tests.
srpatcha
srpatcha previously approved these changes Aug 30, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified — and the header is the part I most want

You and #63 both route the recovery write through eos_recovery_write_in_range, and both are right to: the helper already exists, already gets the arithmetic right, and a second inline copy of the same bound is how the two drift.

What this PR has that #63 does not is include/eos_recovery.h. On master that file does not exist:

$ git show origin/master:include/eos_recovery.h
fatal: path 'include/eos_recovery.h' does not exist in 'origin/master'

So eos_recovery_write_in_range is a function with no declaration anywhere — reachable only by whoever already knows it is in recovery.c. That is the same shape as eos_hal_linux_register() in eos, which implements the entire host HAL backend and is called from nowhere because nothing declares it. A function nobody can find is a function nobody uses, and the second person to need this bound writes their own instead. Which is exactly what happened: master carries the helper and an inline copy of the same check.

Replacing the inline (uint64_t)offset + len > (uint64_t)slot_size with the helper is also a small correctness gain, not just tidying — the helper checks len before subtracting, so slot_size - len cannot underflow, and it rejects a zero base, which the inline version did not.

Overlap and merge order

This conflicts with #58, and so does #63, for the same mechanical reason: both branches declare uint32_t slot_size = eos_hal_slot_size(slot); at different lines, and the merge keeps one side's call with the other side's declaration. The build then fails with

core/recovery.c:286: error: 'slot_size' undeclared

Restoring that one line fixes it. Verified on top of #58: 0 build errors, ctest 16/16.

Between this and #63 I would take this one, because of the header — and then the delta from #63 is small enough to fold in. Worth the two of you agreeing rather than both rebasing; I have said the same on #63.

Blocker outside this PR

eBoot sets required_signatures: true on master and commits here are unsigned, as are every contributor's — nothing in this repository is mergeable until that policy changes. And master does not build at all; #58 repairs it and must land first.

@srpatcha
srpatcha merged commit ac97e40 into embeddedos-org:master Aug 30, 2026
srpatcha pushed a commit to furqan72672/eBoot that referenced this pull request Aug 31, 2026
Master stopped building/testing clean again after several PRs (embeddedos-org#60,
embeddedos-org#61, embeddedos-org#64, embeddedos-org#67, embeddedos-org#69, embeddedos-org#71) landed back-to-back without an integration
build between them:

- tests/CMakeLists.txt: embeddedos-org#61 registered test_fw_transport without the
  eboot_ prefix embeddedos-org#71's namespace guard now requires, so configure
  aborted with "Test target 'test_fw_transport' is not namespaced."
  Renamed the target to eboot_test_fw_transport (add_test NAME stays
  test_fw_transport per the guard's own guidance).
- tests/CMakeLists.txt: the valgrind foreach block still referenced
  the pre-embeddedos-org#60/embeddedos-org#71 bare target names (test_bootctl, etc.) in
  $<TARGET_FILE:...>, which no longer resolve now that every target is
  eboot_-prefixed. Fixed the generator-expression reference while
  leaving the valgrind_${TEST_NAME} test labels unchanged.
- core/recovery.c: recovery_handle_write() called
  eos_recovery_write_in_range(base, slot_size, ...) with slot_size
  never declared -- embeddedos-org#69 introduced the call but the counterpart
  eos_hal_slot_size(slot) lookup (mirroring the existing
  eos_hal_slot_addr(slot) line right above it) never made it in.
- tests/unit/test_slot_manager.c: two versions of the file's fixture
  and test bodies had been spliced together by a merge (duplicate
  globals/slot_index, an unused old RUN macro next to the real TEST
  macro), and the TEST macro's simulated-flash fixture
  (sim_flash/sim_tick/sim_ops) was referenced but never defined
  anywhere in the file. Rebuilt the file as one coherent suite
  matching its own doc comment and every sibling test file's
  sim_board_ops_t convention; converted the three tests that were
  still plain functions to the same TEST() macro as the rest for
  consistency. Also gave sim_flash_erase real erase_result/
  erased_addr/erased_size bookkeeping -- the generic version copied in
  ignored those script variables entirely, which the erase test relies
  on.

Verified: cmake configure + build, 0 errors. ctest 17/17 (non-valgrind)
and 32/32 including valgrind, all passing -- including
test_recovery's real HAL-level exercise of the new slot_size bounds
check and test_slot_manager's erase-failure/erase-bookkeeping cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VvWBEZhDegTQMaqVtry2mM
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