Add structs for TPM NV simplification

firmware/lib/rollback_index.c

Index: firmware/lib/rollback_index.c
diff --git a/firmware/lib/rollback_index.c b/firmware/lib/rollback_index.c
index 1540bd03d9a180e6397d4a97acbe006be7c49d26..972d4f477bb6e377a814c43eafc09d2e78e2e645 100644
--- a/firmware/lib/rollback_index.c
+++ b/firmware/lib/rollback_index.c
@@ -25,6 +25,7 @@ __pragma(warning (disable: 4127))
     }                                                   \
   } while (0)
 
+
 uint32_t TPMClearAndReenable(void) {
   VBDEBUG(("TPM: Clear and re-enable\n"));
   RETURN_ON_FAILURE(TlclForceClear());
@@ -34,12 +35,13 @@ uint32_t TPMClearAndReenable(void) {
   return TPM_SUCCESS;
 }
 
+
 /* Like TlclWrite(), but checks for write errors due to hitting the 64-write
  * limit and clears the TPM when that happens.  This can only happen when the
  * TPM is unowned, so it is OK to clear it (and we really have no choice).
  * This is not expected to happen frequently, but it could happen.
  */
-static uint32_t SafeWrite(uint32_t index, uint8_t* data, uint32_t length) {
+static uint32_t SafeWrite(uint32_t index, const void* data, uint32_t length) {
   uint32_t result = TlclWrite(index, data, length);
   if (result == TPM_E_MAXNVWRITES) {
     RETURN_ON_FAILURE(TPMClearAndReenable());
@@ -49,6 +51,7 @@ static uint32_t SafeWrite(uint32_t index, uint8_t* data, uint32_t length) {
   }
 }
 
+
 /* Similarly to SafeWrite(), this ensures we don't fail a DefineSpace because
  * we hit the TPM write limit.  This is even less likely to happen than with
  * writes because we only define spaces once at initialization, but we'd rather
@@ -64,49 +67,38 @@ static uint32_t SafeDefineSpace(uint32_t index, uint32_t perm, uint32_t size) {
   }
 }
 
-static uint32_t InitializeKernelVersionsSpaces(void) {
-  RETURN_ON_FAILURE(SafeDefineSpace(KERNEL_VERSIONS_NV_INDEX,
-                                    TPM_NV_PER_PPWRITE, KERNEL_SPACE_SIZE));
-  RETURN_ON_FAILURE(SafeWrite(KERNEL_VERSIONS_NV_INDEX, KERNEL_SPACE_INIT_DATA,
-                              KERNEL_SPACE_SIZE));
-  return TPM_SUCCESS;
+
+/* Functions to read and write firmware and kernel spaces. */
+static uint32_t ReadSpaceFirmware(RollbackSpaceFirmware *rsf) {
+  return TlclRead(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));

[Luigi Semenzato, 2010/08/12 01:12:30]

I think Google's convention is to put the star near the type, not the variable
(wrong, but that's the convention).

[gauravsh, 2010/08/12 02:15:51]

The style guide doesn't enforce any. convention but requires you to be
consistent. I think.

On 2010/08/12 01:12:30, Luigi Semenzato wrote:

[Randall Spangler, 2010/08/12 20:00:47]

Done.

 }
 
-/* When the return value is TPM_SUCCESS, this function sets *|initialized| to 1
- * if the spaces have been fully initialized, to 0 if not.  Otherwise
- * *|initialized| is not changed.
- */
-uint32_t GetSpacesInitialized(int* initialized) {
-  uint32_t space_holder;
-  uint32_t result;
-  result = TlclRead(TPM_IS_INITIALIZED_NV_INDEX,
-                    (uint8_t*) &space_holder, sizeof(space_holder));
-  switch (result) {
-  case TPM_SUCCESS:
-    *initialized = 1;
-    break;
-  case TPM_E_BADINDEX:
-    *initialized = 0;
-    result = TPM_SUCCESS;
-    break;
-  }
-  return result;
+static uint32_t WriteSpaceFirmware(const RollbackSpaceFirmware *rsf) {
+  return SafeWrite(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));
 }
 
-/* Creates the NVRAM spaces, and sets their initial values as needed.
- */
-static uint32_t InitializeSpaces(void) {
-  uint32_t zero = 0;
-  uint32_t firmware_perm = TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE;
+static uint32_t ReadSpaceKernel(RollbackSpaceKernel *rsk) {
+  return TlclRead(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+}
+
+static uint32_t WriteSpaceKernel(const RollbackSpaceKernel *rsk) {
+  return SafeWrite(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+}
+
+
+
+/* Creates the NVRAM spaces, and sets their initial values as needed. */
+static uint32_t InitializeSpaces(RollbackSpaceFirmware *rsf,
+                                 RollbackSpaceKernel *rsk) {
+  static const RollbackSpaceFirmware rsf_init = {
+    ROLLBACK_SPACE_FIRMWARE_VERSION, 0, 0, 0};
+  static const RollbackSpaceKernel rsk_init = {
+    ROLLBACK_SPACE_KERNEL_VERSION, ROLLBACK_SPACE_KERNEL_UID, 0, 0};
   uint8_t nvlocked = 0;
 
   VBDEBUG(("TPM: Initializing spaces\n"));
 
-  /* Force the TPM clear, in case it previously had an owner, so that we can
-   * redefine the NVRAM spaces. */
-  RETURN_ON_FAILURE(TPMClearAndReenable());

[Luigi Semenzato, 2010/08/12 01:12:30]

We just discussed this in person.  We have this code here because of possible
random development flow on devices---but this is going to be so uncommon that we
might as well let the space creation fail and go to recovery mode.

-
-  /* The TPM will not enforce the NV authorization restrictions until the 
+  /* The TPM will not enforce the NV authorization restrictions until the
    * execution of a TPM_NV_DefineSpace with the handle of TPM_NV_INDEX_LOCK.
    * Create that space if it doesn't already exist. */
   RETURN_ON_FAILURE(TlclGetFlags(NULL, NULL, &nvlocked));
@@ -116,135 +108,24 @@ static uint32_t InitializeSpaces(void) {
     RETURN_ON_FAILURE(TlclSetNvLocked());
   }
 
-  RETURN_ON_FAILURE(SafeDefineSpace(FIRMWARE_VERSIONS_NV_INDEX,
-                                    firmware_perm, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeWrite(FIRMWARE_VERSIONS_NV_INDEX,
-                              (uint8_t*) &zero, sizeof(uint32_t)));
-
-  RETURN_ON_FAILURE(InitializeKernelVersionsSpaces());
-
-  /* The space KERNEL_VERSIONS_BACKUP_NV_INDEX is used to protect the kernel
-   * versions.  The content of space KERNEL_MUST_USE_BACKUP determines whether
-   * only the backup value should be trusted.
-   */
-  RETURN_ON_FAILURE(SafeDefineSpace(KERNEL_VERSIONS_BACKUP_NV_INDEX,
-                                    firmware_perm, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeWrite(KERNEL_VERSIONS_BACKUP_NV_INDEX,
-                              (uint8_t*) &zero, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeDefineSpace(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                                    firmware_perm, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeWrite(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                              (uint8_t*) &zero, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeDefineSpace(DEVELOPER_MODE_NV_INDEX,
-                                    firmware_perm, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(SafeWrite(DEVELOPER_MODE_NV_INDEX,
-                              (uint8_t*) &zero, sizeof(uint32_t)));
-
-  /* The space TPM_IS_INITIALIZED_NV_INDEX is used to indicate that the TPM
-   * initialization has completed.  Without it we cannot be sure that the last
-   * space to be created was also initialized (power could have been lost right
-   * after its creation).
-   */
-  RETURN_ON_FAILURE(SafeDefineSpace(TPM_IS_INITIALIZED_NV_INDEX,
-                                    firmware_perm, sizeof(uint32_t)));
-  return TPM_SUCCESS;
-}
-
-static uint32_t SetDistrustKernelSpaceAtNextBoot(uint32_t distrust) {
-  uint32_t must_use_backup;
-  RETURN_ON_FAILURE(TlclRead(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                             (uint8_t*) &must_use_backup, sizeof(uint32_t)));
-  if (must_use_backup != distrust) {
-     RETURN_ON_FAILURE(SafeWrite(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                                 (uint8_t*) &distrust, sizeof(uint32_t)));
-  }
-  return TPM_SUCCESS;
-}
-
-/* Checks if the kernel version space has been mucked with.  If it has,
- * reconstructs it using the backup value.
- */
-uint32_t RecoverKernelSpace(void) {
-  uint32_t perms = 0;
-  uint8_t buffer[KERNEL_SPACE_SIZE];
-  uint32_t backup_combined_versions;
-  uint32_t must_use_backup;
-  uint32_t zero = 0;
-
-  VBDEBUG(("TPM: RecoverKernelSpace()\n"));
-
-  RETURN_ON_FAILURE(TlclRead(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                             (uint8_t*) &must_use_backup, sizeof(uint32_t)));
-  /* must_use_backup is true if the previous boot entered recovery mode. */
-
-  VBDEBUG(("TPM: must_use_backup = %d\n", must_use_backup));
-
-  /* If we can't read the kernel space, or it has the wrong permission, or it
-   * doesn't contain the right identifier, we give up.  This will need to be
-   * fixed by the recovery kernel.  We have to worry about this because at any
-   * time (even with PP turned off) the TPM owner can remove and redefine a
-   * PP-protected space (but not write to it).
-   */
-  RETURN_ON_FAILURE(TlclRead(KERNEL_VERSIONS_NV_INDEX, (uint8_t*) &buffer,
-                             KERNEL_SPACE_SIZE));
-  RETURN_ON_FAILURE(TlclGetPermissions(KERNEL_VERSIONS_NV_INDEX, &perms));
-  if (perms != TPM_NV_PER_PPWRITE ||
-      Memcmp(buffer + sizeof(uint32_t), KERNEL_SPACE_UID,
-              KERNEL_SPACE_UID_SIZE) != 0) {
-    return TPM_E_CORRUPTED_STATE;
-  }
-
-  if (must_use_backup) {
-    /* We must use the backup space because in the preceding boot cycle the
-     * primary space was left unlocked and cannot be trusted.
-     */
-    RETURN_ON_FAILURE(TlclRead(KERNEL_VERSIONS_BACKUP_NV_INDEX,
-                               (uint8_t*) &backup_combined_versions,
-                               sizeof(uint32_t)));
-    RETURN_ON_FAILURE(SafeWrite(KERNEL_VERSIONS_NV_INDEX,
-                                (uint8_t*) &backup_combined_versions,
-                                sizeof(uint32_t)));
-    RETURN_ON_FAILURE(SafeWrite(KERNEL_MUST_USE_BACKUP_NV_INDEX,
-                                (uint8_t*) &zero, 0));
-  }
+  /* Initialize the firmware and kernel spaces */
+  Memcpy(rsf, &rsf_init, sizeof(RollbackSpaceFirmware));
+  /* Initialize the backup copy of the kernel space to the same data
+   * as the kernel space */
+  Memcpy(&rsf->kernel_backup, &rsk_init, sizeof(RollbackSpaceKernel));
+  Memcpy(rsk, &rsk_init, sizeof(RollbackSpaceKernel));
+
+  /* Define and set firmware and kernel spaces */
+  RETURN_ON_FAILURE(SafeDefineSpace(FIRMWARE_NV_INDEX,
+                                    TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE,
+                                    sizeof(RollbackSpaceFirmware)));
+  RETURN_ON_FAILURE(WriteSpaceFirmware(rsf));
+  RETURN_ON_FAILURE(SafeDefineSpace(KERNEL_NV_INDEX, TPM_NV_PER_PPWRITE,
+                                    sizeof(RollbackSpaceKernel)));
+  RETURN_ON_FAILURE(WriteSpaceKernel(rsk));

[Luigi Semenzato, 2010/08/12 01:12:30]

Let's see if I have this right.  You removed the TPM_IS_INITIALIZED_NV_INDEX
because we can detect if the initializing write happened to the KERNEL_NV_INDEX
by the presence of the GRWL.  But now we have an ambiguity: is the GRWL not
present because the initialization didn't complete, or because of an attack or a
bug?  It may not matter.

[Randall Spangler, 2010/08/12 20:00:47]

Either way, we'll need to fix it in recovery mode.

   return TPM_SUCCESS;
 }
 
-static uint32_t BackupKernelSpace(void) {
-  uint32_t kernel_versions;
-  uint32_t backup_versions;
-  VBDEBUG(("TPM: BackupKernelSpace()\n"));
-  RETURN_ON_FAILURE(TlclRead(KERNEL_VERSIONS_NV_INDEX,
-                             (uint8_t*) &kernel_versions, sizeof(uint32_t)));
-  RETURN_ON_FAILURE(TlclRead(KERNEL_VERSIONS_BACKUP_NV_INDEX,
-                             (uint8_t*) &backup_versions, sizeof(uint32_t)));
-  if (kernel_versions == backup_versions) {
-    return TPM_SUCCESS;
-  } else if (kernel_versions < backup_versions) {
-    /* This cannot happen.  We're screwed. */
-    return TPM_E_INTERNAL_INCONSISTENCY;
-  }
-  RETURN_ON_FAILURE(SafeWrite(KERNEL_VERSIONS_BACKUP_NV_INDEX,
-                              (uint8_t*) &kernel_versions, sizeof(uint32_t)));
-  return TPM_SUCCESS;
-}
-
-/* Checks for transitions between protected mode to developer mode.  When going
- * into or out of developer mode, clear the TPM.
- */
-static uint32_t CheckDeveloperModeTransition(uint32_t current_developer) {
-  uint32_t past_developer;
-  RETURN_ON_FAILURE(TlclRead(DEVELOPER_MODE_NV_INDEX,
-                             (uint8_t*) &past_developer,
-                             sizeof(past_developer)));
-  if (past_developer != current_developer) {
-    RETURN_ON_FAILURE(TPMClearAndReenable());
-    RETURN_ON_FAILURE(SafeWrite(DEVELOPER_MODE_NV_INDEX,
-                                (uint8_t*) &current_developer,
-                                sizeof(current_developer)));
-  }
-  return TPM_SUCCESS;
-}
 
 /* SetupTPM starts the TPM and establishes the root of trust for the
  * anti-rollback mechanism.  SetupTPM can fail for three reasons.  1 A bug. 2 a
@@ -265,10 +146,17 @@ static uint32_t CheckDeveloperModeTransition(uint32_t current_developer) {
  * to the TPM flashram at every reboot or wake-up, because of concerns about
  * the durability of the NVRAM.
  */
-uint32_t SetupTPM(int recovery_mode, int developer_mode) {
+uint32_t SetupTPM(int recovery_mode, int developer_mode,
+                  RollbackSpaceFirmware *rsf) {
+
+  RollbackSpaceKernel rsk;
+  int rsf_dirty = 0;
+  uint8_t new_flags = 0;
+
   uint8_t disable;
   uint8_t deactivated;
   uint32_t result;
+  uint32_t perms;
 
   VBDEBUG(("TPM: SetupTPM(r%d, d%d)\n", recovery_mode, developer_mode));
 
@@ -277,48 +165,100 @@ uint32_t SetupTPM(int recovery_mode, int developer_mode) {
 
   RETURN_ON_FAILURE(TlclStartup());
 #ifdef USE_CONTINUE_SELF_TEST
-  /* TODO: ContinueSelfTest() should be faster than SelfTestFull, but may also
-   * not work properly in older TPM firmware.  For now, do the full self test. */
+  /* TODO: ContinueSelfTest() should be faster than SelfTestFull, but
+   * may also not work properly in older TPM firmware.  For now, do
+   * the full self test. */
   RETURN_ON_FAILURE(TlclContinueSelfTest());
 #else
   RETURN_ON_FAILURE(TlclSelfTestFull());
 #endif
   RETURN_ON_FAILURE(TlclAssertPhysicalPresence());
-  /* Checks that the TPM is enabled and activated. */
+
+  /* Check that the TPM is enabled and activated. */
   RETURN_ON_FAILURE(TlclGetFlags(&disable, &deactivated, NULL));
   if (disable || deactivated) {
-    VBDEBUG(("TPM: disabled (%d) or deactivated (%d).  Fixing...\n", disable, deactivated));
+    VBDEBUG(("TPM: disabled (%d) or deactivated (%d).  Fixing...\n",
+             disable, deactivated));
     RETURN_ON_FAILURE(TlclSetEnable());
     RETURN_ON_FAILURE(TlclSetDeactivated(0));
     VBDEBUG(("TPM: Must reboot to re-enable\n"));
     return TPM_E_MUST_REBOOT;
   }
-  result = RecoverKernelSpace();
-  if (result != TPM_SUCCESS) {
-     /* Check if this is the first time we run and the TPM has not been
-      * initialized yet.
-      */
-    int initialized = 0;
-    VBDEBUG(("TPM: RecoverKernelSpace() failed\n"));
-    RETURN_ON_FAILURE(GetSpacesInitialized(&initialized));
-    if (initialized) {
-      VBDEBUG(("TPM: Already initialized, so give up\n"));
-      return result;
+
+  /* Read the firmware space. */
+  result = ReadSpaceFirmware(rsf);
+  if (TPM_E_BADINDEX == result) {
+    /* This is the first time we've run, and the TPM has not been
+     * initialized.  Initialize it. */
+    VBDEBUG(("TPM: Not initialized yet.\n"));
+    RETURN_ON_FAILURE(InitializeSpaces(rsf, &rsk));
+  } else if (TPM_SUCCESS != result) {
+    VBDEBUG(("TPM: Firmware space in a bad state; giving up.\n"));
+    return TPM_E_CORRUPTED_STATE;
+  }
+  VBDEBUG(("TPM: Firmware space sv%d f%x v%x\n",
+           rsf->struct_version, rsf->flags, rsf->fw_versions));
+
+  /* Read the kernel space and verify its permissions.  If the kernel
+   * space has the wrong permission, or it doesn't contain the right
+   * identifier, we give up.  This will need to be fixed by the
+   * recovery kernel.  We have to worry about this because at any time
+   * (even with PP turned off) the TPM owner can remove and redefine a
+   * PP-protected space (but not write to it). */
+  RETURN_ON_FAILURE(ReadSpaceKernel(&rsk));
+  RETURN_ON_FAILURE(TlclGetPermissions(KERNEL_NV_INDEX, &perms));
+  if (TPM_NV_PER_PPWRITE != perms || ROLLBACK_SPACE_KERNEL_UID != rsk.uid)
+    return TPM_E_CORRUPTED_STATE;
+  VBDEBUG(("TPM: Kernel space sv%d v%x\n",
+           rsk.struct_version, rsk.kernel_versions));
+
+  /* If the kernel space and its backup are different, we need to copy
+   * one to the other.  Which one we copy depends on whether the
+   * use-backup flag is set. */
+  if (0 != Memcmp(&rsk, &rsf->kernel_backup, sizeof(RollbackSpaceKernel))) {
+    VBDEBUG(("TPM: kernel space and backup are different\n"));
+
+    if (rsf->flags & FLAG_KERNEL_SPACE_USE_BACKUP) {
+      VBDEBUG(("TPM: use backup kernel space\n"));
+      Memcpy(&rsk, &rsf->kernel_backup, sizeof(RollbackSpaceKernel));
+      RETURN_ON_FAILURE(WriteSpaceKernel(&rsk));
+    } else if (rsk.kernel_versions < rsf->kernel_backup.kernel_versions) {
+      VBDEBUG(("TPM: kernel versions %x < backup versions %x\n",
+               rsk.kernel_versions, rsf->kernel_backup.kernel_versions));
+      return TPM_E_INTERNAL_INCONSISTENCY;
     } else {
-      VBDEBUG(("TPM: Need to initialize spaces.\n"));
-      RETURN_ON_FAILURE(InitializeSpaces());
-      VBDEBUG(("TPM: Retrying RecoverKernelSpace() now that spaces are initialized.\n"));
-      RETURN_ON_FAILURE(RecoverKernelSpace());
+      VBDEBUG(("TPM: copy kernel space to backup\n"));
+      Memcpy(&rsf->kernel_backup, &rsk, sizeof(RollbackSpaceKernel));
+      rsf_dirty = 1;
     }
   }
-  RETURN_ON_FAILURE(BackupKernelSpace());
-  RETURN_ON_FAILURE(SetDistrustKernelSpaceAtNextBoot(recovery_mode));
-  RETURN_ON_FAILURE(CheckDeveloperModeTransition(developer_mode));
 
+  /* Clear ownership if developer flag has toggled */
+  if ((developer_mode ? FLAG_LAST_BOOT_DEVELOPER : 0) !=
+      (rsf->flags & FLAG_LAST_BOOT_DEVELOPER)) {
+    VBDEBUG(("TPM: Developer flag changed; clearing owner.\n"));
+    RETURN_ON_FAILURE(TPMClearAndReenable());
+  }
+
+  /* Update flags */
+  if (developer_mode)
+    new_flags |= FLAG_LAST_BOOT_DEVELOPER;
   if (recovery_mode) {
-    /* In recovery mode global variables are usable. */
-    g_rollback_recovery_mode = 1;
+    new_flags |= FLAG_KERNEL_SPACE_USE_BACKUP;
+    g_rollback_recovery_mode = 1;  /* Global variables are usable in
+                                    * recovery mode */
+  }
+  if (rsf->flags != new_flags) {
+    rsf->flags = new_flags;
+    rsf_dirty = 1;
   }
+
+  /* If firmware space is dirty, flush it back to the TPM */
+  if (rsf_dirty) {
+    VBDEBUG(("TPM: Updating firmware space.\n"));
+    RETURN_ON_FAILURE(WriteSpaceFirmware(rsf));
+  }
+
   VBDEBUG(("TPM: SetupTPM() succeeded\n"));
   return TPM_SUCCESS;
 }
@@ -331,7 +271,8 @@ __pragma(warning (disable: 4100))
 
 /* Dummy implementations which don't support TPM rollback protection */
 
-uint32_t RollbackFirmwareSetup(int developer_mode) {
+uint32_t RollbackFirmwareSetup(int developer_mode,
+                               uint16_t* key_version, uint16_t* version) {
 #ifndef CHROMEOS_ENVIRONMENT
   /* Initialize the TPM, but ignore return codes.  In ChromeOS
    * environment, don't even talk to the TPM. */
@@ -339,10 +280,7 @@ uint32_t RollbackFirmwareSetup(int developer_mode) {
   TlclStartup();
   TlclSelfTestFull();
 #endif
-  return TPM_SUCCESS;
-}
 
-uint32_t RollbackFirmwareRead(uint16_t* key_version, uint16_t* version) {
   *key_version = *version = 0;
   return TPM_SUCCESS;
 }
@@ -381,26 +319,29 @@ uint32_t RollbackKernelLock(void) {
 
 #else
 
-uint32_t RollbackFirmwareSetup(int developer_mode) {
-  return SetupTPM(0, developer_mode);
-}
+uint32_t RollbackFirmwareSetup(int developer_mode, uint16_t* key_version,
+                               uint16_t* version) {
+  RollbackSpaceFirmware rsf;
+
+  RETURN_ON_FAILURE(SetupTPM(0, developer_mode, &rsf));
+  *key_version = (uint16_t)(rsf.fw_versions >> 16);
+  *version = (uint16_t)(rsf.fw_versions & 0xffff);
+
+  VBDEBUG(("TPM: RollbackFirmwareSetup %x %x %x\n", (int)rsf.fw_versions, (int)*key_version, (int)*version));
 
-uint32_t RollbackFirmwareRead(uint16_t* key_version, uint16_t* version) {
-  uint32_t firmware_versions;
-  /* Gets firmware versions. */
-  RETURN_ON_FAILURE(TlclRead(FIRMWARE_VERSIONS_NV_INDEX,
-                             (uint8_t*) &firmware_versions,
-                             sizeof(firmware_versions)));
-  *key_version = (uint16_t) (firmware_versions >> 16);
-  *version = (uint16_t) (firmware_versions & 0xffff);
   return TPM_SUCCESS;
 }
 
 uint32_t RollbackFirmwareWrite(uint16_t key_version, uint16_t version) {
-  uint32_t combined_version = (key_version << 16) & version;
-  return SafeWrite(FIRMWARE_VERSIONS_NV_INDEX,
-                   (uint8_t*) &combined_version,
-                   sizeof(uint32_t));
+  RollbackSpaceFirmware rsf;
+  uint32_t new_versions = ((uint32_t)key_version << 16) | version;
+
+  VBDEBUG(("TPM: RollbackFirmwareWrite(%d, %d)\n", (int)key_version, (int)version));
+
+  RETURN_ON_FAILURE(ReadSpaceFirmware(&rsf));
+  VBDEBUG(("TPM: RollbackFirmwareWrite %x --> %x\n", (int)rsf.fw_versions, (int)new_versions));
+  rsf.fw_versions = new_versions;
+  return WriteSpaceFirmware(&rsf);
 }
 
 uint32_t RollbackFirmwareLock(void) {
@@ -408,52 +349,56 @@ uint32_t RollbackFirmwareLock(void) {
 }
 
 uint32_t RollbackKernelRecovery(int developer_mode) {
-  uint32_t result = SetupTPM(1, developer_mode);
+  RollbackSpaceFirmware rsf;
+  uint32_t result = SetupTPM(1, developer_mode, &rsf);
   /* In recovery mode we ignore TPM malfunctions or corruptions, and leave the
    * TPM completely unlocked if and only if the dev mode switch is ON.  The
    * recovery kernel will fix the TPM (if needed) and lock it ASAP.  We leave
-   * Physical Presence on in either case.
-   */
+   * Physical Presence on in either case. */
   if (!developer_mode) {
     RETURN_ON_FAILURE(TlclSetGlobalLock());
   }
   /* We still return the result of SetupTPM even though we expect the caller to
-   * ignore it.  It's useful in unit testing.
-   */
+   * ignore it.  It's useful in unit testing. */
   return result;
 }
 
 uint32_t RollbackKernelRead(uint16_t* key_version, uint16_t* version) {
-  uint32_t kernel_versions;
   if (g_rollback_recovery_mode) {
     *key_version = 0;
     *version = 0;
   } else {
-    /* Reads kernel versions from TPM. */
-    RETURN_ON_FAILURE(TlclRead(KERNEL_VERSIONS_NV_INDEX,
-                               (uint8_t*) &kernel_versions,
-                               sizeof(kernel_versions)));
-    *key_version = (uint16_t) (kernel_versions >> 16);
-    *version = (uint16_t) (kernel_versions & 0xffff);
+    RollbackSpaceKernel rsk;
+    RETURN_ON_FAILURE(ReadSpaceKernel(&rsk));
+    *key_version = (uint16_t)(rsk.kernel_versions >> 16);
+    *version = (uint16_t)(rsk.kernel_versions & 0xffff);
+    VBDEBUG(("TPM: RollbackKernelRead %x %x %x\n", (int)rsk.kernel_versions, (int)*key_version, (int)*version));
   }
   return TPM_SUCCESS;
 }
 
 uint32_t RollbackKernelWrite(uint16_t key_version, uint16_t version) {
-  if (!g_rollback_recovery_mode) {
-    uint32_t combined_version = (key_version << 16) & version;
-    return SafeWrite(KERNEL_VERSIONS_NV_INDEX,
-                     (uint8_t*) &combined_version,
-                     sizeof(uint32_t));
+
+  VBDEBUG(("TPM: RollbackKernelWrite(%d, %d)\n", (int)key_version, (int)version));
+
+  if (g_rollback_recovery_mode) {
+    return TPM_SUCCESS;
+  } else {
+    RollbackSpaceKernel rsk;
+    uint32_t new_versions = ((uint32_t)key_version << 16) | version;
+
+    RETURN_ON_FAILURE(ReadSpaceKernel(&rsk));
+    VBDEBUG(("TPM: RollbackKernelWrite %x --> %x\n", (int)rsk.kernel_versions, (int)new_versions));
+    rsk.kernel_versions = new_versions;
+    return WriteSpaceKernel(&rsk);
   }
-  return TPM_SUCCESS;
 }
 
 uint32_t RollbackKernelLock(void) {
-  if (!g_rollback_recovery_mode) {
-    return TlclLockPhysicalPresence();
-  } else {
+  if (g_rollback_recovery_mode) {
     return TPM_SUCCESS;
+  } else {
+    return TlclLockPhysicalPresence();
   }
 }