crossystem: allow last nvdata entry to be filledstabilize-quickfix-13421.74.Bstabilize-quickfix-13421.100.Bstabilize-13421.99.Bstabilize-13421.96.Bstabilize-13421.89.Bstabilize-13421.80.Bstabilize-13421.73.Bstabilize-13421.53.Bstabilize-13421.42.Bstabilize-13421.103.Bstabilize-13421.102.Brelease-R86-13421.B

Mosys used to have code (below), which led me to believe that we
always try and leave the last entry unfilled:

        memset(blank, 0xff, VBNV_BLOCK_SIZE);
        for (index = 0; index < len / VBNV_BLOCK_SIZE; index++) {
                unsigned int offset = index * VBNV_BLOCK_SIZE;
                if (!memcmp(blank, &data[offset], VBNV_BLOCK_SIZE))
                        break;
        }

        if (index == 0) {
                lprintf(LOG_ERR, "VBNV is uninitialized\n");
                return -1;
        } else if (index >= len) {  <---- SEE NOTE
                lprintf(LOG_ERR, "VBNV is full\n");   <--- unreachable
                return -1;
        } else {
                return index - 1;
        }

The statement at "SEE NOTE" will always be false, so this code fooled
me to believe that we consider VBNV without a row of 0xFF*16 to be
empty.

And so I implemented and wrote unit tests for what I believed the
correct behavior to be :/

Anyway, this is causing us issues since AP firmware does not implement
it that way.  So allow the last row to be filled.

BUG=chromium:1112578
BRANCH=none
TEST=unit tests

Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
Change-Id: Ib3da78eddef69a688d081cdb5391a25000dac9d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2402385
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
(cherry picked from commit 176e01ded3bcefb6cb8baa984a158d42562bb1e9)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2410559

2 files changed, 6 insertions, 6 deletions

diff --git a/host/lib/crossystem.c b/host/lib/crossystem.c
index 63f20c42..59acc0d2 100644
--- a/host/lib/crossystem.c
+++ b/host/lib/crossystem.c
@@ -685,8 +685,8 @@ static int vb2_nv_index(const uint8_t *buf, uint32_t buf_sz, int vbnv_size)
 			break;
 	}
 
-	if (!index || index == buf_sz / vbnv_size) {
-		fprintf(stderr, "VBNV is either uninitialized or corrupted.\n");
+	if (!index) {
+		fprintf(stderr, "VBNV is uninitialized.\n");
 		return -1;
 	}
 
@@ -737,7 +737,7 @@ int vb2_write_nv_storage_flashrom(struct vb2_context *ctx)
 	}
 
 	next_index = current_index + 1;
-	if ((next_index + 1) * vbnv_size == flash_size) {
+	if (next_index * vbnv_size == flash_size) {
 		/* VBNV is full.  Erase and write at beginning. */
 		memset(flash_buf, 0xff, flash_size);
 		next_index = 0;
diff --git a/tests/vb2_host_nvdata_flashrom_tests.c b/tests/vb2_host_nvdata_flashrom_tests.c
index 7a147856..33b435d9 100644
--- a/tests/vb2_host_nvdata_flashrom_tests.c
+++ b/tests/vb2_host_nvdata_flashrom_tests.c
@@ -147,12 +147,12 @@ static void test_read_ok_full(void)
 
 	reset_test_data(&ctx, sizeof(test_nvdata_16b));
 
-	for (int entry = 0; entry < fake_flash_entry_count - 2; entry++)
+	for (int entry = 0; entry < fake_flash_entry_count - 1; entry++)
 		memcpy(fake_flash_region + (entry * VB2_NVDATA_SIZE),
 		       test_nvdata_16b, sizeof(test_nvdata_16b));
 
 	memcpy(fake_flash_region +
-	       ((fake_flash_entry_count - 2) * VB2_NVDATA_SIZE),
+	       ((fake_flash_entry_count - 1) * VB2_NVDATA_SIZE),
 	       test_nvdata2_16b, sizeof(test_nvdata2_16b));
 
 	TEST_EQ(vb2_read_nv_storage_flashrom(&ctx), 0,
@@ -222,7 +222,7 @@ static void test_write_ok_full(void)
 
 	reset_test_data(&ctx, sizeof(test_nvdata_16b));
 
-	for (int entry = 0; entry < fake_flash_entry_count - 1; entry++)
+	for (int entry = 0; entry < fake_flash_entry_count; entry++)
 		memcpy(fake_flash_region + (entry * VB2_NVDATA_SIZE),
 		       test_nvdata_16b, sizeof(test_nvdata_16b));