nvmem: protect flash accesses with a mutex (take two)

Multiple tasks could be trying to modify NVMEM concurrently. To avoid
data corruption add a mutex which guarantees that only one thread of
execution has access to the flash storing NVMEM objects.

Various paths accessing flash contents are now protected by the same
mutex. Mutex control functions are put in wrappers, which makes it
easier to add debugging code when needed.

BRANCH=cr50, cr50-mp
BUG=b:69907320, b:130828517
TEST=attempts to take a device through RMA open do not fail any more.

Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1584586
Reviewed-by: Andrey Pronin <apronin@chromium.org>
(cherry picked from commit 6e97f4aab723afd67946c4fab2ecd59c1579deaf)

Change-Id: I4de6ff5e79a8a76a0a1dba06eb391780481515a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1644289
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>
(cherry picked from commit 173e8e605f33d834216906931fd90d4831afede0)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1705710
(cherry picked from commit 96c18ae93052afcd6d59a2858797a0260a39005f)

3 files changed, 97 insertions, 13 deletions

diff --git a/common/new_nvmem.c b/common/new_nvmem.c
index 9a9973bd4b..de26e0cf90 100644
--- a/common/new_nvmem.c
+++ b/common/new_nvmem.c
@@ -19,6 +19,7 @@
 #include "nvmem_vars.h"
 #include "shared_mem.h"
 #include "system.h"
+#include "task.h"
 #include "timer.h"
 
 /*
@@ -315,6 +316,42 @@ static uint8_t page_list[NEW_NVMEM_TOTAL_PAGES];
 static uint32_t next_evict_obj_base;
 
 /*
+ * Mutex to protect flash space containing NVMEM objects. All operations
+ * modifying the flash contents or relying on its consistency (like searching
+ * in the flash) should acquire this mutex before proceeding.
+ *
+ * The interfaces grabbing this mutex are
+ *
+ *  new_nvmem_migrate()
+ *  new_nvmem_init()
+ *  new_nvmem_save()
+ *  getvar()
+ *  setvar()
+ *  nvmem_erase_tpm_data()
+ *
+ * The only static function using the mutex is browse_flash_contents() which
+ * can be invoked from the CLI and while it never modifies the flash contents,
+ * it still has to be protected to be able to properly iterate over the entire
+ * flash contents.
+ */
+static struct mutex flash_mtx;
+
+/*
+ * Wrappers around locking/unlocking mutex make it easier to debug issues by
+ * adding with minimal code changes like printouts of line numbers where the
+ * functions are invoked from.
+ */
+static void lock_mutex(int line_num)
+{
+	mutex_lock(&flash_mtx);
+}
+
+static void unlock_mutex(int line_num)
+{
+	mutex_unlock(&flash_mtx);
+}
+
+/*
  * Total space taken by key, value pairs in flash. It is limited to give TPM
  * objects priority.
  */
@@ -654,8 +691,9 @@ static uint32_t aligned_container_size(const struct nn_container *ch)
  *  EC_ERROR_MEMORY_ALLOCATION  if 'erased' object reached (not an error).
  *  EC_ERROR_INVAL	        if verification failed or read is out of sync.
  */
-enum ec_error_list get_next_object(struct access_tracker *at,
-				   struct nn_container *ch, int include_deleted)
+test_export_static enum ec_error_list get_next_object(struct access_tracker *at,
+						      struct nn_container *ch,
+						      int include_deleted)
 {
 	uint32_t salt[4];
 	uint8_t ctype;
@@ -1437,6 +1475,8 @@ enum ec_error_list new_nvmem_migrate(unsigned int act_partition)
 
 	ch = get_scratch_buffer(CONFIG_FLASH_BANK_SIZE);
 
+	lock_mutex(__LINE__);
+
 	/* Populate half of page_list with available page offsets. */
 	for (i = 0; i < ARRAY_SIZE(page_list) / 2; i++)
 		page_list[i] = flash_base / CONFIG_FLASH_BANK_SIZE + i;
@@ -1453,6 +1493,8 @@ enum ec_error_list new_nvmem_migrate(unsigned int act_partition)
 
 	add_final_delimiter();
 
+	unlock_mutex(__LINE__);
+
 	if (browse_flash_contents(0) != EC_SUCCESS)
 		/* Never returns. */
 		report_no_payload_failure(NVMEMF_MIGRATION_FAILURE);
@@ -2188,6 +2230,8 @@ enum ec_error_list new_nvmem_init(void)
 	/* Initialize NVMEM indices. */
 	NvEarlyStageFindHandle(0);
 
+	lock_mutex(__LINE__);
+
 	init_page_list();
 
 	start = get_time();
@@ -2196,6 +2240,8 @@ enum ec_error_list new_nvmem_init(void)
 
 	init = get_time();
 
+	unlock_mutex(__LINE__);
+
 	ccprintf("init took %d\n", (uint32_t)(init.val - start.val));
 
 	return rv;
@@ -2488,7 +2534,7 @@ static enum ec_error_list save_new_object(uint16_t obj_base, void *buf)
 	return save_container(ch);
 }
 
-enum ec_error_list new_nvmem_save(void)
+static enum ec_error_list new_nvmem_save_(void)
 {
 	const void *fence_ph;
 	size_t i;
@@ -2599,6 +2645,17 @@ enum ec_error_list new_nvmem_save(void)
 	return EC_SUCCESS;
 }
 
+enum ec_error_list new_nvmem_save(void)
+{
+	enum ec_error_list rv;
+
+	lock_mutex(__LINE__);
+	rv = new_nvmem_save_();
+	unlock_mutex(__LINE__);
+
+	return rv;
+}
+
 /* Caller must free memory allocated by this function! */
 static struct max_var_container *find_var(const uint8_t *key, size_t key_len,
 					  struct access_tracker *at)
@@ -2644,7 +2701,9 @@ const struct tuple *getvar(const uint8_t *key, uint8_t key_len)
 	if (!key || !key_len)
 		return NULL;
 
+	lock_mutex(__LINE__);
 	vc = find_var(key, key_len, &at);
+	unlock_mutex(__LINE__);
 
 	if (vc)
 		return &vc->t_header;
@@ -2687,8 +2746,8 @@ static enum ec_error_list save_container(struct nn_container *nc)
 	return save_object(nc);
 }
 
-int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
-	   uint8_t val_len)
+static int setvar_(const uint8_t *key, uint8_t key_len, const uint8_t *val,
+		   uint8_t val_len)
 {
 	enum ec_error_list rv;
 	int erase_request;
@@ -2790,6 +2849,18 @@ int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
 	return rv;
 }
 
+int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
+	   uint8_t val_len)
+{
+	int rv;
+
+	lock_mutex(__LINE__);
+	rv = setvar_(key, key_len, val, val_len);
+	unlock_mutex(__LINE__);
+
+	return rv;
+}
+
 static void dump_contents(const struct nn_container *ch)
 {
 	const uint8_t *buf = (const void *)ch;
@@ -2824,6 +2895,8 @@ int nvmem_erase_tpm_data(void)
 
 	ch = get_scratch_buffer(CONFIG_FLASH_BANK_SIZE);
 
+	lock_mutex(__LINE__);
+
 	while (get_next_object(&at, ch, 0) == EC_SUCCESS) {
 
 		if ((ch->container_type != NN_OBJ_TPM_RESERVED) &&
@@ -2833,6 +2906,8 @@ int nvmem_erase_tpm_data(void)
 		delete_object(&at, ch);
 	}
 
+	unlock_mutex(__LINE__);
+
 	shared_mem_release(ch);
 
 	/*
@@ -2891,7 +2966,9 @@ int nvmem_erase_tpm_data(void)
 
 	} while (master_at.list_index != (saved_list_index + 1));
 
+	lock_mutex(__LINE__);
 	rv = compact_nvmem();
+	unlock_mutex(__LINE__);
 
 	if (rv == EC_SUCCESS)
 		rv = new_nvmem_init();
@@ -2914,6 +2991,7 @@ test_export_static enum ec_error_list browse_flash_contents(int print)
 	struct access_tracker at = {};
 
 	ch = get_scratch_buffer(CONFIG_FLASH_BANK_SIZE);
+	lock_mutex(__LINE__);
 
 	while ((rv = get_next_object(&at, ch, 1)) == EC_SUCCESS) {
 		uint8_t ctype = ch->container_type;
@@ -2981,6 +3059,8 @@ test_export_static enum ec_error_list browse_flash_contents(int print)
 		}
 	}
 
+	unlock_mutex(__LINE__);
+
 	shared_mem_release(ch);
 
 	if (rv == EC_ERROR_MEMORY_ALLOCATION) {
diff --git a/include/new_nvmem.h b/include/new_nvmem.h
index 110cfd1667..37399702f8 100644
--- a/include/new_nvmem.h
+++ b/include/new_nvmem.h
@@ -127,13 +127,14 @@ struct access_tracker {
 	uint8_t list_index;
 };
 
+/*
+ * New nvmem interface functions, each of them could be blocking because each
+ * of them acquires nvmem flash protectioin mutex before proceeding.
+ */
 enum ec_error_list new_nvmem_init(void);
 enum ec_error_list new_nvmem_migrate(unsigned int nvmem_act_partition);
 enum ec_error_list new_nvmem_save(void);
-
-enum ec_error_list get_next_object(struct access_tracker *at,
-				   struct nn_container *ch,
-				   int include_deleted);
+int nvmem_erase_tpm_data(void);
 
 #if defined(TEST_BUILD) && !defined(TEST_FUZZ)
 #define NVMEM_TEST_BUILD
@@ -145,11 +146,10 @@ int is_uninitialized(const void *p, size_t size);
 size_t init_object_offsets(uint16_t *offsets, size_t count);
 struct nn_page_header *list_element_to_ph(size_t el);
 void *evictable_offs_to_addr(uint16_t offset);
+enum ec_error_list get_next_object(struct access_tracker *at,
+				   struct nn_container *ch,
+				   int include_deleted);
 #endif
 
-/*
- * Clear tpm data from nvmem.
- */
-int nvmem_erase_tpm_data(void);
 
 #endif /* ! __TPM2_NVMEM_TEST_NEW_NVMEM_H */
diff --git a/include/nvmem_vars.h b/include/nvmem_vars.h
index eec1e757a2..08bd9164c8 100644
--- a/include/nvmem_vars.h
+++ b/include/nvmem_vars.h
@@ -70,6 +70,8 @@ int initvars(void);
  * The val_len field in the passed in tuple indicates how much room is
  * available, the actual value size could be smaller.
  *
+ * Could block as it acquires the flash protection mutex before proceeding.
+ *
  * Return:
  *
  * EC_SUCCESS - if the key was found and there was enough room in the passed
@@ -98,6 +100,8 @@ const uint8_t *tuple_val(const struct tuple *);
 /*
  * Save the tuple in the RAM buffer. If val is NULL or val_len is 0, the
  * tuple is deleted (if it existed). Returns EC_SUCCESS or error code.
+ *
+ * Could block as it acquires the flash protection mutex before proceeding.
  */
 int setvar(const uint8_t *key, uint8_t key_len,
 	   const uint8_t *val, uint8_t val_len);