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

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

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().
  *

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;

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);

vboot_firmware/include/load_firmware_fw.h

@@ -11,6 +11,43 @@
 
 #include <stdint.h>
 
+/* 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 */
+
+typedef struct LoadFirmwareParams {
+  /* Inputs to LoadFirmware() */
+  void *firmware_root_key_blob;  /* Key used to sign firmware header */
+  void *verification_block_0;    /* Key block + preamble for firmware 0 */
+  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. */
+
+  /* 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
@@ -24,43 +61,11 @@
  * 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
+ * Returns 0 if successful or non-zero if error. */
- * in [*size].  On error, returns NULL. */
+int GetFirmwareBody(LoadFirmwareParams* params, uint64_t firmware_index);
-void *GetFirmwareBody(uint64_t firmware_index, uint64_t* size);
 
 
-/* Interface provided by verified boot library to PEI */
+/* Functions provided by verified boot library to PEI */
-
-/* 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 */
-  void *verification_block_0;    /* Key block + preamble for firmware 0 */
-  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 */
-
-  /* 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. */
-} LoadFirmwareParams;
-
 
 /* Attempts to load the rewritable firmware.
  *
@@ -68,4 +73,13 @@ typedef struct LoadFirmwareParams {
 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_ */

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().
  *

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_ */

vboot_firmware/lib/load_firmware_fw.c

@@ -12,48 +12,15 @@
 #include "utility.h"
 
 
-static const char kFakeKernelBlob[] = "Fake kernel sign key blob";
+static const char kFakeKernelBlob[2088] = "Fake kernel sign key blob";
-
-void UpdateFirmwareBodyHash(uint8_t* data, uint64_t size) {
-  /* TODO: actually update the hash. */
-}
 
 
 int LoadFirmware(LoadFirmwareParams* params) {
-  /* TODO: real implementation!  For now, this just chains to the old
+  /* TODO: real implementation!  This is now sufficiently broken due
-   * implementation. */
+   * to refactoring that we'll just trust firmware A. */
-
+  Memcpy(params->kernel_sign_key_blob, kFakeKernelBlob,
-  uint8_t* fw0;
+         sizeof(kFakeKernelBlob));
-  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;
   params->kernel_sign_key_size = sizeof(kFakeKernelBlob);
-
+  params->firmware_index = 0;
-  switch(rv) {
+  return LOAD_FIRMWARE_SUCCESS;
-    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;
-
 }

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;
 

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;
+typedef struct VbLoadFirmwareInternal {
-static uint64_t body_size_accum = 0;
+  DigestContext body_digest_context;
-static int inside_load_firmware = 0;
+  uint64_t body_size_accum;
+} VbLoadFirmwareInternal;
 
-void UpdateFirmwareBodyHash2(uint8_t* data, uint64_t size) {
 
-  if (!inside_load_firmware) {
+void UpdateFirmwareBodyHash(LoadFirmwareParams* params,
-    debug("UpdateFirmwareBodyHash() called outside LoadFirmware()\n");
+                             uint8_t* data, uint64_t size) {
-    return;
+  VbLoadFirmwareInternal* lfi =
-  }
+      (VbLoadFirmwareInternal*)params->load_firmware_internal;
 
-  DigestUpdate(&ctx, data, size);
+  DigestUpdate(&lfi->body_digest_context, data, size);
-  body_size_accum += 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);
+    DigestInit(&lfi->body_digest_context, data_key->algorithm);
-    body_size_accum = 0;
+    lfi->body_size_accum = 0;
-    inside_load_firmware = 1;
+    if ((0 != GetFirmwareBody(params, index)) ||
-    body_data = GetFirmwareBody(index, &body_size);
+        (lfi->body_size_accum != preamble->body_signature.data_size)) {
-    inside_load_firmware = 0;
-    body_digest = DigestFinal(&ctx);
-    if (!body_data || (body_size != preamble->body_signature.data_size) ||
-        (body_size_accum != body_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) {
 

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;

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;
+int GetFirmwareBody(LoadFirmwareParams* params, uint64_t index) {
-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) {
 
+  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;
+      size = ci->firmwareA_size;
-      fw = g_firmwareA;
+      fw = ci->firmwareA;
     case 1:
-      *size = g_firmwareB_size;
+      size = ci->firmwareB_size;
-      fw = g_firmwareB;
+      fw = ci->firmwareB;
     default:
       /* Anything else is invalid */
-      *size = 0;
+      return 1;
-      return NULL;
   }
 
   /* 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 */
+  /* Success */
-  return fw;
+  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;
+  ci.firmwareA = firmwareA;
-  g_firmwareB = firmwareB;
+  ci.firmwareB = firmwareB;
 
   /* TODO: YOU NEED TO PASS IN THE FIRMWARE VOLUME SIZES SOMEHOW */
-  g_firmwareA_size = 0;
+  ci.firmwareA_size = 0;
-  g_firmwareB_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) {