Refactor LoadFirmware() to avoid global variables, which don't work when running out of ROM

Review URL: http://codereview.chromium.org/2848006

11 files changed, 156 insertions, 156 deletions

diff --git a/host/include/host_key.h b/host/include/host_key.h
index 6d964a0f..efdf79db 100644
--- a/host/include/host_key.h
+++ b/host/include/host_key.h
@@ -33,21 +33,11 @@ VbPrivateKey* PrivateKeyRead(const char* filename, uint64_t algorithm);
 void PrivateKeyFree(VbPrivateKey* key);
 
 
-/* Initialize a public key to refer to [key_data]. */
-void PublicKeyInit(VbPublicKey* key, uint8_t* key_data, uint64_t key_size);
-
-
 /* Allocate a new public key with space for a [key_size] byte key. */
 VbPublicKey* PublicKeyAlloc(uint64_t key_size, uint64_t algorithm,
                             uint64_t version);
 
 
-/* Copy a public key from [src] to [dest].
- *
- * Returns 0 if success, non-zero if error. */
-int PublicKeyCopy(VbPublicKey* dest, const VbPublicKey* src);
-
-
 /* Read a public key from a .vbpubk file.  Caller owns the returned
  * pointer, and must free it with Free().
  *
diff --git a/host/lib/host_key.c b/host/lib/host_key.c
index c5f49d31..da350fd4 100644
--- a/host/lib/host_key.c
+++ b/host/lib/host_key.c
@@ -72,14 +72,6 @@ void PrivateKeyFree(VbPrivateKey* key) {
 }
 
 
-void PublicKeyInit(VbPublicKey* key, uint8_t* key_data, uint64_t key_size) {
-  key->key_offset = OffsetOf(key, key_data);
-  key->key_size = key_size;
-  key->algorithm = kNumAlgorithms; /* Key not present yet */
-  key->key_version = 0;
-}
-
-
 /* Allocate a new public key with space for a [key_size] byte key. */
 VbPublicKey* PublicKeyAlloc(uint64_t key_size, uint64_t algorithm,
                             uint64_t version) {
@@ -95,21 +87,6 @@ VbPublicKey* PublicKeyAlloc(uint64_t key_size, uint64_t algorithm,
 }
 
 
-/* Copy a public key from [src] to [dest].
- *
- * Returns zero if success, non-zero if error. */
-int PublicKeyCopy(VbPublicKey* dest, const VbPublicKey* src) {
-  if (dest->key_size < src->key_size)
-    return 1;
-
-  dest->key_size = src->key_size;
-  dest->algorithm = src->algorithm;
-  dest->key_version = src->key_version;
-  Memcpy(GetPublicKeyData(dest), GetPublicKeyDataC(src), src->key_size);
-  return 0;
-}
-
-
 VbPublicKey* PublicKeyReadKeyb(const char* filename, uint64_t algorithm,
                                uint64_t version) {
   VbPublicKey* key;
diff --git a/host/linktest/main.c b/host/linktest/main.c
index b8db1211..5fb7dd3d 100644
--- a/host/linktest/main.c
+++ b/host/linktest/main.c
@@ -7,9 +7,7 @@ int main(void)
   /* host_key.h */
   PrivateKeyRead(0, 0);
   PrivateKeyFree(0);
-  PublicKeyInit(0, 0, 0);
   PublicKeyAlloc(0, 0, 0);
-  PublicKeyCopy(0, 0);
   PublicKeyRead(0);
   PublicKeyReadKeyb(0, 0, 0);
   PublicKeyWrite(0, 0);
diff --git a/vboot_firmware/include/load_firmware_fw.h b/vboot_firmware/include/load_firmware_fw.h
index 312a0aaa..af971161 100644
--- a/vboot_firmware/include/load_firmware_fw.h
+++ b/vboot_firmware/include/load_firmware_fw.h
@@ -11,36 +11,14 @@
 
 #include <stdint.h>
 
-/* Functions provided by PEI to LoadFirmware() */
-
-/* Get the firmware body data for [firmware_index], which is either
- * 0 (the first firmware image) or 1 (the second firmware image).
- *
- * This function must call UpdateFirmwareBodyHash() before returning,
- * to update the secure hash for the firmware image.  For best
- * performance, the reader should call this function periodically
- * during the read, so that updating the hash can be pipelined with
- * the read.  If the reader cannot update the hash during the read
- * process, it should call UpdateFirmwareBodyHash() on the entire
- * firmeware data after the read, before returning.
- *
- * On success, returns a pointer to the data and stores the data size
- * in [*size].  On error, returns NULL. */
-void *GetFirmwareBody(uint64_t firmware_index, uint64_t* size);
-
-
-/* Interface provided by verified boot library to PEI */
+/* Maximum size of kernel_sign_key_blob in bytes, for implementations
+ * which must preallocate a transfer buffer between boot phases */
+#define LOAD_FIRMWARE_KEY_BLOB_MAX 2104
 
 /* Return codes for LoadFirmware() */
 #define LOAD_FIRMWARE_SUCCESS 0   /* Success */
 #define LOAD_FIRMWARE_RECOVERY 1  /* Reboot to recovery mode */
 
-/* Update the data hash for the current firmware image, extending it
- * by [size] bytes stored in [*data].  This function must only be
- * called inside GetFirmwareBody(). */
-void UpdateFirmwareBodyHash(uint8_t* data, uint64_t size);
-
-
 typedef struct LoadFirmwareParams {
   /* Inputs to LoadFirmware() */
   void *firmware_root_key_blob;  /* Key used to sign firmware header */
@@ -48,24 +26,60 @@ typedef struct LoadFirmwareParams {
   void *verification_block_1;    /* Key block + preamble for firmware 1 */
   uint64_t verification_size_0;  /* Verification block 0 size in bytes */
   uint64_t verification_size_1;  /* Verification block 1 size in bytes */
+  void *kernel_sign_key_blob;    /* Destination buffer for key to use
+                                  * when loading kernel.  Pass this
+                                  * data to LoadKernel() in
+                                  * LoadKernelParams.header_sign_key_blob. */
+  uint64_t kernel_sign_key_size; /* Size of kernel signing key blob
+                                  * buffer, in bytes.  On output, this
+                                  * will contain the actual key blob
+                                  * size placed into the buffer. */
 
   /* Outputs from LoadFirmware(); valid only if LoadFirmware() returns
    * LOAD_FIRMWARE_SUCCESS. */
   uint64_t firmware_index;       /* Firmware index to run. */
-  void *kernel_sign_key_blob;    /* Key to use when loading kernel.
-                                  * Pass this data to LoadKernel() in
-                                  * LoadKernelParams.header_sign_key_blob.
-                                  * Key data may be copied/relocated
-                                  * if necessary. */
-  uint64_t kernel_sign_key_size;  /* Size of kernel signing key blob,
-                                   * in bytes. */
+
+  /* Internal data for LoadFirmware() / UpdateFirmwareBodyHash(). */
+  void* load_firmware_internal;
+
+  /* Internal data for caller / GetFirmwareBody(). */
+  void* caller_internal;
+
 } LoadFirmwareParams;
 
 
+/* Functions provided by PEI to LoadFirmware() */
+
+/* Get the firmware body data for [firmware_index], which is either
+ * 0 (the first firmware image) or 1 (the second firmware image).
+ *
+ * This function must call UpdateFirmwareBodyHash() before returning,
+ * to update the secure hash for the firmware image.  For best
+ * performance, the reader should call this function periodically
+ * during the read, so that updating the hash can be pipelined with
+ * the read.  If the reader cannot update the hash during the read
+ * process, it should call UpdateFirmwareBodyHash() on the entire
+ * firmeware data after the read, before returning.
+ *
+ * Returns 0 if successful or non-zero if error. */
+int GetFirmwareBody(LoadFirmwareParams* params, uint64_t firmware_index);
+
+
+/* Functions provided by verified boot library to PEI */
+
 /* Attempts to load the rewritable firmware.
  *
  * Returns LOAD_FIRMWARE_SUCCESS if successful, error code on failure. */
 int LoadFirmware(LoadFirmwareParams* params);
 
 
+/* Update the data hash for the current firmware image, extending it
+ * by [size] bytes stored in [*data].  This function must only be
+ * called inside GetFirmwareBody(). */
+void UpdateFirmwareBodyHash(LoadFirmwareParams* params,
+                            uint8_t* data, uint64_t size);
+
+
+
+
 #endif  /* VBOOT_REFERENCE_LOAD_FIRMWARE_FW_H_ */
diff --git a/vboot_firmware/lib/include/vboot_common.h b/vboot_firmware/lib/include/vboot_common.h
index 1d811985..3c0bfd2a 100644
--- a/vboot_firmware/lib/include/vboot_common.h
+++ b/vboot_firmware/lib/include/vboot_common.h
@@ -55,6 +55,16 @@ int VerifySignatureInside(const void* parent, uint64_t parent_size,
                           const VbSignature* sig);
 
 
+/* Initialize a public key to refer to [key_data]. */
+void PublicKeyInit(VbPublicKey* key, uint8_t* key_data, uint64_t key_size);
+
+
+/* Copy a public key from [src] to [dest].
+ *
+ * Returns 0 if success, non-zero if error. */
+int PublicKeyCopy(VbPublicKey* dest, const VbPublicKey* src);
+
+
 /* Converts a public key to RsaPublicKey format.  The returned key must
  * be freed using RSAPublicKeyFree().
  *
diff --git a/vboot_firmware/lib/include/vboot_firmware.h b/vboot_firmware/lib/include/vboot_firmware.h
index de9a8133..ac8595f0 100644
--- a/vboot_firmware/lib/include/vboot_firmware.h
+++ b/vboot_firmware/lib/include/vboot_firmware.h
@@ -17,7 +17,6 @@
 #include "vboot_common.h"
 
 /* Alternate LoadFirmware() implementation; see load_firmware_fw.h */
-void UpdateFirmwareBodyHash2(uint8_t* data, uint64_t size);
 int LoadFirmware2(LoadFirmwareParams* params);
 
 #endif  /* VBOOT_REFERENCE_VBOOT_FIRMWARE_H_ */
diff --git a/vboot_firmware/lib/load_firmware_fw.c b/vboot_firmware/lib/load_firmware_fw.c
index 598c894f..0b0bf5e9 100644
--- a/vboot_firmware/lib/load_firmware_fw.c
+++ b/vboot_firmware/lib/load_firmware_fw.c
@@ -12,48 +12,15 @@
 #include "utility.h"
 
 
-static const char kFakeKernelBlob[] = "Fake kernel sign key blob";
-
-void UpdateFirmwareBodyHash(uint8_t* data, uint64_t size) {
-  /* TODO: actually update the hash. */
-}
+static const char kFakeKernelBlob[2088] = "Fake kernel sign key blob";
 
 
 int LoadFirmware(LoadFirmwareParams* params) {
-  /* TODO: real implementation!  For now, this just chains to the old
-   * implementation. */
-
-  uint8_t* fw0;
-  uint8_t* fw1;
-  uint64_t len;
-  int rv;
-
-  /* Get the firmware data */
-  fw0 = GetFirmwareBody(0, &len);
-  fw1 = GetFirmwareBody(1, &len);
-
-  /* Call the old firmware image driver */
-  rv = VerifyFirmwareDriver_f(params->firmware_root_key_blob,
-                              params->verification_block_0,
-                              fw0,
-                              params->verification_block_1,
-                              fw1);
-
-  /* Pass back a dummy key blob, since we can't extract the real
-   * kernel sign key blob yet */
-  params->kernel_sign_key_blob = (void*)kFakeKernelBlob;
+  /* TODO: real implementation!  This is now sufficiently broken due
+   * to refactoring that we'll just trust firmware A. */
+  Memcpy(params->kernel_sign_key_blob, kFakeKernelBlob,
+         sizeof(kFakeKernelBlob));
   params->kernel_sign_key_size = sizeof(kFakeKernelBlob);
-
-  switch(rv) {
-    case BOOT_FIRMWARE_A_CONTINUE:
-      params->firmware_index = 0;
-      return LOAD_FIRMWARE_SUCCESS;
-    case BOOT_FIRMWARE_B_CONTINUE:
-      params->firmware_index = 1;
-      return LOAD_FIRMWARE_SUCCESS;
-  }
-
-  /* If we're still here, we failed */
-  return LOAD_FIRMWARE_RECOVERY;
-
+  params->firmware_index = 0;
+  return LOAD_FIRMWARE_SUCCESS;
 }
diff --git a/vboot_firmware/lib/vboot_common.c b/vboot_firmware/lib/vboot_common.c
index 1256b676..f76eed45 100644
--- a/vboot_firmware/lib/vboot_common.c
+++ b/vboot_firmware/lib/vboot_common.c
@@ -85,6 +85,26 @@ int VerifySignatureInside(const void* parent, uint64_t parent_size,
 }
 
 
+void PublicKeyInit(VbPublicKey* key, uint8_t* key_data, uint64_t key_size) {
+  key->key_offset = OffsetOf(key, key_data);
+  key->key_size = key_size;
+  key->algorithm = kNumAlgorithms; /* Key not present yet */
+  key->key_version = 0;
+}
+
+
+int PublicKeyCopy(VbPublicKey* dest, const VbPublicKey* src) {
+  if (dest->key_size < src->key_size)
+    return 1;
+
+  dest->key_size = src->key_size;
+  dest->algorithm = src->algorithm;
+  dest->key_version = src->key_version;
+  Memcpy(GetPublicKeyData(dest), GetPublicKeyDataC(src), src->key_size);
+  return 0;
+}
+
+
 RSAPublicKey* PublicKeyToRSA(const VbPublicKey* key) {
   RSAPublicKey *rsa;
 
diff --git a/vboot_firmware/lib/vboot_firmware.c b/vboot_firmware/lib/vboot_firmware.c
index 8ff673c2..8c5f0f1d 100644
--- a/vboot_firmware/lib/vboot_firmware.c
+++ b/vboot_firmware/lib/vboot_firmware.c
@@ -17,25 +17,26 @@
  * optimal to have static variables in a library, but in UEFI the
  * caller is deep inside a different firmware stack and doesn't have a
  * good way to pass the params struct back to us. */
-static DigestContext ctx;
-static uint64_t body_size_accum = 0;
-static int inside_load_firmware = 0;
+typedef struct VbLoadFirmwareInternal {
+  DigestContext body_digest_context;
+  uint64_t body_size_accum;
+} VbLoadFirmwareInternal;
 
-void UpdateFirmwareBodyHash2(uint8_t* data, uint64_t size) {
 
-  if (!inside_load_firmware) {
-    debug("UpdateFirmwareBodyHash() called outside LoadFirmware()\n");
-    return;
-  }
+void UpdateFirmwareBodyHash(LoadFirmwareParams* params,
+                             uint8_t* data, uint64_t size) {
+  VbLoadFirmwareInternal* lfi =
+      (VbLoadFirmwareInternal*)params->load_firmware_internal;
 
-  DigestUpdate(&ctx, data, size);
-  body_size_accum += size;
+  DigestUpdate(&lfi->body_digest_context, data, size);
+  lfi->body_size_accum += size;
 }
 
 
 int LoadFirmware2(LoadFirmwareParams* params) {
 
   VbPublicKey* root_key = (VbPublicKey*)params->firmware_root_key_blob;
+  VbLoadFirmwareInternal* lfi;
 
   uint16_t tpm_key_version = 0;
   uint16_t tpm_fw_version = 0;
@@ -61,6 +62,12 @@ int LoadFirmware2(LoadFirmwareParams* params) {
                              &tpm_key_version, &tpm_fw_version))
     return LOAD_FIRMWARE_RECOVERY;
 
+  /* Allocate our internal data */
+  lfi = (VbLoadFirmwareInternal*)Malloc(sizeof(VbLoadFirmwareInternal));
+  if (!lfi)
+    return LOAD_FIRMWARE_RECOVERY;
+  params->load_firmware_internal = lfi;
+
   /* Loop over indices */
   for (index = 0; index < 2; index++) {
     VbKeyBlockHeader* key_block;
@@ -68,8 +75,6 @@ int LoadFirmware2(LoadFirmwareParams* params) {
     VbFirmwarePreambleHeader* preamble;
     RSAPublicKey* data_key;
     uint64_t key_version;
-    uint8_t* body_data;
-    uint64_t body_size;
     uint8_t* body_digest;
 
     /* Verify the key block */
@@ -127,20 +132,16 @@ int LoadFirmware2(LoadFirmwareParams* params) {
       continue;
 
     /* Read the firmware data */
-    DigestInit(&ctx, data_key->algorithm);
-    body_size_accum = 0;
-    inside_load_firmware = 1;
-    body_data = GetFirmwareBody(index, &body_size);
-    inside_load_firmware = 0;
-    body_digest = DigestFinal(&ctx);
-    if (!body_data || (body_size != preamble->body_signature.data_size) ||
-        (body_size_accum != body_size)) {
+    DigestInit(&lfi->body_digest_context, data_key->algorithm);
+    lfi->body_size_accum = 0;
+    if ((0 != GetFirmwareBody(params, index)) ||
+        (lfi->body_size_accum != preamble->body_signature.data_size)) {
       RSAPublicKeyFree(data_key);
-      Free(body_digest);
       continue;
     }
 
     /* Verify firmware data */
+    body_digest = DigestFinal(&lfi->body_digest_context);
     if (0 != VerifyDigest(body_digest, &preamble->body_signature, data_key)) {
       RSAPublicKeyFree(data_key);
       Free(body_digest);
@@ -152,13 +153,24 @@ int LoadFirmware2(LoadFirmwareParams* params) {
     Free(body_digest);
 
     /* If we're still here, the firmware is valid. */
-    /* Save the first good firmware we find; that's the one we'll boot */
     if (-1 == good_index) {
+      VbPublicKey *kdest = (VbPublicKey*)params->kernel_sign_key_blob;
+
+      /* Copy the kernel sign key blob into the destination buffer */
+      PublicKeyInit(kdest, (uint8_t*)(kdest + 1),
+                    (params->kernel_sign_key_size - sizeof(VbPublicKey)));
+
+      if (0 != PublicKeyCopy(kdest, &preamble->kernel_subkey))
+        continue;  /* The firmware signature was good, but the public
+                    * key was bigger that the caller can handle. */
+
+      /* Save the key size we actually used */
+      params->kernel_sign_key_size = kdest->key_offset + kdest->key_size;
+
+      /* Save the good index, now that we're sure we can actually use
+       * this firmware.  That's the one we'll boot. */
       good_index = index;
       params->firmware_index = index;
-      params->kernel_sign_key_blob = &preamble->kernel_subkey;
-      params->kernel_sign_key_size = (preamble->kernel_subkey.key_offset +
-                                      preamble->kernel_subkey.key_size);
 
       /* If the good firmware's key version is the same as the tpm,
        * then the TPM doesn't need updating; we can stop now.
@@ -170,6 +182,10 @@ int LoadFirmware2(LoadFirmwareParams* params) {
     }
   }
 
+  /* Free internal data */
+  Free(lfi);
+  params->load_firmware_internal = NULL;
+
   /* Handle finding good firmware */
   if (good_index >= 0) {
 
diff --git a/vboot_firmware/linktest/main.c b/vboot_firmware/linktest/main.c
index 169b6578..10500809 100644
--- a/vboot_firmware/linktest/main.c
+++ b/vboot_firmware/linktest/main.c
@@ -37,7 +37,6 @@ int main(void)
   GetLogicalKernelVersion(0);
 
   /* load_firmware_fw.h */
-  UpdateFirmwareBodyHash(0, 0);
   LoadFirmware(0);
 
   /* load_kernel_fw.h */
@@ -77,6 +76,8 @@ int main(void)
   VerifyMemberInside(0, 0, 0, 0, 0, 0);
   VerifyPublicKeyInside(0, 0, 0);
   VerifySignatureInside(0, 0, 0);
+  PublicKeyInit(0, 0, 0);
+  PublicKeyCopy(0, 0);
   PublicKeyToRSA(0);
   VerifyData(0, 0, 0);
   VerifyDigest(0, 0, 0);
@@ -88,7 +89,7 @@ int main(void)
   LoadKernel2(0);
 
   /* vboot_firmware.h */
-  UpdateFirmwareBodyHash2(0, 0);
+  UpdateFirmwareBodyHash(0, 0, 0);
   LoadFirmware2(0);
 
   return 0;
diff --git a/vboot_firmware/stub/load_firmware_stub.c b/vboot_firmware/stub/load_firmware_stub.c
index dea7f304..5bca57da 100644
--- a/vboot_firmware/stub/load_firmware_stub.c
+++ b/vboot_firmware/stub/load_firmware_stub.c
@@ -12,42 +12,43 @@
 #include "firmware_image_fw.h"
 #include "utility.h"
 
+typedef struct CallerInternal {
+  uint8_t *firmwareA;
+  uint64_t firmwareA_size;
+  uint8_t *firmwareB;
+  uint64_t firmwareB_size;
+} CallerInternal;
 
-static uint8_t *g_firmwareA;
-static uint64_t g_firmwareA_size;
-static uint8_t *g_firmwareB;
-static uint64_t g_firmwareB_size;
-
-
-void *GetFirmwareBody(uint64_t firmware_index, uint64_t* size) {
+int GetFirmwareBody(LoadFirmwareParams* params, uint64_t index) {
 
+  CallerInternal* ci = (CallerInternal*)params->caller_internal;
   uint8_t *fw;
+  uint64_t size;
 
   /* In a real implementation, GetFirmwareBody() should be what reads
    * and decompresses the firmware volume.  In this temporary hack, we
    * just pass the pointer which we got from
    * VerifyFirmwareDriver_Stub(). */
-  switch(firmware_index) {
+  switch(index) {
     case 0:
-      *size = g_firmwareA_size;
-      fw = g_firmwareA;
+      size = ci->firmwareA_size;
+      fw = ci->firmwareA;
     case 1:
-      *size = g_firmwareB_size;
-      fw = g_firmwareB;
+      size = ci->firmwareB_size;
+      fw = ci->firmwareB;
     default:
       /* Anything else is invalid */
-      *size = 0;
-      return NULL;
+      return 1;
   }
 
   /* Need to call UpdateFirmwareBodyHash() with the firmware volume
    * data.  In this temporary hack, the FV is already decompressed, so
    * we pass in the entire volume at once.  In a real implementation,
    * you should call this as the FV is being decompressed. */
-  UpdateFirmwareBodyHash(fw, *size);
+  UpdateFirmwareBodyHash(params, fw, size);
 
-  /* Return the firmware body pointer */
-  return fw;
+  /* Success */
+  return 0;
 }
 
 
@@ -64,20 +65,27 @@ int VerifyFirmwareDriver_stub(uint8_t* root_key_blob,
 
   int rv;
 
+  CallerInternal ci;
+
   /* Copy the firmware volume pointers to our global variables. */
-  g_firmwareA = firmwareA;
-  g_firmwareB = firmwareB;
+  ci.firmwareA = firmwareA;
+  ci.firmwareB = firmwareB;
 
   /* TODO: YOU NEED TO PASS IN THE FIRMWARE VOLUME SIZES SOMEHOW */
-  g_firmwareA_size = 0;
-  g_firmwareB_size = 0;
+  ci.firmwareA_size = 0;
+  ci.firmwareB_size = 0;
 
   /* Set up the params for LoadFirmware() */
   LoadFirmwareParams p;
+  p.caller_internal = &ci;
   p.firmware_root_key_blob = root_key_blob;
   p.verification_block_0 = verification_headerA;
   p.verification_block_1 = verification_headerB;
 
+  /* Allocate a key blob buffer */
+  p.kernel_sign_key_blob = Malloc(LOAD_FIRMWARE_KEY_BLOB_MAX);
+  p.kernel_sign_key_size = LOAD_FIRMWARE_KEY_BLOB_MAX;
+
   /* Call LoadFirmware() */
   rv = LoadFirmware(&p);
   if (LOAD_FIRMWARE_SUCCESS == rv) {