VbNvStorage cleanup and comments

BUG=12282
TEST=make && make runtests

Review URL: http://codereview.chromium.org/6469059

Change-Id: I912b53ae33d65305353a747cc0bdd2b1ea62a04f

6 files changed, 119 insertions, 60 deletions

diff --git a/firmware/include/load_firmware_fw.h b/firmware/include/load_firmware_fw.h
index e3777660..76fa9904 100644
--- a/firmware/include/load_firmware_fw.h
+++ b/firmware/include/load_firmware_fw.h
@@ -10,6 +10,7 @@
 #define VBOOT_REFERENCE_LOAD_FIRMWARE_FW_H_
 
 #include "sysincludes.h"
+#include "vboot_nvstorage.h"
 
 /* Recommended size of kernel_sign_key_blob in bytes, for
  * implementations which must preallocate a transfer buffer between
@@ -42,6 +43,12 @@ typedef struct LoadFirmwareParams {
                                   * will contain the actual key blob
                                   * size placed into the buffer. */
   uint64_t boot_flags;           /* Boot flags */
+  VbNvContext* nv_context;       /* Context for NV storage.  nv_context->raw
+                                  * must be filled before calling
+                                  * LoadFirmware().  On output, check
+                                  * nv_context->raw_changed to see if
+                                  * nv_context->raw has been modified and
+                                  * needs saving. */
 
   /* Outputs from LoadFirmware(); valid only if LoadFirmware() returns
    * LOAD_FIRMWARE_SUCCESS. */
diff --git a/firmware/include/load_kernel_fw.h b/firmware/include/load_kernel_fw.h
index 16dade1f..191ebe2a 100644
--- a/firmware/include/load_kernel_fw.h
+++ b/firmware/include/load_kernel_fw.h
@@ -10,6 +10,7 @@
 #define VBOOT_REFERENCE_LOAD_KERNEL_FW_H_
 
 #include "sysincludes.h"
+#include "vboot_nvstorage.h"
 
 /* Interface provided by verified boot library to BDS */
 
@@ -33,14 +34,20 @@
 
 typedef struct LoadKernelParams {
   /* Inputs to LoadKernel() */
-  void *header_sign_key_blob;   /* Key blob used to sign the kernel header */
+  void* header_sign_key_blob;   /* Key blob used to sign the kernel header */
   uint64_t bytes_per_lba;       /* Bytes per lba sector on current device */
   uint64_t ending_lba;          /* Last addressable lba sector on current
                                  * device */
-  void *kernel_buffer;          /* Destination buffer for kernel
+  void* kernel_buffer;          /* Destination buffer for kernel
                                  * (normally at 0x100000) */
   uint64_t kernel_buffer_size;  /* Size of kernel buffer in bytes */
   uint64_t boot_flags;          /* Boot flags */
+  VbNvContext* nv_context;       /* Context for NV storage.  nv_context->raw
+                                  * must be filled before calling
+                                  * LoadKernel().  On output, check
+                                  * nv_context->raw_changed to see if
+                                  * nv_context->raw has been modified and
+                                  * needs saving. */
 
   /* Outputs from LoadKernel(); valid only if LoadKernel() returns
    * LOAD_KERNEL_SUCCESS */
diff --git a/firmware/include/vboot_nvstorage.h b/firmware/include/vboot_nvstorage.h
index cd4f89dd..31bc539e 100644
--- a/firmware/include/vboot_nvstorage.h
+++ b/firmware/include/vboot_nvstorage.h
@@ -3,17 +3,17 @@
  * found in the LICENSE file.
  */
 
-/* Non-volatile storage routines.
+/* Non-volatile storage routines for verified boot.
  */
 
 #ifndef VBOOT_REFERENCE_NVSTORAGE_H_
 #define VBOOT_REFERENCE_NVSTORAGE_H_
 
-#define NV_BLOCK_SIZE 16  /* Size of NV storage block in bytes */
+#define VBNV_BLOCK_SIZE 16  /* Size of NV storage block in bytes */
 
 typedef struct VbNvContext {
   /* Raw NV data.  Caller must fill this before calling VbNvSetup(). */
-  uint8_t raw[NV_BLOCK_SIZE];
+  uint8_t raw[VBNV_BLOCK_SIZE];
   /* Flag indicating whether raw data has changed.  Set by VbNvTeardown() if
    * the raw data has changed and needs to be stored to the underlying
    * non-volatile data store. */
@@ -28,45 +28,109 @@ typedef struct VbNvContext {
 
 /* Parameter type for VbNvGet(), VbNvSet(). */
 typedef enum VbNvParam {
+  /* Parameter values have been reset to defaults (flag for firmware).
+   * 0=clear; 1=set. */
   VBNV_FIRMWARE_SETTINGS_RESET = 0,
+  /* Parameter values have been reset to defaults (flag for kernel).
+   * 0=clear; 1=set. */
   VBNV_KERNEL_SETTINGS_RESET,
+  /* Request debug reset on next S3->S0 transition.  0=clear; 1=set. */
   VBNV_DEBUG_RESET_MODE,
+  /* Number of times to try booting RW firmware slot B before slot A.
+   * Valid range: 0-15. */
   VBNV_TRY_B_COUNT,
+  /* Request recovery mode on next boot; see VBNB_RECOVERY_* below for
+   * currently defined reason codes.  8-bit value. */
   VBNV_RECOVERY_REQUEST,
+  /* Localization index for screen bitmaps displayed by firmware.
+   * 8-bit value. */
   VBNV_LOCALIZATION_INDEX,
+  /* Field reserved for kernel/user-mode use; 32-bit value. */
   VBNV_KERNEL_FIELD,
 } VbNvParam;
 
 
+/* Recovery reason codes for VBNV_RECOVERY_REQUEST */
+/* Recovery not requested. */
+#define VBNV_RECOVERY_NOT_REQUESTED   0x00
+/* Recovery requested from legacy utility.  (Prior to the NV storage
+ * spec, recovery mode was a single bitfield; this value is reserved
+ * so that scripts which wrote 1 to the recovery field are
+ * distinguishable from scripts whch use the recovery reasons listed
+ * here. */
+#define VBNV_RECOVERY_LEGACY          0x01
+/* User manually requested recovery via recovery button */
+#define VBNV_RECOVERY_RO_MANUAL       0x02
+/* RW firmware failed signature check (neither RW firmware slot was valid) */
+#define VBNV_RECOVERY_RO_INVALID_RW   0x03
+/* S3 resume failed */
+#define VBNV_RECOVERY_RO_S3_RESUME    0x04
+/* TPM error in read-only firmware */
+#define VBNV_RECOVERY_RO_TPM_ERROR    0x05
+/* Unspecified/unknown error in read-only firmware */
+#define VBNV_RECOVERY_RO_UNSPECIFIED  0x3F
+/* User manually requested recovery by pressing a key at developer
+ * warning screen */
+#define VBNV_RECOVERY_RW_DEV_SCREEN   0x41
+/* No OS kernel detected */
+#define VBNV_RECOVERY_RW_NO_OS        0x42
+/* OS kernel failed signature check */
+#define VBNV_RECOVERY_RW_INVALID_OS   0x43
+/* TPM error in rewritable firmware */
+#define VBNV_RECOVERY_RW_TPM_ERROR    0x44
+/* Unspecified/unknown error in rewritable firmware */
+#define VBNV_RECOVERY_RW_UNSPECIFIED  0x7F
+/* DM-verity error */
+#define VBNV_RECOVERY_KE_DM_VERITY    0x81
+/* Unspecified/unknown error in kernel */
+#define VBNV_RECOVERY_KE_UNSPECIFIED  0xBF
+/* Recovery mode test from user-mode */
+#define VBNV_RECOVERY_US_TEST         0xC1
+/* Unspecified/unknown error in user-mode */
+#define VBNV_RECOVERY_US_UNSPECIFIED  0xFF
+
+
 /* Initialize the NV storage library.  This must be called before any
- * other functions in this library. Returns 0 if success, non-zero if
+ * other functions in this library.  Returns 0 if success, non-zero if
  * error.
  *
- * If you have access to global variables, you may want to wrap this
- * in your own VbNvOpen() function which allocates a context, acquires
- * a lock to prevent race conditions accessing the underlying storage,
- * reads the raw data from underlying storage, and calls VbNvSetup().
- * We don't do that in here because there are no global variables in
- * UEFI BIOS during PEI phase. */
+ * Proper calling procedure:
+ *    1) Allocate a context struct.
+ *    2) If multi-threaded/multi-process, acquire a lock to prevent
+ *       other processes from modifying the underlying storage.
+ *    3) Read underlying storage and fill in context->raw.
+ *    4) Call VbNvSetup().
+ *
+ * If you have access to global variables, you may want to wrap all
+ * that in your own VbNvOpen() function.  We don't do that in here
+ * because there are no global variables in UEFI BIOS during the PEI
+ * phase (that's also why we have to pass around a context pointer). */
 int VbNvSetup(VbNvContext* context);
 
 /* Clean up and flush changes back to the raw data.  This must be
- * called after other functions in this library.  Caller must check
- * context.raw_changed after calling this function.  Returns 0 if
+ * called after other functions in this library.  Returns 0 if
  * success, non-zero if error.
  *
+ * Proper calling procedure:
+ *    1) Call VbNvExit().
+ *    2) If context.raw_changed, write data back to underlying storage.
+ *    3) Release any lock you acquired before calling VbNvSetup().
+ *    4) Free the context struct.
+ *
  * If you have access to global variables, you may want to wrap this
- * in your own VbNvClose() function which calls VbNvTeardown(), writes
- * the underlying storage if context.raw_changed, releases the lock
- * acquired in VbNvOpen, and frees the context. */
+ * in your own VbNvClose() function. */
 int VbNvTeardown(VbNvContext* context);
 
 /* Read a NV storage parameter into *dest.  Returns 0 if success,
- * non-zero if error. */
+ * non-zero if error.
+ *
+ * This may only be called between VbNvSetup() and VbNvTeardown(). */
 int VbNvGet(VbNvContext* context, VbNvParam param, uint32_t* dest);
 
 /* Set a NV storage param to a new value.  Returns 0 if success,
- * non-zero if error. */
+ * non-zero if error.
+ *
+ * This may only be called between VbNvSetup() and VbNvTeardown(). */
 int VbNvSet(VbNvContext* context, VbNvParam param, uint32_t value);
 
 
diff --git a/firmware/lib/vboot_nvstorage.c b/firmware/lib/vboot_nvstorage.c
index 81816ac4..6e8c1c79 100644
--- a/firmware/lib/vboot_nvstorage.c
+++ b/firmware/lib/vboot_nvstorage.c
@@ -61,7 +61,7 @@ int VbNvSetup(VbNvContext* context) {
       || (Crc8(raw, CRC_OFFSET) != raw[CRC_OFFSET])) {
 
     /* Data is inconsistent (bad CRC or header), so reset defaults */
-    Memset(raw, 0, NV_BLOCK_SIZE);
+    Memset(raw, 0, VBNV_BLOCK_SIZE);
     raw[HEADER_OFFSET] = (HEADER_SIGNATURE | HEADER_FIRMWARE_SETTINGS_RESET |
                           HEADER_KERNEL_SETTINGS_RESET);
 
@@ -153,19 +153,30 @@ int VbNvSet(VbNvContext* context, VbNvParam param, uint32_t value) {
       if (value)
         raw[BOOT_OFFSET] |= BOOT_DEBUG_RESET_MODE;
       else
-        raw[BOOT_OFFSET] &= BOOT_DEBUG_RESET_MODE;
+        raw[BOOT_OFFSET] &= ~BOOT_DEBUG_RESET_MODE;
       break;
 
     case VBNV_TRY_B_COUNT:
+      /* Clip to valid range. */
+      if (value > BOOT_TRY_B_COUNT)
+        value = BOOT_TRY_B_COUNT - 1;
+
       raw[BOOT_OFFSET] &= ~BOOT_TRY_B_COUNT;
-      raw[BOOT_OFFSET] |= (uint8_t)(value & BOOT_TRY_B_COUNT);
+      raw[BOOT_OFFSET] |= (uint8_t)value;
       break;
 
     case VBNV_RECOVERY_REQUEST:
+      /* Map values outside the valid range to the legacy reason, since we
+       * can't determine if we're called from kernel or user mode. */
+      if (value > 0xFF)
+        value = VBNV_RECOVERY_LEGACY;
       raw[RECOVERY_OFFSET] = (uint8_t)value;
       break;
 
     case VBNV_LOCALIZATION_INDEX:
+      /* Map values outside the valid range to the default index. */
+      if (value > 0xFF)
+        value = 0;
       raw[LOCALIZATION_OFFSET] = (uint8_t)value;
       break;
 
diff --git a/firmware/linktest/main.c b/firmware/linktest/main.c
index fbf4a2f5..b171f195 100644
--- a/firmware/linktest/main.c
+++ b/firmware/linktest/main.c
@@ -10,6 +10,7 @@
 #include "tlcl.h"
 #include "vboot_common.h"
 #include "vboot_kernel.h"
+#include "vboot_nvstorage.h"
 
 
 int main(void)
@@ -79,5 +80,10 @@ int main(void)
   VerifyFirmwarePreamble(0, 0, 0);
   VerifyKernelPreamble(0, 0, 0);
 
+  VbNvSetup(0);
+  VbNvGet(0, 0, 0);
+  VbNvSet(0, 0, 0);
+  VbNvTeardown(0);
+
   return 0;
 }
diff --git a/tests/vboot_nvstorage_test.c b/tests/vboot_nvstorage_test.c
index 8f62317b..d5807c87 100644
--- a/tests/vboot_nvstorage_test.c
+++ b/tests/vboot_nvstorage_test.c
@@ -13,44 +13,8 @@
 #include "vboot_common.h"
 #include "vboot_nvstorage.h"
 
-#if 0
 
-/* Initialize the NV storage library.  This must be called before any
- * other functions in this library. Returns 0 if success, non-zero if
- * error. */
-int VbNvSetup(VbNvContext* context);
-
-/* Clean up and flush changes back to the raw data.  This must be
- * called after other functions in this library.  Caller must check
- * context.raw_changed after calling tis function.  Returns 0 if
- * success, non-zero if error. */
-int VbNvTeardown(VbNvContext* context);
-
-/* Read a NV storage parameter into *dest.  Returns 0 if success,
- * non-zero if error. */
-int VbNvGet(VbNvContext* context, VbNvParam param, uint32_t* dest);
-
-/* Set a NV storage param to a new value.  Returns 0 if success,
- * non-zero if error. */
-int VbNvSet(VbNvContext* context, VbNvParam param, uint32_t value);
-
-typedef struct VbNvContext {
-  /* Raw NV data.  Caller must fill this before calling VbNvStart(). */
-  uint8_t raw[NV_BLOCK_SIZE];
-  /* Flag indicating whether raw data has changed.  Set by VbNvStop() if
-   * the raw data has changed and needs to be stored to the underlying
-   * non-volatile data store. */
-  int raw_changed;
-
-  /* Internal data for NV storage routines.  Caller should not touch
-   * these fields. */
-  int regenerate_crc;
-
-} VbNvContext;
-
-#endif
-
-static void VbStorageTest(void) {
+static void VbNvStorageTest(void) {
 
   VbNvContext c;
   uint8_t goodcrc;
@@ -201,7 +165,7 @@ __pragma(warning (disable: 4100))
 int main(int argc, char* argv[]) {
   int error_code = 0;
 
-  VbStorageTest();
+  VbNvStorageTest();
 
   if (!gTestSuccess)
     error_code = 255;