From 8cb57f3498f00cf599be8acc54e272896237ec85 Mon Sep 17 00:00:00 2001 From: Joel Kitching Date: Sat, 8 Feb 2020 15:15:54 +0800 Subject: vboot: integrate BROKEN screen recovery request logic into VBSLK CL:1940398 brought us towards the goal of deferring clearing recovery requests until kernel verification stage. However, now we are modifying recovery requests from multiple locations in kernel verification code -- namely, also on the BROKEN screen in UI code. Integrate the logic into a function called vb2_clear_recovery to be called from VbSelectAndLoadKernel. Add tests to ensure that recovery requests get properly updated *before* entering the UI. BUG=b:124141368, b:35576380 TEST=make clean && make runtests BRANCH=none Change-Id: I5b0f4f7556c045ccc0d0739acc2668905a2a93e9 Signed-off-by: Joel Kitching Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2044954 Tested-by: Joel Kitching Reviewed-by: Julius Werner Commit-Queue: Joel Kitching --- firmware/2lib/2misc.c | 18 ++++++ firmware/2lib/include/2misc.h | 22 +++++++ firmware/lib/vboot_api_kernel.c | 17 ++--- firmware/lib/vboot_ui_legacy_clamshell.c | 19 ------ firmware/lib/vboot_ui_legacy_menu.c | 13 ---- tests/vb2_misc_tests.c | 48 ++++++++++++-- tests/vboot_api_kernel4_tests.c | 106 ++++++++++++++++++------------- 7 files changed, 150 insertions(+), 93 deletions(-) diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c index 43d39f8b..b289f315 100644 --- a/firmware/2lib/2misc.c +++ b/firmware/2lib/2misc.c @@ -416,6 +416,24 @@ int vb2_allow_recovery(struct vb2_context *ctx) return !!(vb2_get_sd(ctx)->flags & VB2_SD_FLAG_MANUAL_RECOVERY); } +void vb2_clear_recovery(struct vb2_context *ctx) +{ + struct vb2_shared_data *sd = vb2_get_sd(ctx); + + VB2_DEBUG("Clearing recovery request: %#x / %#x\n", + vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), + vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE)); + + /* Clear recovery request for both cases. */ + vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, VB2_RECOVERY_NOT_REQUESTED); + + if (!vb2_allow_recovery(ctx)) { + VB2_DEBUG("Stow recovery reason as subcode (%#x)\n", + sd->recovery_reason); + vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, sd->recovery_reason); + } +} + int vb2api_need_reboot_for_display(struct vb2_context *ctx) { if (!(vb2_get_sd(ctx)->flags & VB2_SD_FLAG_DISPLAY_AVAILABLE)) { diff --git a/firmware/2lib/include/2misc.h b/firmware/2lib/include/2misc.h index 6a61f9e5..b80f6906 100644 --- a/firmware/2lib/include/2misc.h +++ b/firmware/2lib/include/2misc.h @@ -188,4 +188,26 @@ vb2_error_t vb2_enable_developer_mode(struct vb2_context *ctx); */ int vb2_allow_recovery(struct vb2_context *ctx); +/** + * Clear recovery request appropriately. + * + * To avoid the recovery request "sticking" and the user being in a permanent + * recovery loop, the recovery request must be cleared and committed to nvdata. + * Note that this should be done at some point after we are certain the system + * does not require any reboots for non-vboot-related reasons (e.g. FSP + * initialization), and before triggering a reboot to exit a transient recovery + * mode (e.g. memory retraining request). + * + * In BROKEN cases, the recovery reason will be stowed away as subcode, to be + * retrieved after the user reboots in manual recovery. In manual recovery, + * subcode will be left alone to keep available for subsequent manual recovery + * requests, or for accessing from userspace on the next boot. + * + * This function modifies nvdata in vb2_context, but the caller is still + * expected to call vb2_commit_data. + * + * @param ctx Vboot context + */ +void vb2_clear_recovery(struct vb2_context *ctx); + #endif /* VBOOT_REFERENCE_2MISC_H_ */ diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c index 57a827cb..36bac6ed 100644 --- a/firmware/lib/vboot_api_kernel.c +++ b/firmware/lib/vboot_api_kernel.c @@ -374,23 +374,18 @@ vb2_error_t VbSelectAndLoadKernel(struct vb2_context *ctx, /* Select boot path */ if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) { + vb2_clear_recovery(ctx); + /* - * Clear recovery request and subcode from nvdata, so that we - * don't get stuck in recovery mode after reboot. Should be - * called at some point after we are certain the system does - * not require any reboots for non-vboot-related reasons (e.g. - * FSP initialization), and before triggering a reboot to exit - * transient recovery mode (e.g. memory retraining request). + * Need to commit nvdata changes immediately, since we will be + * entering either manual recovery UI or BROKEN screen shortly. */ - vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, - VB2_RECOVERY_NOT_REQUESTED); - vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, - VB2_RECOVERY_NOT_REQUESTED); + 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"); + VB2_DEBUG("Reboot after retraining in recovery\n"); rv = VBERROR_REBOOT_REQUIRED; goto VbSelectAndLoadKernel_exit; } diff --git a/firmware/lib/vboot_ui_legacy_clamshell.c b/firmware/lib/vboot_ui_legacy_clamshell.c index cc0c4175..8efcfe9e 100644 --- a/firmware/lib/vboot_ui_legacy_clamshell.c +++ b/firmware/lib/vboot_ui_legacy_clamshell.c @@ -440,25 +440,6 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx) VB2_DEBUG("recovery UI - start\n"); if (!vb2_allow_recovery(ctx)) { - /* - * We have to save the reason here so that it will survive - * coming up three-finger-salute. We're saving it in - * VB2_RECOVERY_SUBCODE to avoid a recovery loop. - * If we save the reason in VB2_RECOVERY_REQUEST, we will come - * back here, thus, we won't be able to give a user a chance to - * reboot to workaround a boot hiccup. - */ - VB2_DEBUG("recovery UI - saving recovery reason (%#x)\n", - sd->recovery_reason); - vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, sd->recovery_reason); - - /* - * Non-manual recovery mode is meant to be left via three-finger - * salute (into manual recovery mode). Need to commit nvdata - * changes immediately. Ignore commit errors in recovery mode. - */ - vb2_commit_data(ctx); - VbDisplayScreen(ctx, VB_SCREEN_OS_BROKEN, 0, NULL); VB2_DEBUG("recovery UI - waiting for manual recovery\n"); while (1) { diff --git a/firmware/lib/vboot_ui_legacy_menu.c b/firmware/lib/vboot_ui_legacy_menu.c index 2635a7f3..f441b410 100644 --- a/firmware/lib/vboot_ui_legacy_menu.c +++ b/firmware/lib/vboot_ui_legacy_menu.c @@ -837,19 +837,6 @@ vb2_error_t VbBootDeveloperLegacyMenu(struct vb2_context *ctx) /* Main function that handles non-manual recovery (BROKEN) menu functionality */ static vb2_error_t broken_ui(struct vb2_context *ctx) { - VbSharedDataHeader *vbsd = vb2_get_sd(ctx)->vbsd; - - /* - * Temporarily stash recovery reason in subcode so we'll still know what - * to display if the user reboots into manual recovery from here. Commit - * immediately since the user may hard-reset out of here. - */ - VB2_DEBUG("saving recovery reason (%#x)\n", vbsd->recovery_reason); - vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, vbsd->recovery_reason); - - /* Ignore commit errors in recovery mode. */ - vb2_commit_data(ctx); - enter_recovery_base_screen(ctx); /* Loop and wait for the user to reset or shut down. */ diff --git a/tests/vb2_misc_tests.c b/tests/vb2_misc_tests.c index b3da6454..ac7671e2 100644 --- a/tests/vb2_misc_tests.c +++ b/tests/vb2_misc_tests.c @@ -23,12 +23,12 @@ static struct vb2_shared_data *sd; static struct vb2_gbb_header gbb; /* Mocked function data */ -enum vb2_resource_index mock_resource_index; -void *mock_resource_ptr; -uint32_t mock_resource_size; -int mock_tpm_clear_called; -int mock_tpm_clear_retval; - +static enum vb2_resource_index mock_resource_index; +static void *mock_resource_ptr; +static uint32_t mock_resource_size; +static int mock_tpm_clear_called; +static int mock_tpm_clear_retval; +static int allow_recovery_retval; static void reset_common_data(void) { @@ -49,9 +49,16 @@ static void reset_common_data(void) mock_tpm_clear_called = 0; mock_tpm_clear_retval = VB2_SUCCESS; + allow_recovery_retval = 0; }; /* Mocked functions */ + +int vb2_allow_recovery(struct vb2_context *c) +{ + return allow_recovery_retval; +} + struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *c) { return &gbb; @@ -750,6 +757,34 @@ static void need_reboot_for_display_tests(void) " not set display request"); } +static void clear_recovery_tests(void) +{ + + /* Manual recovery */ + reset_common_data(); + allow_recovery_retval = 1; + sd->recovery_reason = 4; + vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5); + vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13); + vb2_clear_recovery(ctx); + TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), + 0, " request cleared"); + TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE), + 13, " subcode retained"); + + /* BROKEN recovery */ + reset_common_data(); + allow_recovery_retval = 0; + sd->recovery_reason = 4; + vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5); + vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13); + vb2_clear_recovery(ctx); + TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), + 0, " request cleared"); + TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE), + 4, " subcode shifted"); +} + int main(int argc, char* argv[]) { init_workbuf_tests(); @@ -761,6 +796,7 @@ int main(int argc, char* argv[]) tpm_clear_tests(); select_slot_tests(); need_reboot_for_display_tests(); + clear_recovery_tests(); return gTestSuccess ? 0 : 255; } diff --git a/tests/vboot_api_kernel4_tests.c b/tests/vboot_api_kernel4_tests.c index 91ba5198..6ac25bd8 100644 --- a/tests/vboot_api_kernel4_tests.c +++ b/tests/vboot_api_kernel4_tests.c @@ -22,6 +22,7 @@ #include "vboot_test.h" /* Mock data */ + static uint8_t workbuf[VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE] __attribute__((aligned(VB2_WORKBUF_ALIGN))); static struct vb2_context *ctx; @@ -39,13 +40,14 @@ static int commit_data_called; static vb2_error_t secdata_kernel_init_retval; static vb2_error_t secdata_fwmp_init_retval; static vb2_error_t kernel_phase1_retval; +static uint32_t current_recovery_reason; +static uint32_t expected_recovery_reason; static uint32_t mock_switches[8]; static uint32_t mock_switches_count; static int mock_switches_are_stuck; -/* Reset mock data (for use before each test) */ -static void ResetMocks(void) +static void reset_common_data(void) { memset(&kparams, 0, sizeof(kparams)); @@ -73,12 +75,23 @@ static void ResetMocks(void) secdata_kernel_init_retval = VB2_SUCCESS; secdata_fwmp_init_retval = VB2_SUCCESS; kernel_phase1_retval = VB2_SUCCESS; + current_recovery_reason = 0; + expected_recovery_reason = 0; memset(mock_switches, 0, sizeof(mock_switches)); mock_switches_count = 0; mock_switches_are_stuck = 0; } +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"); +} + /* Mock functions */ struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *c) @@ -96,6 +109,7 @@ vb2_error_t vb2api_kernel_phase1(struct vb2_context *c) 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; } @@ -139,6 +153,10 @@ vb2_error_t VbBootDeveloperLegacyClamshell(struct vb2_context *c) 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"); + shared->kernel_version_tpm = new_version; if (vbboot_retval == -3) @@ -155,15 +173,6 @@ vb2_error_t VbBootDiagnosticLegacyClamshell(struct vb2_context *c) return vbboot_retval; } -static void test_slk(vb2_error_t retval, int recovery_reason, const char *desc) -{ - TEST_EQ(VbSelectAndLoadKernel(ctx, shared, &kparams), retval, desc); - TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST), - recovery_reason, " recovery reason"); - if (recovery_reason) - TEST_TRUE(commit_data_called, " didn't commit nvdata"); -} - uint32_t VbExGetSwitches(uint32_t request_mask) { if (mock_switches_are_stuck) @@ -181,67 +190,67 @@ vb2_error_t vb2ex_tpm_set_mode(enum vb2_tpm_mode mode_val) /* Tests */ -static void VbSlkTest(void) +static void select_and_load_kernel_tests(void) { /* Normal boot */ - ResetMocks(); + reset_common_data(); test_slk(0, 0, "Normal"); TEST_EQ(kernel_version, 0x10002, " version"); TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0, " EC sync complete"); /* Check EC sync toggling */ - ResetMocks(); + reset_common_data(); ctx->flags |= VB2_CONTEXT_EC_SYNC_SUPPORTED; gbb.flags |= VB2_GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC; test_slk(0, 0, "EC sync disabled by GBB"); TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0, " EC sync complete"); - ResetMocks(); + reset_common_data(); ctx->flags |= VB2_CONTEXT_EC_SYNC_SUPPORTED; test_slk(0, 0, "Normal with EC sync"); TEST_NEQ(sd->flags & VB2_SD_STATUS_EC_SYNC_COMPLETE, 0, " EC sync complete"); - ResetMocks(); + reset_common_data(); new_version = 0x20003; test_slk(0, 0, "Roll forward"); TEST_EQ(kernel_version, 0x20003, " version"); - ResetMocks(); + reset_common_data(); vb2_nv_set(ctx, VB2_NV_FW_RESULT, VB2_FW_RESULT_TRYING); new_version = 0x20003; test_slk(0, 0, "Don't roll forward kernel when trying new FW"); TEST_EQ(kernel_version, 0x10002, " version"); - ResetMocks(); + reset_common_data(); vb2_nv_set(ctx, VB2_NV_KERNEL_MAX_ROLLFORWARD, 0x30005); new_version = 0x40006; test_slk(0, 0, "Limit max roll forward"); TEST_EQ(kernel_version, 0x30005, " version"); - ResetMocks(); + reset_common_data(); vb2_nv_set(ctx, VB2_NV_KERNEL_MAX_ROLLFORWARD, 0x10001); new_version = 0x40006; test_slk(0, 0, "Max roll forward can't rollback"); TEST_EQ(kernel_version, 0x10002, " version"); - ResetMocks(); + 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 */ - ResetMocks(); + reset_common_data(); vbboot_retval = -1; test_slk(VB2_ERROR_MOCK, 0, "Normal boot bad"); /* Check that NV_DIAG_REQUEST triggers diagnostic UI */ if (DIAGNOSTIC_UI) { - ResetMocks(); + reset_common_data(); mock_switches[1] = VB_SWITCH_FLAG_PHYS_PRESENCE_PRESSED; vb2_nv_set(ctx, VB2_NV_DIAG_REQUEST, 1); vbboot_retval = -4; @@ -254,12 +263,12 @@ static void VbSlkTest(void) } /* Boot normal - phase1 failure */ - ResetMocks(); + reset_common_data(); kernel_phase1_retval = VB2_ERROR_MOCK; test_slk(VB2_ERROR_MOCK, 0, "Normal phase1 failure"); /* Boot normal - commit data failures */ - ResetMocks(); + 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"); @@ -274,75 +283,84 @@ static void VbSlkTest(void) "Normal unknown commit error aborts"); /* Boot dev */ - ResetMocks(); + reset_common_data(); sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED; vbboot_retval = -2; test_slk(VB2_ERROR_MOCK, 0, "Dev boot bad"); - ResetMocks(); + reset_common_data(); sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED; new_version = 0x20003; test_slk(0, 0, "Dev doesn't roll forward"); TEST_EQ(kernel_version, 0x10002, " version"); /* Boot dev - phase1 failure */ - ResetMocks(); + reset_common_data(); sd->flags |= VB2_SD_FLAG_DEV_MODE_ENABLED; kernel_phase1_retval = VB2_ERROR_MOCK; test_slk(VB2_ERROR_MOCK, 0, "Dev phase1 failure"); /* Boot recovery */ - ResetMocks(); + reset_common_data(); sd->recovery_reason = 123; vbboot_retval = -3; test_slk(VB2_ERROR_MOCK, 0, "Recovery boot bad"); - ResetMocks(); + 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"); /* Boot recovery - phase1 failure */ - ResetMocks(); + 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 */ - ResetMocks(); + 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"); - commit_data_retval = VB2_ERROR_UNKNOWN; - test_slk(0, 0, "Recovery return unknown write error"); - /* Boot recovery - nvstorage cleared */ - ResetMocks(); + reset_common_data(); sd->recovery_reason = 123; - vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, 5); - vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13); - test_slk(0, 0, "Recovery with nvstorage"); - TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE), - 0, " recovery subcode cleared"); + commit_data_retval = VB2_ERROR_UNKNOWN; + test_slk(0, 0, "Recovery return unknown write error"); /* Boot recovery - memory retraining */ - ResetMocks(); + reset_common_data(); sd->recovery_reason = VB2_RECOVERY_TRAIN_AND_REBOOT; test_slk(VBERROR_REBOOT_REQUIRED, 0, "Recovery train and reboot"); - // todo: rkr/w/l fail ignored if recovery - + /* 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"); + /* Boot manual recovery */ + reset_common_data(); + sd->recovery_reason = VB2_RECOVERY_RO_MANUAL; + vb2_nv_set(ctx, VB2_NV_RECOVERY_SUBCODE, 13); + sd->flags |= VB2_SD_FLAG_MANUAL_RECOVERY; + test_slk(0, 0, "Manual recovery"); } int main(void) { - VbSlkTest(); + select_and_load_kernel_tests(); return gTestSuccess ? 0 : 255; } -- cgit v1.2.1