Do not advance kernel version in TPM if we are in firmware B trying a new firmware

firmware/lib/vboot_kernel.c

 /* Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  *
  * Functions for loading a kernel from disk.
  * (Firmware portion)
  */
 
 #include "vboot_kernel.h"
 
@@ skipping 579 matching lines @@
   /* Write and free GPT data */
   WriteAndFreeGptData(&gpt);
 
   /* Handle finding a good partition */
   if (good_partition >= 0) {
     VBDEBUG(("Good_partition >= 0\n"));
     if (shcall)
       shcall->check_result = VBSD_LKC_CHECK_GOOD_PARTITION;
 
     /* See if we need to update the TPM */
-    if (kBootRecovery != boot_mode && good_partition_key_block_valid) {
-      /* We only update the TPM in normal and developer boot modes.  In
-       * developer mode, we only advanced lowest_version for kernels with valid
-       * key blocks, and didn't count self-signed key blocks.  In recovery
-       * mode, the TPM stays PP-unlocked, so anything we write gets blown away
-       * by the firmware when we go back to normal mode. */
-      VBDEBUG(("Boot_flags = not recovery\n"));
+    if ((kBootNormal == boot_mode) &&
+        !((1 == shared->firmware_index) && (shared->flags & VBSD_FWB_TRIED))) {

[Will Drewry, 2011/04/17 17:53:18]

What case will shared->firmware_index==1 without VBSD_FWB_TRIED?  Do we need to
worry about that case too? (E.g., corrupted FWA). Or is this only when it
appears an AU triggered the fw volume use?

+      /* We only update the TPM in normal mode.  We don't advance the
+       * TPM if we're trying a new firmware B, because that firmware
+       * may have a key change and roll forward the TPM too soon. */
+      VBDEBUG(("Checking if TPM kernel version needs advancing\n"));
 
       if ((lowest_version > tpm_version) &&

[Will Drewry, 2011/04/17 17:53:18]

Since we only increment when the lowest_version of both kernels is >
tpm_version, why would we ever trigger a bug on a kernel version update if in
VSD_FWB_TRIED?  This seems more relevant for a firmware version change, but
maybe I'm missing something?

Is this just in case there is a bug in the new firmware that doesn't respect the
existing lower-versioning kernels?

If so, please spell that out in the comments!  It will likely answer all my
questions :)

[gauravsh, 2011/04/17 19:32:24]

Belated comment: I'd really like to see a check for the following invariant:
lowest_version is not valid until it has been compared to both kernel versions.
It is not written until it is valid. 

Is there any situation where the invariant won't be true? 

This should, hopefully, catch other code paths where we can run into bad
situations.

           (lowest_version != LOWEST_TPM_VERSION)) {
         status = RollbackKernelWrite((uint32_t)lowest_version);
         if (0 != status) {
           VBDEBUG(("Error writing kernel versions to TPM.\n"));
           if (status == TPM_E_MUST_REBOOT)
             retval = LOAD_KERNEL_REBOOT;
           else
             recovery = VBNV_RECOVERY_RW_TPM_ERROR;
           goto LoadKernelExit;
         }
@@ skipping 47 matching lines @@
 
     /* Save timer values */
     shared->timer_load_kernel_enter = timer_enter;
     shared->timer_load_kernel_exit = VbGetTimer();
     /* Store how much shared data we used, if any */
     params->shared_data_size = shared->data_used;
   }
 
   return retval;
 }