Skip to content

Commit 9ca1d43

Browse files
dgarskedanielinux
authored andcommitted
Peer review fixes (copilot)
src/fdt.c, include/fdt.h - Propagate fdt_fixup_initrd error in fit_load_ramdisk so a /chosen patch failure no longer silently boots a kernel with no initrd. - Add fit_load_image_to(): decompress (or memcpy) directly to a caller-supplied destination buffer instead of going through the FIT-declared `load` address. fit_load_ramdisk now uses this when WOLFBOOT_LOAD_RAMDISK_ADDRESS is set, so the override is a real safety bound for compressed ramdisks (previously the gzip stream was still inflated to the FIT `load` and only memcpy'd afterward). - Refactor fit_load_image_ex into a shared inner helper. - Reword the WOLFBOOT_FIT_MAX_DECOMP comment: the cap is a sanity ceiling, not a per-destination memory-safety bound. Authenticity is provided by the outer wolfBoot signature; tighter bounds need fit_load_image_ex / _to with an explicit out_max / dst_max. - Add WOLFBOOT_FIT_MAX_RAMDISK (defaults to WOLFBOOT_FIT_MAX_DECOMP) so targets can pin a tighter ramdisk decompression bound. src/update_ram.c, src/update_disk.c - Panic when fit_load_image() returns NULL for the kernel subimage instead of letting load_address=NULL propagate into do_boot(). tools/unit-tests/unit-gzip.c - Add deterministic stored / fixed-Huffman / dynamic-Huffman gzip fixtures so the inflater's BTYPE 00/01/10 paths are exercised independent of host gzip(1) heuristics. - Add FEXTRA / FNAME / FCOMMENT / FHCRC and combined-flag fixtures plus a truncated-FEXTRA negative case to cover the optional gzip header parser. tools/unit-tests/unit-fit-gzip.c (new), tools/unit-tests/Makefile - New libcheck binary covering the FIT loader's compression branches: gzip success, gzip stream corruption, unknown compression, compression="none" baseline, and the no-load fail-closed path. Built twice from the same source - once with WOLFBOOT_GZIP for the success / runtime-failure paths, and once without it so the compile-time fail-closed branch is also tested.
1 parent d920530 commit 9ca1d43

7 files changed

Lines changed: 632 additions & 28 deletions

File tree

include/fdt.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,12 @@ const char* fit_find_images(void* fdt, const char** pkernel, const char** pflat_
172172
const char** pramdisk);
173173
void* fit_load_image(void* fdt, const char* image, int* lenp);
174174
void* fit_load_image_ex(void* fdt, const char* image, int* lenp, uint32_t out_max);
175+
/* Load (and, if compressed, decompress) a FIT subimage directly to a
176+
* caller-supplied destination buffer. Overrides the FIT image's
177+
* `load` and `entry` properties - dst is both the destination and the
178+
* value returned. dst_max bounds the decompressed size. */
179+
void* fit_load_image_to(void* fdt, const char* image, void* dst,
180+
uint32_t dst_max, int* lenp);
175181

176182
/* FDT initrd fixup: writes /chosen/linux,initrd-{start,end} as 64-bit
177183
* big-endian properties. Creates /chosen if missing. Returns 0 on success

src/fdt.c

Lines changed: 72 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,15 @@
3434
#include "gzip.h"
3535
#endif
3636

37-
/* Default upper bound on a single FIT subimage's decompressed size.
38-
* The outer wolfBoot signature already authenticates the FIT, but a
39-
* concrete cap defends against a malformed-but-signed FIT scribbling
40-
* across unrelated memory. Override per target via:
37+
/* Coarse upper bound on a single FIT subimage's decompressed size.
38+
* This is a sanity ceiling, not a per-destination memory-safety
39+
* bound. Authenticity of the FIT bytes is provided by the outer
40+
* wolfBoot signature; this cap is a belt-and-suspenders limit so a
41+
* malformed-but-signed gzip stream cannot inflate without bound.
42+
* Callers that need a tighter, RAM-window-aware bound should use
43+
* fit_load_image_ex() (FIT-`load` destination + explicit out_max)
44+
* or fit_load_image_to() (caller-supplied destination + dst_max).
45+
* Override per target via:
4146
* CFLAGS+=-DWOLFBOOT_FIT_MAX_DECOMP=...
4247
*/
4348
#ifndef WOLFBOOT_FIT_MAX_DECOMP
@@ -904,13 +909,22 @@ int fdt_fixup_initrd(void* fdt, uint64_t start, uint64_t size)
904909
#define WOLFBOOT_LOAD_RAMDISK_ADDRESS 0
905910
#endif
906911

912+
/* Upper bound on the (decompressed) ramdisk size. Defaults to the
913+
* generic FIT decompression cap. Targets with a tighter known-safe
914+
* RAM window for the ramdisk should override this. */
915+
#ifndef WOLFBOOT_FIT_MAX_RAMDISK
916+
#define WOLFBOOT_FIT_MAX_RAMDISK WOLFBOOT_FIT_MAX_DECOMP
917+
#endif
918+
907919
/* Load a FIT ramdisk subimage and patch the DTB's /chosen
908920
* linux,initrd-{start,end} to point at it. If
909-
* WOLFBOOT_LOAD_RAMDISK_ADDRESS is nonzero, the ramdisk is relocated
910-
* to that fixed address (overrides the FIT's `load` property);
911-
* otherwise the address fit_load_image returned (FIT-specified or
912-
* in-FIT pointer) is used as-is. Caller passes the DTB pointer for
913-
* the initrd fixup, or NULL to skip the fixup.
921+
* WOLFBOOT_LOAD_RAMDISK_ADDRESS is nonzero, the ramdisk is loaded
922+
* directly to that fixed address - bypassing the FIT's `load`
923+
* property entirely - so a gzip-compressed ramdisk is decompressed
924+
* straight into the override (with the override capacity acting as
925+
* the safety bound). Otherwise the address fit_load_image returned
926+
* (FIT-specified or in-FIT pointer) is used as-is. Caller passes the
927+
* DTB pointer for the initrd fixup, or NULL to skip the fixup.
914928
*
915929
* Returns 0 on success, -1 if the ramdisk node was found but the
916930
* load failed. The current callers ignore the return value
@@ -926,41 +940,52 @@ int fit_load_ramdisk(void* fit, const char* ramdisk_node, void* dts_addr)
926940
return -1;
927941
}
928942

929-
rd_ptr = (uint8_t*)fit_load_image(fit, ramdisk_node, &rd_size);
930-
if (rd_ptr == NULL || rd_size <= 0) {
931-
wolfBoot_printf("FIT: ramdisk node present but load failed\n");
932-
return -1;
933-
}
934-
935943
if (WOLFBOOT_LOAD_RAMDISK_ADDRESS != 0) {
936944
rd_dst = (uint8_t*)WOLFBOOT_LOAD_RAMDISK_ADDRESS;
937-
if (rd_ptr != rd_dst) {
938-
wolfBoot_printf("Loading ramdisk: %p -> %p (%d bytes)\n",
939-
rd_ptr, rd_dst, rd_size);
940-
memcpy(rd_dst, rd_ptr, rd_size);
941-
}
942-
else {
943-
wolfBoot_printf("Loaded ramdisk: %p (%d bytes)\n",
944-
rd_dst, rd_size);
945+
rd_ptr = (uint8_t*)fit_load_image_to(fit, ramdisk_node,
946+
rd_dst, (uint32_t)WOLFBOOT_FIT_MAX_RAMDISK, &rd_size);
947+
if (rd_ptr == NULL || rd_size <= 0) {
948+
wolfBoot_printf("FIT: ramdisk node present but load failed\n");
949+
return -1;
945950
}
951+
wolfBoot_printf("Loaded ramdisk: %p (%d bytes)\n",
952+
rd_dst, rd_size);
946953
}
947954
else {
955+
rd_ptr = (uint8_t*)fit_load_image(fit, ramdisk_node, &rd_size);
956+
if (rd_ptr == NULL || rd_size <= 0) {
957+
wolfBoot_printf("FIT: ramdisk node present but load failed\n");
958+
return -1;
959+
}
948960
rd_dst = rd_ptr;
949961
wolfBoot_printf("Loaded ramdisk: %p (%d bytes)\n",
950962
rd_dst, rd_size);
951963
}
952964

953965
if (dts_addr != NULL) {
954-
(void)fdt_fixup_initrd(dts_addr,
966+
int frc = fdt_fixup_initrd(dts_addr,
955967
(uint64_t)(uintptr_t)rd_dst, (uint64_t)rd_size);
968+
if (frc != 0) {
969+
wolfBoot_printf("FIT: fdt_fixup_initrd failed (rc=%d); "
970+
"kernel will not see initrd\n", frc);
971+
return -1;
972+
}
956973
}
957974

958975
return 0;
959976
}
960977
#endif /* WOLFBOOT_FIT_RAMDISK */
961978

962-
void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
963-
uint32_t out_max)
979+
/* Inner implementation shared by fit_load_image_ex and fit_load_image_to.
980+
* When dst_override is non-NULL it replaces the FIT image's `load`
981+
* property as the destination, so a compressed (gzip) payload is
982+
* decompressed directly into the caller's buffer rather than being
983+
* routed through the FIT-declared address. The `entry` property is
984+
* also ignored when dst_override is in effect, since the caller wants
985+
* the override address back.
986+
*/
987+
static void* fit_load_image_inner(void* fdt, const char* image, int* lenp,
988+
uint32_t out_max, void* dst_override)
964989
{
965990
void *load, *entry, *data = NULL;
966991
int off, len = 0;
@@ -976,6 +1001,12 @@ void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
9761001
data = (void*)fdt_getprop(fdt, off, "data", &len);
9771002
load = fdt_getprop_address(fdt, off, "load");
9781003
entry = fdt_getprop_address(fdt, off, "entry");
1004+
if (dst_override != NULL) {
1005+
/* Caller-supplied destination replaces the FIT load
1006+
* property and disables `entry` resolution. */
1007+
load = dst_override;
1008+
entry = NULL;
1009+
}
9791010
if (data != NULL) {
9801011
int is_gzip = 0;
9811012
int is_unknown_comp = 0;
@@ -1073,9 +1104,24 @@ void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
10731104

10741105
}
10751106

1107+
void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
1108+
uint32_t out_max)
1109+
{
1110+
return fit_load_image_inner(fdt, image, lenp, out_max, NULL);
1111+
}
1112+
10761113
void* fit_load_image(void* fdt, const char* image, int* lenp)
10771114
{
10781115
return fit_load_image_ex(fdt, image, lenp, WOLFBOOT_FIT_MAX_DECOMP);
10791116
}
10801117

1118+
void* fit_load_image_to(void* fdt, const char* image, void* dst,
1119+
uint32_t dst_max, int* lenp)
1120+
{
1121+
if (dst == NULL) {
1122+
return NULL;
1123+
}
1124+
return fit_load_image_inner(fdt, image, lenp, dst_max, dst);
1125+
}
1126+
10811127
#endif /* (MMU || WOLFBOOT_FDT) && !BUILD_LOADER_STAGE1 */

src/update_disk.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,13 @@ void RAMFUNCTION wolfBoot_start(void)
548548

549549
(void)fit_find_images(fit, &kernel, &flat_dt, &ramdisk);
550550
if (kernel != NULL) {
551-
load_address = fit_load_image(fit, kernel, NULL);
551+
void *new_load = fit_load_image(fit, kernel, NULL);
552+
if (new_load == NULL) {
553+
wolfBoot_printf("FIT: failed to load kernel '%s'\r\n",
554+
kernel);
555+
wolfBoot_panic();
556+
}
557+
load_address = new_load;
552558
}
553559
if (flat_dt != NULL) {
554560
uint8_t *dts_ptr = fit_load_image(fit, flat_dt, (int*)&dts_size);

src/update_ram.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,12 @@ void RAMFUNCTION wolfBoot_start(void)
375375

376376
(void)fit_find_images(fit, &kernel, &flat_dt, &ramdisk);
377377
if (kernel != NULL) {
378-
load_address = fit_load_image(fit, kernel, NULL);
378+
void *new_load = fit_load_image(fit, kernel, NULL);
379+
if (new_load == NULL) {
380+
wolfBoot_printf("FIT: failed to load kernel '%s'\n", kernel);
381+
wolfBoot_panic();
382+
}
383+
load_address = new_load;
379384
}
380385
if (flat_dt != NULL) {
381386
uint8_t *dts_ptr = fit_load_image(fit, flat_dt, (int*)&dts_size);

tools/unit-tests/Makefile

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128
5959
unit-keygen-xmss-params
6060
TESTS+=unit-tpm-check-rot-auth
6161
TESTS+=unit-tpm-api-names
62+
TESTS+=unit-fit-gzip unit-fit-nogzip
6263

6364
include unit-sign-encrypted-output.mkfrag
6465

@@ -324,6 +325,19 @@ unit-delta: ../../include/target.h unit-delta.c
324325
unit-gzip: ../../include/target.h unit-gzip.c
325326
gcc -o $@ unit-gzip.c $(CFLAGS) -DWOLFBOOT_GZIP $(LDFLAGS)
326327

328+
# FIT-loader gzip / unsupported-compression branch coverage. Built twice
329+
# from the same source: once with WOLFBOOT_GZIP (success + decompress
330+
# failure paths) and once without (compile-time fail-closed path).
331+
unit-fit-gzip: ../../include/target.h unit-fit-gzip.c
332+
gcc -o $@ unit-fit-gzip.c $(CFLAGS) -DWOLFBOOT_FDT -DWOLFBOOT_GZIP \
333+
-DWOLFBOOT_NO_PRINTF \
334+
-ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections
335+
336+
unit-fit-nogzip: ../../include/target.h unit-fit-gzip.c
337+
gcc -o $@ unit-fit-gzip.c $(CFLAGS) -DWOLFBOOT_FDT \
338+
-DWOLFBOOT_NO_PRINTF \
339+
-ffunction-sections -fdata-sections $(LDFLAGS) -Wl,--gc-sections
340+
327341
unit-update-flash: ../../include/target.h unit-update-flash.c
328342
gcc -o $@ unit-update-flash.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)
329343

0 commit comments

Comments
 (0)