From 33c843ad457f8341b8a277e6d9481937d3925951 Mon Sep 17 00:00:00 2001 From: Paul Selkirk Date: Mon, 13 Feb 2017 23:16:21 -0500 Subject: Add some comments for things I figured out while reviewing code. --- ks_attribute.c | 9 +++++++ ks_flash.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- ks_index.c | 42 ++++++++++++++++++++++++----- rpc_pkey.c | 3 ++- 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 #include #include @@ -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, -- cgit v1.2.3