Add some comments for things I figured out while reviewing code.

4 files changed, 123 insertions, 14 deletions

diff --git a/ks_attribute.c b/ks_attribute.c
index 92e450d..ec674f5 100644
--- a/ks_attribute.c
+++ b/ks_attribute.c
@@ -120,11 +120,18 @@ hal_error_t hal_ks_attribute_delete(uint8_t *bytes, const size_t bytes_len,
   if (bytes == NULL || attributes == NULL || attributes_len == NULL || total_len == NULL)
     return HAL_ERROR_BAD_ARGUMENTS;
 
+  /*
+   * Search for attribute by type. Note that there can be only one
+   * attribute of any given type.
+   */
+
   int i = 0;
 
   while (i < *attributes_len && attributes[i].type != type)
     i++;
 
+  /* If not found, great, it's already deleted from the key. */
+
   if (i == *attributes_len)
     return HAL_OK;
 
@@ -152,6 +159,8 @@ hal_error_t hal_ks_attribute_insert(uint8_t *bytes, const size_t bytes_len,
       total_len == NULL || value == NULL)
     return HAL_ERROR_BAD_ARGUMENTS;
 
+  /* Delete the existing attribute value (if present), then write the new value. */
+
   hal_error_t err
     = hal_ks_attribute_delete(bytes, bytes_len, attributes, attributes_len, total_len, type);
 
diff --git a/ks_flash.c b/ks_flash.c
index c21d668..b9397f8 100644
--- a/ks_flash.c
+++ b/ks_flash.c
@@ -33,6 +33,15 @@
  * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+/*
+ * This keystore driver operates over bare flash, versus over a flash file
+ * system or flash translation layer. The block size is large enough to
+ * hold an AES-keywrapped 4096-bit RSA key. Any remaining space in the key
+ * block may be used to store attributes (opaque TLV blobs). If the
+ * attributes overflow the key block, additional blocks may be added, but
+ * no attribute may exceed the block size.
+ */
+
 #include <stddef.h>
 #include <string.h>
 #include <assert.h>
@@ -312,7 +321,7 @@ static hal_crc32_t calculate_block_crc(const flash_block_t * const block)
 }
 
 /*
- * Calculate block offset.
+ * Calculate offset of the block in the flash address space.
  */
 
 static inline uint32_t block_offset(const unsigned blockno)
@@ -537,6 +546,11 @@ static hal_error_t block_update(const unsigned b1, flash_block_t *block,
 
   cache_mark_used(block, b2);
 
+  /*
+   * Erase the first block in the free list. In case of restart, this
+   * puts the block back at the head of the free list.
+   */
+
   return block_erase_maybe(db.ksi.index[db.ksi.used]);
 }
 
@@ -579,6 +593,12 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
                   sizeof(*db.ksi.names) * NUM_FLASH_BLOCKS +
                   sizeof(*db.cache)     * KS_FLASH_CACHE_SIZE);
 
+    /*
+     * This is done as a single large allocation, rather than 3 smaller
+     * allocations, to make it atomic - we need all 3, so either all
+     * succeed or all fail.
+     */
+
     uint8_t *mem = hal_allocate_static_memory(len);
 
     if (mem == NULL) {
@@ -634,7 +654,7 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
      * 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.
+     * we want the block to end up near the end of the free list.
      */
 
     err = block_read(i, block);
@@ -742,20 +762,22 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
    *
    * For any tombstone we find, we start by looking for all the blocks
    * with a matching UUID, then see what valid sequences we can
-   * construct from what we found.
+   * construct from what we found. This basically works in reverse of
+   * the update sequence in ks_set_attributes().
    *
    * If we can construct a valid sequence of live blocks, the complete
-   * update was written out, and we just need to zero the tombstones.
+   * update was written out, and we just need to finish zeroing the
+   * tombstones.
    *
    * Otherwise, if we can construct a complete sequence of tombstone
    * blocks, the update failed before it was completely written, so we
    * have to zero the incomplete sequence of live blocks then restore
-   * from the tombstones.
+   * the tombstones.
    *
    * Otherwise, if the live and tombstone blocks taken together form a
    * valid sequence, the update failed while deprecating the old live
-   * blocks, and the update itself was not written, so we need to
-   * restore the tombstones and leave the live blocks alone.
+   * blocks, and none of the new data was written, so we need to restore
+   * the tombstones and leave the live blocks alone.
    *
    * If none of the above applies, we don't understand what happened,
    * which is a symptom of either a bug or a hardware failure more
@@ -775,11 +797,25 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
     if ((err = hal_ks_index_find_range(&db.ksi, &name, 0, &n_blocks, NULL, &where, 0)) != HAL_OK)
       goto done;
 
+    /*
+     * hal_ks_index_find_range does a binary search, not a linear search,
+     * so it may not return the first instance of a block with the given
+     * name and chunk=0. Search backwards to make sure we have all chunks.
+     */
+
     while (where > 0 && !hal_uuid_cmp(&name, &db.ksi.names[db.ksi.index[where - 1]].name)) {
       where--;
       n_blocks++;
     }
 
+    /*
+     * Rather than calling hal_ks_index_find_range with an array pointer
+     * to get the list of matching blocks (because of the binary search
+     * issue), we're going to fondle the index directly. This is really
+     * not something to do in regular code, but this is error-recovery
+     * code.
+     */
+
     int live_ok = 1, tomb_ok = 1, join_ok = 1;
     unsigned n_live = 0, n_tomb = 0;
     unsigned i_live = 0, i_tomb = 0;
@@ -823,6 +859,12 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
       goto done;
     }
 
+    /*
+     * If live_ok or tomb_ok, we have to zero out some blocks, and adjust
+     * the index. Again, don't fondle the index directly, outside of error
+     * recovery.
+     */
+
     if (live_ok) {
       for (int j = 0; j < n_tomb; j++) {
         const unsigned b = tomb_blocks[j];
@@ -861,6 +903,10 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
       n_blocks = n_tomb;
     }
 
+    /*
+     * Restore tombstone blocks (tomb_ok or join_ok).
+     */
+
     for (int j = 0; j < n_blocks; j++) {
       int hint = where + j;
       unsigned b1 = db.ksi.index[hint], b2;
@@ -878,6 +924,10 @@ static hal_error_t ks_init(const hal_ks_driver_t * const driver, const int alloc
     }
   }
 
+  /*
+   * Fetch or create the PIN block.
+   */
+
   err = fetch_pin_block(NULL, &block);
 
   if (err == HAL_OK) {
@@ -1112,9 +1162,17 @@ static hal_error_t ks_delete(hal_ks_t *ks,
   hal_ks_lock();
 
   {
+    /*
+     * Get the count of blocks to delete.
+     */
+
     if ((err = hal_ks_index_delete_range(&db.ksi, &slot->name, 0, &n, NULL, &slot->hint)) != HAL_OK)
       goto done;
 
+    /*
+     * Then delete them.
+     */
+
     unsigned b[n];
 
     if ((err = hal_ks_index_delete_range(&db.ksi, &slot->name, n, NULL, b, &slot->hint)) != HAL_OK)
@@ -1123,10 +1181,19 @@ static hal_error_t ks_delete(hal_ks_t *ks,
     for (int i = 0; i < n; i++)
       cache_release(cache_find_block(b[i]));
 
+    /*
+     * Zero the blocks, to mark them as recently used.
+     */
+
     for (int i = 0; i < n; i++)
       if ((err = block_zero(b[i])) != HAL_OK)
         goto done;
 
+    /*
+     * Erase the first block in the free list. In case of restart, this
+     * puts the block back at the head of the free list.
+     */
+
     err = block_erase_maybe(db.ksi.index[db.ksi.used]);
   }
 
@@ -1455,6 +1522,8 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
 
     /*
      * Phase 0.2: Merge new attributes into updated_attributes[].
+     * For each new attribute type, mark any existing attributes of that
+     * type for deletion. Append new attributes to updated_attributes[].
      */
 
     for (int i = 0; i < attributes_len; i++) {
diff --git a/ks_index.c b/ks_index.c
index 0c12fcc..c47451c 100644
--- a/ks_index.c
+++ b/ks_index.c
@@ -55,8 +55,8 @@ static inline int ks_name_cmp(const hal_ks_name_t * const name1, const hal_ks_na
 }
 
 /*
- * Return value indicates whether the name is present in the index.
- * "where" indicates the name's position whether present or not.
+ * Find a block in the index, return true (found) or false (not found).
+ * "where" indicates the name's position, or the position of the first free block.
  *
  * NB: This does NOT return a block number, it returns an index into
  * ksi->index[].
@@ -145,6 +145,10 @@ static inline void ks_heapsort(hal_ks_index_t *ksi)
   }
 }
 
+/*
+ * Perform a consistency check on the index.
+ */
+
 #define fsck(_ksi) \
   do { hal_error_t _err = hal_ks_index_fsck(_ksi); if (_err != HAL_OK) return _err; } while (0)
 
@@ -179,16 +183,16 @@ hal_error_t hal_ks_index_fsck(hal_ks_index_t *ksi)
   return HAL_OK;
 }
 
+/*
+ * Set up the index. Only setup task we have at the moment is sorting the index.
+ */
+
 hal_error_t hal_ks_index_setup(hal_ks_index_t *ksi)
 {
   if (ksi == NULL || ksi->index == NULL || ksi->names == NULL ||
       ksi->size == 0 || ksi->used > ksi->size)
     return HAL_ERROR_BAD_ARGUMENTS;
 
-  /*
-   * Only setup task we have at the moment is sorting the index.
-   */
-
   ks_heapsort(ksi);
 
   /*
@@ -200,6 +204,10 @@ hal_error_t hal_ks_index_setup(hal_ks_index_t *ksi)
   return HAL_OK;
 }
 
+/*
+ * Find a single block by name and chunk number.
+ */
+
 hal_error_t hal_ks_index_find(hal_ks_index_t *ksi,
 			      const hal_uuid_t * const name,
                               const unsigned chunk,
@@ -225,6 +233,11 @@ hal_error_t hal_ks_index_find(hal_ks_index_t *ksi,
   return ok ? HAL_OK : HAL_ERROR_KEY_NOT_FOUND;
 }
 
+/*
+ * Find all blocks with the given name.
+ * If 'strict' is set, expect it to be a well-ordered set of chunks.
+ */
+
 hal_error_t hal_ks_index_find_range(hal_ks_index_t *ksi,
                                     const hal_uuid_t * const name,
                                     const unsigned max_blocks,
@@ -266,6 +279,10 @@ hal_error_t hal_ks_index_find_range(hal_ks_index_t *ksi,
   return HAL_OK;
 }
 
+/*
+ * Add a single block to the index.
+ */
+
 hal_error_t hal_ks_index_add(hal_ks_index_t *ksi,
 			     const hal_uuid_t * const name,
                              const unsigned chunk,
@@ -309,6 +326,10 @@ hal_error_t hal_ks_index_add(hal_ks_index_t *ksi,
   return HAL_OK;
 }
 
+/*
+ * Delete a single block from the index.
+ */
+
 hal_error_t hal_ks_index_delete(hal_ks_index_t *ksi,
 				const hal_uuid_t * const name,
                                 const unsigned chunk,
@@ -348,6 +369,11 @@ hal_error_t hal_ks_index_delete(hal_ks_index_t *ksi,
   return HAL_OK;
 }
 
+/*
+ * Delete all blocks with the given name. If blocknos is NULL, return a
+ * count of the matching blocks without deleting anything.
+ */
+
 hal_error_t hal_ks_index_delete_range(hal_ks_index_t *ksi,
                                       const hal_uuid_t * const name,
                                       const unsigned max_blocks,
@@ -404,6 +430,10 @@ hal_error_t hal_ks_index_delete_range(hal_ks_index_t *ksi,
   return HAL_OK;
 }
 
+/*
+ * Replace a single block in the index.
+ */
+
 hal_error_t hal_ks_index_replace(hal_ks_index_t *ksi,
                                  const hal_uuid_t * const name,
                                  const unsigned chunk,
diff --git a/rpc_pkey.c b/rpc_pkey.c
index c0a6479..ba75f7e 100644
--- a/rpc_pkey.c
+++ b/rpc_pkey.c
@@ -227,7 +227,8 @@ static inline hal_error_t ks_open_from_flags(hal_ks_t **ks, const hal_key_flags_
 }
 
 /*
- * Receive key from application, store it with supplied name, return a key handle.
+ * Receive key from application, generate a name (UUID), store it, and
+ * return a key handle and the name.
  */
 
 static hal_error_t pkey_local_load(const hal_client_handle_t client,