nvmem: fix delimiter creation during setvar() (take two)

The (key, value) objects should not be treated differently from TPM
objects when initializing NVMEM from some inconsistent state.

Saving of a modified (key, value) object should include the
'incomplete delimiter' phase when the new value has been already
saved, but the old value has not yet been eliminated.

Added tests verifying various failure modes.

BRANCH=cr50, cr50-mp
BUG=b:69907320, b:129710256
TEST='make run-nvmem' succeeds

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

Change-Id: Iaaca3fc1786ea74adae8b918fc69d95481d622bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1644286
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 21b0c907a9c90eb5dae787a95ce9a633d0011f7c)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1705707
(cherry picked from commit bed74f99eb720b5799624541bf88a22d4b79c3f3)

3 files changed, 158 insertions, 22 deletions

diff --git a/common/new_nvmem.c b/common/new_nvmem.c
index 714481c0fb..60d8704a9b 100644
--- a/common/new_nvmem.c
+++ b/common/new_nvmem.c
@@ -312,7 +312,6 @@ static struct delete_candidates {
  * list.
  */
 static uint8_t page_list[NEW_NVMEM_TOTAL_PAGES];
-static uint8_t migration_in_progress;
 static uint32_t next_evict_obj_base;
 
 /*
@@ -1353,11 +1352,8 @@ static enum ec_error_list save_var(const uint8_t *key, uint8_t key_len,
 	vc->c_header.size = sizeof(struct tuple) + val_len + key_len;
 
 	rv = save_container(&vc->c_header);
-	if (rv == EC_SUCCESS) {
+	if (rv == EC_SUCCESS)
 		total_var_space += key_len + val_len;
-		if (!migration_in_progress)
-			add_final_delimiter();
-	}
 
 	if (local_alloc)
 		shared_mem_release(vc);
@@ -1445,10 +1441,8 @@ enum ec_error_list new_nvmem_migrate(unsigned int act_partition)
 	ch->encrypted = 1;
 	ch->generation = 0;
 
-	migration_in_progress = 1;
 	migrate_vars(ch);
 	migrate_tpm_nvmem(ch);
-	migration_in_progress = 0;
 
 	shared_mem_release(ch);
 
@@ -1888,6 +1882,10 @@ static enum ec_error_list verify_last_section(
 		union {
 			uint32_t handle; /* For evictables. */
 			uint8_t id;	 /* For reserved objects. */
+			struct {	 /* For tuples. */
+				uint32_t key_hash;
+				uint8_t key_len;
+			};
 		};
 	};
 	struct new_objects {
@@ -1900,6 +1898,7 @@ static enum ec_error_list verify_last_section(
 	struct object *po;
 	uint8_t ctype;
 	struct page_tracker top_del;
+	struct max_var_container *vc;
 	int i;
 
 	newobjs = get_scratch_buffer(sizeof(struct new_objects));
@@ -1926,6 +1925,14 @@ static enum ec_error_list verify_last_section(
 		case NN_OBJ_TPM_EVICTABLE:
 			po->handle = *((uint32_t *)(ch + 1));
 			break;
+
+		case NN_OBJ_TUPLE:
+			vc = (struct max_var_container *)ch;
+			po->key_len = vc->t_header.key_len;
+			app_compute_hash_wrapper(vc->t_header.data_,
+						 po->key_len, &po->key_hash,
+						 sizeof(po->key_hash));
+			break;
 		default:
 			continue;
 		}
@@ -1975,15 +1982,33 @@ static enum ec_error_list verify_last_section(
 			key = *((uint32_t *)(ch + 1));
 			key_size = sizeof(uint32_t);
 			break;
+
+		case NN_OBJ_TUPLE:
+			vc = (struct max_var_container *)ch;
+			key_size = vc->t_header.key_len;
+			app_compute_hash_wrapper(vc->t_header.data_, key_size,
+						 &key, sizeof(key));
+			break;
+
 		default:
 			continue;
 		}
 
 		for (i = 0, po = newobjs->objects; i < newobjs->num_objects;
 		     i++, po++) {
-			if ((po->cont_type != ctype) ||
-			    ((key_size == 1) && (po->id != key)) ||
-			    ((key_size == 4) && (po->handle != key)))
+			if (po->cont_type != ctype)
+				continue;
+
+			if ((ctype == NN_OBJ_TPM_RESERVED) && (po->id != key))
+				continue;
+
+			if ((ctype == NN_OBJ_TPM_EVICTABLE) &&
+			    (po->handle != key))
+				continue;
+
+			if ((ctype == NN_OBJ_TUPLE) &&
+			    ((po->key_len != key_size) ||
+			     (key != po->key_hash)))
 				continue;
 
 			/*
@@ -2056,11 +2081,8 @@ static enum ec_error_list verify_delimiter(struct nn_container *nc)
 				dpt.list_index = i;
 	}
 
-	while ((rv = get_next_object(&dpt, nc, 0)) == EC_SUCCESS) {
-		if (nc->container_type == NN_OBJ_TUPLE)
-			continue;
+	while ((rv = get_next_object(&dpt, nc, 0)) == EC_SUCCESS)
 		delete_object(&dpt, nc);
-	}
 
 	if (rv == EC_ERROR_INVAL) {
 		/*
@@ -2669,6 +2691,7 @@ int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
 	size_t old_var_space;
 	struct max_var_container *vc;
 	struct access_tracker at = {};
+	const struct nn_container *del;
 
 	if (!key || !key_len)
 		return EC_ERROR_INVAL;
@@ -2715,7 +2738,11 @@ int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
 			/* No, it will not. */
 			return EC_ERROR_OVERFLOW;
 
-		return save_var(key, key_len, val, val_len, vc);
+		rv = save_var(key, key_len, val, val_len, vc);
+		if (rv == EC_SUCCESS)
+			add_final_delimiter();
+
+		return rv;
 	}
 
 	/* The variable was found, let's see if the value is being changed. */
@@ -2737,12 +2764,23 @@ int setvar(const uint8_t *key, uint8_t key_len, const uint8_t *val,
 	vc->c_header.generation++;
 	rv = save_var(key, key_len, val, val_len, vc);
 	shared_mem_release(vc);
+	del = page_cursor(&master_at.mt);
+#if defined(NVMEM_TEST_BUILD)
+	if (failure_mode == TEST_FAIL_SAVING_VAR)
+		return EC_SUCCESS;
+#endif
+	add_delimiter();
 	if (rv == EC_SUCCESS) {
 		rv = invalidate_object(
 			(struct nn_container *)((uintptr_t)at.ct.ph +
 						at.ct.data_offset));
-		if (rv == EC_SUCCESS)
+		if (rv == EC_SUCCESS) {
 			total_var_space -= old_var_space;
+#if defined(NVMEM_TEST_BUILD)
+			if (failure_mode != TEST_FAIL_FINALIZING_VAR)
+#endif
+				finalize_delimiter(del);
+		}
 	}
 	return rv;
 }
diff --git a/test/nvmem.c b/test/nvmem.c
index dc5cde1eee..a7e339ebf1 100644
--- a/test/nvmem.c
+++ b/test/nvmem.c
@@ -1330,15 +1330,110 @@ static int test_tpm_nvmem_modify_evictable_objects(void)
 	return EC_SUCCESS;
 }
 
+static int test_nvmem_tuple_updates(void)
+{
+	size_t i;
+	const char *modified_var1 = "var one after";
+	const struct tuple *t;
+
+	const struct {
+		uint8_t *key;
+		uint8_t *value;
+	} kv_pairs[] = {
+		/* Use # as the delimiter to allow \0 in keys/values. */
+		{"key0", "var zero before"},
+		{"key1", "var one before"}
+	};
+
+	TEST_ASSERT(post_init_from_scratch(0xff) == EC_SUCCESS);
+
+	/* Save vars in the nvmem. */
+	for (i = 0; i < ARRAY_SIZE(kv_pairs); i++)
+		TEST_ASSERT(setvar(kv_pairs[i].key, strlen(kv_pairs[i].key),
+				   kv_pairs[i].value,
+				   strlen(kv_pairs[i].value)) == EC_SUCCESS);
+
+	TEST_ASSERT(nvmem_init() == EC_SUCCESS);
+	/* Verify the vars are still there. */
+	for (i = 0; i < ARRAY_SIZE(kv_pairs); i++) {
+		const struct tuple *t;
+
+		t = getvar(kv_pairs[i].key, strlen(kv_pairs[i].key));
+		TEST_ASSERT(t);
+		TEST_ASSERT(t->val_len == strlen(kv_pairs[i].value));
+		TEST_ASSERT(!memcmp(t->data_ + strlen(kv_pairs[i].key),
+				    kv_pairs[i].value, t->val_len));
+		freevar(t);
+	}
+
+	/*
+	 * Now, let's try updating variable 'key1' introducing various failure
+	 * modes.
+	 */
+	failure_mode = TEST_FAIL_SAVING_VAR;
+	TEST_ASSERT(setvar(kv_pairs[1].key, strlen(kv_pairs[1].key),
+			   modified_var1, strlen(modified_var1)) == EC_SUCCESS);
+	TEST_ASSERT(nvmem_init() == EC_SUCCESS);
+	/* No change should be seen. */
+	for (i = 0; i < ARRAY_SIZE(kv_pairs); i++) {
+		t = getvar(kv_pairs[i].key, strlen(kv_pairs[i].key));
+		TEST_ASSERT(t);
+		TEST_ASSERT(t->val_len == strlen(kv_pairs[i].value));
+		TEST_ASSERT(!memcmp(t->data_ + strlen(kv_pairs[i].key),
+				    kv_pairs[i].value, t->val_len));
+		freevar(t);
+	}
+	failure_mode = TEST_FAIL_FINALIZING_VAR;
+	TEST_ASSERT(setvar(kv_pairs[1].key, strlen(kv_pairs[1].key),
+			   modified_var1, strlen(modified_var1)) == EC_SUCCESS);
+	failure_mode = TEST_NO_FAILURE;
+	TEST_ASSERT(nvmem_init() == EC_SUCCESS);
+
+	/* First variable should be still unchanged. */
+	t = getvar(kv_pairs[0].key, strlen(kv_pairs[0].key));
+	TEST_ASSERT(t);
+	TEST_ASSERT(t->val_len == strlen(kv_pairs[0].value));
+	TEST_ASSERT(!memcmp(t->data_ + strlen(kv_pairs[0].key),
+			    kv_pairs[0].value, t->val_len));
+	freevar(t);
+
+	/* Second variable should be updated. */
+	t = getvar(kv_pairs[1].key, strlen(kv_pairs[1].key));
+	TEST_ASSERT(t);
+	TEST_ASSERT(t->val_len == strlen(modified_var1));
+	TEST_ASSERT(!memcmp(t->data_ + strlen(kv_pairs[1].key), modified_var1,
+			    t->val_len));
+	freevar(t);
+
+	/* A corrupted attempt to update second variable. */
+	failure_mode = TEST_FAIL_FINALIZING_VAR;
+	TEST_ASSERT(setvar(kv_pairs[1].key, strlen(kv_pairs[1].key),
+			   kv_pairs[1].value, strlen(kv_pairs[1].value))
+		    == EC_SUCCESS);
+	failure_mode = TEST_NO_FAILURE;
+	TEST_ASSERT(nvmem_init() == EC_SUCCESS);
+
+	/* Is there an instance of the second variable still in the flash. */
+	t = getvar(kv_pairs[1].key, strlen(kv_pairs[1].key));
+	TEST_ASSERT(t);
+	freevar(t);
+
+	/* Delete the remaining instance of the variable. */
+	TEST_ASSERT(setvar(kv_pairs[1].key, strlen(kv_pairs[1].key),
+			   NULL, 0) == EC_SUCCESS);
+
+	/* Verify that it is indeed deleted before and after re-init. */
+	TEST_ASSERT(!getvar(kv_pairs[1].key, strlen(kv_pairs[1].key)));
+	TEST_ASSERT(nvmem_init() == EC_SUCCESS);
+	TEST_ASSERT(!getvar(kv_pairs[1].key, strlen(kv_pairs[1].key)));
+
+	return EC_SUCCESS;
+}
+
 void run_test(void)
 {
 	run_test_setup();
 
-	if (0) {
-		RUN_TEST(test_nvmem_incomplete_transaction);
-		test_print_result();
-		return;
-	}
 	RUN_TEST(test_migration);
 	RUN_TEST(test_corrupt_nvmem);
 	RUN_TEST(test_fully_erased_nvmem);
@@ -1351,6 +1446,7 @@ void run_test(void)
 	RUN_TEST(test_tpm_nvmem_modify_reserved_objects);
 	RUN_TEST(test_tpm_nvmem_modify_evictable_objects);
 	RUN_TEST(test_nvmem_incomplete_transaction);
+	RUN_TEST(test_nvmem_tuple_updates);
 	failure_mode = TEST_NO_FAILURE; /* In case the above test failed. */
 	RUN_TEST(test_nvmem_interrupted_compaction);
 	failure_mode = TEST_NO_FAILURE; /* In case the above test failed. */
diff --git a/test/nvmem_test.h b/test/nvmem_test.h
index f8f166dc5e..9e050582ac 100644
--- a/test/nvmem_test.h
+++ b/test/nvmem_test.h
@@ -17,7 +17,9 @@ enum test_failure_mode {
 	TEST_NO_FAILURE,
 	TEST_FAIL_WHEN_SAVING,
 	TEST_FAIL_WHEN_INVALIDATING,
-	TEST_FAIL_WHEN_COMPACTING
+	TEST_FAIL_WHEN_COMPACTING,
+	TEST_FAIL_SAVING_VAR,
+	TEST_FAIL_FINALIZING_VAR
 };
 
 extern enum test_failure_mode failure_mode;