vboot: do not call vb2_commit_data at end of VBSLK

Under normal circumstances, data should be committed by
depthcharge after execution flow leaves VbSelectAndLoadKernel
API call.

Since depthcharge needs to be able to respond with the
appropriate vb2api_fail call for specific data commit
errors anyways, this logic is moved directly into
vb2ex_commit_data in CL:2053765.

Remove the vb2_commit_data wrapper as was originally
intended.  vboot code may now directly call
vb2ex_commit_data and depend on depthcharge to call
vb2api_fail appropriately.

BUG=b:124141368, chromium:972956, chromium:1006689
TEST=make clean && make runtests
BRANCH=none

Change-Id: I55bdb3274210869d4ad1411837b6ef6c579dccad
Cq-Depend: chromium:2053765
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2037906
Reviewed-by: Julius Werner <jwerner@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>

8 files changed, 35 insertions, 138 deletions

diff --git a/firmware/lib/include/vboot_kernel.h b/firmware/lib/include/vboot_kernel.h
index c147c316..6f1a31d0 100644
--- a/firmware/lib/include/vboot_kernel.h
+++ b/firmware/lib/include/vboot_kernel.h
@@ -78,17 +78,4 @@ vb2_error_t VbBootDeveloperLegacyMenu(struct vb2_context *ctx);
  */
 vb2_error_t VbBootRecoveryLegacyMenu(struct vb2_context *ctx);
 
-/**
- * Writes modified secdata spaces and nvdata.
- *
- * This is a temporary wrapper around vb2ex_commit_data, until secdata-writing
- * functions are relocated into depthcharge.
- *
- * (See chromium:972956, chromium:1006689.)
- *
- * @param ctx		Vboot context
- * @returns VB2_SUCCESS, or non-zero error code.
- */
-vb2_error_t vb2_commit_data(struct vb2_context *ctx);
-
 #endif  /* VBOOT_REFERENCE_VBOOT_KERNEL_H_ */
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index c5328d23..e2c1a6c1 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -46,7 +46,7 @@ static vb2_error_t handle_battery_cutoff(struct vb2_context *ctx)
 		vb2_nv_set(ctx, VB2_NV_BATTERY_CUTOFF_REQUEST, 0);
 
 		/* May lose power immediately, so commit our update now. */
-		rv = vb2_commit_data(ctx);
+		rv = vb2ex_commit_data(ctx);
 		if (rv)
 			return rv;
 
@@ -283,55 +283,12 @@ static void vb2_kernel_fill_kparams(struct vb2_context *ctx,
 	       sizeof(kparams->partition_guid));
 }
 
-vb2_error_t vb2_commit_data(struct vb2_context *ctx)
-{
-	vb2_error_t rv = vb2ex_commit_data(ctx);
-
-	switch (rv) {
-	case VB2_SUCCESS:
-		break;
-
-	case VB2_ERROR_SECDATA_FIRMWARE_WRITE:
-		if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
-			vb2api_fail(ctx, VB2_RECOVERY_RW_TPM_W_ERROR, rv);
-			/* Run again to set recovery reason in nvdata. */
-			vb2ex_commit_data(ctx);
-			return rv;
-		}
-		break;
-
-	case VB2_ERROR_SECDATA_KERNEL_WRITE:
-		if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
-			vb2api_fail(ctx, VB2_RECOVERY_RW_TPM_W_ERROR, rv);
-			/* Run again to set recovery reason in nvdata. */
-			vb2ex_commit_data(ctx);
-			return rv;
-		}
-		break;
-
-	default:
-		VB2_DEBUG("unknown commit error: %#x\n", rv);
-		__attribute__ ((fallthrough));
-
-	case VB2_ERROR_NV_WRITE:
-		/*
-		 * We can't write to nvdata, so it's impossible to
-		 * trigger recovery mode.  Skip calling vb2api_fail
-		 * and just die (unless already in recovery).
-		 */
-		VB2_REC_OR_DIE(ctx, "write nvdata failed\n");
-		break;
-	}
-
-	return VB2_SUCCESS;
-}
-
 vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 				  VbSharedDataHeader *shared,
 				  VbSelectAndLoadKernelParams *kparams)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
-	vb2_error_t rv, call_rv;
+	vb2_error_t rv;
 
 	/* Init nvstorage space. TODO(kitching): Remove once we add assertions
 	   to vb2_nv_get and vb2_nv_set. */
@@ -339,11 +296,11 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 
 	rv = vb2_kernel_setup(ctx, shared, kparams);
 	if (rv)
-		goto VbSelectAndLoadKernel_exit;
+		return rv;
 
 	rv = vb2api_kernel_phase1(ctx);
 	if (rv)
-		goto VbSelectAndLoadKernel_exit;
+		return rv;
 
 	VB2_DEBUG("GBB flags are %#x\n", vb2_get_gbb(ctx)->flags);
 
@@ -354,35 +311,34 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 	if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
 		rv = vb2api_ec_sync(ctx);
 		if (rv)
-			goto VbSelectAndLoadKernel_exit;
+			return rv;
 
 		rv = vb2api_auxfw_sync(ctx);
 		if (rv)
-			goto VbSelectAndLoadKernel_exit;
+			return rv;
 
 		rv = handle_battery_cutoff(ctx);
 		if (rv)
-			goto VbSelectAndLoadKernel_exit;
+			return rv;
 	}
 
 	/* Select boot path */
 	if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) {
 		vb2_clear_recovery(ctx);
 
-		/*
-		 * Need to commit nvdata changes immediately, since we will be
-		 * entering either manual recovery UI or BROKEN screen shortly.
-		 */
-		vb2_commit_data(ctx);
-
 		/* If we're in recovery mode just to do memory retraining, all
 		   we need to do is reboot. */
 		if (sd->recovery_reason == VB2_RECOVERY_TRAIN_AND_REBOOT) {
 			VB2_DEBUG("Reboot after retraining in recovery\n");
-			rv = VBERROR_REBOOT_REQUIRED;
-			goto VbSelectAndLoadKernel_exit;
+			return VBERROR_REBOOT_REQUIRED;
 		}
 
+		/*
+		 * Need to commit nvdata changes immediately, since we will be
+		 * entering either manual recovery UI or BROKEN screen shortly.
+		 */
+		vb2ex_commit_data(ctx);
+
 		/* Recovery boot.  This has UI. */
 		if (LEGACY_MENU_UI)
 			rv = VbBootRecoveryLegacyMenu(ctx);
@@ -416,21 +372,15 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx,
 		rv = VbBootNormal(ctx);
 	}
 
- VbSelectAndLoadKernel_exit:
+	/* No need to fill kparams or convert vboot1 flags on failure. */
+	if (rv)
+		return rv;
 
-	if (rv == VB2_SUCCESS)
-		vb2_kernel_fill_kparams(ctx, kparams);
+	vb2_kernel_fill_kparams(ctx, kparams);
 
 	/* Translate vboot2 flags and fields into vboot1. */
 	if (sd->flags & VB2_SD_FLAG_KERNEL_SIGNED)
 		sd->vbsd->flags |= VBSD_KERNEL_KEY_VERIFIED;
 
-	/* Commit data, but retain any previous errors */
-	call_rv = vb2_commit_data(ctx);
-	if (rv == VB2_SUCCESS)
-		rv = call_rv;
-
-	/* Pass through return value from boot path */
-	VB2_DEBUG("Returning %#x\n", rv);
 	return rv;
 }
diff --git a/firmware/lib/vboot_display.c b/firmware/lib/vboot_display.c
index 40f5bb66..3a5f602a 100644
--- a/firmware/lib/vboot_display.c
+++ b/firmware/lib/vboot_display.c
@@ -406,7 +406,7 @@ vb2_error_t VbCheckDisplayKey(struct vb2_context *ctx, uint32_t key,
 		 */
 		if ((ctx->flags & VB2_CONTEXT_RECOVERY_MODE) &&
 		    !vb2_allow_recovery(ctx))
-			 vb2_commit_data(ctx);
+			 vb2ex_commit_data(ctx);
 
 		/* Force redraw of current screen */
 		return VbDisplayScreen(ctx, disp_current_screen, 1, data);
diff --git a/firmware/lib/vboot_ui_legacy_common.c b/firmware/lib/vboot_ui_legacy_common.c
index 8b6a1799..948b63e2 100644
--- a/firmware/lib/vboot_ui_legacy_common.c
+++ b/firmware/lib/vboot_ui_legacy_common.c
@@ -68,7 +68,7 @@ void vb2_try_altfw(struct vb2_context *ctx, int allowed,
 		return;
 	}
 
-	if (vb2_commit_data(ctx)) {
+	if (vb2ex_commit_data(ctx)) {
 		vb2_error_notify("Error committing data on legacy boot.\n",
 				 NULL, VB_BEEP_FAILED);
 		return;
diff --git a/firmware/lib/vboot_ui_legacy_menu.c b/firmware/lib/vboot_ui_legacy_menu.c
index f441b410..b0cfb43b 100644
--- a/firmware/lib/vboot_ui_legacy_menu.c
+++ b/firmware/lib/vboot_ui_legacy_menu.c
@@ -319,7 +319,7 @@ static vb2_error_t language_action(struct vb2_context *ctx)
 	 */
 	if ((ctx->flags & VB2_CONTEXT_RECOVERY_MODE) &&
 	    !vb2_allow_recovery(ctx))
-		vb2_commit_data(ctx);
+		vb2ex_commit_data(ctx);
 
 	/* Return to previous menu. */
 	switch (prev_menu) {
diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c
index 8b736508..7c314d0f 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -35,7 +35,6 @@ static struct vb2_gbb_header gbb;
 static uint32_t kernel_version;
 static uint32_t new_version;
 static vb2_error_t vbboot_retval;
-static vb2_error_t commit_data_retval;
 static int commit_data_called;
 static vb2_error_t secdata_kernel_init_retval;
 static vb2_error_t secdata_fwmp_init_retval;
@@ -70,7 +69,6 @@ static void reset_common_data(void)
 	memset(&shared_data, 0, sizeof(shared_data));
 
 	kernel_version = new_version = 0x10002;
-	commit_data_retval = VB2_SUCCESS;
 	vbboot_retval = VB2_SUCCESS;
 	secdata_kernel_init_retval = VB2_SUCCESS;
 	secdata_fwmp_init_retval = VB2_SUCCESS;
@@ -90,9 +88,8 @@ static void test_slk(vb2_error_t retval, int recovery_reason, const char *desc)
 
 	expected_recovery_reason = recovery_reason;
 	TEST_EQ(VbSelectAndLoadKernel(ctx, shared, &kparams), retval, desc);
-	TEST_EQ(current_recovery_reason, expected_recovery_reason,
-		"  recovery reason");
-	TEST_TRUE(commit_data_called, "  commit nvdata");
+	TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+		recovery_reason, "  recovery reason");
 }
 
 /* Mock functions */
@@ -114,7 +111,7 @@ vb2_error_t vb2ex_commit_data(struct vb2_context *c)
 {
 	current_recovery_reason = vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST);
 	commit_data_called = 1;
-	return commit_data_retval;
+	return VB2_SUCCESS;
 }
 
 vb2_error_t vb2_secdata_kernel_init(struct vb2_context *c)
@@ -158,7 +155,7 @@ vb2_error_t VbBootRecoveryLegacyClamshell(struct vb2_context *c)
 {
 	TEST_EQ(current_recovery_reason, expected_recovery_reason,
 		"  recovery reason");
-	TEST_TRUE(commit_data_called, "  commit nvdata");
+	TEST_TRUE(commit_data_called, "  commit data");
 
 	shared->kernel_version_tpm = new_version;
 
@@ -201,6 +198,7 @@ static void select_and_load_kernel_tests(void)
 	TEST_EQ(kernel_version, 0x10002, "  version");
 	TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0,
 		 "  EC sync complete");
+	TEST_FALSE(commit_data_called, "  no commit data");
 
 	/* Check EC sync toggling */
 	reset_common_data();
@@ -239,13 +237,6 @@ static void select_and_load_kernel_tests(void)
 	test_slk(0, 0, "Max roll forward can't rollback");
 	TEST_EQ(kernel_version, 0x10002, "  version");
 
-
-	reset_common_data();
-	new_version = 0x20003;
-	commit_data_retval = VB2_ERROR_SECDATA_KERNEL_WRITE;
-	test_slk(VB2_ERROR_SECDATA_KERNEL_WRITE,
-		 VB2_RECOVERY_RW_TPM_W_ERROR, "Write kernel rollback");
-
 	/* Boot normal */
 	reset_common_data();
 	vbboot_retval = -1;
@@ -261,8 +252,6 @@ static void select_and_load_kernel_tests(void)
 			 "Normal boot with diag");
 		TEST_EQ(vb2_nv_get(ctx, VB2_NV_DIAG_REQUEST),
 			0, "  diag not requested");
-		TEST_TRUE(commit_data_called,
-			  "  didn't commit nvdata");
 	}
 
 	/* Boot normal - phase1 failure */
@@ -270,26 +259,12 @@ static void select_and_load_kernel_tests(void)
 	kernel_phase1_retval = VB2_ERROR_MOCK;
 	test_slk(VB2_ERROR_MOCK, 0, "Normal phase1 failure");
 
-	/* Boot normal - commit data failures */
-	reset_common_data();
-	commit_data_retval = VB2_ERROR_SECDATA_FIRMWARE_WRITE;
-	test_slk(commit_data_retval, VB2_RECOVERY_RW_TPM_W_ERROR,
-		 "Normal secdata_firmware write error triggers recovery");
-	commit_data_retval = VB2_ERROR_SECDATA_KERNEL_WRITE;
-	test_slk(commit_data_retval, VB2_RECOVERY_RW_TPM_W_ERROR,
-		 "Normal secdata_kernel write error triggers recovery");
-	commit_data_retval = VB2_ERROR_NV_WRITE;
-	TEST_ABORT(VbSelectAndLoadKernel(ctx, shared, &kparams),
-		   "Normal nvdata write error aborts");
-	commit_data_retval = VB2_ERROR_UNKNOWN;
-	TEST_ABORT(VbSelectAndLoadKernel(ctx, shared, &kparams),
-		   "Normal unknown commit error aborts");
-
 	/* Boot dev */
 	reset_common_data();
 	ctx->flags |= VB2_CONTEXT_DEVELOPER_MODE;
 	vbboot_retval = -2;
 	test_slk(VB2_ERROR_MOCK, 0, "Dev boot bad");
+	TEST_FALSE(commit_data_called, "  no commit data");
 
 	reset_common_data();
 	ctx->flags |= VB2_CONTEXT_DEVELOPER_MODE;
@@ -308,50 +283,34 @@ static void select_and_load_kernel_tests(void)
 	sd->recovery_reason = 123;
 	vbboot_retval = -3;
 	test_slk(VB2_ERROR_MOCK, 0, "Recovery boot bad");
+	TEST_TRUE(commit_data_called, "  commit data");
 
 	reset_common_data();
 	sd->recovery_reason = 123;
 	new_version = 0x20003;
 	test_slk(0, 0, "Recovery doesn't roll forward");
 	TEST_EQ(kernel_version, 0x10002, "  version");
+	TEST_TRUE(commit_data_called, "  commit data");
 
 	/* Boot recovery - phase1 failure */
 	reset_common_data();
 	sd->recovery_reason = 123;
 	kernel_phase1_retval = VB2_ERROR_MOCK;
 	test_slk(VB2_ERROR_MOCK, 0, "Recovery phase1 failure");
-
-	/* Boot recovery - commit data failures */
-	reset_common_data();
-	sd->recovery_reason = 123;
-	commit_data_retval = VB2_ERROR_SECDATA_FIRMWARE_WRITE;
-	test_slk(0, 0, "Recovery ignore secdata_firmware write error");
-
-	reset_common_data();
-	sd->recovery_reason = 123;
-	commit_data_retval = VB2_ERROR_SECDATA_KERNEL_WRITE;
-	test_slk(0, 0, "Recovery ignore secdata_kernel write error");
-
-	reset_common_data();
-	sd->recovery_reason = 123;
-	commit_data_retval = VB2_ERROR_NV_WRITE;
-	test_slk(0, 0, "Recovery return nvdata write error");
-
-	reset_common_data();
-	sd->recovery_reason = 123;
-	commit_data_retval = VB2_ERROR_UNKNOWN;
-	test_slk(0, 0, "Recovery return unknown write error");
+	TEST_FALSE(commit_data_called, "  no commit data");
 
 	/* Boot recovery - memory retraining */
 	reset_common_data();
 	sd->recovery_reason = VB2_RECOVERY_TRAIN_AND_REBOOT;
 	test_slk(VBERROR_REBOOT_REQUIRED, 0, "Recovery train and reboot");
+	TEST_FALSE(commit_data_called, "  no commit data");
 
 	/* Boot BROKEN recovery */
 	reset_common_data();
 	sd->recovery_reason = 123;
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
 	test_slk(0, 0, "BROKEN recovery");
+	TEST_TRUE(commit_data_called, "  commit data");
 
 	/* Boot manual recovery */
 	reset_common_data();
@@ -359,6 +318,7 @@ static void select_and_load_kernel_tests(void)
 	vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13);
 	sd->flags |= VB2_SD_FLAG_MANUAL_RECOVERY;
 	test_slk(0, 0, "Manual recovery");
+	TEST_TRUE(commit_data_called, "  commit data");
 }
 
 int main(void)
diff --git a/tests/vboot_display_tests.c b/tests/vboot_display_tests.c
index 970025b9..a640556c 100644
--- a/tests/vboot_display_tests.c
+++ b/tests/vboot_display_tests.c
@@ -65,7 +65,7 @@ vb2_error_t VbExDisplayDebugInfo(const char *info_str, int full_info)
 	return VB2_SUCCESS;
 }
 
-vb2_error_t vb2_commit_data(struct vb2_context *c)
+vb2_error_t vb2ex_commit_data(struct vb2_context *c)
 {
 	return VB2_SUCCESS;
 }
diff --git a/tests/vboot_legacy_clamshell_beep_tests.c b/tests/vboot_legacy_clamshell_beep_tests.c
index 5edd3d19..a8c00d78 100644
--- a/tests/vboot_legacy_clamshell_beep_tests.c
+++ b/tests/vboot_legacy_clamshell_beep_tests.c
@@ -143,7 +143,7 @@ struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *c)
 	return &gbb;
 }
 
-vb2_error_t vb2_commit_data(struct vb2_context *c)
+vb2_error_t vb2ex_commit_data(struct vb2_context *c)
 {
 	return VB2_SUCCESS;
 }