From 000c05c7623f3a938f6d8917da219c8c889f7bbd Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 18 Aug 2026 03:04:27 +0200 Subject: [PATCH] F-9759: bound P1021 QE microcode offsets and length to the NAND buffer qe_upload_firmware() validated only the self-declared consistency of the QE microcode blob (QEF magic, version, count range, length equal to the computed size, optional CRC32). The per-microcode code_offset and count fields and the total length were never bounded to the 64 KiB buffer actually read from NAND - which, on the shipped p1021 config, lives in the update partition (QE_FW_NAND 0x01F00000 inside the 0x1200000-0x21FFFFF update range). A structurally consistent blob with a large code_offset or word count made the upload loop read arbitrarily far past the buffer and copy it into QE instruction RAM before the bootloader authenticated anything. Reject a blob whose declared length exceeds QE_FW_LENGTH, and reject any microcode whose code_offset + count words extends past the declared length (checked in 64-bit so the offset/count sum cannot wrap). This bounds every read the upload performs - the microcode words and the trailing CRC32 - to the validated region, and fails closed. The finding's other recommendation - cryptographic authentication of the microcode before activation - is a design decision (keys, signed manifest, build flow; QE is also needed for NAND access, so it runs in hal_init() ahead of image authentication) and is tracked as a note on the finding rather than forced in here. Added tools/unit-tests/unit-p1021-qe-firmware: it compiles the real hal/nxp_p1021.c (generated host copy, PowerPC accessors shadowed) and drives qe_upload_firmware() with crafted blobs: a valid blob (accepted, words counted in QE instruction RAM), a microcode sitting past the end of the declared length (rejected; pre-fix accepted), a self-consistent blob longer than the 64 KiB buffer (rejected; pre-fix accepted), and a bad magic (rejected). Verified: unit-p1021-qe-firmware 4/4 green post-fix (both out-of-bounds cases accepted pre-fix), tools/unit-tests suite green (112 binaries). --- hal/nxp_p1021.c | 19 ++ tools/unit-tests/Makefile | 10 + tools/unit-tests/unit-p1021-qe-firmware.c | 254 ++++++++++++++++++++++ 3 files changed, 283 insertions(+) create mode 100644 tools/unit-tests/unit-p1021-qe-firmware.c diff --git a/hal/nxp_p1021.c b/hal/nxp_p1021.c index a21b00e3..5d2ad7d7 100644 --- a/hal/nxp_p1021.c +++ b/hal/nxp_p1021.c @@ -1229,6 +1229,7 @@ static void qe_upload_microcode(const struct qe_firmware *firmware, static int qe_upload_firmware(const struct qe_firmware *firmware) { unsigned int i, j; + uint64_t mcode_end; #ifdef ENABLE_QE_CRC32 uint32_t crc; #endif @@ -1273,6 +1274,24 @@ static int qe_upload_firmware(const struct qe_firmware *firmware) return -1; } + /* The blob must fit in the buffer actually read from NAND, and every + * microcode (and the trailing CRC) must stay inside it: count and + * code_offset are attacker-controlled until proven otherwise. */ + if (length > QE_FW_LENGTH) { + wolfBoot_printf("QE length %d exceeds the %d byte buffer\n", + length, QE_FW_LENGTH); + return -1; + } + for (i = 0; i < firmware->count; i++) { + mcode_end = (uint64_t)firmware->microcode[i].code_offset + + (uint64_t)sizeof(uint32_t) * firmware->microcode[i].count; + + if (mcode_end > length) { + wolfBoot_printf("QE microcode %d out of bounds\n", i); + return -1; + } + } + #ifdef ENABLE_QE_CRC32 /* Validate the CRC */ crc = *(uint32_t *)((void *)firmware + calc_size); diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 4bc6b263..689768ca 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -107,6 +107,7 @@ TESTS+=unit-zynq-erase-loop TESTS+=unit-versal-qspi-dma TESTS+=unit-sdhci-uhs-recover TESTS+=unit-ti-hercules-write +TESTS+=unit-p1021-qe-firmware # The x86-64 EFI unit test needs the gnu-efi development headers (same # dependency as the CMake x86_64_efi target). Probe and only add the test @@ -868,6 +869,15 @@ ti_hercules_write_extract.h: ../../hal/ti_hercules.c unit-ti-hercules-write: unit-ti-hercules-write.c ti_hercules_write_extract.h gcc -o $@ unit-ti-hercules-write.c $(CFLAGS) $(LDFLAGS) +# unit-p1021-qe-firmware drives the real qe_upload_firmware() from +# hal/nxp_p1021.c (F-9759 bounds part: microcode offsets/counts were +# never bounded to the 64 KiB buffer read from the update partition). +# Reuses the nxp_p1021_host.c generated for the FCM tests, with the +# PowerPC accessors shadowed the same way. + +unit-p1021-qe-firmware: unit-p1021-qe-firmware.c nxp_p1021_host.c + gcc -o $@ unit-p1021-qe-firmware.c -DTARGET_nxp_p1021 -I../../hal $(CFLAGS) $(LDFLAGS) + # unit-efi-x86-open-image includes hal/x86_64_efi.c directly, with in-test # host mocks standing in for the gnu-efi runtime (BS, LibFileInfo, the # x86-64 efi_callN() trampolines). This is the only host build coverage diff --git a/tools/unit-tests/unit-p1021-qe-firmware.c b/tools/unit-tests/unit-p1021-qe-firmware.c new file mode 100644 index 00000000..5ab40465 --- /dev/null +++ b/tools/unit-tests/unit-p1021-qe-firmware.c @@ -0,0 +1,254 @@ +/* unit-p1021-qe-firmware.c + * + * Regression test for F-9759 (bounds part): qe_upload_firmware() in + * hal/nxp_p1021.c validated only the self-declared consistency of the + * QE microcode blob (magic, version, count range, length == computed + * size, optional CRC32). The per-microcode code_offset/count fields and + * the total length were never bounded to the 64 KiB buffer actually + * read from NAND (which, on the shipped p1021 config, sits in the + * update partition). A structurally consistent blob with a large + * code_offset or word count made the upload loop read (and copy into + * QE instruction RAM) arbitrarily far past the buffer. + * + * Note: the full F-9759 recommendation also calls for cryptographic + * authentication of the microcode before activation. That is a design + * decision (keys, signed manifest, build flow) beyond a minimal patch + * and is tracked as a note on the finding; this test covers the + * fail-closed bounding that can be fixed in the code. + * + * The test compiles the real hal/nxp_p1021.c (as nxp_p1021_host.c, + * generated by the Makefile) with the PowerPC accessors shadowed by + * host equivalents, and drives qe_upload_firmware() directly with + * crafted blobs. + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#include +#include +#include +#include + +#define TARGET_nxp_p1021 + +/* Back the CCSR register space with host memory so MMIO-style accesses + * land somewhere harmless. */ +static uint32_t g_ccsr_regs[0x200000 / sizeof(uint32_t)]; + +#include "nxp_ppc.h" + +#undef CCSRBAR +#define CCSRBAR ((uintptr_t)g_ccsr_regs) + +/* Shadow the PowerPC asm accessors with host equivalents (see + * unit-p1021-fcm-bytes.c for the rationale). set32 goes through a hook + * so the tests can count writes to QE instruction RAM. */ +#define get8(addr) (*(const volatile uint8_t *)(addr)) +#define set8(addr, v) (*(volatile uint8_t *)(addr) = (uint8_t)(v)) +#define get16(addr) (*(const volatile uint16_t *)(addr)) +#define set16(addr, v) (*(volatile uint16_t *)(addr) = (uint16_t)(v)) +#define get32(addr) (*(const volatile uint32_t *)(addr)) + +static uint32_t g_iram_writes; +/* Defined below, after the HAL include (needs the QE register macros). */ +static void p1021_set32(volatile uint32_t *addr, uint32_t v); +#define set32(addr, v) p1021_set32((addr), (uint32_t)(v)) +#undef mtspr +#define mtspr(rn, v) ((void)0) +#undef mfspr +#define mfspr(rn) (0) +#undef mfmsr +#define mfmsr() (0) +#undef mtmsr +#define mtmsr(v) ((void)0) + +/* PPC runtime symbols referenced by the emitted init code; never called + * by the tests. */ +uint32_t _secondary_start_page; +uint32_t _second_half_boot_page; +uint32_t _bootpg_addr; +uint32_t _spin_table; +uint32_t _spin_table_addr; + +unsigned long long get_ticks(void) +{ + return 0; +} +void wait_ticks(unsigned long long t) +{ + (void)t; +} +unsigned long get_pc(void) +{ + return 0; +} +void set_tlb(uint8_t tlb, uint8_t esel, uint32_t epn, uint32_t rpn, + uint32_t urpn, uint8_t perms, uint8_t wimge, uint8_t ts, + uint8_t tsize, uint8_t iprot) +{ + (void)tlb; (void)esel; (void)epn; (void)rpn; (void)urpn; + (void)perms; (void)wimge; (void)ts; (void)tsize; (void)iprot; +} +void disable_tlb1(uint8_t esel) +{ + (void)esel; +} +void flush_cache(uint32_t start_addr, uint32_t size) +{ + (void)start_addr; (void)size; +} +void set_law(uint8_t idx, uint32_t addr_h, uint32_t addr_l, + uint32_t trgt_id, uint32_t law_sz, int reset) +{ + (void)idx; (void)addr_h; (void)addr_l; + (void)trgt_id; (void)law_sz; (void)reset; +} + +/* The code under test (its statics become visible here). */ +#include "nxp_p1021_host.c" + +static void p1021_set32(volatile uint32_t *addr, uint32_t v) +{ + if (addr == (volatile uint32_t *)QE_IRAM_IDATA) + g_iram_writes++; + *(volatile uint32_t *)addr = v; +} + +/* Blob workspace: 512 KiB so that pre-fix out-of-bounds reads (up to + * 256 KiB past a 64 KiB blob) stay inside host memory. */ +static uint8_t g_blob[512 * 1024]; + +static uint32_t consistent_length(uint32_t nwords) +{ + return sizeof(struct qe_firmware) + nwords * sizeof(uint32_t) + + sizeof(uint32_t); +} + +static struct qe_firmware *make_blob(uint32_t code_offset, + uint32_t nwords, uint32_t length) +{ + struct qe_firmware *fw = (struct qe_firmware *)g_blob; + + memset(g_blob, 0, sizeof(g_blob)); + fw->header.length = length; + fw->header.magic[0] = 'Q'; + fw->header.magic[1] = 'E'; + fw->header.magic[2] = 'F'; + fw->header.version = 1; + fw->split = 1; + fw->count = 1; + fw->microcode[0].code_offset = code_offset; + fw->microcode[0].count = nwords; + + return fw; +} + +static void setup(void) +{ + memset(g_ccsr_regs, 0, sizeof(g_ccsr_regs)); + g_iram_writes = 0; +} + +static void teardown(void) +{ +} + +/* A well-formed blob with its code right after the header is accepted + * and its words are uploaded to instruction RAM. */ +START_TEST(test_valid_blob_accepted) +{ + const uint32_t n = 64; + struct qe_firmware *fw = + make_blob(sizeof(struct qe_firmware), n, consistent_length(n)); + + ck_assert_int_eq(qe_upload_firmware(fw), 0); + ck_assert_uint_eq(g_iram_writes, n); +} +END_TEST + +/* A blob whose microcode sits past the end of the declared length is + * rejected even though the length is self-consistent. Pre-fix this + * passed validation and read/copyed past the buffer. */ +START_TEST(test_mcode_out_of_bounds_rejected) +{ + const uint32_t n = 64; + const uint32_t offset = sizeof(struct qe_firmware) + 0x44; + struct qe_firmware *fw = + make_blob(offset, n, consistent_length(n)); + + ck_assert_int_eq(qe_upload_firmware(fw), -1); + ck_assert_uint_eq(g_iram_writes, 0); +} +END_TEST + +/* A self-consistent blob longer than the 64 KiB buffer that was read + * from NAND is rejected. Pre-fix this passed validation and the upload + * loop read up to 256 KiB past the buffer. */ +START_TEST(test_length_exceeds_buffer_rejected) +{ + const uint32_t n = QE_FW_LENGTH / sizeof(uint32_t); /* 16384 words */ + struct qe_firmware *fw = + make_blob(sizeof(struct qe_firmware), n, consistent_length(n)); + + ck_assert_int_eq(qe_upload_firmware(fw), -1); + ck_assert_uint_eq(g_iram_writes, 0); +} +END_TEST + +/* A blob with a bad magic is rejected (existing behavior, guard). */ +START_TEST(test_bad_magic_rejected) +{ + const uint32_t n = 16; + struct qe_firmware *fw = + make_blob(sizeof(struct qe_firmware), n, consistent_length(n)); + uint8_t *p = (uint8_t *)fw; + + p[4] = 'X'; /* clobber the magic */ + ck_assert_int_eq(qe_upload_firmware(fw), -1); + ck_assert_uint_eq(g_iram_writes, 0); +} +END_TEST + +Suite *p1021_qe_suite(void) +{ + Suite *s = suite_create("p1021-qe-firmware"); + TCase *tc = tcase_create("p1021-qe-firmware"); + + tcase_add_checked_fixture(tc, setup, teardown); + tcase_add_test(tc, test_valid_blob_accepted); + tcase_add_test(tc, test_mcode_out_of_bounds_rejected); + tcase_add_test(tc, test_length_exceeds_buffer_rejected); + tcase_add_test(tc, test_bad_magic_rejected); + + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + Suite *s = p1021_qe_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + + return fails; +}