Debug new ks_flash code.

3 files changed, 54 insertions, 24 deletions

diff --git a/hal.h b/hal.h
index 55397a4..4e69981 100644
--- a/hal.h
+++ b/hal.h
@@ -145,6 +145,7 @@
   DEFINE_HAL_ERROR(HAL_ERROR_MASTERKEY_BAD_LENGTH,      "Master key of unacceptable length")            \
   DEFINE_HAL_ERROR(HAL_ERROR_KS_DRIVER_NOT_FOUND,       "Keystore driver not found")                    \
   DEFINE_HAL_ERROR(HAL_ERROR_KEYSTORE_BAD_CRC,          "Bad CRC in keystore")                          \
+  DEFINE_HAL_ERROR(HAL_ERROR_KEYSTORE_BAD_BLOCK_TYPE,   "Unsupported keystore block type")              \
   END_OF_HAL_ERROR_LIST
 
 /* Marker to forestall silly line continuation errors */
diff --git a/hal_internal.h b/hal_internal.h
index e779168..ade908f 100644
--- a/hal_internal.h
+++ b/hal_internal.h
@@ -306,9 +306,16 @@ static inline hal_crc32_t hal_crc32_finalize(hal_crc32_t crc)
  *
  * Plus we need a bit of AES-keywrap overhead, since we're storing the
  * wrapped form (see hal_aes_keywrap_cyphertext_length()).
+ *
+ * A buffer big enough for a 8192-bit RSA key would overflow one
+ * sub-sector on the flash chip we're using on the Alpha.  We could
+ * invent some more complex scheme where key blocks are allowed to
+ * span multiple sub-sectors, but since an 8192-bit RSA key would also
+ * be unusably slow with the current RSA implementation, for the
+ * moment we take the easy way out and cap this at 4096-bit RSA.
  */
 
-#define HAL_KS_WRAPPED_KEYSIZE  ((4655 + 15) & ~7)
+#define HAL_KS_WRAPPED_KEYSIZE  ((2351 + 15) & ~7)
 
 /*
  * PINs.
diff --git a/ks_flash.c b/ks_flash.c
index 37c2563..1479ce7 100644
--- a/ks_flash.c
+++ b/ks_flash.c
@@ -307,7 +307,7 @@ static hal_crc32_t calculate_block_crc(const flash_block_t * const block)
 
   crc = hal_crc32_update(crc,
                          block->bytes + sizeof(flash_block_header_t),
-                         sizeof(block) - sizeof(flash_block_header_t));
+                         sizeof(*block) - sizeof(flash_block_header_t));
 
   return hal_crc32_finalize(crc);
 }
@@ -340,25 +340,22 @@ static hal_error_t block_read(const unsigned blockno, flash_block_t *block)
   assert(block != NULL && blockno < NUM_FLASH_BLOCKS && sizeof(*block) == KEYSTORE_SUBSECTOR_SIZE);
 
   /* Sigh, magic numeric return codes */
-  if (keystore_read_data(block_offset(blockno), block->bytes, sizeof(block)) != 1)
+  if (keystore_read_data(block_offset(blockno), block->bytes, sizeof(*block)) != 1)
     return HAL_ERROR_KEYSTORE_ACCESS;
 
   switch (block_get_type(block)) {
   case FLASH_KEYBLK:
   case FLASH_PINBLK:
-    if (calculate_block_crc(block) != block->header.crc1)
-      return HAL_ERROR_KEYSTORE_BAD_CRC;
-    break;
+    return calculate_block_crc(block) == block->header.crc1 ? HAL_OK : HAL_ERROR_KEYSTORE_BAD_CRC;
   case FLASH_KEYOLD:
   case FLASH_PINOLD:
-    if (calculate_block_crc(block) != block->header.crc2)
-      return HAL_ERROR_KEYSTORE_BAD_CRC;
-    break;
+    return calculate_block_crc(block) == block->header.crc2 ? HAL_OK : HAL_ERROR_KEYSTORE_BAD_CRC;
+  case FLASH_ERASED:
+  case FLASH_ZEROED:
+    return HAL_OK;
   default:
-    break;
+    return HAL_ERROR_KEYSTORE_BAD_BLOCK_TYPE;
   }
-
-  return HAL_OK;
 }
 
 /*
@@ -407,7 +404,7 @@ static hal_error_t block_write(const unsigned blockno, flash_block_t *block)
   }
 
   /* Sigh, magic numeric return codes */
-  if (keystore_write_data(block_offset(blockno), block->bytes, sizeof(block)) != 1)
+  if (keystore_write_data(block_offset(blockno), block->bytes, sizeof(*block)) != 1)
     return HAL_ERROR_KEYSTORE_ACCESS;
 
   return HAL_OK;
@@ -465,7 +462,13 @@ static hal_error_t block_erase_maybe(const unsigned blockno)
   if (block == NULL)
     return HAL_ERROR_IMPOSSIBLE;
 
-  if ((err = block_read(blockno, block)) != HAL_OK)
+  err = block_read(blockno, block);
+
+  if (err == HAL_ERROR_KEYSTORE_BAD_BLOCK_TYPE ||
+      err == HAL_ERROR_KEYSTORE_BAD_CRC)
+    return block_erase(blockno);
+
+  if (err != HAL_OK)
     return err;
 
   for (int i = 0; i < sizeof(*block); i++)
@@ -516,13 +519,15 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver)
   for (int i = 0; i < NUM_FLASH_BLOCKS; i++) {
 
     /*
-     * Read one block.  If the CRC is bad, it's old data we don't
-     * understand, something we were writing when we crashed, or bad
-     * flash; in any of these cases, we want the block to ends up near
-     * the end of the free list.
+     * Read one block.  If the CRC is bad or the block type is
+     * unknown, it's old data we don't understand, something we were
+     * writing when we crashed, or bad flash; in any of these cases,
+     * we want the block to ends up near the end of the free list.
      */
 
-    if ((err = block_read(i, block)) == HAL_ERROR_KEYSTORE_BAD_CRC)
+    err = block_read(i, block);
+
+    if (err == HAL_ERROR_KEYSTORE_BAD_CRC || err == HAL_ERROR_KEYSTORE_BAD_BLOCK_TYPE)
       block_types[i] = FLASH_UNKNOWN;
 
     else if (err == HAL_OK)
@@ -1070,12 +1075,27 @@ hal_error_t hal_set_pin(const hal_user_t user,
 
 #if HAL_MKM_FLASH_BACKUP_KLUDGE
 
+/*
+ * Horrible insecure kludge in lieu of a battery for the MKM.
+ *
+ * API here is a little strange:
+ *
+ * - NULL buffer on read means do all the work without returning the
+ *   value;
+ *
+ * - All calls pass a length parameter, but any length other than the
+ *   compiled in constant just returns an immediate error, there's no
+ *   notion of buffer max length vs buffer used length, querying for
+ *   the size of buffer really needed, or anything like that.
+ *
+ * We might want to rewrite this some day, if we don't replace it with
+ * a battery first.  For now we just preserve the API as we found it
+ * while re-implementing it on top of the new keystore.
+ */
+
 hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len)
 {
-  if (buf == NULL)
-    return HAL_ERROR_BAD_ARGUMENTS;
-
-  if (len != KEK_LENGTH)
+  if (buf != NULL && len != KEK_LENGTH)
     return HAL_ERROR_MASTERKEY_BAD_LENGTH;
 
   flash_block_t *block;
@@ -1088,7 +1108,9 @@ hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len)
   if (block->pin.kek_set != FLASH_KEK_SET)
     return HAL_ERROR_MASTERKEY_NOT_SET;
 
-  memcpy(buf, block->pin.kek, len);
+  if (buf != NULL)
+    memcpy(buf, block->pin.kek, len);
+
   return HAL_OK;
 }