PR 892: address 3 Fenrir review findings

- libwolfboot.c: keep the partition state APIs (set/get_partition_state) available to CUSTOM_PARTITION_TRAILER / WOLFBOOT_NO_PARTITIONS builds; guard the fixed-partition APIs (sector flags, erase, trigger, success) behind WOLFBOOT_FIXED_PARTITIONS. Add the unit-custom-trailer-nopart compile target to lock in that configuration. - unit-p1021-fcm-bytes.c: add program tests for status 0x81 (fail) and 0x00 (write-protected), asserting no later page is programmed. - unit-p1021-erase-advance.c: add an erase test for status 0x00 (write-protected), asserting failure before advancing to the next block.
2026-09-16 17:19:16 +02:00 · 2026-09-16 17:19:16 +02:00 · c3f9717fb5
parent 6814b503ef
commit c3f9717fb5
7 changed files with 237 additions and 2 deletions
--- a/include/image.h
+++ b/include/image.h
@ -1732,8 +1732,10 @@ int wolfBoot_open_self_address(struct wolfBoot_image *img, uint8_t *hdr,
 int wolfBoot_verify_integrity(struct wolfBoot_image *img);
 int wolfBoot_verify_authenticity(struct wolfBoot_image *img);
 int wolfBoot_set_partition_state(uint8_t part, uint8_t newst);
+#ifdef WOLFBOOT_FIXED_PARTITIONS
 int wolfBoot_get_update_sector_flag(uint16_t sector, uint8_t *flag);
 int wolfBoot_set_update_sector_flag(uint16_t sector, uint8_t newflag);
+#endif /* WOLFBOOT_FIXED_PARTITIONS */

 #ifdef WOLFBOOT_ELF_FLASH_SCATTER
 /* Support for ELF scatter/gather format */
--- a/include/wolfboot/wolfboot.h
+++ b/include/wolfboot/wolfboot.h
@ -665,8 +665,10 @@ extern "C" {
    #include "image.h"
 #endif

+#ifdef WOLFBOOT_FIXED_PARTITIONS
 void wolfBoot_update_trigger(void);
 void wolfBoot_success(void);
+#endif /* WOLFBOOT_FIXED_PARTITIONS */
 uint32_t wolfBoot_image_size(uint8_t *image);
 uint32_t wolfBoot_get_blob_version(uint8_t *blob);
 uint16_t wolfBoot_get_blob_type(uint8_t *blob);
--- a/src/libwolfboot.c
+++ b/src/libwolfboot.c
@ -685,6 +685,11 @@ static void RAMFUNCTION set_partition_state(uint8_t part, uint8_t val)
    set_trailer_at(part, 1, val);
 }

+/* Update-sector flag helpers and the fixed-partition APIs below need the
+ * fixed partition addresses and wolfboot_magic_trail, which a
+ * CUSTOM_PARTITION_TRAILER / WOLFBOOT_NO_PARTITIONS build does not define.
+ * The partition state APIs above stay available to custom-trailer builds. */
+#ifdef WOLFBOOT_FIXED_PARTITIONS
 /**
 * @brief Set the flags of an update sector.
 *
@ -710,6 +715,7 @@ static uint8_t* RAMFUNCTION get_update_sector_flags(uint32_t pos)
 {
    return (uint8_t *)get_trailer_at(PART_UPDATE, 2 + pos);
 }
+#endif /* WOLFBOOT_FIXED_PARTITIONS */

 /**
 * @brief Set the state of a partition.
@ -735,6 +741,7 @@ int RAMFUNCTION wolfBoot_set_partition_state(uint8_t part, uint8_t newst)
    return 0;
 }

+#ifdef WOLFBOOT_FIXED_PARTITIONS
 /**
 * @brief Set the flag for sector
 *
@ -765,6 +772,7 @@ int RAMFUNCTION wolfBoot_set_update_sector_flag(uint16_t sector,
        set_update_sector_flags(pos, fl_value);
    return 0;
 }
+#endif /* WOLFBOOT_FIXED_PARTITIONS */

 /**
 * @brief Get the state of a partition.
@ -789,6 +797,7 @@ int RAMFUNCTION wolfBoot_get_partition_state(uint8_t part, uint8_t *st)
    return 0;
 }

+#ifdef WOLFBOOT_FIXED_PARTITIONS
 /**
 * @brief Get the flag for sector
 *
@ -956,6 +965,7 @@ void RAMFUNCTION wolfBoot_success(void)
    wolfBoot_erase_encrypt_key();
 #endif
 }
+#endif /* WOLFBOOT_FIXED_PARTITIONS */
 #ifdef __CCRX__
 #pragma section
 #endif
--- a/tools/unit-tests/Makefile
+++ b/tools/unit-tests/Makefile
@ -66,7 +66,7 @@ TESTS:=unit-parser unit-parser-large-header unit-fdt unit-extflash unit-string \
       unit-enc-nvm-flagshome unit-delta unit-gzip unit-update-flash unit-update-flash-delta \
       unit-update-flash-hook \
       unit-update-flash-self-update \
-       unit-update-flash-enc unit-update-flash-enc-full unit-update-ram unit-update-ram-uboot unit-update-ram-enc unit-update-ram-enc-nopart unit-update-ram-nofixed unit-update-ram-nofixed-noramboot unit-update-ram-noramboot unit-update-ram-custom-trailer unit-update-flash-hwswap unit-pkcs11_store unit-psa_store unit-wolfhsm_flash_hal unit-disk \
+       unit-update-flash-enc unit-update-flash-enc-full unit-update-ram unit-update-ram-uboot unit-update-ram-enc unit-update-ram-enc-nopart unit-update-ram-nofixed unit-update-ram-nofixed-noramboot unit-update-ram-noramboot unit-update-ram-custom-trailer unit-custom-trailer-nopart unit-update-flash-hwswap unit-pkcs11_store unit-psa_store unit-wolfhsm_flash_hal unit-disk \
       unit-update-disk unit-update-disk-fsp unit-update-disk-oob unit-update-disk-fit unit-multiboot unit-boot-x86-fsp unit-loader-tpm-init unit-qspi-flash unit-fwtpm-stub unit-tpm-rsa-exp \
       unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-image-dts \
       unit-image-dts-sha384 unit-image-dts-sha3-384 unit-store-sbrk \
@ -361,6 +361,16 @@ unit-update-ram-custom-trailer:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN \
 	-DWOLFBOOT_RAMBOOT_MAX_SIZE=WOLFBOOT_PARTITION_SIZE \
 	-DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT \
 	-DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE
+# CUSTOM_PARTITION_TRAILER + WOLFBOOT_NO_PARTITIONS: no WOLFBOOT_FIXED_PARTITIONS,
+# so wolfboot_magic_trail and the fixed partition addresses are excluded from
+# libwolfboot.c. Proves the partition state API compiles and works through the
+# custom get/set_trailer_at backend without fixed partitions (F-1130279967).
+unit-custom-trailer-nopart:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN \
+	-DUNIT_TEST_AUTH -DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED \
+	-DWOLFBOOT_NO_PARTITIONS \
+	-DCUSTOM_PARTITION_TRAILER \
+	-DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT \
+	-DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE
 # Bound the non-FSP disk load to this test's 64-byte load_buffer (TEST_PAYLOAD_SIZE),
 # the cap update_disk.c now requires; all images here are exactly that size.
 unit-update-disk:CFLAGS+=-DMOCK_PARTITIONS -DPRINTF_ENABLED -DWOLFBOOT_RAMBOOT_MAX_SIZE=0x40 \
@ -946,6 +956,9 @@ unit-update-ram-noramboot: ../../include/target.h unit-update-ram-noramboot.c
 unit-update-ram-custom-trailer: ../../include/target.h unit-update-ram-custom-trailer.c
 	gcc -o $@ unit-update-ram-custom-trailer.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)

+unit-custom-trailer-nopart: ../../include/target.h unit-custom-trailer-nopart.c
+	gcc -o $@ unit-custom-trailer-nopart.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)
+
 unit-update-flash-hwswap: ../../include/target.h unit-update-flash-hwswap.c
 	gcc -o $@ unit-update-flash-hwswap.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)

--- a/tools/unit-tests/unit-custom-trailer-nopart.c
+++ b/tools/unit-tests/unit-custom-trailer-nopart.c
@ -0,0 +1,141 @@
+/* unit-custom-trailer-nopart.c
+ *
+ * Compile + behaviour gate for the CUSTOM_PARTITION_TRAILER /
+ * WOLFBOOT_NO_PARTITIONS configuration: no WOLFBOOT_FIXED_PARTITIONS, so
+ * wolfboot_magic_trail and the fixed partition addresses are excluded from
+ * libwolfboot.c. The partition state API must still compile and work through
+ * the externally-defined get/set_trailer_at backend, while the
+ * fixed-partition functions (sector flags, erase, trigger, success) are
+ * absent.
+ */
+#ifndef WOLFBOOT_HASH_SHA256
+    #define WOLFBOOT_HASH_SHA256
+#endif
+
+#define NO_FORK 1
+
+#include <check.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include "target.h"
+
+#include "user_settings.h"
+#include "wolfboot/wolfboot.h"
+
+/* Custom partition trailer backend (mocked).
+ * Layout per partition: [state(1)][magic(4)]
+ *   get_trailer_at(part, 0) -> magic (base+1 .. base+4)
+ *   get_trailer_at(part, 1) -> state (base+0)
+ */
+static uint8_t mock_trailer_boot[5];
+static uint8_t mock_trailer_update[5];
+
+uint8_t* get_trailer_at(uint8_t part, uint32_t at)
+{
+    uint8_t *base = (part == PART_BOOT) ? mock_trailer_boot
+                                        : mock_trailer_update;
+    if (at == 0)
+        return &base[1];
+    return &base[at - 1];
+}
+
+void set_trailer_at(uint8_t part, uint32_t at, uint8_t val)
+{
+    uint8_t *base = (part == PART_BOOT) ? mock_trailer_boot
+                                        : mock_trailer_update;
+    if (at == 0)
+        base[1] = val;
+    else
+        base[at - 1] = val;
+}
+
+void set_partition_magic(uint8_t part)
+{
+    uint8_t *base = (part == PART_BOOT) ? mock_trailer_boot
+                                        : mock_trailer_update;
+    /* WOLFBOOT_MAGIC_TRAIL = 0x544F4F42 on LE: 'B','O','O','T' */
+    base[1] = 'B';
+    base[2] = 'O';
+    base[3] = 'O';
+    base[4] = 'T';
+    (void)part;
+}
+
+#include "libwolfboot.c"
+#include "unit-mock-flash.c"
+
+static void reset_trailers(void)
+{
+    memset(mock_trailer_boot, 0, sizeof(mock_trailer_boot));
+    memset(mock_trailer_update, 0, sizeof(mock_trailer_update));
+}
+
+/* State API round-trips through the custom backend with no fixed
+ * partitions present. */
+START_TEST(test_set_get_partition_state)
+{
+    uint8_t st = 0;
+
+    reset_trailers();
+
+    /* No magic yet: set writes the magic then the state. */
+    ck_assert_int_eq(wolfBoot_set_partition_state(PART_BOOT,
+        IMG_STATE_TESTING), 0);
+    ck_assert_int_eq(wolfBoot_get_partition_state(PART_BOOT, &st), 0);
+    ck_assert_uint_eq(st, IMG_STATE_TESTING);
+
+    /* Update partition is independent. */
+    ck_assert_int_eq(wolfBoot_set_partition_state(PART_UPDATE,
+        IMG_STATE_UPDATING), 0);
+    ck_assert_int_eq(wolfBoot_get_partition_state(PART_UPDATE, &st), 0);
+    ck_assert_uint_eq(st, IMG_STATE_UPDATING);
+
+    /* PART_NONE is rejected. */
+    ck_assert_int_eq(wolfBoot_set_partition_state(PART_NONE, 0), -1);
+    ck_assert_int_eq(wolfBoot_get_partition_state(PART_NONE, &st), -1);
+
+    /* get on a partition without magic is rejected. */
+    reset_trailers();
+    ck_assert_int_eq(wolfBoot_get_partition_state(PART_BOOT, &st), -1);
+}
+END_TEST
+
+/* Fixed-partition APIs must be absent in this configuration. */
+START_TEST(test_fixed_partition_apis_absent)
+{
+#ifndef WOLFBOOT_FIXED_PARTITIONS
+    ck_assert_int_eq(0, 0);
+#else
+    ck_abort_msg("WOLFBOOT_FIXED_PARTITIONS must be undefined here");
+#endif
+}
+END_TEST
+
+int main(int argc, char *argv[])
+{
+    int failed;
+    Suite *s;
+    TCase *tc;
+    SRunner *sr;
+
+    s = suite_create("custom-trailer-nopart");
+    tc = tcase_create("state-api");
+
+    tcase_add_checked_fixture(tc, reset_trailers, NULL);
+    tcase_add_test(tc, test_set_get_partition_state);
+    tcase_add_test(tc, test_fixed_partition_apis_absent);
+    suite_add_tcase(s, tc);
+
+    sr = srunner_create(s);
+    srunner_set_fork_status(sr, CK_NOFORK);
+    srunner_run_all(sr, CK_NORMAL);
+    failed = srunner_ntests_failed(sr);
+    srunner_free(sr);
+    (void)argc;
+    (void)argv;
+    return (failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
--- a/tools/unit-tests/unit-p1021-erase-advance.c
+++ b/tools/unit-tests/unit-p1021-erase-advance.c
@ -49,9 +49,10 @@ static uint32_t g_status;

 /* ONFI status byte: DQ0 set = program/erase fail, DQ7 clear = protected.
 * 0x80 is a clean success (no fail, not protected); 0x81 is an
- * erase/program failure (DQ0 set). */
+ * erase/program failure (DQ0 set); 0x00 is write-protected (DQ7 clear). */
 #define STATUS_OK      0x80
 #define STATUS_ERASE_FAIL 0x81
+#define STATUS_WP_PROTECTED 0x00

 static void mock_reset(int cmd_ret, uint32_t status)
 {
@ -146,6 +147,23 @@ START_TEST (test_erase_stops_on_status_fail)
 }
 END_TEST

+START_TEST (test_erase_stops_on_write_protected)
+{
+    int ret;
+
+    /* The command sequence completes (cmd_ret 0) but the NAND reports the
+     * block as write-protected in the status byte (DQ7 clear). The erase
+     * must fail and not advance to the next block. */
+    mock_reset(0, STATUS_WP_PROTECTED);
+
+    ret = ext_flash_erase(0, 2 * (int)TEST_BLOCK_SIZE);
+
+    ck_assert_int_eq(ret, -1);
+    ck_assert_int_eq(g_page_calls, 1);
+    ck_assert_int_eq(g_pages[0], 0);
+}
+END_TEST
+
 Suite *p1021_erase_suite(void)
 {
    Suite *s  = suite_create("p1021 erase advance");
@ -154,6 +172,7 @@ Suite *p1021_erase_suite(void)
    tcase_add_test(tc, test_erase_advances_through_blocks);
    tcase_add_test(tc, test_erase_stops_on_command_error);
    tcase_add_test(tc, test_erase_stops_on_status_fail);
+    tcase_add_test(tc, test_erase_stops_on_write_protected);
    tcase_set_timeout(tc, 10);
    suite_add_tcase(s, tc);
    return s;
--- a/tools/unit-tests/unit-p1021-fcm-bytes.c
+++ b/tools/unit-tests/unit-p1021-fcm-bytes.c
@ -459,6 +459,52 @@ START_TEST(test_p1021_write_unaligned)
 }
 END_TEST

+/* A program that reports failure in the status byte (DQ0 set) must stop:
+ * the first page is the last programmed, the loop does not advance to the
+ * next page. setup() pins MDR to 0x80 (success), so override it here. */
+START_TEST(test_p1021_write_status_fail)
+{
+    uint8_t data[2 * 1024];
+    size_t i;
+
+    fill(data, sizeof(data), 0x50);
+    /* DQ0 set = program failed, DQ7 set = not protected. */
+    set32(ELBC_MDR, 0x81);
+
+    ck_assert_int_eq(ext_flash_write(0, data, 600), -1);
+
+    /* Only the first page was programmed; the loop stopped before page 1. */
+    ck_assert_int_eq(g_fbcr_n, 1);
+    ck_assert_uint_eq(g_fbcr_log[0], 0);
+    for (i = 0; i < 512; i++)
+        ck_assert_uint_eq(g_nand[i], data[i]);
+    for (i = 0; i < 512; i++)
+        ck_assert_uint_eq(NAND(1, i), 0xFF);
+}
+END_TEST
+
+/* A program on a write-protected block (DQ7 clear) must stop the same way:
+ * no later page is programmed. */
+START_TEST(test_p1021_write_write_protected)
+{
+    uint8_t data[2 * 1024];
+    size_t i;
+
+    fill(data, sizeof(data), 0x60);
+    /* DQ0 clear = no fail, DQ7 clear = write protected. */
+    set32(ELBC_MDR, 0x00);
+
+    ck_assert_int_eq(ext_flash_write(0, data, 600), -1);
+
+    ck_assert_int_eq(g_fbcr_n, 1);
+    ck_assert_uint_eq(g_fbcr_log[0], 0);
+    for (i = 0; i < 512; i++)
+        ck_assert_uint_eq(g_nand[i], data[i]);
+    for (i = 0; i < 512; i++)
+        ck_assert_uint_eq(NAND(1, i), 0xFF);
+}
+END_TEST
+
 /* A full-page read from column 0 must keep BC = 0 (full page + spare,
 * the only ECC-checking setting). */
 START_TEST(test_p1021_read_full_page)
@ -573,6 +619,8 @@ Suite *p1021_fcm_suite(void)
    tcase_add_test(tc, test_p1021_write_partial);
    tcase_add_test(tc, test_p1021_write_multipart);
    tcase_add_test(tc, test_p1021_write_unaligned);
+    tcase_add_test(tc, test_p1021_write_status_fail);
+    tcase_add_test(tc, test_p1021_write_write_protected);
    tcase_add_test(tc, test_p1021_read_full_page);
    tcase_add_test(tc, test_p1021_read_short_spare_loaded);
    tcase_add_test(tc, test_p1021_read_multipart);