Fix anti-rollback to use authenticated TLV counter

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);
 
@@ -185,6 +194,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;
@@ -202,6 +247,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;
         }
 
@@ -236,8 +286,20 @@ 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);
+        /* 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;
+        }
+
+        /* 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);
@@ -309,11 +371,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;
         }
 
@@ -384,6 +452,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;
@@ -408,28 +487,51 @@ 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;
+        /* 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;
+        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++;
         eos_hal_watchdog_feed();
-
-        if (file_size > 0 && total_received >= file_size) {
-            /* File complete — wait for EOT */
-        }
     }
 
     return EOS_OK;

core/fw_update.c

@@ -10,6 +10,8 @@
 #include "eos_fw_update.h"
 #include "eos_hal.h"
 #include "eos_fwsvc.h"
+#include "eos_rollback.h"
+#include "eos_image_tlv.h"
 #include <string.h>
 
 int eos_fw_update_begin(eos_fw_update_ctx_t *ctx, eos_slot_t slot)
@@ -60,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;
 
@@ -85,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,
@@ -134,20 +156,66 @@ 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.
+     * 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.
+     *
+     * 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;
 }
 
 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) {
@@ -195,10 +263,19 @@ int eos_fw_update_finalize(eos_fw_update_ctx_t *ctx, eos_upgrade_mode_t mode)
         return sig_rc;
     }
 
-    /* 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;
@@ -229,8 +306,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;
     }
 
@@ -240,6 +321,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;