From 6b881dfa81a0d51d4897c62de5abdb94c1aba0b7 Mon Sep 17 00:00:00 2001
From: Rob Austein <sra@hactrn.net>
Date: Tue, 30 May 2017 19:52:32 -0400
Subject: Hold keystore lock before calling keystore driver methods.

Most keystore methods already followed this rule, but hal_ks_*_init()
and hal_ks_*_logout() were confused, in different ways.
---
 ks.c          | 34 ++++++++++++++++++++++++++--------
 ks_token.c    | 26 ++++++--------------------
 ks_volatile.c | 29 +++++++++++------------------
 3 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/ks.c b/ks.c
index 92dc303..665a2fd 100644
--- a/ks.c
+++ b/ks.c
@@ -205,10 +205,19 @@ hal_error_t hal_ks_block_update(hal_ks_t *ks,
 
 hal_error_t hal_ks_init(hal_ks_t *ks, const int alloc)
 {
-  return
-    ks == NULL || ks->driver == NULL  ? HAL_ERROR_BAD_ARGUMENTS   :
-    ks->driver->init == NULL          ? HAL_ERROR_NOT_IMPLEMENTED :
-    ks->driver->init(ks, alloc);
+  if (ks == NULL || ks->driver == NULL)
+    return HAL_ERROR_BAD_ARGUMENTS;
+
+  if (ks->driver->init == NULL)
+    return HAL_ERROR_NOT_IMPLEMENTED;
+
+  hal_ks_lock();
+
+  const hal_error_t err = ks->driver->init(ks, alloc);
+
+  hal_ks_unlock();
+
+  return err;
 }
 
 static inline void *gnaw(uint8_t **mem, size_t *len, const size_t size)
@@ -466,10 +475,19 @@ hal_error_t hal_ks_init_common(hal_ks_t *ks)
 
 hal_error_t hal_ks_logout(hal_ks_t *ks, const hal_client_handle_t client)
 {
-  return
-    ks == NULL || ks->driver == NULL ? HAL_ERROR_BAD_ARGUMENTS   :
-    ks->driver->logout == NULL       ? HAL_ERROR_NOT_IMPLEMENTED :
-    ks->driver->logout(ks, client);
+  if (ks == NULL || ks->driver == NULL)
+    return HAL_ERROR_BAD_ARGUMENTS;
+
+  if (ks->driver->logout == NULL)
+    return HAL_ERROR_NOT_IMPLEMENTED;
+
+  hal_ks_lock();
+
+  const hal_error_t err = ks->driver->logout(ks, client);
+
+  hal_ks_unlock();
+
+  return err;
 }
 
 /*
diff --git a/ks_token.c b/ks_token.c
index e29a90d..38ca5d8 100644
--- a/ks_token.c
+++ b/ks_token.c
@@ -313,13 +313,11 @@ static hal_error_t ks_token_init(hal_ks_t *ks, const int alloc)
   hal_ks_block_t *block = NULL;
   hal_error_t err = HAL_OK;
 
-  hal_ks_lock();
-
   if (alloc && (err = hal_ks_alloc_common(ks, NUM_FLASH_BLOCKS, KS_TOKEN_CACHE_SIZE, NULL, 0)) != HAL_OK)
-    goto done;
+    return err;
 
   if ((err = hal_ks_init_common(ks)) != HAL_OK)
-    goto done;
+    return err;
 
   /*
    * Fetch or create the PIN block.
@@ -337,10 +335,7 @@ static hal_error_t ks_token_init(hal_ks_t *ks, const int alloc)
     db->user_pin  = block->pin.user_pin;
   }
 
-  else if (err != HAL_ERROR_KEY_NOT_FOUND)
-    goto done;
-
-  else {
+  else if (err == HAL_ERROR_KEY_NOT_FOUND) {
     /*
      * We found no PIN block, so create one, with the user and so PINs
      * cleared and the wheel PIN set to the last-gasp value.  The
@@ -351,10 +346,8 @@ static hal_error_t ks_token_init(hal_ks_t *ks, const int alloc)
 
     unsigned b;
 
-    if ((block = hal_ks_cache_pick_lru(ks)) == NULL) {
-      err = HAL_ERROR_IMPOSSIBLE;
-      goto done;
-    }
+    if ((block = hal_ks_cache_pick_lru(ks)) == NULL)
+      return HAL_ERROR_IMPOSSIBLE;
 
     memset(block, 0xFF, sizeof(*block));
 
@@ -366,22 +359,15 @@ static hal_error_t ks_token_init(hal_ks_t *ks, const int alloc)
     block->pin.user_pin  = db->user_pin;
 
     if ((err = hal_ks_index_add(ks, &hal_ks_pin_uuid, &b, NULL)) != HAL_OK)
-      goto done;
+      return err;
 
     hal_ks_cache_mark_used(ks, block, b);
 
     err = ks_token_write(ks, b, block);
 
     hal_ks_cache_release(ks, block);
-
-    if (err != HAL_OK)
-      goto done;
   }
 
-  err = HAL_OK;
-
- done:
-  hal_ks_unlock();
   return err;
 }
 
diff --git a/ks_volatile.c b/ks_volatile.c
index 2b5bb61..1586f3d 100644
--- a/ks_volatile.c
+++ b/ks_volatile.c
@@ -254,35 +254,28 @@ static hal_error_t ks_volatile_init(hal_ks_t *ks, const int alloc)
   if (ks != hal_ks_volatile)
     return HAL_ERROR_IMPOSSIBLE;
 
-  hal_error_t err = HAL_OK;
   void *mem = NULL;
+  hal_error_t err;
 
-  hal_ks_lock();
+  if (alloc &&
+      (err = hal_ks_alloc_common(ks, STATIC_KS_VOLATILE_SLOTS, KS_VOLATILE_CACHE_SIZE,
+                                 &mem, sizeof(*db->keys) * STATIC_KS_VOLATILE_SLOTS)) != HAL_OK)
+    return err;
 
-  if (alloc) {
-    if ((err = hal_ks_alloc_common(ks, STATIC_KS_VOLATILE_SLOTS, KS_VOLATILE_CACHE_SIZE,
-                                   &mem, sizeof(*db->keys) * STATIC_KS_VOLATILE_SLOTS)) != HAL_OK)
-      goto done;
+  if (alloc)
     db->keys = mem;
-  }
 
-  if (db->keys == NULL) {
-    err = HAL_ERROR_IMPOSSIBLE;
-    goto done;
-  }
+  if (db->keys == NULL)
+    return HAL_ERROR_IMPOSSIBLE;
 
   for (unsigned b = 0; b < db->ks.size; b++)
     if ((err = hal_ks_block_erase(ks, b)) != HAL_OK)
-      goto done;
+      return err;
 
   if ((err = hal_ks_init_common(ks)) != HAL_OK)
-    goto done;
-
-  err = HAL_OK;
+    return err;
 
- done:
-  hal_ks_unlock();
-  return err;
+  return HAL_OK;
 }
 
 /*
-- 
cgit v1.2.3