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>
diff --git a/firmware/lib/include/vboot_kernel.h b/firmware/lib/include/vboot_kernel.h
index c147c31..6f1a31d 100644
--- a/firmware/lib/include/vboot_kernel.h
+++ b/firmware/lib/include/vboot_kernel.h
@@ -78,17 +78,4 @@
*/
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 c5328d2..e2c1a6c 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -46,7 +46,7 @@
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 @@
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 @@
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 @@
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 @@
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 40f5bb6..3a5f602 100644
--- a/firmware/lib/vboot_display.c
+++ b/firmware/lib/vboot_display.c
@@ -406,7 +406,7 @@
*/
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 8b6a179..948b63e 100644
--- a/firmware/lib/vboot_ui_legacy_common.c
+++ b/firmware/lib/vboot_ui_legacy_common.c
@@ -68,7 +68,7 @@
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 f441b41..b0cfb43 100644
--- a/firmware/lib/vboot_ui_legacy_menu.c
+++ b/firmware/lib/vboot_ui_legacy_menu.c
@@ -319,7 +319,7 @@
*/
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 8b73650..7c314d0 100644
--- a/tests/vboot_api_kernel4_tests.c
+++ b/tests/vboot_api_kernel4_tests.c
@@ -35,7 +35,6 @@
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 @@
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 @@
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 @@
{
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 @@
{
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 @@
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 @@
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 @@
"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 @@
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 @@
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 @@
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 970025b..a640556 100644
--- a/tests/vboot_display_tests.c
+++ b/tests/vboot_display_tests.c
@@ -65,7 +65,7 @@
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 5edd3d1..a8c00d7 100644
--- a/tests/vboot_legacy_clamshell_beep_tests.c
+++ b/tests/vboot_legacy_clamshell_beep_tests.c
@@ -143,7 +143,7 @@
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;
}
