image_parse_header: remove undefined behavior and other updates (#61)

boardloader: bootloader: remove undefined behavior in header parsing, code cleanup

embed/boardloader/main.c

@@ -10,8 +10,8 @@
 #include "lowlevel.h"
 #include "version.h"
 
-#define IMAGE_MAGIC   0x425A5254 // TRZB
+#define BOOTLOADER_IMAGE_MAGIC   0x425A5254 // TRZB
-#define IMAGE_MAXSIZE (1 * 64 * 1024 + 7 * 128 * 1024)
+#define BOOTLOADER_IMAGE_MAXSIZE (1 * 128 * 1024)
 
 static uint32_t check_sdcard(void)
 {
@@ -35,7 +35,7 @@ static uint32_t check_sdcard(void)
 
     image_header hdr;
 
-    if (image_parse_header((const uint8_t *)buf, IMAGE_MAGIC, IMAGE_MAXSIZE, &hdr)) {
+    if (image_parse_header((const uint8_t *)buf, BOOTLOADER_IMAGE_MAGIC, BOOTLOADER_IMAGE_MAXSIZE, &hdr)) {
         return hdr.codelen;
     } else {
         return 0;
@@ -176,7 +176,7 @@ int main(void)
     image_header hdr;
 
     ensure(
-        image_parse_header((const uint8_t *)BOOTLOADER_START, IMAGE_MAGIC, IMAGE_MAXSIZE, &hdr),
+        image_parse_header((const uint8_t *)BOOTLOADER_START, BOOTLOADER_IMAGE_MAGIC, BOOTLOADER_IMAGE_MAXSIZE, &hdr),
         "invalid bootloader header");
 
     ensure(

embed/bootloader/main.c

@@ -17,8 +17,8 @@
 #include "messages.h"
 #include "style.h"
 
-#define IMAGE_MAGIC   0x465A5254 // TRZF
+#define FIRMWARE_IMAGE_MAGIC   0x465A5254 // TRZF
-#define IMAGE_MAXSIZE (7 * 128 * 1024)
+#define FIRMWARE_IMAGE_MAXSIZE (6 * 128 * 1024)
 
 void display_fade(int start, int end, int delay)
 {
@@ -266,15 +266,15 @@ int main(void)
         hal_delay(1);
     }
 
+    vendor_header vhdr;
+
     // start the bootloader if user touched the screen or no firmware installed
-    if (touched || !vendor_parse_header((const uint8_t *)FIRMWARE_START, NULL)) {
+    if (touched || !vendor_parse_header((const uint8_t *)FIRMWARE_START, &vhdr)) {
         if (!bootloader_loop()) {
             shutdown();
         }
     }
 
-    vendor_header vhdr;
-
     ensure(
         vendor_parse_header((const uint8_t *)FIRMWARE_START, &vhdr),
         "invalid vendor header");
@@ -286,7 +286,7 @@ int main(void)
     image_header hdr;
 
     ensure(
-        image_parse_header((const uint8_t *)(FIRMWARE_START + vhdr.hdrlen), IMAGE_MAGIC, IMAGE_MAXSIZE, &hdr),
+        image_parse_header((const uint8_t *)(FIRMWARE_START + vhdr.hdrlen), FIRMWARE_IMAGE_MAGIC, FIRMWARE_IMAGE_MAXSIZE, &hdr),
         "invalid firmware header");
 
     ensure(

embed/bootloader/messages.c

@@ -212,7 +212,8 @@ void process_msg_Initialize(uint8_t iface_num, uint32_t msg_size, uint8_t *buf)
     MSG_SEND_ASSIGN_VALUE(minor_version, VERSION_MINOR);
     MSG_SEND_ASSIGN_VALUE(patch_version, VERSION_PATCH);
     MSG_SEND_ASSIGN_VALUE(bootloader_mode, true);
-    bool firmware_present = vendor_parse_header((const uint8_t *)FIRMWARE_START, NULL);
+    vendor_header vhdr;
+    bool firmware_present = vendor_parse_header((const uint8_t *)FIRMWARE_START, &vhdr);
     MSG_SEND_ASSIGN_VALUE(firmware_present, firmware_present);
     MSG_SEND(Features);
 }

embed/trezorhal/image.c

@@ -29,13 +29,8 @@ static bool compute_pubkey(uint8_t sig_m, uint8_t sig_n, const uint8_t * const *
     return 0 == ed25519_cosi_combine_publickeys(res, keys, sig_m);
 }
 
-bool image_parse_header(const uint8_t *data, uint32_t magic, uint32_t maxsize, image_header *hdr)
+bool image_parse_header(const uint8_t * const data, const uint32_t magic, const uint32_t maxsize, image_header * const hdr)
 {
-    if (!hdr) {
-        image_header h;
-        hdr = &h;
-    }
-
     memcpy(&hdr->magic, data, 4);
     if (hdr->magic != magic) return false;
 
@@ -43,11 +38,13 @@ bool image_parse_header(const uint8_t *data, uint32_t magic, uint32_t maxsize, i
     if (hdr->hdrlen != HEADER_SIZE) return false;
 
     memcpy(&hdr->expiry, data + 8, 4);
+    // TODO: expiry mechanism needs to be ironed out before production or those
+    // devices won't accept expiring bootloaders (due to boardloader write protection).
     if (hdr->expiry != 0) return false;
 
     memcpy(&hdr->codelen, data + 12, 4);
-    if (hdr->hdrlen + hdr->codelen < 4 * 1024) return false;
+    if (hdr->codelen > (maxsize - hdr->hdrlen)) return false;
-    if (hdr->hdrlen + hdr->codelen > maxsize) return false;
+    if ((hdr->hdrlen + hdr->codelen) < 4 * 1024) return false;
     if ((hdr->hdrlen + hdr->codelen) % 512 != 0) return false;
 
     memcpy(&hdr->version, data + 16, 4);
@@ -79,17 +76,13 @@ bool image_check_signature(const uint8_t *data, const image_header *hdr, uint8_t
     return 0 == ed25519_sign_open(hash, BLAKE2S_DIGEST_LENGTH, pub, *(const ed25519_signature *)hdr->sig);
 }
 
-bool vendor_parse_header(const uint8_t *data, vendor_header *vhdr)
+bool vendor_parse_header(const uint8_t * const data, vendor_header * const vhdr)
 {
-    if (!vhdr) {
-        vendor_header h;
-        vhdr = &h;
-    }
-
     memcpy(&vhdr->magic, data, 4);
     if (vhdr->magic != 0x565A5254) return false; // TRZV
 
     memcpy(&vhdr->hdrlen, data + 4, 4);
+    // TODO: sanity check hdr->hdrlen as it is used as a src to memcpy below
 
     memcpy(&vhdr->expiry, data + 8, 4);
     if (vhdr->expiry != 0) return false;

embed/trezorhal/image.h

@@ -34,11 +34,11 @@ typedef struct {
     uint8_t sig[64];
 } vendor_header;
 
-bool image_parse_header(const uint8_t *data, uint32_t magic, uint32_t maxsize, image_header *hdr);
+bool image_parse_header(const uint8_t * const data, const uint32_t magic, const uint32_t maxsize, image_header * const hdr);
 
 bool image_check_signature(const uint8_t *data, const image_header *hdr, uint8_t key_m, uint8_t key_n, const uint8_t * const *keys);
 
-bool vendor_parse_header(const uint8_t *data, vendor_header *vhdr);
+bool vendor_parse_header(const uint8_t * const data, vendor_header * const vhdr);
 
 bool vendor_check_signature(const uint8_t *data, const vendor_header *vhdr, uint8_t key_m, uint8_t key_n, const uint8_t * const *keys);