Addressed reviewers comments

Author: danielinux

src/image.c

@@ -1308,9 +1308,9 @@ int wolfBoot_open_image_address(struct wolfBoot_image *img, uint8_t *image)
 
 #ifdef WOLFBOOT_FIXED_PARTITIONS
     if (img->fw_size > (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE)) {
-        wolfBoot_printf("Image size %d > max %d\n",
+        wolfBoot_printf("Image size %u > max %u\n",
             (unsigned int)img->fw_size,
-            (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE));
+            (unsigned int)(WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE));
         img->fw_size = WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE;
         return -1;
     }
@@ -1321,7 +1321,7 @@ int wolfBoot_open_image_address(struct wolfBoot_image *img, uint8_t *image)
 #else
 #ifdef WOLFBOOT_RAMBOOT_MAX_SIZE
     if (img->fw_size > WOLFBOOT_RAMBOOT_MAX_SIZE) {
-        wolfBoot_printf("Image size %d > max %d\n",
+        wolfBoot_printf("Image size %u > max %u\n",
             (unsigned int)img->fw_size,
             (unsigned int)WOLFBOOT_RAMBOOT_MAX_SIZE);
         return -1;

src/update_disk.c

@@ -121,7 +121,10 @@ static uint32_t get_decrypted_blob_version(uint8_t *hdr)
         return 0;
 
     /* Search for version TLV */
-    while (p + 4 < max_p) {
+    while ((size_t)(max_p - p) >= 4U) {
+        size_t remaining = (size_t)(max_p - p);
+        size_t tlv_total;
+
         tlv_type = *((uint16_t*)p);
         tlv_len = *((uint16_t*)(p + 2));
 
@@ -134,15 +137,16 @@ static uint32_t get_decrypted_blob_version(uint8_t *hdr)
             continue;
         }
 
-        if (p + 4 + tlv_len > max_p)
+        tlv_total = 4U + (size_t)tlv_len;
+        if (remaining < tlv_total)
             break;
 
         if (tlv_type == HDR_VERSION && tlv_len == 4) {
             uint32_t ver = *((uint32_t*)(p + 4));
             return ver;
         }
 
-        p += 4 + tlv_len;
+        p += tlv_total;
     }
     return 0;
 }

tools/unit-tests/unit-tpm-blob.c

@@ -39,6 +39,7 @@ static int oversized_pub_read_attempted;
 static int oversized_priv_read_attempted;
 static int forcezero_calls;
 static word32 last_forcezero_len;
+static word32 last_pub_read_request_sz;
 static uint8_t test_hdr[64];
 static uint8_t test_modulus[256];
 static uint8_t test_exponent_der[] = { 0xAA, 0x01, 0x00, 0x01, 0x7B };
@@ -369,22 +370,31 @@ int wolfTPM2_NVReadAuth(WOLFTPM2_DEV* dev, WOLFTPM2_NV* nv,
     switch (nvread_calls) {
         case 1:
             if (current_mode == MOCK_OVERSIZE_PUB) {
-                *(uint16_t*)dataBuf = (uint16_t)(sizeof(TPM2B_PUBLIC) + 1);
+                uint16_t value = (uint16_t)(sizeof(TPM2B_PUBLIC) + 1);
+                memcpy(dataBuf, &value, sizeof(value));
             }
             else {
-                *(uint16_t*)dataBuf = 4;
+                uint16_t value = 4;
+                memcpy(dataBuf, &value, sizeof(value));
             }
             *pDataSz = sizeof(uint16_t);
             return 0;
         case 2:
             if (current_mode == MOCK_OVERSIZE_PUB) {
-                oversized_pub_read_attempted = 1;
+                last_pub_read_request_sz = *pDataSz;
+                if (*pDataSz > sizeof(TPM2B_PUBLIC)) {
+                    oversized_pub_read_attempted = 1;
+                }
                 return -100;
             }
             memset(dataBuf, 0, *pDataSz);
             return 0;
         case 3:
-            *(uint16_t*)dataBuf = (uint16_t)(sizeof(((WOLFTPM2_KEYBLOB*)0)->priv.buffer) + 1);
+        {
+            uint16_t value =
+                (uint16_t)(sizeof(((WOLFTPM2_KEYBLOB*)0)->priv.buffer) + 1);
+            memcpy(dataBuf, &value, sizeof(value));
+        }
             *pDataSz = sizeof(uint16_t);
             return 0;
         case 4:
@@ -410,6 +420,7 @@ static void setup(void)
     oversized_priv_read_attempted = 0;
     forcezero_calls = 0;
     last_forcezero_len = 0;
+    last_pub_read_request_sz = 0;
     memset(test_hdr, 0x22, sizeof(test_hdr));
     memset(test_modulus, 0x33, sizeof(test_modulus));
 }
@@ -426,6 +437,7 @@ START_TEST(test_wolfBoot_read_blob_rejects_oversized_public_area)
 
     ck_assert_int_eq(rc, BUFFER_E);
     ck_assert_int_eq(nvread_calls, 1);
+    ck_assert_uint_eq(last_pub_read_request_sz, 0);
     ck_assert_int_eq(oversized_pub_read_attempted, 0);
 }
 END_TEST
@@ -527,6 +539,8 @@ START_TEST(test_wolfBoot_unseal_blob_rejects_output_larger_than_capacity)
 
     ck_assert_int_eq(rc, BUFFER_E);
     ck_assert_int_eq(secret_sz, 0);
+    ck_assert_int_eq(forcezero_calls, 1);
+    ck_assert_uint_eq(last_forcezero_len, sizeof(Unseal_Out));
     for (i = 0; i < (int)sizeof(output.canary); i++) {
         ck_assert_uint_eq(output.canary[i], 0xA5);
     }

tools/unit-tests/unit-update-disk.c

@@ -132,24 +132,29 @@ void disk_close(int drv)
 int disk_part_read(int drv, int part, uint64_t off, uint64_t sz, uint8_t *buf)
 {
     uint8_t *image;
+    uint64_t max = IMAGE_HEADER_SIZE + TEST_PAYLOAD_SIZE;
 
     (void)drv;
     image = (part == BOOT_PART_B) ? part_b_image : part_a_image;
-    if (off + sz > IMAGE_HEADER_SIZE + TEST_PAYLOAD_SIZE)
+    if ((off > max) || (sz > (max - off)))
         return -1;
     memcpy(buf, image + off, (size_t)sz);
     return (int)sz;
 }
 
 int wolfBoot_open_image_address(struct wolfBoot_image* img, uint8_t* image)
 {
-    uint32_t magic = *(uint32_t *)image;
+    uint32_t magic;
+    uint32_t fw_size;
+
+    memcpy(&magic, image, sizeof(magic));
 
     if (magic != WOLFBOOT_MAGIC)
         return -1;
     memset(img, 0, sizeof(*img));
     img->hdr = image;
-    img->fw_size = *(uint32_t *)(image + sizeof(uint32_t));
+    memcpy(&fw_size, image + sizeof(uint32_t), sizeof(fw_size));
+    img->fw_size = fw_size;
     img->fw_base = image + IMAGE_HEADER_SIZE;
     img->hdr_ok = 1;
     return 0;