Peer review fixes

Author: dgarske

docs/Targets.md

@@ -1257,7 +1257,7 @@ MACHINE=mpfs-video-kit bitbake mchp-base-image-sdk
 
 Build images are output to: `./tmp-glibc/deploy/images/mpfs-video-kit/`
 
-#### Custom FIT image, signing and coping to SDCard
+#### Custom FIT image, signing and copying to SDCard
 
 wolfBoot can either decompress a gzipped kernel at boot time (`GZIP=1`,
 the default for `polarfire_mpfs250.config` and `polarfire_mpfs250_qspi.config`)
@@ -3455,6 +3455,13 @@ and the `bl31`/`fsbl` versus `bl31`/`plm` boot chain. Set `GZIP=0` in
 `.config` if you want to keep using an uncompressed `Image` plus
 `compression = "none"`.
 
+The decompressed-output bound for any single FIT subimage defaults to
+`WOLFBOOT_FIT_MAX_DECOMP = 256 MB`. Override per target via
+`CFLAGS+=-DWOLFBOOT_FIT_MAX_DECOMP=...` if a kernel/ramdisk legitimately
+expands beyond that. The outer wolfBoot signature still authenticates the
+entire FIT; this cap is defense-in-depth against a malformed-but-signed
+stream.
+
 **FIT ramdisk (initramfs)**
 
 When PetaLinux is built with `INITRAMFS_IMAGE_BUNDLE = "0"` the rootfs cpio
@@ -3497,6 +3504,7 @@ images {
         data = /incbin/("rootfs.cpio.gz");
         type = "ramdisk";
         compression = "gzip";  /* or "none" */
+        load = <0x40000000>;   /* required for decompression / relocation */
         hash-1 { algo = "sha256"; };
     };
 };

src/fdt.c

@@ -40,6 +40,16 @@
 #endif
 #endif /* WOLFBOOT_GZIP */
 
+/* Default upper bound on a single FIT subimage's decompressed size.
+ * The outer wolfBoot signature already authenticates the FIT, but a
+ * concrete cap defends against a malformed-but-signed FIT scribbling
+ * across unrelated memory. Override per target via:
+ *   CFLAGS+=-DWOLFBOOT_FIT_MAX_DECOMP=...
+ */
+#ifndef WOLFBOOT_FIT_MAX_DECOMP
+#define WOLFBOOT_FIT_MAX_DECOMP (256U * 1024U * 1024U)
+#endif
+
 uint32_t cpu_to_fdt32(uint32_t x)
 {
 #ifdef BIG_ENDIAN_ORDER
@@ -911,6 +921,9 @@ static int fit_verify_hash(const void *fdt, int img_off,
     int hash_off, len = 0;
     const char *algo = NULL;
     const uint8_t *value = NULL;
+#if defined(WOLFBOOT_HASH_SHA256) || defined(WOLFBOOT_HASH_SHA384)
+    int did_init = 0;
+#endif
 #ifdef WOLFBOOT_HASH_SHA256
     wc_Sha256 sha256_ctx;
     uint8_t sha256_digest[WC_SHA256_DIGEST_SIZE];
@@ -950,14 +963,20 @@ static int fit_verify_hash(const void *fdt, int img_off,
         }
         if (ret == 0) {
             ret = wc_InitSha256(&sha256_ctx);
+            if (ret == 0) {
+                did_init = 1;
+            }
         }
         if (ret == 0) {
             ret = wc_Sha256Update(&sha256_ctx, data, (word32)data_len);
         }
         if (ret == 0) {
             ret = wc_Sha256Final(&sha256_ctx, sha256_digest);
         }
-        wc_Sha256Free(&sha256_ctx);
+        if (did_init) {
+            wc_Sha256Free(&sha256_ctx);
+            did_init = 0;
+        }
         if (ret != 0) {
             wolfBoot_printf("FIT hash-1 (sha256): wc_Sha256 failed rc=%d\n",
                 ret);
@@ -982,14 +1001,20 @@ static int fit_verify_hash(const void *fdt, int img_off,
         }
         if (ret == 0) {
             ret = wc_InitSha384(&sha384_ctx);
+            if (ret == 0) {
+                did_init = 1;
+            }
         }
         if (ret == 0) {
             ret = wc_Sha384Update(&sha384_ctx, data, (word32)data_len);
         }
         if (ret == 0) {
             ret = wc_Sha384Final(&sha384_ctx, sha384_digest);
         }
-        wc_Sha384Free(&sha384_ctx);
+        if (did_init) {
+            wc_Sha384Free(&sha384_ctx);
+            did_init = 0;
+        }
         if (ret != 0) {
             wolfBoot_printf("FIT hash-1 (sha384): wc_Sha384 failed rc=%d\n",
                 ret);
@@ -1031,53 +1056,88 @@ void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
         data = (void*)fdt_getprop(fdt, off, "data", &len);
         load = fdt_getprop_address(fdt, off, "load");
         entry = fdt_getprop_address(fdt, off, "entry");
-        if (data != NULL && load != NULL && data != load) {
-            /* Detect compression unconditionally so we can warn (and fail
-             * closed) when the build lacks support for it instead of
-             * silently copying compressed bytes as if they were raw. */
+        if (data != NULL) {
+            int is_gzip = 0;
+            int is_unknown_comp = 0;
+            /* Detect compression unconditionally (independent of whether
+             * a valid distinct load destination is available) so we can
+             * fail closed when the build lacks support, when the scheme
+             * is unknown, or when there is no place to decompress to -
+             * instead of silently passing compressed bytes through as
+             * raw. */
             comp = (const char*)fdt_getprop(fdt, off, "compression",
                 &complen);
-            if (comp != NULL && complen > 0 && strcmp(comp, "gzip") == 0) {
-#ifdef WOLFBOOT_GZIP
-                uint32_t out_len = 0;
-                int rc;
-                wolfBoot_printf("Decompressing Image %s (gzip): "
-                    "%p -> %p (%d bytes)\n", image, data, load, len);
-                rc = wolfBoot_gunzip((const uint8_t*)data, (uint32_t)len,
-                    (uint8_t*)load, out_max, &out_len);
-                if (rc != 0) {
-                    wolfBoot_printf("FIT gunzip failed for %s: rc=%d "
-                        "(wrote %u bytes)\n", image, rc, out_len);
-                    return NULL;
+            if (comp != NULL && complen > 0) {
+                if (strcmp(comp, "gzip") == 0) {
+                    is_gzip = 1;
+                }
+                else if (strcmp(comp, "none") != 0) {
+                    is_unknown_comp = 1;
                 }
-                len = (int)out_len;
-                wolfBoot_printf("Decompressed %s: %u bytes\n", image,
-                    out_len);
+            }
+            if (load != NULL && data != load) {
+                if (is_gzip) {
+#ifdef WOLFBOOT_GZIP
+                    uint32_t out_len = 0;
+                    int rc;
+                    wolfBoot_printf("Decompressing Image %s (gzip): "
+                        "%p -> %p (%d bytes)\n", image, data, load, len);
+                    rc = wolfBoot_gunzip((const uint8_t*)data,
+                        (uint32_t)len, (uint8_t*)load, out_max, &out_len);
+                    if (rc != 0) {
+                        wolfBoot_printf("FIT gunzip failed for %s: rc=%d "
+                            "(wrote %u bytes)\n", image, rc, out_len);
+                        return NULL;
+                    }
+                    len = (int)out_len;
+                    wolfBoot_printf("Decompressed %s: %u bytes\n", image,
+                        out_len);
 #else
-                wolfBoot_printf("FIT: subimage '%s' has compression="
-                    "\"gzip\" but WOLFBOOT_GZIP is not enabled in this "
-                    "build (rebuild with GZIP=1)\n", image);
-                return NULL;
+                    wolfBoot_printf("FIT: subimage '%s' has compression="
+                        "\"gzip\" but WOLFBOOT_GZIP is not enabled in "
+                        "this build (rebuild with GZIP=1)\n", image);
+                    return NULL;
 #endif
-            }
-            else {
-                wolfBoot_printf("Loading Image %s: %p -> %p (%d bytes)\n",
-                    image, data, load, len);
-                memcpy(load, data, len);
-            }
+                }
+                else if (is_unknown_comp) {
+                    /* Unknown compression scheme; fail closed rather
+                     * than silently memcpy compressed bytes as raw. */
+                    wolfBoot_printf("FIT: subimage '%s' has unsupported "
+                        "compression=\"%s\"\n", image, comp);
+                    return NULL;
+                }
+                else {
+                    wolfBoot_printf("Loading Image %s: %p -> %p "
+                        "(%d bytes)\n", image, data, load, len);
+                    memcpy(load, data, len);
+                }
 
 #ifdef WOLFBOOT_GZIP
-            /* Defense-in-depth: verify FIT hash-1 against loaded bytes */
-            if (fit_verify_hash(fdt, off, (const uint8_t*)load,
-                    (uint32_t)len) != 0) {
-                wolfBoot_printf("FIT hash verification failed for %s\n",
-                    image);
-                return NULL;
-            }
+                /* Defense-in-depth: verify FIT hash-1 against loaded
+                 * bytes */
+                if (fit_verify_hash(fdt, off, (const uint8_t*)load,
+                        (uint32_t)len) != 0) {
+                    wolfBoot_printf("FIT hash verification failed for "
+                        "%s\n", image);
+                    return NULL;
+                }
 #endif
 
-            /* load should always have entry, but if not use load address */
-            data = (entry != NULL) ? entry : load;
+                /* load should always have entry, but if not use load
+                 * address */
+                data = (entry != NULL) ? entry : load;
+            }
+            else if (is_gzip || is_unknown_comp) {
+                /* Compression declared but no distinct destination to
+                 * decompress into. Refuse rather than hand the caller
+                 * a pointer to still-compressed bytes. */
+                wolfBoot_printf("FIT: subimage '%s' declares "
+                    "compression=\"%s\" but has no distinct load "
+                    "destination (load=%p, data=%p); refusing to pass "
+                    "compressed bytes through as raw\n",
+                    image, comp, load, data);
+                return NULL;
+            }
         }
         wolfBoot_printf("Image %s: %p (%d bytes)\n", image, data, len);
     }
@@ -1093,7 +1153,7 @@ void* fit_load_image_ex(void* fdt, const char* image, int* lenp,
 
 void* fit_load_image(void* fdt, const char* image, int* lenp)
 {
-    return fit_load_image_ex(fdt, image, lenp, 0xFFFFFFFFU);
+    return fit_load_image_ex(fdt, image, lenp, WOLFBOOT_FIT_MAX_DECOMP);
 }
 
 #endif /* (MMU || WOLFBOOT_FDT) && !BUILD_LOADER_STAGE1 */

src/gzip.c

@@ -206,10 +206,12 @@ static int gz_huff_build(gz_huff_t *h, const uint8_t *lengths, int n)
         left = 1;
         for (len = 1; (len <= GZIP_MAX_HUFF_BITS) && (ret == 0); len++) {
             left <<= 1;
-            left -= (int)h->counts[len];
-            if (left < 0) {
+            if ((int)h->counts[len] > left) {
                 ret = WOLFBOOT_GZIP_E_HUFFMAN;
             }
+            else {
+                left -= (int)h->counts[len];
+            }
         }
     }
 
@@ -428,6 +430,10 @@ static int gz_inflate_dynamic(gz_state_t *s)
     int i, total, idx, sym;
     uint8_t prev = 0;
 
+    for (i = 0; i < (int)(sizeof(code_lens) / sizeof(code_lens[0])); i++) {
+        code_lens[i] = 0;
+    }
+
     ret = gz_get_bits(s, GZIP_HLIT_BITS, &hlit);
     if (ret == 0) {
         hlit += GZIP_HLIT_BASE;

tools/config.mk

@@ -76,7 +76,6 @@ ifeq ($(ARCH),)
   WOLFBOOT_DTS_UPDATE_ADDRESS=0x50000
   WOLFBOOT_LOAD_ADDRESS?=0x200000
   WOLFBOOT_LOAD_DTS_ADDRESS?=0x400000
-  WOLFBOOT_LOAD_RAMDISK_ADDRESS?=0
   WOLFBOOT_SMALL_STACK?=0
   DELTA_UPDATES?=0
   DELTA_BLOCK_SIZE?=256
@@ -94,6 +93,11 @@ ifeq ($(ARCH),)
   WOLFHSM_CLIENT_LOCAL_KEYS=0
 endif
 
+# Global default: 0 means "use the FIT image's `load` property verbatim".
+# Defaulted globally (outside the CI ifeq block above) so RAMDISK=1 can
+# be toggled on any target without forcing an explicit address.
+WOLFBOOT_LOAD_RAMDISK_ADDRESS?=0
+
 CONFIG_VARS:= ARCH TARGET SIGN HASH MCUXSDK MCUXPRESSO MCUXPRESSO_CPU MCUXPRESSO_DRIVERS \
 	MCUXPRESSO_CMSIS FREEDOM_E_SDK STM32CUBE CYPRESS_PDL CYPRESS_CORE_LIB CYPRESS_TARGET_LIB DEBUG VTOR \
 	CORTEX_M0 CORTEX_M7 CORTEX_M33 NO_ASM EXT_FLASH SPI_FLASH NO_XIP UART_FLASH ALLOW_DOWNGRADE NVM_FLASH_WRITEONCE \

tools/unit-tests/unit-gzip.c

@@ -35,7 +35,7 @@
 #include "gzip.h"
 
 /* Pull in the implementation under test directly */
-#include "gzip.c"
+#include "../../src/gzip.c"
 
 /* ------------------------------------------------------------------------- */
 /* Helpers: gzip(1) on the host produces our test fixtures                   */
@@ -45,41 +45,47 @@ static uint8_t *gz_compress_buf(const uint8_t *in, size_t in_len, size_t *out_le
 {
     char in_path[64], out_path[64];
     char cmd[256];
-    FILE *f;
+    FILE *f = NULL;
     long n;
-    uint8_t *buf;
+    uint8_t *buf = NULL;
 
     snprintf(in_path,  sizeof(in_path),  "/tmp/wb-gz-in-%d.bin",  getpid());
     snprintf(out_path, sizeof(out_path), "/tmp/wb-gz-out-%d.gz",  getpid());
 
     f = fopen(in_path, "wb");
-    if (f == NULL) return NULL;
+    if (f == NULL) goto cleanup;
     if (in_len > 0) {
         if (fwrite(in, 1, in_len, f) != in_len) {
             fclose(f);
-            return NULL;
+            f = NULL;
+            goto cleanup;
         }
     }
     fclose(f);
+    f = NULL;
 
     snprintf(cmd, sizeof(cmd), "gzip -nc %s > %s", in_path, out_path);
-    if (system(cmd) != 0) return NULL;
+    if (system(cmd) != 0) goto cleanup;
 
     f = fopen(out_path, "rb");
-    if (f == NULL) return NULL;
-    fseek(f, 0, SEEK_END);
+    if (f == NULL) goto cleanup;
+    if (fseek(f, 0, SEEK_END) != 0) goto cleanup;
     n = ftell(f);
-    fseek(f, 0, SEEK_SET);
+    if (n < 0) goto cleanup;
+    if (fseek(f, 0, SEEK_SET) != 0) goto cleanup;
     buf = (uint8_t*)malloc((size_t)n);
-    if (buf == NULL) { fclose(f); return NULL; }
+    if (buf == NULL) goto cleanup;
     if (fread(buf, 1, (size_t)n, f) != (size_t)n) {
-        free(buf); fclose(f);
-        return NULL;
+        free(buf);
+        buf = NULL;
+        goto cleanup;
     }
-    fclose(f);
+    *out_len = (size_t)n;
+
+cleanup:
+    if (f != NULL) fclose(f);
     unlink(in_path);
     unlink(out_path);
-    *out_len = (size_t)n;
     return buf;
 }