[Cryptech-Commits] [sw/libhal] 03/03: Avoid deadlock triggered by low-probability race condition.

[git at cryptech.is]

This is an automated email from the git hooks/post-receive script.

sra at hactrn.net pushed a commit to branch ksng
in repository sw/libhal.

commit c9fc4a5779db08a6c8a0029b779826a188d8b438
Author: Rob Austein <sra at hactrn.net>
AuthorDate: Sun Apr 23 18:30:50 2017 -0400

    Avoid deadlock triggered by low-probability race condition.
    
    Static code analysis (Doxygen call graph) detected a low-probability
    race condition which could have triggered a deadlock on the keystore
    mutex if the mkmif code returns with an error like HAL_ERROR_CORE_BUSY
    when we're trying to fetch the KEK.
    
    This is a knock-on effect of the awful kludge of backing up the KEK in
    the keystore flash as an alternative to powering the MKM with a
    battery as called for in the design.  This code path should not exist
    at all, but, for now, we avoid the deadlock by making it the caller's
    responsibility to grab the keystore mutex before looking up the KEK.
---
 hal_internal.h |  1 +
 ks_flash.c     | 18 +++++++++++-------
 mkm.c          | 10 +++++++++-
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hal_internal.h b/hal_internal.h
index f17179c..56d0936 100644
--- a/hal_internal.h
+++ b/hal_internal.h
@@ -421,6 +421,7 @@ extern hal_error_t hal_mkm_volatile_erase(const size_t len);
 /* #warning MKM flash backup kludge enabled.  Do NOT use this in production! */
 
 extern hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len);
+extern hal_error_t hal_mkm_flash_read_no_lock(uint8_t *buf, const size_t len);
 extern hal_error_t hal_mkm_flash_write(const uint8_t * const buf, const size_t len);
 extern hal_error_t hal_mkm_flash_erase(const size_t len);
 
diff --git a/ks_flash.c b/ks_flash.c
index b14e568..8aadc37 100644
--- a/ks_flash.c
+++ b/ks_flash.c
@@ -2119,7 +2119,7 @@ hal_error_t hal_set_pin(const hal_user_t user,
  * while re-implementing it on top of the new keystore.
  */
 
-hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len)
+hal_error_t hal_mkm_flash_read_no_lock(uint8_t *buf, const size_t len)
 {
   if (buf != NULL && len != KEK_LENGTH)
     return HAL_ERROR_MASTERKEY_BAD_LENGTH;
@@ -2128,18 +2128,22 @@ hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len)
   hal_error_t err;
   unsigned b;
 
-  hal_ks_lock();
-
   if ((err = fetch_pin_block(&b, &block)) != HAL_OK)
-    goto done;
+    return err;
 
   if (block->pin.kek_set != FLASH_KEK_SET)
-    err = HAL_ERROR_MASTERKEY_NOT_SET;
+    return HAL_ERROR_MASTERKEY_NOT_SET;
 
-  else if (buf != NULL)
+  if (buf != NULL)
     memcpy(buf, block->pin.kek, len);
 
- done:
+  return HAL_OK;
+}
+
+hal_error_t hal_mkm_flash_read(uint8_t *buf, const size_t len)
+{
+  hal_ks_lock();
+  const hal_error_t err = hal_mkm_flash_read_no_lock(buf, len);
   hal_ks_unlock();
   return err;
 }
diff --git a/mkm.c b/mkm.c
index 2b2141f..05c733d 100644
--- a/mkm.c
+++ b/mkm.c
@@ -201,7 +201,15 @@ hal_error_t hal_mkm_get_kek(uint8_t *kek,
 
 #if HAL_MKM_FLASH_BACKUP_KLUDGE
 
-  if (hal_mkm_flash_read(kek, len) == LIBHAL_OK) {
+  /*
+   * It turns out that, in every case where this function is called,
+   * we already hold the keystore lock, so attempting to grab it again
+   * would deadlock.  This almost never happens when the volatile MKM
+   * is set, but there's a race condition that might drop us here if
+   * hal_mkm_volatile_read() returns HAL_ERROR_CORE_BUSY.  Whee!
+   */
+
+  if (hal_mkm_flash_read_no_lock(kek, len) == LIBHAL_OK) {
     *kek_len = len;
     return LIBHAL_OK;
   }