From 73bc2a9a0af97915b351a1ced0d6d16a0c369559 Mon Sep 17 00:00:00 2001 From: Nicholas Bishop Date: Mon, 8 Aug 2022 11:12:38 -0400 Subject: 2misc: Abort before using GBB if gbb_offset is not initialized In vb2_get_gbb, abort if gbb_offset is zero. This ensures that functions like vb2api_gbb_get_flags won't try to read garbage GBB data if the context hasn't been properly initialized. Some additional changes made to fix tests: 1. In vb2_set_boot_mode, don't access GBB unless needed. 2. In vb2api_get_dev_default_boot_target, use vb2api_gbb_get_flags instead of vb2_get_gbb to make it easier to mock. This is needed for depthcharge tests. 3. Make vb2api_get_debug_info tolerant of GBB not being set. This is needed for depthcharge tests. BUG=b:237093169 BRANCH=none TEST=make && make runtests Cq-Depend: chromium:3820402 Change-Id: I921d6cc4a5d91c8114c5e46748b4576a1e7716d0 Signed-off-by: Nicholas Bishop Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/3817941 Reviewed-by: Julius Werner --- Makefile | 2 ++ firmware/2lib/2misc.c | 28 ++++++++++++++++++---------- tests/vb2_gbb_init_tests.c | 26 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 10 deletions(-) create mode 100644 tests/vb2_gbb_init_tests.c diff --git a/Makefile b/Makefile index df906a2f..95d14239 100644 --- a/Makefile +++ b/Makefile @@ -738,6 +738,7 @@ TEST2X_NAMES = \ tests/vb2_crypto_tests \ tests/vb2_ec_sync_tests \ tests/vb2_firmware_tests \ + tests/vb2_gbb_init_tests \ tests/vb2_gbb_tests \ tests/vb2_host_flashrom_tests \ tests/vb2_host_key_tests \ @@ -1294,6 +1295,7 @@ run2tests: install_for_test ${RUNTEST} ${BUILD_RUN}/tests/vb2_crypto_tests ${RUNTEST} ${BUILD_RUN}/tests/vb2_ec_sync_tests ${RUNTEST} ${BUILD_RUN}/tests/vb2_firmware_tests + ${RUNTEST} ${BUILD_RUN}/tests/vb2_gbb_init_tests ${RUNTEST} ${BUILD_RUN}/tests/vb2_gbb_tests ${RUNTEST} ${BUILD_RUN}/tests/vb2_host_key_tests ${RUNTEST} ${BUILD_RUN}/tests/vb2_load_kernel_tests diff --git a/firmware/2lib/2misc.c b/firmware/2lib/2misc.c index 57351278..62a1ab3e 100644 --- a/firmware/2lib/2misc.c +++ b/firmware/2lib/2misc.c @@ -34,6 +34,8 @@ test_mockable struct vb2_gbb_header *vb2_get_gbb(struct vb2_context *ctx) { struct vb2_shared_data *sd = vb2_get_sd(ctx); + if (sd->gbb_offset == 0) + VB2_DIE("gbb_offset is not initialized\n"); return (struct vb2_gbb_header *)((void *)sd + sd->gbb_offset); } @@ -530,9 +532,7 @@ int vb2api_diagnostic_ui_enabled(struct vb2_context *ctx) enum vb2_dev_default_boot_target vb2api_get_dev_default_boot_target( struct vb2_context *ctx) { - struct vb2_gbb_header *gbb = vb2_get_gbb(ctx); - - if (gbb->flags & VB2_GBB_FLAG_DEFAULT_DEV_BOOT_ALTFW) + if (vb2api_gbb_get_flags(ctx) & VB2_GBB_FLAG_DEFAULT_DEV_BOOT_ALTFW) return VB2_DEV_DEFAULT_BOOT_TARGET_ALTFW; switch (vb2_nv_get(ctx, VB2_NV_DEV_DEFAULT_BOOT)) { @@ -608,7 +608,7 @@ char *vb2api_get_debug_info(struct vb2_context *ctx) uint32_t used = 0; struct vb2_shared_data *sd = vb2_get_sd(ctx); - struct vb2_gbb_header *gbb = vb2_get_gbb(ctx); + struct vb2_gbb_header *gbb = NULL; struct vb2_workbuf wb; char sha1sum[VB2_SHA1_DIGEST_SIZE * 2 + 1]; @@ -621,8 +621,14 @@ char *vb2api_get_debug_info(struct vb2_context *ctx) vb2_workbuf_from_ctx(ctx, &wb); + if (sd->gbb_offset == 0) { + DEBUG_INFO_APPEND("GBB: {INVALID}"); + } else { + gbb = vb2_get_gbb(ctx); + } + /* Add hardware ID */ - { + if (gbb) { char hwid[VB2_GBB_HWID_MAX_SIZE]; uint32_t size = sizeof(hwid); rv = vb2api_gbb_read_hwid(ctx, hwid, &size); @@ -674,10 +680,12 @@ char *vb2api_get_debug_info(struct vb2_context *ctx) sd->fw_version_secdata, sd->kernel_version_secdata); /* Add GBB flags */ - DEBUG_INFO_APPEND("\ngbb.flags: %#.8x", gbb->flags); + if (gbb) { + DEBUG_INFO_APPEND("\ngbb.flags: %#.8x", gbb->flags); + } /* Add sha1sum for Root & Recovery keys */ - { + if (gbb) { struct vb2_packed_key *key; struct vb2_workbuf wblocal = wb; rv = vb2_gbb_read_root_key(ctx, &key, NULL, &wblocal); @@ -687,7 +695,7 @@ char *vb2api_get_debug_info(struct vb2_context *ctx) } } - { + if (gbb) { struct vb2_packed_key *key; struct vb2_workbuf wblocal = wb; rv = vb2_gbb_read_recovery_key(ctx, &key, NULL, &wblocal); @@ -713,7 +721,6 @@ char *vb2api_get_debug_info(struct vb2_context *ctx) void vb2_set_boot_mode(struct vb2_context *ctx) { struct vb2_shared_data *sd = vb2_get_sd(ctx); - struct vb2_gbb_header *gbb = vb2_get_gbb(ctx); /* Cast boot mode to non-constant and assign */ enum vb2_boot_mode *boot_mode = (enum vb2_boot_mode *)&ctx->boot_mode; @@ -730,7 +737,8 @@ void vb2_set_boot_mode(struct vb2_context *ctx) (ctx->flags & VB2_CONTEXT_EC_TRUSTED)) { *boot_mode = VB2_BOOT_MODE_MANUAL_RECOVERY; } else if (sd->recovery_reason) { - if (gbb->flags & VB2_GBB_FLAG_FORCE_MANUAL_RECOVERY) + vb2_gbb_flags_t gbb_flags = vb2api_gbb_get_flags(ctx); + if (gbb_flags & VB2_GBB_FLAG_FORCE_MANUAL_RECOVERY) *boot_mode = VB2_BOOT_MODE_MANUAL_RECOVERY; else *boot_mode = VB2_BOOT_MODE_BROKEN_SCREEN; diff --git a/tests/vb2_gbb_init_tests.c b/tests/vb2_gbb_init_tests.c new file mode 100644 index 00000000..9a3989c0 --- /dev/null +++ b/tests/vb2_gbb_init_tests.c @@ -0,0 +1,26 @@ +/* Copyright 2022 The ChromiumOS Authors. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + * + * Test that vb2_get_gbb aborts if gbb_offset is not initialized. This + * is in a separate file from vb2_gbb_tests so that vb2_get_gbb is not + * mocked. + */ + +#include "2common.h" +#include "common/tests.h" + +static void test_abort_if_gbb_uninit(void) { + struct vb2_context *ctx; + uint8_t workbuf[VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE] + __attribute__((aligned(VB2_WORKBUF_ALIGN))); + TEST_SUCC(vb2api_init(workbuf, sizeof(workbuf), &ctx), + "vb2api_init failed"); + TEST_ABORT(vb2_get_gbb(ctx), "gbb_offset is not initialized"); +} + +int main(int argc, char *argv[]) +{ + test_abort_if_gbb_uninit(); + return gTestSuccess ? 0 : 255; +} -- cgit v1.2.1