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

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 cfdd9b4c0b891b957f0bd21819a8908ccf59de4b..0e9c15ba7ecaa107efbfe8aa5918a5c7a19997e3 100644
--- a/firmware/lib/vboot_kernel.c
+++ b/firmware/lib/vboot_kernel.c
@@ -597,13 +597,12 @@ int LoadKernel(LoadKernelParams* params) {
       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)) {