Add checksum to TPM RollbackSpace regions for FW and kernel.

BUG=chrome-os-partner:9707
TEST=manual

  make
  make runtests

You can also test it by clearing the TPM, then manually looking at the TPM
regions. In dev-mode, clear the regions and you'll see something like this:

  localhost ~ # tpmc read 1007 a
  1 0 0 0 0 0 0 0 0 0
  localhost ~ # tpmc read 1008 d
  1 4c 57 52 47 0 0 0 0 0 0 0 0
  localhost ~ #

Go back to normal mode and reboot, and you'll see something like this:

  localhost ~ # tpmc read 1007 a
  2 0 1 0 1 0 0 0 0 4f
  localhost ~ # tpmc read 1008 d
  2 4c 57 52 47 1 0 1 0 0 0 0 55
  localhost ~ #

The important things are that the first number is now 2, instead of 1, and
the last number is not zero (it's a checksum, so it'll vary depending on the
other numbers, which will themselves vary according to the firmware and
kernel versions).

Change-Id: Ia4040311c2a4b2819792549b883377c8b6b89d48
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/22856
Reviewed-by: Randall Spangler <rspangler@chromium.org>

9 files changed, 416 insertions, 46 deletions

diff --git a/firmware/Makefile b/firmware/Makefile
index 8ea647e2..811c158b 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -62,6 +62,7 @@ LIB_SRCS = \
 	./lib/cgptlib/cgptlib.c \
 	./lib/cgptlib/cgptlib_internal.c \
 	./lib/cgptlib/crc32.c \
+	./lib/crc8.c \
 	./lib/cryptolib/padding.c \
 	./lib/cryptolib/rsa.c \
 	./lib/cryptolib/rsa_utility.c \
diff --git a/firmware/include/sysincludes.h b/firmware/include/sysincludes.h
index 1560291e..777440e6 100644
--- a/firmware/include/sysincludes.h
+++ b/firmware/include/sysincludes.h
@@ -18,6 +18,7 @@
 #ifdef CHROMEOS_ENVIRONMENT
 
 #include <inttypes.h>  /* For PRIu64 */
+#include <stddef.h>
 #include <stdint.h>
 #include <stdlib.h>
 
diff --git a/firmware/lib/crc8.c b/firmware/lib/crc8.c
new file mode 100644
index 00000000..fa770253
--- /dev/null
+++ b/firmware/lib/crc8.c
@@ -0,0 +1,28 @@
+/* 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.
+ */
+
+#include "crc8.h"
+
+/* Return CRC-8 of the data, using x^8 + x^2 + x + 1 polynomial.  A
+ * table-based algorithm would be faster, but for only a few bytes it isn't
+ * worth the code size. */
+uint8_t Crc8(const void* vptr, int len) {
+  const uint8_t *data = vptr;
+  unsigned crc = 0;
+  int i, j;
+
+  for (j = len; j; j--, data++) {
+    crc ^= (*data << 8);
+    for(i = 8; i; i--) {
+      if (crc & 0x8000)
+        crc ^= (0x1070 << 3);
+      crc <<= 1;
+    }
+  }
+
+  return (uint8_t)(crc >> 8);
+}
+
+
diff --git a/firmware/lib/include/crc8.h b/firmware/lib/include/crc8.h
new file mode 100644
index 00000000..95bc9862
--- /dev/null
+++ b/firmware/lib/include/crc8.h
@@ -0,0 +1,13 @@
+/* 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.
+ *
+ * Very simple 8-bit CRC function.
+ */
+#ifndef VBOOT_REFERENCE_CRC8_H_
+#define VBOOT_REFERENCE_CRC8_H_
+#include "sysincludes.h"
+
+uint8_t Crc8(const void* data, int len);
+
+#endif /* VBOOT_REFERENCE_CRC8_H_ */
diff --git a/firmware/lib/include/rollback_index.h b/firmware/lib/include/rollback_index.h
index 4d92bb71..4d847626 100644
--- a/firmware/lib/include/rollback_index.h
+++ b/firmware/lib/include/rollback_index.h
@@ -21,14 +21,15 @@
 __pragma(pack(push, 1)) /* Support packing for MSVC. */
 
 /* Kernel space - KERNEL_NV_INDEX, locked with physical presence. */
-#define ROLLBACK_SPACE_KERNEL_VERSION 1
+#define ROLLBACK_SPACE_KERNEL_VERSION 2
 #define ROLLBACK_SPACE_KERNEL_UID 0x4752574C  /* 'GRWL' */
 typedef struct RollbackSpaceKernel {
   uint8_t  struct_version;      /* Struct version, for backwards
                                  * compatibility */
   uint32_t uid;                 /* Unique ID to detect space redefinition */
   uint32_t kernel_versions;     /* Kernel versions */
-  uint32_t reserved;            /* Reserved for future expansion */
+  uint8_t reserved[3];          /* Reserved for future expansion */
+  uint8_t crc8;                 /* Checksum (v2 and later only) */
 } __attribute__((packed)) RollbackSpaceKernel;
 
 
@@ -41,13 +42,14 @@ typedef struct RollbackSpaceKernel {
  * from the backup copy. */
 #define FLAG_KERNEL_SPACE_USE_BACKUP 0x02
 
-#define ROLLBACK_SPACE_FIRMWARE_VERSION 1
+#define ROLLBACK_SPACE_FIRMWARE_VERSION 2
 /* Firmware space - FIRMWARE_NV_INDEX, locked with global lock. */
 typedef struct RollbackSpaceFirmware {
   uint8_t  struct_version;  /* Struct version, for backwards compatibility */
   uint8_t  flags;           /* Flags (see FLAG_* above) */
   uint32_t fw_versions;     /* Firmware versions */
-  uint32_t reserved;            /* Reserved for future expansion */
+  uint8_t reserved[3];      /* Reserved for future expansion */
+  uint8_t crc8;             /* Checksum (v2 and later only) */
 } __attribute__((packed)) RollbackSpaceFirmware;
 
 __pragma(pack(pop)) /* Support packing for MSVC. */
diff --git a/firmware/lib/rollback_index.c b/firmware/lib/rollback_index.c
index 1d0f1d0f..3b4e4665 100644
--- a/firmware/lib/rollback_index.c
+++ b/firmware/lib/rollback_index.c
@@ -6,12 +6,17 @@
  * stored in the TPM NVRAM.
  */
 
+#include "crc8.h"
 #include "rollback_index.h"
 #include "tlcl.h"
 #include "tss_constants.h"
 #include "utility.h"
 #include "vboot_api.h"
 
+#ifndef offsetof
+#define offsetof(A,B) __builtin_offsetof(A,B)
+#endif
+
 #ifdef ROLLBACK_UNITTEST
 /* Compiling for unit test, so we need the real implementations of
  * rollback functions.  The unit test mocks the underlying tlcl
@@ -67,31 +72,134 @@ uint32_t SafeDefineSpace(uint32_t index, uint32_t perm, uint32_t size) {
 
 
 /* Functions to read and write firmware and kernel spaces. */
-static uint32_t ReadSpaceFirmware(RollbackSpaceFirmware* rsf) {
-  return TlclRead(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));
+uint32_t ReadSpaceFirmware(RollbackSpaceFirmware* rsf) {
+  uint32_t r;
+  int attempts = 3;
+
+  while (attempts--) {
+    r = TlclRead(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));
+    if (r != TPM_SUCCESS)
+      return r;
+
+    /* No CRC in this version, so we'll create one when we write it. Note that
+     * we're marking this as version 2, not ROLLBACK_SPACE_FIRMWARE_VERSION,
+     * because version 2 just added the CRC. Later versions will need to
+     * set default values for any extra fields explicitly (probably here). */
+    if (rsf->struct_version < 2) {
+      rsf->struct_version = 2;          /* Danger Will Robinson! Danger! */
+      return TPM_SUCCESS;
+    }
+
+    /* If the CRC is good, we're done. If it's bad, try a couple more times to
+     * see if it gets better before we give up. It could just be noise. */
+    if (rsf->crc8 == Crc8(rsf, offsetof(RollbackSpaceFirmware, crc8)))
+      return TPM_SUCCESS;
+
+    VBDEBUG(("TPM: %s() - bad CRC\n", __func__));
+  }
+
+  VBDEBUG(("TPM: %s() - too many bad CRCs, giving up\n", __func__));
+  return TPM_E_CORRUPTED_STATE;
 }
 
-static uint32_t WriteSpaceFirmware(const RollbackSpaceFirmware* rsf) {
-  return SafeWrite(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));
+uint32_t WriteSpaceFirmware(RollbackSpaceFirmware* rsf) {
+  RollbackSpaceFirmware rsf2;
+  uint32_t r;
+  int attempts = 3;
+
+  /* All writes should use struct_version 2 or greater. */
+  if (rsf->struct_version < 2)
+    rsf->struct_version = 2;
+  rsf->crc8 = Crc8(rsf, offsetof(RollbackSpaceFirmware, crc8));
+
+  while (attempts--) {
+    r = SafeWrite(FIRMWARE_NV_INDEX, rsf, sizeof(RollbackSpaceFirmware));
+    /* Can't write, not gonna try again */
+    if (r != TPM_SUCCESS)
+      return r;
+
+    /* Read it back to be sure it got the right values. */
+    r = ReadSpaceFirmware(&rsf2);    /* This checks the CRC */
+    if (r == TPM_SUCCESS)
+      return r;
+
+    VBDEBUG(("TPM: %s() - bad CRC\n", __func__));
+    /* Try writing it again. Maybe it was garbled on the way out. */
+  }
+
+  VBDEBUG(("TPM: %s() - too many bad CRCs, giving up\n", __func__));
+  return TPM_E_CORRUPTED_STATE;
 }
 
-#ifndef DISABLE_ROLLBACK_TPM
-static uint32_t ReadSpaceKernel(RollbackSpaceKernel* rsk) {
-  return TlclRead(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+uint32_t ReadSpaceKernel(RollbackSpaceKernel* rsk) {
+  uint32_t r;
+  int attempts = 3;
+
+  while (attempts--) {
+    r = TlclRead(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+    if (r != TPM_SUCCESS)
+      return r;
+
+    /* No CRC in this version, so we'll create one when we write it. Note that
+     * we're marking this as version 2, not ROLLBACK_SPACE_KERNEL_VERSION,
+     * because version 2 just added the CRC. Later versions will need to
+     * set default values for any extra fields explicitly (probably here). */
+    if (rsk->struct_version < 2) {
+      rsk->struct_version = 2;          /* Danger Will Robinson! Danger! */
+      return TPM_SUCCESS;
+    }
+
+    /* If the CRC is good, we're done. If it's bad, try a couple more times to
+     * see if it gets better before we give up. It could just be noise. */
+    if (rsk->crc8 == Crc8(rsk, offsetof(RollbackSpaceKernel, crc8)))
+      return TPM_SUCCESS;
+
+    VBDEBUG(("TPM: %s() - bad CRC\n", __func__));
+  }
+
+  VBDEBUG(("TPM: %s() - too many bad CRCs, giving up\n", __func__));
+  return TPM_E_CORRUPTED_STATE;
 }
-#endif
 
-static uint32_t WriteSpaceKernel(const RollbackSpaceKernel* rsk) {
-  return SafeWrite(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+uint32_t WriteSpaceKernel(RollbackSpaceKernel* rsk) {
+  RollbackSpaceKernel rsk2;
+  uint32_t r;
+  int attempts = 3;
+
+  /* All writes should use struct_version 2 or greater. */
+  if (rsk->struct_version < 2)
+    rsk->struct_version = 2;
+  rsk->crc8 = Crc8(rsk, offsetof(RollbackSpaceKernel, crc8));
+
+  while (attempts--) {
+    r = SafeWrite(KERNEL_NV_INDEX, rsk, sizeof(RollbackSpaceKernel));
+    /* Can't write, not gonna try again */
+    if (r != TPM_SUCCESS)
+      return r;
+
+    /* Read it back to be sure it got the right values. */
+    r = ReadSpaceKernel(&rsk2);    /* This checks the CRC */
+    if (r == TPM_SUCCESS)
+      return r;
+
+    VBDEBUG(("TPM: %s() - bad CRC\n", __func__));
+    /* Try writing it again. Maybe it was garbled on the way out. */
+  }
+
+  VBDEBUG(("TPM: %s() - too many bad CRCs, giving up\n", __func__));
+  return TPM_E_CORRUPTED_STATE;
 }
 
 
 uint32_t OneTimeInitializeTPM(RollbackSpaceFirmware* rsf,
                               RollbackSpaceKernel* rsk) {
   static const RollbackSpaceFirmware rsf_init = {
-    ROLLBACK_SPACE_FIRMWARE_VERSION, 0, 0, 0};
+    .struct_version = ROLLBACK_SPACE_FIRMWARE_VERSION,
+  };
   static const RollbackSpaceKernel rsk_init = {
-    ROLLBACK_SPACE_KERNEL_VERSION, ROLLBACK_SPACE_KERNEL_UID, 0, 0};
+    .struct_version = ROLLBACK_SPACE_KERNEL_VERSION,
+    .uid = ROLLBACK_SPACE_KERNEL_UID,
+  };
   TPM_PERMANENT_FLAGS pflags;
   uint32_t result;
 
diff --git a/firmware/lib/vboot_nvstorage.c b/firmware/lib/vboot_nvstorage.c
index e96a17fe..7361817b 100644
--- a/firmware/lib/vboot_nvstorage.c
+++ b/firmware/lib/vboot_nvstorage.c
@@ -6,6 +6,7 @@
 /* Non-volatile storage routines.
  */
 
+#include "crc8.h"
 #include "utility.h"
 #include "vboot_common.h"
 #include "vboot_nvstorage.h"
@@ -39,26 +40,6 @@
 #define CRC_OFFSET                  15
 
 
-/* Return CRC-8 of the data, using x^8 + x^2 + x + 1 polynomial.  A
- * table-based algorithm would be faster, but for only 15 bytes isn't
- * worth the code size. */
-static uint8_t Crc8(const uint8_t* data, int len) {
-  unsigned crc = 0;
-  int i, j;
-
-  for (j = len; j; j--, data++) {
-    crc ^= (*data << 8);
-    for(i = 8; i; i--) {
-      if (crc & 0x8000)
-        crc ^= (0x1070 << 3);
-      crc <<= 1;
-    }
-  }
-
-  return (uint8_t)(crc >> 8);
-}
-
-
 int VbNvSetup(VbNvContext* context) {
   uint8_t* raw = context->raw;
 
diff --git a/tests/rollback_index2_tests.c b/tests/rollback_index2_tests.c
index 7acb0989..1307ed08 100644
--- a/tests/rollback_index2_tests.c
+++ b/tests/rollback_index2_tests.c
@@ -11,6 +11,7 @@
 
 #define _STUB_IMPLEMENTATION_  /* So we can use memset() ourselves */
 
+#include "crc8.h"
 #include "rollback_index.h"
 #include "test_common.h"
 #include "tlcl.h"
@@ -38,6 +39,12 @@ static int mock_count = 0;
 static int fail_at_count = 0;
 static uint32_t fail_with_error = TPM_SUCCESS;
 
+/* Similar, to determine when to inject noise during reads & writes */
+#define MAX_NOISE_COUNT 64              /* no noise after this many */
+static int noise_count = 0;             /* read/write attempt (zero-based) */
+static int noise_on[MAX_NOISE_COUNT];   /* calls to inject noise on */
+
+
 /* Params / backing store for mocked Tlcl functions. */
 static TPM_PERMANENT_FLAGS mock_pflags;
 static RollbackSpaceFirmware mock_rsf;
@@ -51,6 +58,8 @@ static void ResetMocks(int fail_on_call, uint32_t fail_with_err) {
   mock_count = 0;
   fail_at_count = fail_on_call;
   fail_with_error = fail_with_err;
+  noise_count = 0;
+  Memset(&noise_on, 0, sizeof(noise_on));
 
   Memset(&mock_pflags, 0, sizeof(mock_pflags));
   Memset(&mock_rsf, 0, sizeof(mock_rsf));
@@ -59,6 +68,16 @@ static void ResetMocks(int fail_on_call, uint32_t fail_with_err) {
 }
 
 /****************************************************************************/
+/* Function to garble data on its way to or from the TPM */
+static void MaybeInjectNoise(void* data, uint32_t length) {
+  if (noise_count < MAX_NOISE_COUNT && noise_on[noise_count]) {
+    uint8_t *val = data;
+    val[length-1]++;
+  }
+  noise_count++;
+}
+
+/****************************************************************************/
 /* Mocks for tlcl functions which log the calls made to mock_calls[]. */
 
 uint32_t TlclLibInit(void) {
@@ -97,9 +116,11 @@ uint32_t TlclRead(uint32_t index, void* data, uint32_t length) {
   if (FIRMWARE_NV_INDEX == index) {
     TEST_EQ(length, sizeof(mock_rsf), "TlclRead rsf size");
     Memcpy(data, &mock_rsf, length);
+    MaybeInjectNoise(data, length);
   } else if (KERNEL_NV_INDEX == index) {
     TEST_EQ(length, sizeof(mock_rsk), "TlclRead rsk size");
     Memcpy(data, &mock_rsk, length);
+    MaybeInjectNoise(data, length);
   } else {
     Memset(data, 0, length);
   }
@@ -113,9 +134,11 @@ uint32_t TlclWrite(uint32_t index, const void* data, uint32_t length) {
   if (FIRMWARE_NV_INDEX == index) {
     TEST_EQ(length, sizeof(mock_rsf), "TlclWrite rsf size");
     Memcpy(&mock_rsf, data, length);
+    MaybeInjectNoise(&mock_rsf, length);
   } else if (KERNEL_NV_INDEX == index) {
     TEST_EQ(length, sizeof(mock_rsk), "TlclWrite rsk size");
     Memcpy(&mock_rsk, data, length);
+    MaybeInjectNoise(&mock_rsk, length);
   }
 
   return (++mock_count == fail_at_count) ? fail_with_error : TPM_SUCCESS;
@@ -183,6 +206,204 @@ uint32_t TlclGetPermissions(uint32_t index, uint32_t* permissions) {
   return (++mock_count == fail_at_count) ? fail_with_error : TPM_SUCCESS;
 }
 
+
+/****************************************************************************/
+/* Tests for CRC errors  */
+
+extern uint32_t ReadSpaceFirmware(RollbackSpaceFirmware* rsf);
+extern uint32_t WriteSpaceFirmware(RollbackSpaceFirmware* rsf);
+
+static void CrcTestFirmware(void) {
+  RollbackSpaceFirmware rsf;
+
+  /* noise on reading, shouldn't matter here because version == 0 */
+  ResetMocks(0, 0);
+  noise_on[0] = 1;
+  TEST_EQ(ReadSpaceFirmware(&rsf), 0, "ReadSpaceFirmware(), v0");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* But if the version >= 2, it will try three times and fail because the CRC
+   * is no good. */
+  ResetMocks(0, 0);
+  mock_rsf.struct_version = 2;
+  TEST_EQ(ReadSpaceFirmware(&rsf), TPM_E_CORRUPTED_STATE,
+          "ReadSpaceFirmware(), v2, bad CRC");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* OTOH, if the CRC is good and some noise happens, it should recover. */
+  ResetMocks(0, 0);
+  mock_rsf.struct_version = 2;
+  mock_rsf.crc8 = Crc8(&mock_rsf, offsetof(RollbackSpaceFirmware, crc8));
+  noise_on[0] = 1;
+  TEST_EQ(ReadSpaceFirmware(&rsf), 0, "ReadSpaceFirmware(), v2, good CRC");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* A write with version < 2 should convert to v2 and create the CRC */
+  ResetMocks(0, 0);
+  Memset(&rsf, 0, sizeof(rsf));
+  TEST_EQ(WriteSpaceFirmware(&rsf), 0, "WriteSpaceFirmware(), v0");
+  TEST_EQ(mock_rsf.struct_version, 2, "WriteSpaceFirmware(), check v2");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* Same as above, but with some noise during the readback */
+  ResetMocks(0, 0);
+  Memset(&rsf, 0, sizeof(rsf));
+  noise_on[1] = 1;
+  noise_on[2] = 1;
+  TEST_EQ(WriteSpaceFirmware(&rsf), 0, "WriteSpaceFirmware(), read noise");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* With noise during the write, we'll try the write again */
+  ResetMocks(0, 0);
+  Memset(&rsf, 0, sizeof(rsf));
+  noise_on[0] = 1;
+  TEST_EQ(WriteSpaceFirmware(&rsf), 0, "WriteSpaceFirmware(), write noise");
+  TEST_EQ(mock_rsf.struct_version, 2, "WriteSpaceFirmware(), check v2");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+
+  /* Only if it just keeps on failing forever do we eventually give up */
+  ResetMocks(0, 0);
+  Memset(&rsf, 0, sizeof(rsf));
+  Memset(noise_on, 1, sizeof(noise_on));
+  TEST_EQ(WriteSpaceFirmware(&rsf), TPM_E_CORRUPTED_STATE,
+          "WriteSpaceFirmware(), always noise");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
+              "tlcl calls");
+}
+
+extern uint32_t ReadSpaceKernel(RollbackSpaceKernel* rsk);
+extern uint32_t WriteSpaceKernel(RollbackSpaceKernel* rsk);
+
+static void CrcTestKernel(void) {
+  RollbackSpaceKernel rsk;
+
+  /* noise on reading, shouldn't matter here because version == 0 */
+  ResetMocks(0, 0);
+  noise_on[0] = 1;
+  TEST_EQ(ReadSpaceKernel(&rsk), 0, "ReadSpaceKernel(), v0");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* But if the version >= 2, it will try three times and fail because the CRC
+   * is no good. */
+  ResetMocks(0, 0);
+  mock_rsk.struct_version = 2;
+  TEST_EQ(ReadSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE,
+          "ReadSpaceKernel(), v2, bad CRC");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* OTOH, if the CRC is good and some noise happens, it should recover. */
+  ResetMocks(0, 0);
+  mock_rsk.struct_version = 2;
+  mock_rsk.crc8 = Crc8(&mock_rsk, offsetof(RollbackSpaceKernel, crc8));
+  noise_on[0] = 1;
+  TEST_EQ(ReadSpaceKernel(&rsk), 0, "ReadSpaceKernel(), v2, good CRC");
+  TEST_STR_EQ(mock_calls,
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* A write with version < 2 should convert to v2 and create the CRC */
+  ResetMocks(0, 0);
+  Memset(&rsk, 0, sizeof(rsk));
+  TEST_EQ(WriteSpaceKernel(&rsk), 0, "WriteSpaceKernel(), v0");
+  TEST_EQ(mock_rsk.struct_version, 2, "WriteSpaceKernel(), check v2");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* Same as above, but with some noise during the readback */
+  ResetMocks(0, 0);
+  Memset(&rsk, 0, sizeof(rsk));
+  noise_on[1] = 1;
+  noise_on[2] = 1;
+  TEST_EQ(WriteSpaceKernel(&rsk), 0, "WriteSpaceKernel(), read noise");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* With noise during the write, we'll try the write again */
+  ResetMocks(0, 0);
+  Memset(&rsk, 0, sizeof(rsk));
+  noise_on[0] = 1;
+  TEST_EQ(WriteSpaceKernel(&rsk), 0, "WriteSpaceKernel(), write noise");
+  TEST_EQ(mock_rsk.struct_version, 2, "WriteSpaceKernel(), check v2");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+
+  /* Only if it just keeps on failing forever do we eventually give up */
+  ResetMocks(0, 0);
+  Memset(&rsk, 0, sizeof(rsk));
+  Memset(noise_on, 1, sizeof(noise_on));
+  TEST_EQ(WriteSpaceKernel(&rsk), TPM_E_CORRUPTED_STATE,
+          "WriteSpaceKernel(), always noise");
+  TEST_STR_EQ(mock_calls,
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
+              "tlcl calls");
+}
+
 /****************************************************************************/
 /* Tests for misc helper functions */
 
@@ -265,9 +486,11 @@ static void OneTimeInitTest(void) {
               /* kernel space */
               "TlclDefineSpace(0x1008, 0x1, 13)\n"
               "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
               /* firmware space */
               "TlclDefineSpace(0x1007, 0x8001, 10)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
   TEST_EQ(mock_rsf.struct_version, ROLLBACK_SPACE_FIRMWARE_VERSION, "rsf ver");
   TEST_EQ(mock_rsf.flags, 0, "rsf flags");
@@ -290,9 +513,11 @@ static void OneTimeInitTest(void) {
               /* kernel space */
               "TlclDefineSpace(0x1008, 0x1, 13)\n"
               "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
               /* firmware space */
               "TlclDefineSpace(0x1007, 0x8001, 10)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
 
   /* NV locking already initialized */
@@ -309,9 +534,11 @@ static void OneTimeInitTest(void) {
               /* kernel space */
               "TlclDefineSpace(0x1008, 0x1, 13)\n"
               "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
               /* firmware space */
               "TlclDefineSpace(0x1007, 0x8001, 10)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
 
   /* Self test error */
@@ -397,8 +624,10 @@ static void SetupTpmTest(void) {
               "TlclSetDeactivated(0)\n"
               "TlclDefineSpace(0x1008, 0x1, 13)\n"
               "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n"
               "TlclDefineSpace(0x1007, 0x8001, 10)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
 
   /* Other firmware space error is passed through */
@@ -425,7 +654,8 @@ static void SetupTpmTest(void) {
               "TlclForceClear()\n"
               "TlclSetEnable()\n"
               "TlclSetDeactivated(0)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
   TEST_EQ(mock_rsf.flags, FLAG_LAST_BOOT_DEVELOPER, "fw space flags to dev");
 
@@ -441,7 +671,8 @@ static void SetupTpmTest(void) {
               "TlclForceClear()\n"
               "TlclSetEnable()\n"
               "TlclSetDeactivated(0)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
   TEST_EQ(mock_rsf.flags, 0, "fw space flags from dev");
 
@@ -492,7 +723,8 @@ static void RollbackFirmwareTest(void) {
               "TlclForceClear()\n"
               "TlclSetEnable()\n"
               "TlclSetDeactivated(0)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
   TEST_EQ(mock_rsf.flags, FLAG_LAST_BOOT_DEVELOPER, "fw space flags to dev");
 
@@ -502,7 +734,8 @@ static void RollbackFirmwareTest(void) {
   TEST_EQ(mock_rsf.fw_versions, 0xBEAD1234, "RollbackFirmwareWrite() version");
   TEST_STR_EQ(mock_calls,
               "TlclRead(0x1007, 10)\n"
-              "TlclWrite(0x1007, 10)\n",
+              "TlclWrite(0x1007, 10)\n"
+              "TlclRead(0x1007, 10)\n",
               "tlcl calls");
 
   ResetMocks(1, TPM_E_IOERROR);
@@ -573,7 +806,8 @@ static void RollbackKernelTest(void) {
           "RollbackKernelWrite() version");
   TEST_STR_EQ(mock_calls,
               "TlclRead(0x1008, 13)\n"
-              "TlclWrite(0x1008, 13)\n",
+              "TlclWrite(0x1008, 13)\n"
+              "TlclRead(0x1008, 13)\n",
               "tlcl calls");
 
   ResetMocks(1, TPM_E_IOERROR);
@@ -622,6 +856,8 @@ __pragma(warning (disable: 4100))
 int main(int argc, char* argv[]) {
   int error_code = 0;
 
+  CrcTestFirmware();
+  CrcTestKernel();
   MiscTest();
   OneTimeInitTest();
   SetupTpmTest();
diff --git a/tests/run_vboot_ec_tests.sh b/tests/run_vboot_ec_tests.sh
index b5b0e7b1..302a3578 100755
--- a/tests/run_vboot_ec_tests.sh
+++ b/tests/run_vboot_ec_tests.sh
@@ -12,7 +12,7 @@
 check_test_keys
 
 for priv in ${TESTKEY_DIR}/*.vbprivk; do
-  root=$(basename ${i%.vbprivk})
+  root=$(basename ${priv%.vbprivk})
   pub="${priv%.vbprivk}.vbpubk"
   echo "Trying $root ..."
   ${TEST_DIR}/vboot_ec_tests "$priv" "$pub"