Ignore TPM return codes in recovery mode

firmware/lib/vboot_kernel.c

Index: firmware/lib/vboot_kernel.c
diff --git a/firmware/lib/vboot_kernel.c b/firmware/lib/vboot_kernel.c
index 5533f11c8d2e06bb6ff4eb92a1af12e5ff320b39..e6658d7518a69546c8a3fc743f748d97d16c1a46 100644
--- a/firmware/lib/vboot_kernel.c
+++ b/firmware/lib/vboot_kernel.c
@@ -124,10 +124,9 @@ int LoadKernel(LoadKernelParams* params) {
   uint16_t tpm_kernel_version = 0;
   uint64_t lowest_key_version = 0xFFFF;
   uint64_t lowest_kernel_version = 0xFFFF;
-  int is_dev = ((BOOT_FLAG_DEVELOPER & params->boot_flags) &&
-                !(BOOT_FLAG_RECOVERY & params->boot_flags));
-  int is_normal = (!(BOOT_FLAG_DEVELOPER & params->boot_flags) &&
-                   !(BOOT_FLAG_RECOVERY & params->boot_flags));
+  int is_dev = (BOOT_FLAG_DEVELOPER & params->boot_flags);
+  int is_rec = (BOOT_FLAG_RECOVERY & params->boot_flags);
+  int is_normal = (!is_dev && !is_rec);
 
   /* Clear output params in case we fail */
   params->partition_number = 0;
@@ -135,11 +134,11 @@ int LoadKernel(LoadKernelParams* params) {
   params->bootloader_size = 0;
 
   /* Let the TPM know if we're in recovery mode */
-  if (BOOT_FLAG_RECOVERY & params->boot_flags) {
-    if (0 != RollbackKernelRecovery(BOOT_FLAG_DEVELOPER & params->boot_flags
-                                    ? 1 : 0)) {
+  if (is_rec) {
+    if (0 != RollbackKernelRecovery(is_dev ? 1 : 0)) {

[Luigi Semenzato, 2010/06/24 20:18:43]

If our convention for booleans is 0 for false, and anything else for true, you
could just pass is_dev here.  That seems to be the convention based on your use
below.

       VBDEBUG(("Error setting up TPM for recovery kernel\n"));
-      return LOAD_KERNEL_RECOVERY;
+      /* Ignore return code, since we need to boot recovery mode to
+       * fix the TPM. */
     }
   }
 
@@ -150,7 +149,7 @@ int LoadKernel(LoadKernelParams* params) {
       VBDEBUG(("Unable to get kernel versions from TPM\n"));
       return LOAD_KERNEL_RECOVERY;
     }
-  } else if (is_dev) {
+  } else if (is_dev && !is_rec) {
     /* In developer mode, we ignore the kernel subkey, and just use
      * the SHA-512 hash to verify the key block. */
     kernel_subkey = NULL;
@@ -205,14 +204,14 @@ int LoadKernel(LoadKernelParams* params) {
 
       /* Check the key block flags against the current boot mode */
       if (!(key_block->key_block_flags &&
-            ((BOOT_FLAG_DEVELOPER & params->boot_flags) ?
-             KEY_BLOCK_FLAG_DEVELOPER_1 : KEY_BLOCK_FLAG_DEVELOPER_0))) {
+            (is_dev ? KEY_BLOCK_FLAG_DEVELOPER_1 :
+             KEY_BLOCK_FLAG_DEVELOPER_0))) {
         VBDEBUG(("Developer flag mismatch.\n"));
         continue;
       }
       if (!(key_block->key_block_flags &&
-            ((BOOT_FLAG_RECOVERY & params->boot_flags) ?
-             KEY_BLOCK_FLAG_RECOVERY_1 : KEY_BLOCK_FLAG_RECOVERY_0))) {
+            (is_rec ? KEY_BLOCK_FLAG_RECOVERY_1 :
+             KEY_BLOCK_FLAG_RECOVERY_0))) {
         VBDEBUG(("Recovery flag mismatch.\n"));
         continue;
       }
@@ -374,10 +373,12 @@ int LoadKernel(LoadKernelParams* params) {
       }
     }
 
-    /* Lock the kernel versions, since we're about to boot the kernel */
+    /* Lock the kernel versions */

[Luigi Semenzato, 2010/06/24 20:18:43]

I think that here we may want to invert the conditionals.  We don't want to lock
PP in recovery mode.

Oh I see.  I already take care of that in RollbackKernelLock.  So you don't
really have to check for is_rec, because in that case the Lock call is a noop,
which is always successful.

Either way is fine.

     if (0 != RollbackKernelLock()) {
       VBDEBUG(("Error locking kernel versions.\n"));
-      return LOAD_KERNEL_RECOVERY;
+      /* Don't reboot to recovery mode if we're already there */
+      if (!is_rec)
+        return LOAD_KERNEL_RECOVERY;
     }
 
     /* Success! */