From c9fe84c4b26c990cd956712da8c7b9a205d675cc Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Fri, 4 Sep 2026 00:42:14 +0530 Subject: [PATCH 1/8] Fix anti-rollback to use authenticated TLV counter --- core/fw_update.c | 18 ++++++++++++++---- stage1/jump_app.c | 22 ++++++++++++++++++---- tests/unit/test_tlv_auth.c | 37 +++++++++++++++++++++++++++++++++++-- 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/core/fw_update.c b/core/fw_update.c index 269349d..1e2157b 100644 --- a/core/fw_update.c +++ b/core/fw_update.c @@ -10,6 +10,7 @@ #include "eos_fw_update.h" #include "eos_hal.h" #include "eos_fwsvc.h" +#include "eos_rollback.h" #include int eos_fw_update_begin(eos_fw_update_ctx_t *ctx, eos_slot_t slot) @@ -184,10 +185,19 @@ int eos_fw_update_finalize(eos_fw_update_ctx_t *ctx, eos_upgrade_mode_t mode) } } - /* Anti-rollback check */ - extern int eos_image_check_rollback(uint32_t candidate_version); - int rb_rc = eos_image_check_rollback(ctx->header.image_version); - if (rb_rc != EOS_OK && rb_rc != EOS_ERR_NOT_SUPPORTED) { + /* Persistent floor: authenticated TLV security counter, not image_version. + * Finalize installs the image; it does not boot it, so the counter is + * verified here but not staged. */ + uint32_t img_counter = 0; + int rb_rc = eos_rollback_read_image_counter(ctx->target_addr, &img_counter); + if (rb_rc != EOS_OK) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = rb_rc; + return rb_rc; + } + + rb_rc = eos_rollback_verify(img_counter); + if (rb_rc != EOS_OK) { ctx->state = EOS_FW_STATE_ERROR; ctx->last_error = rb_rc; return rb_rc; diff --git a/stage1/jump_app.c b/stage1/jump_app.c index fe8b3e2..e69a717 100644 --- a/stage1/jump_app.c +++ b/stage1/jump_app.c @@ -14,6 +14,7 @@ #include "eos_hal.h" #include "eos_bootctl.h" #include "eos_image.h" +#include "eos_rollback.h" #include "eos_mpu_boot.h" /* Forward declarations from boot_log */ @@ -56,13 +57,26 @@ int eboot_jump_to_app(eos_bootctl_t *bctl, eos_slot_t slot) return rc; } - /* Anti-rollback check */ - extern int eos_image_check_rollback(uint32_t candidate_version); - rc = eos_image_check_rollback(hdr.image_version); - if (rc != EOS_OK && rc != EOS_ERR_NOT_SUPPORTED) { + /* Persistent floor from the device monotonic counter. The value compared + * here is the authenticated EOS_TLV_MIN_SEC_VER, not image_version: + * firmware versions and OTP security counters are different scales, and + * comparing the former to the latter lets a high version / low TLV image + * walk past the floor. Same sequence as eos_secure_boot() step 5b. */ + uint32_t img_counter = 0; + rc = eos_rollback_read_image_counter(addr, &img_counter); + if (rc != EOS_OK) { eos_boot_log_append(EOS_LOG_IMAGE_INVALID, slot, rc); return rc; } + + rc = eos_rollback_verify(img_counter); + if (rc != EOS_OK) { + eos_boot_log_append(EOS_LOG_IMAGE_INVALID, slot, rc); + return rc; + } + + /* Raised only once the image is confirmed good; see eos_bootctl_confirm(). */ + eos_rollback_stage(img_counter); /* Increment boot attempts before jumping */ eos_bootctl_increment_attempts(bctl); diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c index 28c0e41..be3b543 100644 --- a/tests/unit/test_tlv_auth.c +++ b/tests/unit/test_tlv_auth.c @@ -290,11 +290,43 @@ TEST(test_oversized_tlv_len_is_rejected) ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_ERR_INVALID); } +/* + * Regression: the production boot and update paths used to feed + * hdr.image_version into eos_image_check_rollback(), which compares that + * number to the OTP monotonic floor. Firmware versions (0x00MMmmpp) and + * TLV security counters are different scales. An old image with + * image_version = 0x00010000 and authenticated MIN_SEC_VER = 3 therefore + * passed a floor of 9, defeating the anti-rollback gate. + * + * The hardware-floor decision is eos_rollback_verify(tlv_counter). + */ +extern int eos_image_check_rollback(uint32_t candidate_version); + +TEST(test_hw_floor_uses_tlv_counter_not_image_version) +{ + build_image(3, true, true); + + eos_image_header_t hdr; + ASSERT(eos_image_parse_header(SLOT_A_ADDR, &hdr) == EOS_OK); + ASSERT(hdr.image_version == 0x00010000u); + + uint32_t counter = 0; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_OK); + ASSERT(counter == 3); + + sim_counter = 9; + + /* The old production check: image_version against the HW floor. */ + ASSERT(eos_image_check_rollback(hdr.image_version) == EOS_OK); + + /* The authenticated counter is below the floor and must be rejected. */ + ASSERT(eos_rollback_verify(counter) == EOS_ERR_ANTI_ROLLBACK); +} + int main(void) { printf("TLV authentication (anti-rollback counter)\n\n"); - tests_run = 8; run_test_authenticated_tlv_counter_is_read(); run_test_tampered_tlv_counter_is_rejected(); run_test_tamper_is_invisible_to_signature_and_integrity(); @@ -302,8 +334,9 @@ int main(void) run_test_image_without_tlv_area_still_reports_zero(); run_test_tlv_binding_fields_are_inside_the_signed_prefix(); run_test_oversized_tlv_len_is_rejected(); + run_test_hw_floor_uses_tlv_counter_not_image_version(); - tests_run = 7; + tests_run = 8; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; } From 8c9a363fe4fa57e1c2097187bcd55bb4f150e293 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sat, 5 Sep 2026 16:58:24 +0530 Subject: [PATCH 2/8] test(rollback): cover authenticated counter in update finalize --- core/image_verify.c | 20 ---- tests/CMakeLists.txt | 7 +- tests/unit/test_fw_update.c | 194 ++++++++++++++++++++++++++++++++++++ tests/unit/test_tlv_auth.c | 28 ++++-- 4 files changed, 220 insertions(+), 29 deletions(-) create mode 100644 tests/unit/test_fw_update.c diff --git a/core/image_verify.c b/core/image_verify.c index 95a6e8e..5ebc027 100644 --- a/core/image_verify.c +++ b/core/image_verify.c @@ -226,23 +226,3 @@ int eos_image_check_version(uint32_t candidate_version, uint32_t min_version) return EOS_OK; } - -int eos_image_check_rollback(uint32_t candidate_version) -{ - /* Read monotonic counter from hardware */ - uint32_t hw_min_version = 0; - int rc = eos_hal_monotonic_read(&hw_min_version); - if (rc == EOS_ERR_NOT_SUPPORTED) { - /* No HW counter — version check only */ - return EOS_OK; - } - if (rc != EOS_OK) { - return rc; - } - - if (candidate_version < hw_min_version) { - return EOS_ERR_ANTI_ROLLBACK; - } - - return EOS_OK; -} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..47e04a0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -40,6 +40,11 @@ add_executable(eboot_test_fw_transport unit/test_fw_transport.c) target_link_libraries(eboot_test_fw_transport PRIVATE eboot_core) add_test(NAME test_fw_transport COMMAND eboot_test_fw_transport) +# --- test_fw_update: firmware update finalize / anti-rollback wiring --- +add_executable(eboot_test_fw_update unit/test_fw_update.c) +target_link_libraries(eboot_test_fw_update PRIVATE eboot_core) +add_test(NAME test_fw_update COMMAND eboot_test_fw_update) + # --- test_slot_size_bounds: verify_slot() must reject image_size > slot capacity --- add_executable(eboot_test_slot_size_bounds unit/test_slot_size_bounds.c) target_link_libraries(eboot_test_slot_size_bounds PRIVATE eboot_core) @@ -119,7 +124,7 @@ if(VALGRIND) test_multicore test_board_registry test_slot_manager test_boot_log test_image_verify test_image_abi test_recovery test_slot_size_bounds test_fw_transport - test_tlv_auth) + test_fw_update test_tlv_auth) add_test( NAME valgrind_${TEST_NAME} COMMAND ${VALGRIND} ${VALGRIND_OPTS} $ diff --git a/tests/unit/test_fw_update.c b/tests/unit/test_fw_update.c new file mode 100644 index 0000000..d1e0540 --- /dev/null +++ b/tests/unit/test_fw_update.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_fw_update.c + * @brief Host tests for the firmware-update pipeline. + * + * The anti-rollback regression drives eos_fw_update_finalize() so the + * production wiring in core/fw_update.c is what reads the authenticated + * TLV counter and compares it to the hardware floor. + */ + +#include "eos_fw_update.h" +#include "eos_image.h" +#include "eos_image_tlv.h" +#include "eos_crypto_boot.h" +#include "eos_hal.h" +#include +#include +#include + +#define SIM_FLASH_SIZE (128 * 1024) +#define SIM_SLOT_A_ADDR 0x00000u +#define SIM_SLOT_A_SIZE 0x08000u +#define SIM_SLOT_B_ADDR 0x10000u +#define SIM_SLOT_B_SIZE 0x08000u +#define PAYLOAD_SIZE 256u + +static uint8_t sim_flash[SIM_FLASH_SIZE]; +static uint32_t sim_counter; + +static int sim_range_ok(uint32_t addr, size_t len) +{ + return len <= SIM_FLASH_SIZE && (size_t)addr <= SIM_FLASH_SIZE - len; +} + +static int sim_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (!sim_range_ok(addr, len)) return EOS_ERR_FLASH; + memcpy(buf, &sim_flash[addr], len); + return EOS_OK; +} + +static int sim_flash_write(uint32_t addr, const void *buf, size_t len) +{ + if (!sim_range_ok(addr, len)) return EOS_ERR_FLASH; + memcpy(&sim_flash[addr], buf, len); + return EOS_OK; +} + +static int sim_flash_erase(uint32_t addr, size_t len) +{ + if (!sim_range_ok(addr, len)) return EOS_ERR_FLASH; + memset(&sim_flash[addr], 0xFF, len); + return EOS_OK; +} + +static int sim_monotonic_read(uint32_t *value) +{ + if (!value) return EOS_ERR_INVALID; + *value = sim_counter; + return EOS_OK; +} + +static const eos_board_ops_t sim_ops = { + .flash_base = 0, + .flash_size = SIM_FLASH_SIZE, + .slot_a_addr = SIM_SLOT_A_ADDR, + .slot_a_size = SIM_SLOT_A_SIZE, + .slot_b_addr = SIM_SLOT_B_ADDR, + .slot_b_size = SIM_SLOT_B_SIZE, + .flash_read = sim_flash_read, + .flash_write = sim_flash_write, + .flash_erase = sim_flash_erase, + .monotonic_read = sim_monotonic_read, +}; + +static int tests_run = 0; +static int tests_passed = 0; + +#define ASSERT(cond) do { \ + if (!(cond)) { \ + printf("[FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + exit(1); \ + } \ +} while(0) + +#define TEST(name) \ + static void name(void); \ + static void run_##name(void) { \ + memset(sim_flash, 0xFF, sizeof(sim_flash)); \ + sim_counter = 0; \ + eos_hal_init(&sim_ops); \ + printf(" %-58s ", #name); \ + name(); \ + tests_passed++; \ + printf("[PASS]\n"); \ + } \ + static void name(void) + +/* [tlv_info(4)][entry_hdr(4)][uint32 value] */ +#define TLV_AREA_LEN (uint16_t)(sizeof(eos_tlv_info_t) + \ + sizeof(eos_tlv_entry_hdr_t) + sizeof(uint32_t)) +#define TLV_VALUE_OFF (uint32_t)(sizeof(eos_tlv_info_t) + sizeof(eos_tlv_entry_hdr_t)) +#define STREAMED_LEN (sizeof(eos_image_header_t) + PAYLOAD_SIZE) +#define IMAGE_BUF_LEN (STREAMED_LEN + TLV_AREA_LEN) + +static void build_image(uint8_t *out, uint32_t sec_ver) +{ + uint8_t payload[PAYLOAD_SIZE]; + uint32_t i; + + for (i = 0; i < PAYLOAD_SIZE; i++) + payload[i] = (uint8_t)(i * 7u + 1u); + + eos_image_header_t hdr; + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(eos_image_header_t); + hdr.image_size = PAYLOAD_SIZE; + hdr.load_addr = 0; + hdr.entry_addr = 0; + hdr.image_version = 0x00010000u; + hdr.flags = EOS_IMG_FLAG_HASH_SHA256; + eos_sha256(payload, PAYLOAD_SIZE, hdr.hash); + hdr.sig_type = EOS_SIG_NONE; + hdr.sig_len = 0; + + uint8_t tlv[TLV_AREA_LEN]; + eos_tlv_info_t info = { EOS_TLV_INFO_MAGIC, TLV_AREA_LEN }; + eos_tlv_entry_hdr_t ent = { EOS_TLV_MIN_SEC_VER, sizeof(uint32_t) }; + memcpy(tlv, &info, sizeof(info)); + memcpy(tlv + sizeof(info), &ent, sizeof(ent)); + memcpy(tlv + TLV_VALUE_OFF, &sec_ver, sizeof(sec_ver)); + + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + eos_sha256(tlv, TLV_AREA_LEN, digest); + hdr.tlv_len = TLV_AREA_LEN; + memcpy(hdr.tlv_hash, digest, EOS_IMG_TLV_HASH_LEN); + + memset(out, 0, IMAGE_BUF_LEN); + memcpy(out, &hdr, sizeof(hdr)); + memcpy(out + sizeof(hdr), payload, PAYLOAD_SIZE); + memcpy(out + STREAMED_LEN, tlv, TLV_AREA_LEN); +} + +/* + * eos_fw_update_write() streams header + image_size payload only. The + * authenticated TLV sits after that; finalize reads it from flash at + * target_addr + hdr_size + image_size. Place those bytes so the production + * read sees MIN_SEC_VER rather than erased 0xFF. + */ +static void place_tlv_after_payload(const eos_fw_update_ctx_t *ctx, + const uint8_t *image) +{ + uint32_t tlv_addr = ctx->target_addr + (uint32_t)sizeof(eos_image_header_t) + + PAYLOAD_SIZE; + ASSERT(eos_hal_flash_write(tlv_addr, image + STREAMED_LEN, TLV_AREA_LEN) + == EOS_OK); +} + +TEST(test_finalize_rejects_tlv_counter_below_hw_floor) +{ + uint8_t image[IMAGE_BUF_LEN]; + eos_image_header_t hdr; + eos_fw_update_ctx_t ctx; + + build_image(image, 3); + memcpy(&hdr, image, sizeof(hdr)); + ASSERT(hdr.image_version == 0x00010000u); + ASSERT(hdr.tlv_len == TLV_AREA_LEN); + + ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_write(&ctx, image, STREAMED_LEN) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + place_tlv_after_payload(&ctx, image); + + sim_counter = 9; + + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_ANTI_ROLLBACK); +} + +int main(void) +{ + printf("Firmware update (finalize anti-rollback)\n\n"); + + run_test_finalize_rejects_tlv_counter_below_hw_floor(); + + tests_run = 1; + printf("\n%d/%d passed\n", tests_passed, tests_run); + return tests_passed == tests_run ? 0 : 1; +} diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c index be3b543..318c7a0 100644 --- a/tests/unit/test_tlv_auth.c +++ b/tests/unit/test_tlv_auth.c @@ -291,16 +291,28 @@ TEST(test_oversized_tlv_len_is_rejected) } /* - * Regression: the production boot and update paths used to feed - * hdr.image_version into eos_image_check_rollback(), which compares that - * number to the OTP monotonic floor. Firmware versions (0x00MMmmpp) and - * TLV security counters are different scales. An old image with + * The production boot and update paths used to feed hdr.image_version into + * a hardware-floor comparison. Firmware versions (0x00MMmmpp) and TLV + * security counters are different scales. An old image with * image_version = 0x00010000 and authenticated MIN_SEC_VER = 3 therefore - * passed a floor of 9, defeating the anti-rollback gate. + * passed a floor of 9. * - * The hardware-floor decision is eos_rollback_verify(tlv_counter). + * That helper is gone from production. This is the old comparison, kept + * only so the regression can show why image_version must not be the floor + * input. The hardware-floor decision is eos_rollback_verify(tlv_counter). */ -extern int eos_image_check_rollback(uint32_t candidate_version); +static int legacy_broken_version_floor_check(uint32_t candidate_version) +{ + uint32_t hw_min_version = 0; + int rc = eos_hal_monotonic_read(&hw_min_version); + if (rc == EOS_ERR_NOT_SUPPORTED) + return EOS_OK; + if (rc != EOS_OK) + return rc; + if (candidate_version < hw_min_version) + return EOS_ERR_ANTI_ROLLBACK; + return EOS_OK; +} TEST(test_hw_floor_uses_tlv_counter_not_image_version) { @@ -317,7 +329,7 @@ TEST(test_hw_floor_uses_tlv_counter_not_image_version) sim_counter = 9; /* The old production check: image_version against the HW floor. */ - ASSERT(eos_image_check_rollback(hdr.image_version) == EOS_OK); + ASSERT(legacy_broken_version_floor_check(hdr.image_version) == EOS_OK); /* The authenticated counter is below the floor and must be rejected. */ ASSERT(eos_rollback_verify(counter) == EOS_ERR_ANTI_ROLLBACK); From b83630401f210dca7f3f6772bd83f26e3b375059 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sat, 5 Sep 2026 21:48:28 +0530 Subject: [PATCH 3/8] fix(fw-update): stream and validate authenticated TLVs --- core/fw_update.c | 74 +++++++++- core/rollback.c | 44 ++++++ include/eos_fw_update.h | 15 +- tests/CMakeLists.txt | 7 +- tests/unit/test_fw_update.c | 89 +++++++++--- tests/unit/test_jump_app.c | 271 ++++++++++++++++++++++++++++++++++++ tests/unit/test_tlv_auth.c | 15 +- 7 files changed, 480 insertions(+), 35 deletions(-) create mode 100644 tests/unit/test_jump_app.c diff --git a/core/fw_update.c b/core/fw_update.c index 1e2157b..aea9c78 100644 --- a/core/fw_update.c +++ b/core/fw_update.c @@ -11,6 +11,7 @@ #include "eos_hal.h" #include "eos_fwsvc.h" #include "eos_rollback.h" +#include "eos_image_tlv.h" #include int eos_fw_update_begin(eos_fw_update_ctx_t *ctx, eos_slot_t slot) @@ -61,9 +62,10 @@ static void update_crc(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t len int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t len) { if (!ctx || !data || len == 0) return EOS_ERR_INVALID; - if (ctx->state == EOS_FW_STATE_ERROR || ctx->state == EOS_FW_STATE_COMPLETE) { - return EOS_ERR_INVALID; - } + if (ctx->state == EOS_FW_STATE_ERROR || ctx->state == EOS_FW_STATE_COMPLETE || + ctx->state == EOS_FW_STATE_IDLE || ctx->state == EOS_FW_STATE_VERIFY) { + return EOS_ERR_INVALID; + } size_t offset = 0; @@ -86,18 +88,37 @@ int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t le return EOS_ERR_NO_IMAGE; } - /* Bug Fix: Prevent integer overflow in size checks by checking bounds safely */ + /* header + payload + TLV must fit the slot, overflow-safe. */ if (ctx->header.image_size == 0 || - ctx->slot_size < sizeof(eos_image_header_t) || - ctx->header.image_size > ctx->slot_size - sizeof(eos_image_header_t)) { + ctx->slot_size < sizeof(eos_image_header_t)) { ctx->state = EOS_FW_STATE_ERROR; ctx->last_error = EOS_ERR_FULL; return EOS_ERR_FULL; } + + uint32_t remain = ctx->slot_size - (uint32_t)sizeof(eos_image_header_t); + if (ctx->header.image_size > remain) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = EOS_ERR_FULL; + return EOS_ERR_FULL; + } + remain -= ctx->header.image_size; + if (ctx->header.tlv_len > remain) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = EOS_ERR_FULL; + return EOS_ERR_FULL; + } + if (ctx->header.tlv_len > EOS_TLV_MAX_SIZE) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = EOS_ERR_INVALID; + return EOS_ERR_INVALID; + } ctx->header_parsed = true; ctx->payload_total = ctx->header.image_size; ctx->payload_written = 0; + ctx->tlv_total = ctx->header.tlv_len; + ctx->tlv_written = 0; /* Write header to flash */ int rc = eos_hal_flash_write(ctx->target_addr, ctx->hdr_buf, @@ -135,11 +156,41 @@ int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t le ctx->write_addr += (uint32_t)payload_len; ctx->payload_written += (uint32_t)payload_len; + offset += payload_len; if (ctx->payload_written >= ctx->payload_total) { - ctx->state = EOS_FW_STATE_VERIFY; + ctx->state = (ctx->tlv_total == 0) ? EOS_FW_STATE_VERIFY + : EOS_FW_STATE_TLV; } } + + /* Authenticated TLV tail: exactly tlv_len bytes after the payload. */ + if (ctx->state == EOS_FW_STATE_TLV && offset < len) { + size_t remaining = ctx->tlv_total - ctx->tlv_written; + size_t tlv_len = len - offset; + if (tlv_len > remaining) tlv_len = remaining; + + int rc = eos_hal_flash_write(ctx->write_addr, data + offset, tlv_len); + if (rc != EOS_OK) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = rc; + return rc; + } + + ctx->write_addr += (uint32_t)tlv_len; + ctx->tlv_written += (uint32_t)tlv_len; + offset += tlv_len; + + if (ctx->tlv_written >= ctx->tlv_total) + ctx->state = EOS_FW_STATE_VERIFY; + } + + /* Bytes past the image container must not be discarded as success. */ + if (offset < len) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = EOS_ERR_INVALID; + return EOS_ERR_INVALID; + } ctx->total_received += (uint32_t)len; return EOS_OK; @@ -149,6 +200,15 @@ int eos_fw_update_finalize(eos_fw_update_ctx_t *ctx, eos_upgrade_mode_t mode) { if (!ctx) return EOS_ERR_INVALID; if (ctx->state != EOS_FW_STATE_VERIFY) return EOS_ERR_INVALID; + + /* VERIFY is only entered after the full TLV tail is received. Reject a + * context that was forced here without those bytes so finalize cannot + * hash an erased or partial TLV region. */ + if (ctx->tlv_written != ctx->header.tlv_len) { + ctx->state = EOS_FW_STATE_ERROR; + ctx->last_error = EOS_ERR_INVALID; + return EOS_ERR_INVALID; + } /* Verify SHA-256 hash */ if (ctx->header.flags & EOS_IMG_FLAG_HASH_SHA256) { diff --git a/core/rollback.c b/core/rollback.c index 39405e7..d6d4153 100644 --- a/core/rollback.c +++ b/core/rollback.c @@ -56,6 +56,40 @@ static int tlv_area_matches_header(const eos_image_header_t *hdr, return (diff == 0) ? EOS_OK : EOS_ERR_INVALID; } +/** + * Remaining bytes from @p image_addr to the end of the HAL slot that + * contains it. The image header is stored at the slot base on every + * production path; matching by containment still works if a caller + * passes that base. + */ +static int slot_remain_from(uint32_t image_addr, uint32_t *remain_out) +{ + static const eos_slot_t slots[] = { + EOS_SLOT_A, EOS_SLOT_B, EOS_SLOT_RECOVERY + }; + size_t i; + + if (!remain_out) + return EOS_ERR_INVALID; + + for (i = 0; i < sizeof(slots) / sizeof(slots[0]); i++) { + uint32_t base = eos_hal_slot_addr(slots[i]); + uint32_t size = eos_hal_slot_size(slots[i]); + + if (base == 0 || size == 0) + continue; + if (image_addr < base) + continue; + if (image_addr - base >= size) + continue; + + *remain_out = size - (image_addr - base); + return EOS_OK; + } + + return EOS_ERR_INVALID; +} + int eos_rollback_read_image_counter(uint32_t image_addr, uint32_t *counter_out) { if (!counter_out) return EOS_ERR_INVALID; @@ -66,6 +100,16 @@ int eos_rollback_read_image_counter(uint32_t image_addr, uint32_t *counter_out) int rc = eos_image_parse_header(image_addr, &hdr); if (rc != EOS_OK) return rc; + /* hdr_size + image_size + tlv_len must fit the slot, overflow-safe. */ + uint32_t remain = 0; + rc = slot_remain_from(image_addr, &remain); + if (rc != EOS_OK) return rc; + if (hdr.hdr_size > remain) return EOS_ERR_INVALID; + remain -= hdr.hdr_size; + if (hdr.image_size > remain) return EOS_ERR_INVALID; + remain -= hdr.image_size; + if (hdr.tlv_len > remain) return EOS_ERR_INVALID; + /* The TLV area follows the header and payload. */ uint32_t tlv_addr = image_addr; if (tlv_addr + hdr.hdr_size < tlv_addr) return EOS_ERR_INVALID; diff --git a/include/eos_fw_update.h b/include/eos_fw_update.h index 2adacdb..9614e4d 100644 --- a/include/eos_fw_update.h +++ b/include/eos_fw_update.h @@ -36,9 +36,10 @@ typedef enum { EOS_FW_STATE_IDLE = 0, EOS_FW_STATE_HEADER = 1, EOS_FW_STATE_PAYLOAD = 2, - EOS_FW_STATE_VERIFY = 3, - EOS_FW_STATE_COMPLETE = 4, - EOS_FW_STATE_ERROR = 5, + EOS_FW_STATE_TLV = 3, + EOS_FW_STATE_VERIFY = 4, + EOS_FW_STATE_COMPLETE = 5, + EOS_FW_STATE_ERROR = 6, } eos_fw_update_state_t; typedef struct { @@ -56,6 +57,8 @@ typedef struct { /* Streaming write state */ uint32_t payload_written; uint32_t payload_total; + uint32_t tlv_written; + uint32_t tlv_total; uint32_t write_addr; /* Integrity tracking */ @@ -79,8 +82,10 @@ typedef struct { int eos_fw_update_begin(eos_fw_update_ctx_t *ctx, eos_slot_t slot); /** - * Write a chunk of firmware data. Handles header parsing and - * payload streaming automatically. + * Write a chunk of firmware data. Handles header parsing, payload + * streaming, and the authenticated TLV tail (hdr.tlv_len bytes after + * the payload). Extra bytes beyond that container are rejected; + * they are not discarded with EOS_OK. * * @param ctx Update context. * @param data Chunk of firmware data. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 47e04a0..ba208f1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -45,6 +45,11 @@ add_executable(eboot_test_fw_update unit/test_fw_update.c) target_link_libraries(eboot_test_fw_update PRIVATE eboot_core) add_test(NAME test_fw_update COMMAND eboot_test_fw_update) +# --- test_jump_app: stage-1 jump uses the authenticated TLV counter --- +add_executable(eboot_test_jump_app unit/test_jump_app.c) +target_link_libraries(eboot_test_jump_app PRIVATE eboot_stage1) +add_test(NAME test_jump_app COMMAND eboot_test_jump_app) + # --- test_slot_size_bounds: verify_slot() must reject image_size > slot capacity --- add_executable(eboot_test_slot_size_bounds unit/test_slot_size_bounds.c) target_link_libraries(eboot_test_slot_size_bounds PRIVATE eboot_core) @@ -124,7 +129,7 @@ if(VALGRIND) test_multicore test_board_registry test_slot_manager test_boot_log test_image_verify test_image_abi test_recovery test_slot_size_bounds test_fw_transport - test_fw_update test_tlv_auth) + test_fw_update test_jump_app test_tlv_auth) add_test( NAME valgrind_${TEST_NAME} COMMAND ${VALGRIND} ${VALGRIND_OPTS} $ diff --git a/tests/unit/test_fw_update.c b/tests/unit/test_fw_update.c index d1e0540..9b4f284 100644 --- a/tests/unit/test_fw_update.c +++ b/tests/unit/test_fw_update.c @@ -8,13 +8,15 @@ * * The anti-rollback regression drives eos_fw_update_finalize() so the * production wiring in core/fw_update.c is what reads the authenticated - * TLV counter and compares it to the hardware floor. + * TLV counter and compares it to the hardware floor. The TLV tail is + * delivered through eos_fw_update_write(), not poked into flash. */ #include "eos_fw_update.h" #include "eos_image.h" #include "eos_image_tlv.h" #include "eos_crypto_boot.h" +#include "eos_bootctl.h" #include "eos_hal.h" #include #include @@ -70,6 +72,9 @@ static const eos_board_ops_t sim_ops = { .slot_a_size = SIM_SLOT_A_SIZE, .slot_b_addr = SIM_SLOT_B_ADDR, .slot_b_size = SIM_SLOT_B_SIZE, + .bootctl_addr = 0x0E000u, + .bootctl_backup_addr = 0x0E200u, + .log_addr = 0x0E400u, .flash_read = sim_flash_read, .flash_write = sim_flash_write, .flash_erase = sim_flash_erase, @@ -106,13 +111,18 @@ static int tests_passed = 0; #define STREAMED_LEN (sizeof(eos_image_header_t) + PAYLOAD_SIZE) #define IMAGE_BUF_LEN (STREAMED_LEN + TLV_AREA_LEN) -static void build_image(uint8_t *out, uint32_t sec_ver) +static void fill_payload(uint8_t *payload) { - uint8_t payload[PAYLOAD_SIZE]; uint32_t i; - for (i = 0; i < PAYLOAD_SIZE; i++) payload[i] = (uint8_t)(i * 7u + 1u); +} + +static void build_image(uint8_t *out, uint32_t sec_ver) +{ + uint8_t payload[PAYLOAD_SIZE]; + + fill_payload(payload); eos_image_header_t hdr; memset(&hdr, 0, sizeof(hdr)); @@ -146,22 +156,14 @@ static void build_image(uint8_t *out, uint32_t sec_ver) memcpy(out + STREAMED_LEN, tlv, TLV_AREA_LEN); } -/* - * eos_fw_update_write() streams header + image_size payload only. The - * authenticated TLV sits after that; finalize reads it from flash at - * target_addr + hdr_size + image_size. Place those bytes so the production - * read sees MIN_SEC_VER rather than erased 0xFF. - */ -static void place_tlv_after_payload(const eos_fw_update_ctx_t *ctx, - const uint8_t *image) +static void persist_bootctl(void) { - uint32_t tlv_addr = ctx->target_addr + (uint32_t)sizeof(eos_image_header_t) - + PAYLOAD_SIZE; - ASSERT(eos_hal_flash_write(tlv_addr, image + STREAMED_LEN, TLV_AREA_LEN) - == EOS_OK); + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + ASSERT(eos_bootctl_save(&bctl) == EOS_OK); } -TEST(test_finalize_rejects_tlv_counter_below_hw_floor) +TEST(test_write_streams_tlv_then_finalize_rejects_below_floor) { uint8_t image[IMAGE_BUF_LEN]; eos_image_header_t hdr; @@ -173,22 +175,67 @@ TEST(test_finalize_rejects_tlv_counter_below_hw_floor) ASSERT(hdr.tlv_len == TLV_AREA_LEN); ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); - ASSERT(eos_fw_update_write(&ctx, image, STREAMED_LEN) == EOS_OK); + ASSERT(eos_fw_update_write(&ctx, image, IMAGE_BUF_LEN) == EOS_OK); ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); - place_tlv_after_payload(&ctx, image); + ASSERT(ctx.tlv_written == TLV_AREA_LEN); sim_counter = 9; ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_ANTI_ROLLBACK); } +TEST(test_write_does_not_reach_verify_until_tlv_arrives) +{ + uint8_t image[IMAGE_BUF_LEN]; + eos_fw_update_ctx_t ctx; + + build_image(image, 3); + + ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_write(&ctx, image, STREAMED_LEN) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_TLV); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + +TEST(test_write_rejects_bytes_past_the_image_container) +{ + uint8_t image[IMAGE_BUF_LEN + 1]; + eos_fw_update_ctx_t ctx; + + build_image(image, 3); + image[IMAGE_BUF_LEN] = 0xA5; + + ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_write(&ctx, image, IMAGE_BUF_LEN + 1) == EOS_ERR_INVALID); +} + +TEST(test_finalize_accepts_tlv_counter_equal_to_floor) +{ + uint8_t image[IMAGE_BUF_LEN]; + eos_fw_update_ctx_t ctx; + + persist_bootctl(); + build_image(image, 9); + + ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_write(&ctx, image, IMAGE_BUF_LEN) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + + sim_counter = 9; + + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_OK); +} + int main(void) { printf("Firmware update (finalize anti-rollback)\n\n"); - run_test_finalize_rejects_tlv_counter_below_hw_floor(); + run_test_write_streams_tlv_then_finalize_rejects_below_floor(); + run_test_write_does_not_reach_verify_until_tlv_arrives(); + run_test_write_rejects_bytes_past_the_image_container(); + run_test_finalize_accepts_tlv_counter_equal_to_floor(); - tests_run = 1; + tests_run = 4; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; } diff --git a/tests/unit/test_jump_app.c b/tests/unit/test_jump_app.c new file mode 100644 index 0000000..e9a07e6 --- /dev/null +++ b/tests/unit/test_jump_app.c @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_jump_app.c + * @brief Host tests for eboot_jump_to_app() anti-rollback wiring. + * + * PR #96's test_jump_app_bounds.c is not in this tree. This file follows + * the same simulated-board convention as the other unit tests and uses + * the real parse / integrity / rollback path. Signature verification is + * supplied here so the suite does not have to embed a signing key. + */ + +#include "eos_bootctl.h" +#include "eos_image.h" +#include "eos_image_tlv.h" +#include "eos_crypto_boot.h" +#include "eos_rollback.h" +#include "eos_hal.h" +#include +#include +#include + +extern int eboot_jump_to_app(eos_bootctl_t *bctl, eos_slot_t slot); + +#define SIM_FLASH_SIZE (64 * 1024) +#define SIM_SLOT_A_ADDR 0x4000u +#define SIM_SLOT_A_SIZE 0x8000u +#define PAYLOAD_SIZE 256u + +static uint8_t sim_flash[SIM_FLASH_SIZE]; +static uint32_t sim_counter; +static int sim_jump_count; +static uint32_t sim_jump_addr; + +static int sim_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(buf, &sim_flash[addr], len); + return EOS_OK; +} + +static int sim_flash_write(uint32_t addr, const void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(&sim_flash[addr], buf, len); + return EOS_OK; +} + +static int sim_flash_erase(uint32_t addr, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memset(&sim_flash[addr], 0xFF, len); + return EOS_OK; +} + +static int sim_monotonic_read(uint32_t *value) +{ + if (!value) return EOS_ERR_INVALID; + *value = sim_counter; + return EOS_OK; +} + +static int sim_monotonic_increment(void) +{ + sim_counter++; + return EOS_OK; +} + +static void sim_jump(uint32_t addr) +{ + sim_jump_count++; + sim_jump_addr = addr; +} + +static uint32_t sim_tick; +static uint32_t sim_get_tick(void) { return sim_tick++; } +static void sim_noop(void) {} +static void sim_noop_u32(uint32_t x) { (void)x; } +static eos_reset_reason_t sim_reset_reason(void) { return EOS_RESET_POWER_ON; } +static bool sim_recovery_pin(void) { return false; } +static void sim_system_reset(void) {} + +static const eos_board_ops_t sim_ops = { + .flash_base = 0, + .flash_size = SIM_FLASH_SIZE, + .slot_a_addr = SIM_SLOT_A_ADDR, + .slot_a_size = SIM_SLOT_A_SIZE, + .slot_b_addr = 0xC000u, + .slot_b_size = 0x8000u, + .bootctl_addr = 0x0000u, + .bootctl_backup_addr = 0x1000u, + .log_addr = 0x2000u, + .flash_read = sim_flash_read, + .flash_write = sim_flash_write, + .flash_erase = sim_flash_erase, + .watchdog_init = sim_noop_u32, + .watchdog_feed = sim_noop, + .get_reset_reason = sim_reset_reason, + .system_reset = sim_system_reset, + .recovery_pin_asserted = sim_recovery_pin, + .jump = sim_jump, + .get_tick_ms = sim_get_tick, + .disable_interrupts = sim_noop, + .enable_interrupts = sim_noop, + .deinit_peripherals = sim_noop, + .monotonic_read = sim_monotonic_read, + .monotonic_increment = sim_monotonic_increment, +}; + +/* + * image_verify.c defines parse, integrity and signature together. Providing + * only the signature here pulls that object in and breaks the MSVC link + * (same reason test_slot_manager.c supplies all three). These two still + * check the bytes on the simulated slot; signature returns OK so the + * jump path can reach the TLV floor without embedding a signing key. + */ +int eos_image_parse_header(uint32_t addr, eos_image_header_t *out) +{ + if (!out) + return EOS_ERR_INVALID; + if (eos_hal_flash_read(addr, out, sizeof(*out)) != EOS_OK) + return EOS_ERR_FLASH; + if (out->magic != EOS_IMG_MAGIC) + return EOS_ERR_NO_IMAGE; + if (out->hdr_version == 0 || out->hdr_version > EOS_IMAGE_HDR_VERSION) + return EOS_ERR_INVALID; + if (out->hdr_size < sizeof(eos_image_header_t) || out->hdr_size > 4096) + return EOS_ERR_INVALID; + if (out->image_size == 0) + return EOS_ERR_INVALID; + return EOS_OK; +} + +int eos_image_verify_integrity(const eos_image_header_t *hdr, uint32_t addr) +{ + uint8_t payload[PAYLOAD_SIZE]; + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + + if (!hdr) + return EOS_ERR_INVALID; + if (hdr->image_size != PAYLOAD_SIZE) + return EOS_ERR_INVALID; + if (eos_hal_flash_read(addr + hdr->hdr_size, payload, PAYLOAD_SIZE) != EOS_OK) + return EOS_ERR_FLASH; + eos_sha256(payload, PAYLOAD_SIZE, digest); + if (memcmp(digest, hdr->hash, EOS_SHA256_DIGEST_SIZE) != 0) + return EOS_ERR_CRC; + return EOS_OK; +} + +int eos_image_verify_signature(const eos_image_header_t *hdr) +{ + if (!hdr) return EOS_ERR_INVALID; + return EOS_OK; +} + +#define TLV_AREA_LEN (uint16_t)(sizeof(eos_tlv_info_t) + \ + sizeof(eos_tlv_entry_hdr_t) + sizeof(uint32_t)) +#define TLV_VALUE_OFF (uint32_t)(sizeof(eos_tlv_info_t) + sizeof(eos_tlv_entry_hdr_t)) + +static void build_slot_image(uint32_t sec_ver) +{ + uint8_t payload[PAYLOAD_SIZE]; + uint32_t i; + + for (i = 0; i < PAYLOAD_SIZE; i++) + payload[i] = (uint8_t)(i * 7u + 1u); + + eos_image_header_t hdr; + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(eos_image_header_t); + hdr.image_size = PAYLOAD_SIZE; + hdr.load_addr = 0; + hdr.entry_addr = 0; + hdr.image_version = 0x00010000u; + hdr.flags = EOS_IMG_FLAG_HASH_SHA256; + eos_sha256(payload, PAYLOAD_SIZE, hdr.hash); + hdr.sig_type = EOS_SIG_ED25519; + hdr.sig_len = EOS_SIG_MAX_SIZE; + + uint8_t tlv[TLV_AREA_LEN]; + eos_tlv_info_t info = { EOS_TLV_INFO_MAGIC, TLV_AREA_LEN }; + eos_tlv_entry_hdr_t ent = { EOS_TLV_MIN_SEC_VER, sizeof(uint32_t) }; + memcpy(tlv, &info, sizeof(info)); + memcpy(tlv + sizeof(info), &ent, sizeof(ent)); + memcpy(tlv + TLV_VALUE_OFF, &sec_ver, sizeof(sec_ver)); + + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + eos_sha256(tlv, TLV_AREA_LEN, digest); + hdr.tlv_len = TLV_AREA_LEN; + memcpy(hdr.tlv_hash, digest, EOS_IMG_TLV_HASH_LEN); + + memcpy(&sim_flash[SIM_SLOT_A_ADDR], &hdr, sizeof(hdr)); + memcpy(&sim_flash[SIM_SLOT_A_ADDR + sizeof(hdr)], payload, PAYLOAD_SIZE); + memcpy(&sim_flash[SIM_SLOT_A_ADDR + sizeof(hdr) + PAYLOAD_SIZE], + tlv, TLV_AREA_LEN); +} + +static int tests_run = 0; +static int tests_passed = 0; + +#define ASSERT(cond) do { \ + if (!(cond)) { \ + printf("[FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + exit(1); \ + } \ +} while(0) + +#define TEST(name) \ + static void name(void); \ + static void run_##name(void) { \ + memset(sim_flash, 0xFF, sizeof(sim_flash)); \ + sim_counter = 0; \ + sim_jump_count = 0; \ + sim_jump_addr = 0; \ + sim_tick = 0; \ + eos_hal_init(&sim_ops); \ + eos_rollback_clear_staged(); \ + printf(" %-58s ", #name); \ + name(); \ + tests_passed++; \ + printf("[PASS]\n"); \ + } \ + static void name(void) + +TEST(test_jump_rejects_tlv_counter_below_hw_floor) +{ + eos_bootctl_t bctl; + + build_slot_image(3); + eos_bootctl_init_defaults(&bctl); + sim_counter = 9; + + ASSERT(eboot_jump_to_app(&bctl, EOS_SLOT_A) == EOS_ERR_ANTI_ROLLBACK); + ASSERT(sim_jump_count == 0); + + ASSERT(eos_rollback_commit() == EOS_OK); + ASSERT(sim_counter == 9); +} + +TEST(test_jump_stages_tlv_counter_above_floor) +{ + eos_bootctl_t bctl; + + build_slot_image(9); + eos_bootctl_init_defaults(&bctl); + sim_counter = 5; + + /* Host HAL jump returns, so the production "never reached" tail runs. */ + ASSERT(eboot_jump_to_app(&bctl, EOS_SLOT_A) == EOS_ERR_GENERIC); + ASSERT(sim_jump_count == 1); + + ASSERT(eos_rollback_commit() == EOS_OK); + ASSERT(sim_counter == 9); +} + +int main(void) +{ + printf("Stage-1 jump_app (TLV anti-rollback)\n\n"); + + run_test_jump_rejects_tlv_counter_below_hw_floor(); + run_test_jump_stages_tlv_counter_above_floor(); + + tests_run = 2; + printf("\n%d/%d passed\n", tests_passed, tests_run); + return tests_passed == tests_run ? 0 : 1; +} diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c index 318c7a0..08dcad4 100644 --- a/tests/unit/test_tlv_auth.c +++ b/tests/unit/test_tlv_auth.c @@ -290,6 +290,18 @@ TEST(test_oversized_tlv_len_is_rejected) ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_ERR_INVALID); } +TEST(test_tlv_area_must_fit_in_slot) +{ + build_image(3, true, true); + + eos_board_ops_t tiny = sim_ops; + tiny.slot_a_size = (uint32_t)(sizeof(eos_image_header_t) + PAYLOAD_SIZE + 4u); + eos_hal_init(&tiny); + + uint32_t counter = 0; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_ERR_INVALID); +} + /* * The production boot and update paths used to feed hdr.image_version into * a hardware-floor comparison. Firmware versions (0x00MMmmpp) and TLV @@ -346,9 +358,10 @@ int main(void) run_test_image_without_tlv_area_still_reports_zero(); run_test_tlv_binding_fields_are_inside_the_signed_prefix(); run_test_oversized_tlv_len_is_rejected(); + run_test_tlv_area_must_fit_in_slot(); run_test_hw_floor_uses_tlv_counter_not_image_version(); - tests_run = 8; + tests_run = 9; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; } From 984a0d39a8e8e8a1ee0114e6ef4877fec5d54761 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sat, 5 Sep 2026 22:51:09 +0530 Subject: [PATCH 4/8] fix(transport): handle padded XMODEM update blocks --- core/fw_transport_uart.c | 38 ++++++++-- core/fw_update.c | 34 +++++++-- docs/book/book.md | 2 + include/eos_fw_update.h | 18 +++++ tests/unit/test_fw_transport.c | 131 ++++++++++++++++++++++++++++++--- tests/unit/test_fw_update.c | 64 +++++++++++++++- 6 files changed, 265 insertions(+), 22 deletions(-) diff --git a/core/fw_transport_uart.c b/core/fw_transport_uart.c index 5e8a68e..ab743bb 100644 --- a/core/fw_transport_uart.c +++ b/core/fw_transport_uart.c @@ -236,12 +236,27 @@ static int xmodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) continue; } - /* Write to update context */ - rc = eos_fw_update_write(ctx, block, XMODEM_BLOCK_SIZE); - if (rc != EOS_OK) { - c = XMODEM_CAN; - eos_hal_uart_send(&c, 1); - return rc; + /* XMODEM carries no container length and always sends whole + * 128-byte blocks, so the tail of the final block is padding the + * sender invented. Forward only what the container still wants: + * the rest is not image data, and eos_fw_update_write() rejects + * bytes past the container rather than discarding them. */ + size_t written = 0; + while (written < XMODEM_BLOCK_SIZE) { + uint32_t wanted = eos_fw_update_bytes_wanted(ctx); + if (wanted == 0) break; + + size_t chunk = XMODEM_BLOCK_SIZE - written; + if (chunk > wanted) chunk = (size_t)wanted; + + rc = eos_fw_update_write(ctx, block + written, chunk); + if (rc != EOS_OK) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return rc; + } + + written += chunk; } c = XMODEM_ACK; @@ -384,6 +399,17 @@ static int ymodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) } } + /* Without a usable size the receiver cannot tell image bytes + * from block padding, and the padding would reach + * eos_fw_update_write() as trailing garbage. A missing or + * unparseable size is a protocol error, not an invitation to + * treat the transfer as unbounded. */ + if (file_size == 0) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return EOS_ERR_INVALID; + } + first_block = false; expected_blk = 1; c = XMODEM_ACK; diff --git a/core/fw_update.c b/core/fw_update.c index aea9c78..e475e45 100644 --- a/core/fw_update.c +++ b/core/fw_update.c @@ -185,12 +185,13 @@ int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t le ctx->state = EOS_FW_STATE_VERIFY; } - /* Bytes past the image container must not be discarded as success. */ - if (offset < len) { - ctx->state = EOS_FW_STATE_ERROR; - ctx->last_error = EOS_ERR_INVALID; + /* Bytes past the image container must not be discarded as success. + * The container is complete here, so this reports the error without + * downgrading a finished context: trailing bytes behave the same + * whether they share this call with the last container byte or + * arrive in the next one. */ + if (offset < len) return EOS_ERR_INVALID; - } ctx->total_received += (uint32_t)len; return EOS_OK; @@ -299,6 +300,29 @@ uint8_t eos_fw_update_progress(const eos_fw_update_ctx_t *ctx) return (uint8_t)(((uint64_t)done * 100) / total); } +uint32_t eos_fw_update_bytes_wanted(const eos_fw_update_ctx_t *ctx) +{ + if (!ctx) return 0; + + switch (ctx->state) { + case EOS_FW_STATE_HEADER: + /* image_size and tlv_len live in the header, so the payload and + * TLV remainders are unknown until it has been parsed. Callers + * ask again after each write. */ + return (uint32_t)sizeof(eos_image_header_t) - ctx->hdr_received; + + case EOS_FW_STATE_PAYLOAD: + return (ctx->payload_total - ctx->payload_written) + + (ctx->tlv_total - ctx->tlv_written); + + case EOS_FW_STATE_TLV: + return ctx->tlv_total - ctx->tlv_written; + + default: + return 0; + } +} + eos_fw_update_state_t eos_fw_update_get_state(const eos_fw_update_ctx_t *ctx) { if (!ctx) return EOS_FW_STATE_IDLE; diff --git a/docs/book/book.md b/docs/book/book.md index f85d2a9..fc1a4a4 100644 --- a/docs/book/book.md +++ b/docs/book/book.md @@ -1321,6 +1321,8 @@ typedef struct { } eos_fw_update_ctx_t; ``` +The update container is `[header][payload][TLV area]`, and `eos_fw_update_write()` must receive all three parts. Bytes beyond the complete container are rejected rather than discarded. + ### 16.2 Flash Sector Alignment Most flash memories require sector-aligned writes. The update pipeline handles alignment automatically by accumulating data in a sector buffer before writing complete sectors to flash. diff --git a/include/eos_fw_update.h b/include/eos_fw_update.h index 9614e4d..c115b61 100644 --- a/include/eos_fw_update.h +++ b/include/eos_fw_update.h @@ -94,6 +94,24 @@ int eos_fw_update_begin(eos_fw_update_ctx_t *ctx, eos_slot_t slot); */ int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t len); +/** + * Number of container bytes eos_fw_update_write() will still accept. + * + * A block-framed transport such as XMODEM has no container length of its + * own and pads its final block; this is how it tells image bytes from + * padding instead of pushing the padding into eos_fw_update_write(), which + * rejects anything past the container. + * + * While the header is still arriving only the header remainder is known, + * because image_size and tlv_len are fields inside it. Call this again + * after each write rather than once per block. + * + * @param ctx Update context. + * @return Bytes still wanted, or 0 once the container is complete or the + * context is idle or failed. + */ +uint32_t eos_fw_update_bytes_wanted(const eos_fw_update_ctx_t *ctx); + /** * Finalize the update: verify integrity, update boot control, * and optionally mark for test boot. diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index 61a43ac..aecab3b 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -308,6 +308,13 @@ static int flash_matches_image(void) return memcmp(&sim_flash[SIM_SLOT_B_ADDR], image_buf, image_len) == 0; } +static int slot_b_is_unwritten(void) +{ + for (size_t i = 0; i < image_len; i++) + if (sim_flash[SIM_SLOT_B_ADDR + i] != 0xFF) return 0; + return 1; +} + static int tx_contains(uint8_t b) { for (size_t i = 0; i < tx_len; i++) if (tx_capture[i] == b) return 1; @@ -389,8 +396,9 @@ TEST(test_ymodem_bad_block_complement_is_nakd) /* * Block 0 is attacker-supplied and is not guaranteed to contain a NUL. - * The filename scan is bounded by the block length; this pins the defined - * behaviour (size treated as unknown, transfer still completes). + * The filename scan is bounded by the block length; with no NUL there is no + * size field behind it either, and a length the receiver cannot read is a + * length it cannot use to tell image bytes from block padding. */ TEST(test_ymodem_header_without_nul_is_bounded) { @@ -405,8 +413,8 @@ TEST(test_ymodem_header_without_nul_is_bounded) rx_push_byte(XM_EOT); rx_push_byte(XM_EOT); - ASSERT(run_ymodem() == EOS_OK); - ASSERT(flash_matches_image()); + ASSERT(run_ymodem() == EOS_ERR_INVALID); + ASSERT(slot_b_is_unwritten()); } /* @@ -430,8 +438,8 @@ TEST(test_ymodem_stx_header_without_nul_is_bounded) rx_push_byte(XM_EOT); rx_push_byte(XM_EOT); - ASSERT(run_ymodem() == EOS_OK); - ASSERT(flash_matches_image()); + ASSERT(run_ymodem() == EOS_ERR_INVALID); + ASSERT(slot_b_is_unwritten()); } /* Sequencing and duplicate suppression must apply on the STX path too. */ @@ -459,9 +467,10 @@ TEST(test_ymodem_header_size_overflow_is_rejected) rx_push_byte(XM_EOT); rx_push_byte(XM_EOT); - /* Size is discarded as unusable, so no truncation is applied. */ - ASSERT(run_ymodem() == EOS_OK); - ASSERT(flash_matches_image()); + /* Size is discarded as unusable, and an unusable size is a protocol + * error rather than a transfer of unknown length. */ + ASSERT(run_ymodem() == EOS_ERR_INVALID); + ASSERT(slot_b_is_unwritten()); } /* The first block must be block 0, not an arbitrary data block. */ @@ -475,6 +484,107 @@ TEST(test_ymodem_first_block_must_be_zero) ASSERT(tx_contains(XM_NAK)); } +/* ================================================================ + * XMODEM + * ================================================================ */ + +/* + * XMODEM has no length field and always sends whole 128-byte blocks, so a + * container whose length is not a multiple of 128 arrives with padding on + * the final block. eos_fw_update_write() rejects bytes past the container + * instead of discarding them, so the receiver has to stop at the container + * boundary itself. + * + * 424 bytes is 3 full blocks plus 40, so block 4 carries 88 padding bytes. + */ +#define XM_CONTAINER_LEN 424u +#define XM_PAYLOAD_LEN (XM_CONTAINER_LEN - sizeof(eos_image_header_t)) +#define XM_BLOCKS 4 + +static uint8_t xm_image[XM_CONTAINER_LEN]; + +/* Mirrors update_crc() in core/fw_update.c so finalize sees a matching + * CRC32 on the flags = 0 integrity path. */ +static uint32_t crc32_payload(const uint8_t *data, size_t len) +{ + uint32_t crc = 0xFFFFFFFFu; + for (size_t i = 0; i < len; i++) { + crc ^= data[i]; + for (int bit = 0; bit < 8; bit++) { + if (crc & 1) crc = (crc >> 1) ^ 0xEDB88320u; + else crc >>= 1; + } + } + return ~crc; +} + +static void build_unaligned_image(void) +{ + eos_image_header_t hdr; + uint8_t *payload = &xm_image[sizeof(hdr)]; + size_t i; + + memset(xm_image, 0, sizeof(xm_image)); + for (i = 0; i < XM_PAYLOAD_LEN; i++) + payload[i] = (uint8_t)(0x5A + (i & 0x1F)); + + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(hdr); + hdr.image_size = (uint32_t)XM_PAYLOAD_LEN; + hdr.load_addr = SIM_SLOT_B_ADDR; + hdr.entry_addr = SIM_SLOT_B_ADDR; + hdr.flags = 0; /* CRC32 integrity path */ + hdr.sig_type = EOS_SIG_NONE; + hdr.tlv_len = 0; + + uint32_t crc = crc32_payload(payload, XM_PAYLOAD_LEN); + memcpy(hdr.hash, &crc, sizeof(crc)); + memcpy(xm_image, &hdr, sizeof(hdr)); +} + +static int run_xmodem(eos_fw_update_ctx_t *ctx) +{ + const eos_fw_transport_ops_t *ops = eos_fw_transport_uart_xmodem(); + eos_fw_transport_t tp; + memset(&tp, 0, sizeof(tp)); + tp.ops = ops; + tp.baudrate = 115200; + tp.timeout_ms = 10; + + ASSERT(eos_fw_update_begin(ctx, EOS_SLOT_B) == EOS_OK); + return ops->receive(&tp, ctx); +} + +/* + * Regression: the strict post-container check rejected the padding on the + * last block, so any image that did not happen to be a multiple of 128 + * failed to install over XMODEM. + */ +TEST(test_xmodem_padded_final_block_completes_and_finalizes) +{ + eos_fw_update_ctx_t ctx; + int i; + + build_unaligned_image(); + ASSERT(XM_CONTAINER_LEN % BLOCK != 0); + + for (i = 0; i < XM_BLOCKS; i++) { + size_t off = (size_t)i * BLOCK; + size_t n = XM_CONTAINER_LEN - off; + if (n > BLOCK) n = BLOCK; + push_block((uint8_t)(i + 1), &xm_image[off], n); + } + rx_push_byte(XM_EOT); + + ASSERT(run_xmodem(&ctx) == EOS_OK); + ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], xm_image, XM_CONTAINER_LEN) == 0); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == 0); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_OK); +} + /* ================================================================ * Raw length-prefixed transport * ================================================================ */ @@ -551,11 +661,12 @@ int main(void) run_test_ymodem_stx_duplicate_block_is_not_written_twice(); run_test_ymodem_header_size_overflow_is_rejected(); run_test_ymodem_first_block_must_be_zero(); + run_test_xmodem_padded_final_block_completes_and_finalizes(); run_test_raw_valid_transfer_is_written(); run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 12; + tests_run = 13; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_fw_update.c b/tests/unit/test_fw_update.c index 9b4f284..1579a23 100644 --- a/tests/unit/test_fw_update.c +++ b/tests/unit/test_fw_update.c @@ -209,6 +209,66 @@ TEST(test_write_rejects_bytes_past_the_image_container) ASSERT(eos_fw_update_write(&ctx, image, IMAGE_BUF_LEN + 1) == EOS_ERR_INVALID); } +/* + * A block-framed transport asks how much of the container is still + * outstanding so it can stop before its own padding. The header remainder + * is all that is knowable until the header parses, because image_size and + * tlv_len are fields inside it. + */ +TEST(test_bytes_wanted_tracks_the_remaining_container) +{ + uint8_t image[IMAGE_BUF_LEN]; + eos_fw_update_ctx_t ctx; + + build_image(image, 3); + + ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == sizeof(eos_image_header_t)); + + ASSERT(eos_fw_update_write(&ctx, image, sizeof(eos_image_header_t)) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_PAYLOAD); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == PAYLOAD_SIZE + TLV_AREA_LEN); + + ASSERT(eos_fw_update_write(&ctx, image + sizeof(eos_image_header_t), + PAYLOAD_SIZE) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_TLV); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == TLV_AREA_LEN); + + ASSERT(eos_fw_update_write(&ctx, image + STREAMED_LEN, TLV_AREA_LEN) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == 0); +} + +/* + * The trailing byte used to be treated differently depending on where the + * chunk boundary fell: sharing a call with the last container byte drove the + * context to ERROR, while arriving in its own call did not. Both are the + * same protocol mistake and both must report it without discarding a + * container that was already received in full. + */ +TEST(test_trailing_byte_is_rejected_the_same_across_chunk_boundaries) +{ + uint8_t image[IMAGE_BUF_LEN + 1]; + eos_fw_update_ctx_t single, split; + int rc_single, rc_split; + + build_image(image, 3); + image[IMAGE_BUF_LEN] = 0xA5; + + ASSERT(eos_fw_update_begin(&single, EOS_SLOT_B) == EOS_OK); + rc_single = eos_fw_update_write(&single, image, IMAGE_BUF_LEN + 1); + + ASSERT(eos_fw_update_begin(&split, EOS_SLOT_B) == EOS_OK); + ASSERT(eos_fw_update_write(&split, image, IMAGE_BUF_LEN) == EOS_OK); + rc_split = eos_fw_update_write(&split, &image[IMAGE_BUF_LEN], 1); + + ASSERT(rc_single == EOS_ERR_INVALID); + ASSERT(rc_split == rc_single); + ASSERT(eos_fw_update_get_state(&single) == EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_get_state(&split) == eos_fw_update_get_state(&single)); + ASSERT(single.tlv_written == split.tlv_written); +} + TEST(test_finalize_accepts_tlv_counter_equal_to_floor) { uint8_t image[IMAGE_BUF_LEN]; @@ -233,9 +293,11 @@ int main(void) run_test_write_streams_tlv_then_finalize_rejects_below_floor(); run_test_write_does_not_reach_verify_until_tlv_arrives(); run_test_write_rejects_bytes_past_the_image_container(); + run_test_bytes_wanted_tracks_the_remaining_container(); + run_test_trailing_byte_is_rejected_the_same_across_chunk_boundaries(); run_test_finalize_accepts_tlv_counter_equal_to_floor(); - tests_run = 4; + tests_run = 6; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; } From 180680cc86c522c3f54f791848563b581c761446 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sun, 6 Sep 2026 01:44:41 +0530 Subject: [PATCH 5/8] fix(transport): handle padded XMODEM and YMODEM blocks --- core/fw_transport_uart.c | 101 ++++++++++---- core/fw_update.c | 10 +- tests/unit/test_fw_transport.c | 235 +++++++++++++++++++++++---------- tests/unit/test_fw_update.c | 9 +- 4 files changed, 258 insertions(+), 97 deletions(-) diff --git a/core/fw_transport_uart.c b/core/fw_transport_uart.c index ab743bb..8bbc8fb 100644 --- a/core/fw_transport_uart.c +++ b/core/fw_transport_uart.c @@ -185,6 +185,42 @@ static uint16_t xmodem_crc16(const uint8_t *data, size_t len) return crc; } +/** + * Forward up to @p len bytes of a received block to the update context, + * stopping at the end of the update container. + * + * Both block-framed protocols pad their final block, and + * eos_fw_update_write() rejects bytes past the container rather than + * discarding them, so the padding has to be dropped here instead. + * bytes_wanted() is re-read on every pass because the payload and TLV + * lengths only become known once the header has been parsed. + * + * @param ctx Update context. + * @param data Block data. + * @param len Bytes of @p data that belong to the transfer. + * @return EOS_OK, or the eos_fw_update_write() error. + */ +static int write_container_bytes(eos_fw_update_ctx_t *ctx, + const uint8_t *data, size_t len) +{ + size_t written = 0; + + while (written < len) { + uint32_t wanted = eos_fw_update_bytes_wanted(ctx); + if (wanted == 0) break; + + size_t chunk = len - written; + if (chunk > wanted) chunk = (size_t)wanted; + + int rc = eos_fw_update_write(ctx, data + written, chunk); + if (rc != EOS_OK) return rc; + + written += chunk; + } + + return EOS_OK; +} + static int xmodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) { uint32_t timeout = tp->timeout_ms ? tp->timeout_ms : 60000; @@ -236,27 +272,24 @@ static int xmodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) continue; } - /* XMODEM carries no container length and always sends whole - * 128-byte blocks, so the tail of the final block is padding the - * sender invented. Forward only what the container still wants: - * the rest is not image data, and eos_fw_update_write() rejects - * bytes past the container rather than discarding them. */ - size_t written = 0; - while (written < XMODEM_BLOCK_SIZE) { - uint32_t wanted = eos_fw_update_bytes_wanted(ctx); - if (wanted == 0) break; - - size_t chunk = XMODEM_BLOCK_SIZE - written; - if (chunk > wanted) chunk = (size_t)wanted; - - rc = eos_fw_update_write(ctx, block + written, chunk); - if (rc != EOS_OK) { - c = XMODEM_CAN; - eos_hal_uart_send(&c, 1); - return rc; - } + /* The container was already complete before this block arrived, so + * these bytes are not padding on the final block -- they are an + * entire extra block. XMODEM has no length field, so accepting them + * would accept an unbounded amount of unaccounted data. Padding + * inside the final block is still consumed, just below. */ + if (eos_fw_update_bytes_wanted(ctx) == 0) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return EOS_ERR_INVALID; + } - written += chunk; + /* XMODEM always sends whole 128-byte blocks, so the tail of the + * final block is padding the sender invented. */ + rc = write_container_bytes(ctx, block, XMODEM_BLOCK_SIZE); + if (rc != EOS_OK) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return rc; } c = XMODEM_ACK; @@ -434,20 +467,36 @@ static int ymodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) continue; } - /* Write data block */ - size_t write_len = block_size; - if (file_size > 0 && total_received + write_len > file_size) { - write_len = file_size - total_received; + /* Bytes of the declared file carried by this block. file_size stays + * the YMODEM framing bound and still marks end of file. */ + size_t file_len = block_size; + if (file_size > 0 && total_received + file_len > file_size) { + file_len = file_size - total_received; + } + + /* The declared file has already been received in full, so this block + * falls outside the transfer entirely. eos_fw_update_write() reported + * that as an error when it was handed the resulting empty chunk; it + * stays an error now that the chunk is not handed over at all. */ + if (file_len == 0) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return EOS_ERR_INVALID; } - rc = eos_fw_update_write(ctx, block, write_len); + /* file_size is what the sender claims. The container is what the + * image itself declares, and that is the authority on how many of + * these bytes are image: a declared size rounded up to cover file or + * block padding must not push the padding into + * eos_fw_update_write(), which rejects bytes past the container. */ + rc = write_container_bytes(ctx, block, file_len); if (rc != EOS_OK) { c = XMODEM_CAN; eos_hal_uart_send(&c, 1); return rc; } - total_received += (uint32_t)write_len; + total_received += (uint32_t)file_len; c = XMODEM_ACK; eos_hal_uart_send(&c, 1); expected_blk++; diff --git a/core/fw_update.c b/core/fw_update.c index e475e45..ee0e868 100644 --- a/core/fw_update.c +++ b/core/fw_update.c @@ -189,11 +189,17 @@ int eos_fw_update_write(eos_fw_update_ctx_t *ctx, const uint8_t *data, size_t le * The container is complete here, so this reports the error without * downgrading a finished context: trailing bytes behave the same * whether they share this call with the last container byte or - * arrive in the next one. */ + * arrive in the next one. + * + * Only the bytes actually taken from the container are counted, for + * the same reason: offset equals len whenever the whole input was + * accepted, so the running total no longer depends on the boundary + * either, and rejected trailing bytes are never counted. */ + ctx->total_received += (uint32_t)offset; + if (offset < len) return EOS_ERR_INVALID; - ctx->total_received += (uint32_t)len; return EOS_OK; } diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index aecab3b..f28f2bf 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -14,6 +14,7 @@ #include "eos_fw_transport.h" #include "eos_fw_update.h" #include "eos_image.h" +#include "eos_image_tlv.h" #include "eos_hal.h" #include #include @@ -176,6 +177,7 @@ static int tests_passed = 0; #define XM_EOT 0x04 #define XM_ACK 0x06 #define XM_NAK 0x15 +#define XM_CAN 0x18 #define BLOCK 128 #define BLOCK_L 1024 @@ -289,7 +291,96 @@ static void push_image_block(int index) push_block((uint8_t)(index + 1), &image_buf[(size_t)index * BLOCK], BLOCK); } -static int run_ymodem(void) +/* ---- Update container carrying an authenticated TLV area ---- + * + * 156-byte header + 256-byte payload + 12-byte TLV area = 424 bytes, which is + * three full 128-byte blocks plus 40. The final block therefore carries the + * end of the payload, the payload-to-TLV transition, the whole TLV area and + * 88 bytes of block padding -- every boundary the receiver has to get right. + */ + +#define CONT_PAYLOAD_LEN 256u +#define CONT_TLV_LEN (uint16_t)(sizeof(eos_tlv_info_t) + \ + sizeof(eos_tlv_entry_hdr_t) + \ + sizeof(uint32_t)) +#define CONT_LEN (sizeof(eos_image_header_t) + CONT_PAYLOAD_LEN + \ + CONT_TLV_LEN) +#define CONT_BLOCKS 4 + +static uint8_t container[CONT_LEN]; + +/* Mirrors update_crc() in core/fw_update.c so finalize sees a matching CRC32 + * on the flags = 0 integrity path. */ +static uint32_t crc32_payload(const uint8_t *data, size_t len) +{ + uint32_t crc = 0xFFFFFFFFu; + for (size_t i = 0; i < len; i++) { + crc ^= data[i]; + for (int bit = 0; bit < 8; bit++) { + if (crc & 1) crc = (crc >> 1) ^ 0xEDB88320u; + else crc >>= 1; + } + } + return ~crc; +} + +static void build_container(void) +{ + eos_image_header_t hdr; + uint8_t *payload = &container[sizeof(hdr)]; + uint8_t *tlv = &container[sizeof(hdr) + CONT_PAYLOAD_LEN]; + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + uint32_t sec_ver = 5; + size_t i; + + memset(container, 0, sizeof(container)); + for (i = 0; i < CONT_PAYLOAD_LEN; i++) + payload[i] = (uint8_t)(0x5A + (i & 0x1F)); + + /* [tlv_info][entry_hdr][uint32 counter], the shape the update and + * rollback suites build. */ + eos_tlv_info_t info = { EOS_TLV_INFO_MAGIC, CONT_TLV_LEN }; + eos_tlv_entry_hdr_t ent = { EOS_TLV_MIN_SEC_VER, sizeof(uint32_t) }; + memcpy(tlv, &info, sizeof(info)); + memcpy(tlv + sizeof(info), &ent, sizeof(ent)); + memcpy(tlv + sizeof(info) + sizeof(ent), &sec_ver, sizeof(sec_ver)); + + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(hdr); + hdr.image_size = (uint32_t)CONT_PAYLOAD_LEN; + hdr.load_addr = SIM_SLOT_B_ADDR; + hdr.entry_addr = SIM_SLOT_B_ADDR; + hdr.flags = 0; /* CRC32 integrity path */ + hdr.sig_type = EOS_SIG_NONE; + + uint32_t crc = crc32_payload(payload, CONT_PAYLOAD_LEN); + memcpy(hdr.hash, &crc, sizeof(crc)); + + /* The TLV area is authenticated by the header hash, so rollback will + * accept the counter it carries. */ + eos_sha256(tlv, CONT_TLV_LEN, digest); + hdr.tlv_len = CONT_TLV_LEN; + memcpy(hdr.tlv_hash, digest, EOS_IMG_TLV_HASH_LEN); + + memcpy(container, &hdr, sizeof(hdr)); +} + +/* The container as CONT_BLOCKS fixed 128-byte blocks, the last one padded. */ +static void push_container_blocks(void) +{ + int i; + + for (i = 0; i < CONT_BLOCKS; i++) { + size_t off = (size_t)i * BLOCK; + size_t n = CONT_LEN - off; + if (n > BLOCK) n = BLOCK; + push_block((uint8_t)(i + 1), &container[off], n); + } +} + +static int run_ymodem_ctx(eos_fw_update_ctx_t *ctx) { const eos_fw_transport_ops_t *ops = eos_fw_transport_uart_ymodem(); eos_fw_transport_t tp; @@ -298,9 +389,14 @@ static int run_ymodem(void) tp.baudrate = 115200; tp.timeout_ms = 10; + ASSERT(eos_fw_update_begin(ctx, EOS_SLOT_B) == EOS_OK); + return ops->receive(&tp, ctx); +} + +static int run_ymodem(void) +{ eos_fw_update_ctx_t ctx; - ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); - return ops->receive(&tp, &ctx); + return run_ymodem_ctx(&ctx); } static int flash_matches_image(void) @@ -484,66 +580,36 @@ TEST(test_ymodem_first_block_must_be_zero) ASSERT(tx_contains(XM_NAK)); } -/* ================================================================ - * XMODEM - * ================================================================ */ - /* - * XMODEM has no length field and always sends whole 128-byte blocks, so a - * container whose length is not a multiple of 128 arrives with padding on - * the final block. eos_fw_update_write() rejects bytes past the container - * instead of discarding them, so the receiver has to stop at the container - * boundary itself. - * - * 424 bytes is 3 full blocks plus 40, so block 4 carries 88 padding bytes. + * The declared file size is YMODEM framing, and a sender is free to round it + * up to its own file or block padding. The container is what the image itself + * declares, and that is the authority on how many bytes are image, so the + * surplus must be dropped rather than pushed into eos_fw_update_write(). + * Dropping it is correct framing, not a protocol error, so no CAN is sent. */ -#define XM_CONTAINER_LEN 424u -#define XM_PAYLOAD_LEN (XM_CONTAINER_LEN - sizeof(eos_image_header_t)) -#define XM_BLOCKS 4 - -static uint8_t xm_image[XM_CONTAINER_LEN]; - -/* Mirrors update_crc() in core/fw_update.c so finalize sees a matching - * CRC32 on the flags = 0 integrity path. */ -static uint32_t crc32_payload(const uint8_t *data, size_t len) +TEST(test_ymodem_declared_size_larger_than_container_is_clamped) { - uint32_t crc = 0xFFFFFFFFu; - for (size_t i = 0; i < len; i++) { - crc ^= data[i]; - for (int bit = 0; bit < 8; bit++) { - if (crc & 1) crc = (crc >> 1) ^ 0xEDB88320u; - else crc >>= 1; - } - } - return ~crc; -} - -static void build_unaligned_image(void) -{ - eos_image_header_t hdr; - uint8_t *payload = &xm_image[sizeof(hdr)]; - size_t i; - - memset(xm_image, 0, sizeof(xm_image)); - for (i = 0; i < XM_PAYLOAD_LEN; i++) - payload[i] = (uint8_t)(0x5A + (i & 0x1F)); + eos_fw_update_ctx_t ctx; - memset(&hdr, 0, sizeof(hdr)); - hdr.magic = EOS_IMG_MAGIC; - hdr.hdr_version = EOS_IMAGE_HDR_VERSION; - hdr.hdr_size = (uint16_t)sizeof(hdr); - hdr.image_size = (uint32_t)XM_PAYLOAD_LEN; - hdr.load_addr = SIM_SLOT_B_ADDR; - hdr.entry_addr = SIM_SLOT_B_ADDR; - hdr.flags = 0; /* CRC32 integrity path */ - hdr.sig_type = EOS_SIG_NONE; - hdr.tlv_len = 0; + build_container(); + push_header_block("fw.bin", "512"); /* 424 bytes of image, padded to 512 */ + push_container_blocks(); + rx_push_byte(XM_EOT); + rx_push_byte(XM_EOT); - uint32_t crc = crc32_payload(payload, XM_PAYLOAD_LEN); - memcpy(hdr.hash, &crc, sizeof(crc)); - memcpy(xm_image, &hdr, sizeof(hdr)); + ASSERT(run_ymodem_ctx(&ctx) == EOS_OK); + ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], container, CONT_LEN) == 0); + ASSERT(sim_flash[SIM_SLOT_B_ADDR + CONT_LEN] == 0xFF); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == 0); + ASSERT(ctx.tlv_written == CONT_TLV_LEN); + ASSERT(!tx_contains(XM_CAN)); } +/* ================================================================ + * XMODEM + * ================================================================ */ + static int run_xmodem(eos_fw_update_ctx_t *ctx) { const eos_fw_transport_ops_t *ops = eos_fw_transport_uart_xmodem(); @@ -560,31 +626,62 @@ static int run_xmodem(eos_fw_update_ctx_t *ctx) /* * Regression: the strict post-container check rejected the padding on the * last block, so any image that did not happen to be a multiple of 128 - * failed to install over XMODEM. + * failed to install over XMODEM. The final block also carries the + * payload-to-TLV transition and the whole TLV area. */ TEST(test_xmodem_padded_final_block_completes_and_finalizes) { eos_fw_update_ctx_t ctx; - int i; - build_unaligned_image(); - ASSERT(XM_CONTAINER_LEN % BLOCK != 0); + build_container(); + ASSERT(CONT_LEN == 424); + ASSERT(CONT_LEN % BLOCK != 0); + ASSERT(CONT_BLOCKS * BLOCK - CONT_LEN == 88); - for (i = 0; i < XM_BLOCKS; i++) { - size_t off = (size_t)i * BLOCK; - size_t n = XM_CONTAINER_LEN - off; - if (n > BLOCK) n = BLOCK; - push_block((uint8_t)(i + 1), &xm_image[off], n); - } + push_container_blocks(); rx_push_byte(XM_EOT); ASSERT(run_xmodem(&ctx) == EOS_OK); - ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], xm_image, XM_CONTAINER_LEN) == 0); + ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], container, CONT_LEN) == 0); ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); ASSERT(eos_fw_update_bytes_wanted(&ctx) == 0); + ASSERT(ctx.tlv_written == CONT_TLV_LEN); ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_OK); } +/* + * Padding in the remainder of the final block is framing the protocol + * guarantees. An entire further data block after the container is complete is + * not: XMODEM carries no length, so accepting those blocks would accept an + * unbounded amount of data nothing accounts for. + */ +TEST(test_xmodem_data_block_after_container_is_rejected) +{ + eos_fw_update_ctx_t ctx; + uint8_t extra[BLOCK]; + + build_container(); + push_container_blocks(); + + memset(extra, 0x5C, sizeof(extra)); + push_block((uint8_t)(CONT_BLOCKS + 1), extra, sizeof(extra)); + rx_push_byte(XM_EOT); + + ASSERT(run_xmodem(&ctx) == EOS_ERR_INVALID); + ASSERT(tx_contains(XM_CAN)); + + /* The container that did arrive is intact, and the extra block reached + * neither flash nor the update context. */ + ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], container, CONT_LEN) == 0); + ASSERT(sim_flash[SIM_SLOT_B_ADDR + CONT_LEN] == 0xFF); + + /* eos_fw_transport_update() aborts the context when receive fails, so a + * rejected transfer cannot then be finalized as a good image. */ + eos_fw_update_abort(&ctx); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_IDLE); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + /* ================================================================ * Raw length-prefixed transport * ================================================================ */ @@ -661,12 +758,14 @@ int main(void) run_test_ymodem_stx_duplicate_block_is_not_written_twice(); run_test_ymodem_header_size_overflow_is_rejected(); run_test_ymodem_first_block_must_be_zero(); + run_test_ymodem_declared_size_larger_than_container_is_clamped(); run_test_xmodem_padded_final_block_completes_and_finalizes(); + run_test_xmodem_data_block_after_container_is_rejected(); run_test_raw_valid_transfer_is_written(); run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 13; + tests_run = 15; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_fw_update.c b/tests/unit/test_fw_update.c index 1579a23..a99a68f 100644 --- a/tests/unit/test_fw_update.c +++ b/tests/unit/test_fw_update.c @@ -244,7 +244,8 @@ TEST(test_bytes_wanted_tracks_the_remaining_container) * chunk boundary fell: sharing a call with the last container byte drove the * context to ERROR, while arriving in its own call did not. Both are the * same protocol mistake and both must report it without discarding a - * container that was already received in full. + * container that was already received in full, and both must account for + * exactly the container bytes they consumed. */ TEST(test_trailing_byte_is_rejected_the_same_across_chunk_boundaries) { @@ -267,6 +268,12 @@ TEST(test_trailing_byte_is_rejected_the_same_across_chunk_boundaries) ASSERT(eos_fw_update_get_state(&single) == EOS_FW_STATE_VERIFY); ASSERT(eos_fw_update_get_state(&split) == eos_fw_update_get_state(&single)); ASSERT(single.tlv_written == split.tlv_written); + + /* The rejected byte is counted by neither, so both totals are the + * container length and the reported progress agrees. */ + ASSERT(single.total_received == IMAGE_BUF_LEN); + ASSERT(single.total_received == split.total_received); + ASSERT(eos_fw_update_progress(&single) == eos_fw_update_progress(&split)); } TEST(test_finalize_accepts_tlv_counter_equal_to_floor) From fb978c8e1b206dc362ab702e218e2129dd759dc3 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sun, 6 Sep 2026 22:06:19 +0530 Subject: [PATCH 6/8] fix(transport): reject incomplete and extra update blocks --- core/fw_transport_uart.c | 30 ++++++++++-- core/fw_update.c | 6 ++- tests/unit/test_fw_transport.c | 88 +++++++++++++++++++++++++++++++++- tests/unit/test_fw_update.c | 11 +++++ 4 files changed, 129 insertions(+), 6 deletions(-) diff --git a/core/fw_transport_uart.c b/core/fw_transport_uart.c index 8bbc8fb..b8b537a 100644 --- a/core/fw_transport_uart.c +++ b/core/fw_transport_uart.c @@ -238,6 +238,11 @@ static int xmodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) if (soh == XMODEM_EOT) { c = XMODEM_ACK; eos_hal_uart_send(&c, 1); + + /* The handshake is complete, but the container decides whether + * what arrived is a whole image. A sender that stops early must + * not have its truncated transfer reported as a success. */ + if (eos_fw_update_bytes_wanted(ctx) != 0) return EOS_ERR_INVALID; break; } @@ -357,11 +362,17 @@ static int ymodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) c = XMODEM_ACK; eos_hal_uart_send(&c, 1); - if (first_block) break; + if (!first_block) { + /* YMODEM sends a second EOT */ + c = XMODEM_CRC; + eos_hal_uart_send(&c, 1); + } - /* YMODEM sends a second EOT */ - c = XMODEM_CRC; - eos_hal_uart_send(&c, 1); + /* The handshake is complete, but the container decides whether + * what arrived is a whole image. A sender that stops early must + * not have its truncated transfer reported as a success, and an + * EOT before block 0 has delivered no image at all. */ + if (eos_fw_update_bytes_wanted(ctx) != 0) return EOS_ERR_INVALID; break; } @@ -467,6 +478,17 @@ static int ymodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) continue; } + /* The container was already complete before this block arrived, so + * these bytes are not padding on the final block -- they are an + * entire extra block, and ACKing them would accept an unbounded + * amount of data no length field accounts for. Padding inside the + * final block is still consumed, just below. */ + if (eos_fw_update_bytes_wanted(ctx) == 0) { + c = XMODEM_CAN; + eos_hal_uart_send(&c, 1); + return EOS_ERR_INVALID; + } + /* Bytes of the declared file carried by this block. file_size stays * the YMODEM framing bound and still marks end of file. */ size_t file_len = block_size; diff --git a/core/fw_update.c b/core/fw_update.c index ee0e868..b1cee81 100644 --- a/core/fw_update.c +++ b/core/fw_update.c @@ -295,8 +295,12 @@ uint8_t eos_fw_update_progress(const eos_fw_update_ctx_t *ctx) if (ctx->state == EOS_FW_STATE_COMPLETE) return 100; /* Bug Fix: Prevent potential integer overflow when computing total size */ + /* The container is header + payload + TLV area and total_received + * counts all three, so the denominator has to as well: leaving the + * TLV area out reported 100% while it was still outstanding. */ uint32_t total; - if (__builtin_add_overflow((uint32_t)sizeof(eos_image_header_t), ctx->payload_total, &total)) { + if (__builtin_add_overflow((uint32_t)sizeof(eos_image_header_t), ctx->payload_total, &total) || + __builtin_add_overflow(total, ctx->tlv_total, &total)) { total = 0xFFFFFFFF; } diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index f28f2bf..25deb47 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -606,6 +606,67 @@ TEST(test_ymodem_declared_size_larger_than_container_is_clamped) ASSERT(!tx_contains(XM_CAN)); } +/* + * Clamping the surplus inside the final block is framing. An entire further + * data block once the container is complete is not, and ACKing it would let + * a sender push an unbounded amount of data that no length field accounts + * for. The declared size leaves room for the extra block, so this exercises + * the container check rather than the file-size bound. + */ +TEST(test_ymodem_data_block_after_container_is_rejected) +{ + eos_fw_update_ctx_t ctx; + uint8_t extra[BLOCK]; + + build_container(); + push_header_block("fw.bin", "640"); /* 424 bytes of image, 5 blocks declared */ + push_container_blocks(); + + memset(extra, 0x5C, sizeof(extra)); + push_block((uint8_t)(CONT_BLOCKS + 1), extra, sizeof(extra)); + rx_push_byte(XM_EOT); + rx_push_byte(XM_EOT); + + ASSERT(run_ymodem_ctx(&ctx) == EOS_ERR_INVALID); + ASSERT(tx_contains(XM_CAN)); + + /* The container that did arrive is intact, and the extra block reached + * neither flash nor the update context. */ + ASSERT(memcmp(&sim_flash[SIM_SLOT_B_ADDR], container, CONT_LEN) == 0); + ASSERT(sim_flash[SIM_SLOT_B_ADDR + CONT_LEN] == 0xFF); + ASSERT(ctx.tlv_written == CONT_TLV_LEN); + + /* eos_fw_transport_update() aborts the context when receive fails, so a + * rejected transfer cannot then be finalized as a good image. */ + eos_fw_update_abort(&ctx); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_IDLE); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + +/* + * EOT ends the transfer, but it does not make a short transfer complete. The + * handshake still runs -- no CAN -- and the truncated result is reported as + * an error rather than as a received image. + */ +TEST(test_ymodem_eot_before_container_is_complete_is_rejected) +{ + eos_fw_update_ctx_t ctx; + int i; + + build_container(); + push_header_block("fw.bin", "512"); + for (i = 0; i < CONT_BLOCKS - 1; i++) + push_block((uint8_t)(i + 1), &container[(size_t)i * BLOCK], BLOCK); + rx_push_byte(XM_EOT); + rx_push_byte(XM_EOT); + + ASSERT(run_ymodem_ctx(&ctx) == EOS_ERR_INVALID); + ASSERT(!tx_contains(XM_CAN)); + ASSERT(eos_fw_update_bytes_wanted(&ctx) != 0); + ASSERT(eos_fw_update_get_state(&ctx) != EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + /* ================================================================ * XMODEM * ================================================================ */ @@ -682,6 +743,28 @@ TEST(test_xmodem_data_block_after_container_is_rejected) ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); } +/* + * The XMODEM counterpart: the EOT handshake completes, but three of the four + * blocks leave the payload and TLV area short, so the transfer is an error + * rather than a received image. + */ +TEST(test_xmodem_eot_before_container_is_complete_is_rejected) +{ + eos_fw_update_ctx_t ctx; + int i; + + build_container(); + for (i = 0; i < CONT_BLOCKS - 1; i++) + push_block((uint8_t)(i + 1), &container[(size_t)i * BLOCK], BLOCK); + rx_push_byte(XM_EOT); + + ASSERT(run_xmodem(&ctx) == EOS_ERR_INVALID); + ASSERT(!tx_contains(XM_CAN)); + ASSERT(eos_fw_update_bytes_wanted(&ctx) != 0); + ASSERT(eos_fw_update_get_state(&ctx) != EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + /* ================================================================ * Raw length-prefixed transport * ================================================================ */ @@ -759,13 +842,16 @@ int main(void) run_test_ymodem_header_size_overflow_is_rejected(); run_test_ymodem_first_block_must_be_zero(); run_test_ymodem_declared_size_larger_than_container_is_clamped(); + run_test_ymodem_data_block_after_container_is_rejected(); + run_test_ymodem_eot_before_container_is_complete_is_rejected(); run_test_xmodem_padded_final_block_completes_and_finalizes(); run_test_xmodem_data_block_after_container_is_rejected(); + run_test_xmodem_eot_before_container_is_complete_is_rejected(); run_test_raw_valid_transfer_is_written(); run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 15; + tests_run = 18; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_fw_update.c b/tests/unit/test_fw_update.c index a99a68f..afe54d8 100644 --- a/tests/unit/test_fw_update.c +++ b/tests/unit/test_fw_update.c @@ -194,7 +194,18 @@ TEST(test_write_does_not_reach_verify_until_tlv_arrives) ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); ASSERT(eos_fw_update_write(&ctx, image, STREAMED_LEN) == EOS_OK); ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_TLV); + + /* The TLV area is part of the container, so an outstanding TLV area + * cannot report a finished transfer. */ + ASSERT(eos_fw_update_bytes_wanted(&ctx) != 0); + ASSERT(eos_fw_update_progress(&ctx) < 100); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); + + ASSERT(eos_fw_update_write(&ctx, image + STREAMED_LEN, TLV_AREA_LEN) == EOS_OK); + ASSERT(eos_fw_update_get_state(&ctx) == EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_bytes_wanted(&ctx) == 0); + ASSERT(eos_fw_update_progress(&ctx) == 100); } TEST(test_write_rejects_bytes_past_the_image_container) From d722e9a0ae53d12461380f9a729f71e44b977d83 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Sun, 6 Sep 2026 22:59:44 +0530 Subject: [PATCH 7/8] fix(transport): reject incomplete raw transfers --- core/fw_transport_uart.c | 13 ++++++++---- include/eos_fw_transport.h | 12 ++++++++++-- tests/unit/test_fw_transport.c | 36 ++++++++++++++++++++++++++++++---- 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/core/fw_transport_uart.c b/core/fw_transport_uart.c index b8b537a..be1896a 100644 --- a/core/fw_transport_uart.c +++ b/core/fw_transport_uart.c @@ -120,6 +120,15 @@ static int uart_raw_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) eos_hal_watchdog_feed(); } + /* The length prefix is the sender's framing; the container decides + * whether what arrived is a whole image. A transfer that ends inside + * the container must not be acknowledged as one. */ + if (eos_fw_update_bytes_wanted(ctx) != 0) { + uint8_t nak = UART_RAW_NAK; + eos_hal_uart_send(&nak, 1); + return EOS_ERR_INVALID; + } + uint8_t ack = UART_RAW_ACK; eos_hal_uart_send(&ack, 1); @@ -523,10 +532,6 @@ static int ymodem_receive(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx) eos_hal_uart_send(&c, 1); expected_blk++; eos_hal_watchdog_feed(); - - if (file_size > 0 && total_received >= file_size) { - /* File complete — wait for EOT */ - } } return EOS_OK; diff --git a/include/eos_fw_transport.h b/include/eos_fw_transport.h index 7d0b86e..9e5bba3 100644 --- a/include/eos_fw_transport.h +++ b/include/eos_fw_transport.h @@ -43,8 +43,16 @@ typedef struct { void (*deinit)(eos_fw_transport_t *tp); /** - * Receive firmware into the update context. Blocks until complete, - * error, or timeout. Calls eos_fw_update_write() internally. + * Receive firmware into the update context. Blocks until the update + * container is complete, or until error or timeout. Calls + * eos_fw_update_write() internally. + * + * Completeness is decided by the container, not by the transport's own + * framing. A transfer that satisfies the protocol — a declared length + * fully received, or an end-of-transfer marker — but leaves + * eos_fw_update_bytes_wanted() non-zero is an error, not a success. + * EOS_OK therefore means the whole [header][payload][TLV area] + * container was written and the context is ready to finalize. */ int (*receive)(eos_fw_transport_t *tp, eos_fw_update_ctx_t *ctx); diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index 25deb47..f9bdc22 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -769,7 +769,7 @@ TEST(test_xmodem_eot_before_container_is_complete_is_rejected) * Raw length-prefixed transport * ================================================================ */ -static int run_raw(void) +static int run_raw_ctx(eos_fw_update_ctx_t *ctx) { const eos_fw_transport_ops_t *ops = eos_fw_transport_uart_raw(); eos_fw_transport_t tp; @@ -778,9 +778,14 @@ static int run_raw(void) tp.baudrate = 115200; tp.timeout_ms = 10; + ASSERT(eos_fw_update_begin(ctx, EOS_SLOT_B) == EOS_OK); + return ops->receive(&tp, ctx); +} + +static int run_raw(void) +{ eos_fw_update_ctx_t ctx; - ASSERT(eos_fw_update_begin(&ctx, EOS_SLOT_B) == EOS_OK); - return ops->receive(&tp, &ctx); + return run_raw_ctx(&ctx); } static void push_le32(uint32_t v) @@ -802,6 +807,28 @@ TEST(test_raw_valid_transfer_is_written) ASSERT(tx_contains(XM_ACK)); } +/* + * The length prefix is the sender's framing, not the image's. A transfer + * that delivers every declared byte but stops inside the container leaves + * the TLV area outstanding, and must be NAK'd rather than acknowledged as a + * received image. + */ +TEST(test_raw_transfer_shorter_than_container_is_rejected) +{ + eos_fw_update_ctx_t ctx; + size_t short_len = CONT_LEN - CONT_TLV_LEN; /* header + payload, no TLV */ + + build_container(); + push_le32((uint32_t)short_len); + rx_push(container, short_len); + + ASSERT(run_raw_ctx(&ctx) == EOS_ERR_INVALID); + ASSERT(tx_contains(XM_NAK)); + ASSERT(eos_fw_update_bytes_wanted(&ctx) != 0); + ASSERT(eos_fw_update_get_state(&ctx) != EOS_FW_STATE_VERIFY); + ASSERT(eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST) == EOS_ERR_INVALID); +} + /* * Regression: the 4-byte length prefix is attacker-controlled and was used * unchecked, so a declared length of 0xFFFFFFFF drove a multi-gigabyte @@ -848,10 +875,11 @@ int main(void) run_test_xmodem_data_block_after_container_is_rejected(); run_test_xmodem_eot_before_container_is_complete_is_rejected(); run_test_raw_valid_transfer_is_written(); + run_test_raw_transfer_shorter_than_container_is_rejected(); run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 18; + tests_run = 19; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } From 0aec419ee5e5cb769745b221506282d31b436ee7 Mon Sep 17 00:00:00 2001 From: Tanisha Panchal Date: Mon, 7 Sep 2026 01:52:48 +0530 Subject: [PATCH 8/8] docs: document transport completeness contract --- docs/book/book.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/book/book.md b/docs/book/book.md index fc1a4a4..125fe73 100644 --- a/docs/book/book.md +++ b/docs/book/book.md @@ -743,6 +743,14 @@ typedef struct { } eos_fw_transport_t; ``` +A transport's own framing — a length prefix, a block or packet count, an +end-of-transfer marker — bounds the transfer, but the update container is +authoritative for completeness, and `eos_fw_update_bytes_wanted()` reports how +much of `[header][payload][TLV area]` is still outstanding. Returning `EOS_OK` +while that count is non-zero is a contract violation rather than a partial +success. `EOS_OK` means the complete `[header][payload][TLV area]` update +container has been written. + ### 7.2 Built-in Transports | Transport | Protocol | Use Case |