vboot/altfw: fix up some altfw code

Rather than the caller picking and choosing between vb2_run_altfw
and vb2_try_alt_fw, always use the "try" function, and manually
specify the `allowed` argument.

Fix up some comment styles.

Reverse the order of vb2_commit_data and secdata_kernel_lock
calls, and check the return value of vb2_commit_data.

BUG=b:124141368
TEST=make clean && make runtests
BRANCH=none

Change-Id: Ib6b2752ba1b9be66401612a46f9c1b9353669c27
Signed-off-by: Joel Kitching <kitching@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1776286
Tested-by: Joel Kitching <kitching@chromium.org>
Commit-Queue: Joel Kitching <kitching@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
(cherry picked from commit 7d64b93ccf13623fa26ee865674e3b443ce253bd)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1947717
Reviewed-by: Shelley Chen <shchen@chromium.org>
Commit-Queue: Shelley Chen <shchen@chromium.org>
Tested-by: Shelley Chen <shchen@chromium.org>

4 files changed, 51 insertions, 45 deletions

diff --git a/firmware/lib/include/vboot_ui_common.h b/firmware/lib/include/vboot_ui_common.h
index f4ebe530..2c8337f7 100644
--- a/firmware/lib/include/vboot_ui_common.h
+++ b/firmware/lib/include/vboot_ui_common.h
@@ -32,17 +32,6 @@ void vb2_error_notify(const char *print_msg,
 		      const char *log_msg,
 		      enum vb2_beep_type beep);
 
-/**
- * Run alternative firmware if allowed
- *
- * This will only return if it is not allowed, or the bootloader fails to
- * cannot be found / fails to start
- *
- * @ctx		Context
- * @altfw_num	Number of bootloader to start (0=any, 1=first, etc.)
- */
-void vb2_run_altfw(struct vb2_context *ctx, enum VbAltFwIndex_t altfw_num);
-
 /** Display an error and beep to indicate that altfw is not available */
 void vb2_error_no_altfw(void);
 
@@ -52,14 +41,18 @@ void vb2_error_no_altfw(void);
  * This checks if the operation is permitted. If it is, then it jumps to the
  * selected bootloader and execution continues there, never returning.
  *
- * If the operation is not permitted, or it is permitted but the bootloader
- * cannot be found, it beeps and returns.
+ * Will beep and return if one of the following is true:
+ *   - operation is not permitted (allowed == 0)
+ *   - vboot data fails to commit
+ *   - secdata_kernel fails to lock
+ *   - bootloader cannot be found
+ *   - bootloader fails to start
  *
- * @ctx		Context
- * @allowed	1 if allowed, 0 if not allowed
- * @altfw_num	Number of bootloader to start (0=any, 1=first, etc.)
+ * @param ctx		Context pointer
+ * @param allowed	1 if allowed, 0 if not allowed
+ * @param altfw_num	Number of bootloader to start (0=any, 1=first, etc.)
  */
-void vb2_try_alt_fw(struct vb2_context *ctx, int allowed,
-		    enum VbAltFwIndex_t altfw_num);
+void vb2_try_altfw(struct vb2_context *ctx, int allowed,
+		   enum VbAltFwIndex_t altfw_num);
 
 #endif  /* VBOOT_REFERENCE_VBOOT_UI_COMMON_H_ */
diff --git a/firmware/lib/vboot_ui.c b/firmware/lib/vboot_ui.c
index bcfd2008..6e9e5169 100644
--- a/firmware/lib/vboot_ui.c
+++ b/firmware/lib/vboot_ui.c
@@ -211,7 +211,7 @@ static vb2_error_t vb2_altfw_ui(struct vb2_context *ctx)
 			 * This will not return if successful. Drop out to
 			 * developer mode on failure.
 			 */
-			vb2_run_altfw(ctx, key - '0');
+			vb2_try_altfw(ctx, 1, key - '0');
 			active = 0;
 			break;
 		default:
@@ -476,7 +476,7 @@ static vb2_error_t vb2_diagnostics_ui(struct vb2_context *ctx)
 
 		/*
 		 * The following helps avoid use of the TPM after
-		 * it's disabled (e.g., when vb2_run_altfw() calls
+		 * it's disabled (e.g., when vb2_try_altfw() calls
 		 * secdata_kernel_lock() ).
 		 */
 
@@ -488,7 +488,7 @@ static vb2_error_t vb2_diagnostics_ui(struct vb2_context *ctx)
 			VB2_DEBUG("Failed to disable TPM\n");
 			vb2api_fail(ctx, VB2_RECOVERY_TPM_DISABLE_FAILED, 0);
 		} else {
-			vb2_run_altfw(ctx, VB_ALTFW_DIAGNOSTIC);
+			vb2_try_altfw(ctx, 1, VB_ALTFW_DIAGNOSTIC);
 			VB2_DEBUG("Diagnostic failed to run\n");
 			/*
 			 * Assuming failure was due to bad hash, though
@@ -742,7 +742,7 @@ static vb2_error_t vb2_developer_ui(struct vb2_context *ctx)
 			VB2_DEBUG("VbBootDeveloper() - "
 				  "user pressed key '%c': Boot alternative "
 				  "firmware\n", key);
-			vb2_try_alt_fw(ctx, allow_legacy, key - '0');
+			vb2_try_altfw(ctx, allow_legacy, key - '0');
 			break;
 		default:
 			VB2_DEBUG("VbBootDeveloper() - pressed key %#x\n", key);
@@ -758,7 +758,7 @@ static vb2_error_t vb2_developer_ui(struct vb2_context *ctx)
 	/* If defaulting to legacy boot, try that unless Ctrl+D was pressed */
 	if (use_legacy && !ctrl_d_pressed) {
 		VB2_DEBUG("VbBootDeveloper() - defaulting to legacy\n");
-		vb2_try_alt_fw(ctx, allow_legacy, 0);
+		vb2_try_altfw(ctx, allow_legacy, 0);
 	}
 
 	if ((use_usb && !ctrl_d_pressed) && allow_usb) {
diff --git a/firmware/lib/vboot_ui_common.c b/firmware/lib/vboot_ui_common.c
index 5f51f530..5bd0500c 100644
--- a/firmware/lib/vboot_ui_common.c
+++ b/firmware/lib/vboot_ui_common.c
@@ -41,20 +41,6 @@ void vb2_error_notify(const char *print_msg,
 	vb2_error_beep(beep);
 }
 
-void vb2_run_altfw(struct vb2_context *ctx, enum VbAltFwIndex_t altfw_num)
-{
-	if (secdata_kernel_lock(ctx)) {
-		vb2_error_notify("Error locking kernel versions on legacy "
-				 "boot.\n", NULL, VB_BEEP_FAILED);
-	} else {
-		/* TODO: Figure out what to do on commit error in altfw. */
-		vb2_commit_data(ctx);
-		VbExLegacy(altfw_num);	/* will not return if found */
-		vb2_error_notify("Legacy boot failed. Missing BIOS?\n", NULL,
-				 VB_BEEP_FAILED);
-	}
-}
-
 void vb2_error_no_altfw(void)
 {
 	VB2_DEBUG("Legacy boot is disabled\n");
@@ -64,11 +50,38 @@ void vb2_error_no_altfw(void)
 	vb2_error_beep(VB_BEEP_NOT_ALLOWED);
 }
 
-void vb2_try_alt_fw(struct vb2_context *ctx, int allowed,
-		    enum VbAltFwIndex_t altfw_num)
+/**
+ * Run alternative firmware
+ *
+ * This will only return if vboot data fails to commit, secdata_kernel fails to
+ * lock, or the bootloader cannot be found / fails to start.
+ *
+ * @param ctx		Context pointer
+ * @param altfw_num	Number of bootloader to start (0=any, 1=first, etc.)
+ */
+void vb2_try_altfw(struct vb2_context *ctx, int allowed,
+		   enum VbAltFwIndex_t altfw_num)
 {
-	if (allowed)
-		vb2_run_altfw(ctx, altfw_num);	/* will not return if found */
-	else
+	if (!allowed) {
 		vb2_error_no_altfw();
+		return;
+	}
+
+	if (vb2_commit_data(ctx)) {
+		vb2_error_notify("Error committing data on legacy boot.\n",
+				 NULL, VB_BEEP_FAILED);
+		return;
+	}
+
+	if (secdata_kernel_lock(ctx)) {
+		vb2_error_notify("Error locking kernel versions on legacy "
+				 "boot.\n", NULL, VB_BEEP_FAILED);
+		return;
+	}
+
+	/* Will not return if successful */
+	VbExLegacy(altfw_num);
+
+	vb2_error_notify("Legacy boot failed. Missing BIOS?\n", NULL,
+			 VB_BEEP_FAILED);
 }
diff --git a/firmware/lib/vboot_ui_menu.c b/firmware/lib/vboot_ui_menu.c
index bc3d882c..08da08cb 100644
--- a/firmware/lib/vboot_ui_menu.c
+++ b/firmware/lib/vboot_ui_menu.c
@@ -162,7 +162,7 @@ static vb2_error_t boot_legacy_action(struct vb2_context *ctx)
 		return VBERROR_KEEP_LOOPING;
 	}
 
-	vb2_run_altfw(ctx, VB_ALTFW_DEFAULT);
+	vb2_try_altfw(ctx, 1, VB_ALTFW_DEFAULT);
 	vb2_flash_screen(ctx);
 	return VBERROR_KEEP_LOOPING;
 }
@@ -341,7 +341,7 @@ static vb2_error_t language_action(struct vb2_context *ctx)
 /* Action when selecting a bootloader in the alternative firmware menu. */
 static vb2_error_t altfw_action(struct vb2_context *ctx)
 {
-	vb2_run_altfw(ctx, current_menu_idx + 1);
+	vb2_try_altfw(ctx, 1, current_menu_idx + 1);
 	vb2_flash_screen(ctx);
 	VB2_DEBUG(no_legacy);
 	VbExDisplayDebugInfo(no_legacy, 0);
@@ -796,7 +796,7 @@ static vb2_error_t vb2_developer_menu(struct vb2_context *ctx)
 			VB2_DEBUG("VbBootDeveloper() - "
 				  "user pressed key '%c': Boot alternative "
 				  "firmware\n", key);
-			vb2_try_alt_fw(ctx, altfw_allowed, key - '0');
+			vb2_try_altfw(ctx, altfw_allowed, key - '0');
 			ret = VBERROR_KEEP_LOOPING;
 			break;
 		default: