futility: updater: Improve error message when key conflicts

Many firmware developers will try to flash a local built firmware (i.e,
DEV key signed) on a MP device (with write protection enabled).

The updater used to provide feedback like:

ERROR: verify_keyblock: Failed verifying key block.
INFO: Current (RO) firmware image has root key: ade780ffd0...732867181bae
WARNING: Target (RW) image is signed by rootkey: b11d74edd2...e1135b49e7f0.
ERROR: RW not signed by same RO root key
>> FAILED: Firmware updater aborted.

This is correctly identifying the root cause, but not helpful for
developers to figure out what to do, and may be confused with the DEV
re-key safety check (which needs --force).

Also, when developers try to do "--mode=factory --force", the message
was:

updater_setup_config: Factory mode needs WP disabled.

Where the 'WP' is again not clear enough.

With this change, we're improving the error messages so that:
 - Being consistent on 'root key' instead of 'rootkey'.
 - Being consistent for having period for error messages, except those
   ended with root key hash (for easier copy-paste).
 - Say 'Write Protection' instead of 'WP'.
 - When re-keying with WP enabled, print a better hint:
   "To change keys in RO area, you have to first remove write protection
   (https://goo.gl/ces83U)."

BUG=None
TEST=make futil; tests/futility/run_test_scripts.sh $(pwd)/build/futility
BRANCH=none

Change-Id: Ia74d7b113766d09428a4d0897918b4f17b4afae7
Reviewed-on: https://chromium-review.googlesource.com/1465709
Commit-Ready: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Matthew Blecker <matthewb@chromium.org>

2 files changed, 30 insertions, 19 deletions

diff --git a/futility/updater.c b/futility/updater.c
index d1352ef1..0880208d 100644
--- a/futility/updater.c
+++ b/futility/updater.c
@@ -26,6 +26,7 @@
 #define COMMAND_BUFFER_SIZE 256
 #define RETURN_ON_FAILURE(x) do {int r = (x); if (r) return r;} while (0);
 #define FLASHROM_OUTPUT_WP_PATTERN "write protect is "
+#define REMOVE_WP_URL "https://goo.gl/ces83U"
 
 /* System environment values. */
 static const char * const FWACT_A = "A",
@@ -1291,9 +1292,9 @@ static enum rootkey_compat_result check_compatible_root_key(
 			      "Maybe RW corrupted?");
 			return ROOTKEY_COMPAT_ERROR;
 		}
-		WARN("Target (RW) image is signed by rootkey: %s.",
+		WARN("Target (RW) image is signed by root key: %s%s",
 		     rootkey_rw ? packed_key_sha1_string(rootkey_rw) :
-		     "<invalid>");
+		     "<invalid>", to_dev ? " (DEV/unsigned)" : "");
 		return to_dev ? ROOTKEY_COMPAT_REKEY_TO_DEV :
 				ROOTKEY_COMPAT_REKEY;
 	}
@@ -1421,7 +1422,8 @@ const char * const updater_error_messages[] = {
 	[UPDATE_ERR_WRITE_FIRMWARE] = "Failed writing firmware.",
 	[UPDATE_ERR_PLATFORM] = "Your system platform is not compatible.",
 	[UPDATE_ERR_TARGET] = "No valid RW target to update. Abort.",
-	[UPDATE_ERR_ROOT_KEY] = "RW not signed by same RO root key",
+	[UPDATE_ERR_ROOT_KEY] = "RW signed by incompatible root key "
+			        "(different from RO).",
 	[UPDATE_ERR_TPM_ROLLBACK] = "RW not usable due to TPM anti-rollback.",
 	[UPDATE_ERR_UNKNOWN] = "Unknown error.",
 };
@@ -1501,7 +1503,7 @@ static enum updater_error_codes update_try_rw_firmware(
  * This was also known as --mode=recovery, --wp=1 in legacy updater.
  * Returns UPDATE_ERR_DONE if success, otherwise error.
  */
-static enum updater_error_codes update_rw_firmrware(
+static enum updater_error_codes update_rw_firmware(
 		struct updater_config *cfg,
 		struct firmware_image *image_from,
 		struct firmware_image *image_to)
@@ -1560,7 +1562,6 @@ static enum updater_error_codes update_whole_firmware(
 	if (preserve_images(cfg))
 		DEBUG("Failed to preserve some sections - ignore.");
 
-
 	INFO("Checking compatibility...");
 	if (check_compatible_tpm_keys(cfg, image_to))
 		return UPDATE_ERR_TPM_ROLLBACK;
@@ -1598,7 +1599,9 @@ static enum updater_error_codes update_whole_firmware(
  */
 enum updater_error_codes update_firmware(struct updater_config *cfg)
 {
-	int wp_enabled;
+	int wp_enabled, done = 0;
+	enum updater_error_codes r = UPDATE_ERR_UNKNOWN;
+
 	struct firmware_image *image_from = &cfg->image_current,
 			      *image_to = &cfg->image;
 	if (!image_to->data)
@@ -1649,18 +1652,25 @@ enum updater_error_codes update_firmware(struct updater_config *cfg)
 		return update_legacy_firmware(cfg, image_to);
 
 	if (cfg->try_update) {
-		enum updater_error_codes r;
 		r = update_try_rw_firmware(cfg, image_from, image_to,
 					   wp_enabled);
-		if (r != UPDATE_ERR_NEED_RO_UPDATE)
-			return r;
-		WARN("%s", updater_error_messages[r]);
+		if (r == UPDATE_ERR_NEED_RO_UPDATE)
+			WARN("%s", updater_error_messages[r]);
+		else
+			done = 1;
 	}
 
-	if (wp_enabled)
-		return update_rw_firmrware(cfg, image_from, image_to);
-	else
-		return update_whole_firmware(cfg, image_to);
+	if (!done) {
+		r = wp_enabled ? update_rw_firmware(cfg, image_from, image_to) :
+				 update_whole_firmware(cfg, image_to);
+	}
+
+	/* Providing more hints for what to do on failure. */
+	if (r == UPDATE_ERR_ROOT_KEY && wp_enabled)
+		ERROR("To change keys in RO area, you must first remove "
+		      "write protection ( " REMOVE_WP_URL " ).");
+
+	return r;
 }
 
 /*
@@ -2060,7 +2070,8 @@ int updater_setup_config(struct updater_config *cfg,
 	}
 	if (check_wp_disabled && is_write_protection_enabled(cfg)) {
 		errorcnt++;
-		ERROR("Factory mode needs WP disabled.");
+		ERROR("Please remove write protection for factory mode "
+		      "( " REMOVE_WP_URL " ).");
 	}
 	if (!errorcnt && do_output) {
 		const char *r = arg->output_dir;
diff --git a/tests/futility/test_update.sh b/tests/futility/test_update.sh
index 302bf26a..eac9933a 100755
--- a/tests/futility/test_update.sh
+++ b/tests/futility/test_update.sh
@@ -215,7 +215,7 @@ test_update "RW update (incompatible platform)" \
 	-i "${LINK_BIOS}" --wp=1 --sys_props 0,0x10001,1
 
 test_update "RW update (incompatible rootkey)" \
-	"${FROM_DIFFERENT_ROOTKEY_IMAGE}" "!RW not signed by same RO root key" \
+	"${FROM_DIFFERENT_ROOTKEY_IMAGE}" "!RW signed by incompatible root key" \
 	-i "${TO_IMAGE}" --wp=1 --sys_props 0,0x10001,1
 
 test_update "RW update (TPM Anti-rollback: data key)" \
@@ -243,7 +243,7 @@ test_update "RW update (incompatible platform)" \
 	-i "${LINK_BIOS}" -t --wp=1 --sys_props 0x10001,1
 
 test_update "RW update (incompatible rootkey)" \
-	"${FROM_DIFFERENT_ROOTKEY_IMAGE}" "!RW not signed by same RO root key" \
+	"${FROM_DIFFERENT_ROOTKEY_IMAGE}" "!RW signed by incompatible root key" \
 	-i "${TO_IMAGE}" -t --wp=1 --sys_props 0,0x10001,1
 
 test_update "RW update (TPM Anti-rollback: data key)" \
@@ -277,11 +277,11 @@ test_update "Factory mode update (WP=0)" \
 	--factory -i "${TO_IMAGE}" --wp=0 --sys_props 0,0x10001,1
 
 test_update "Factory mode update (WP=1)" \
-	"${FROM_IMAGE}" "!needs WP disabled" \
+	"${FROM_IMAGE}" "!remove write protection for factory mode" \
 	-i "${TO_IMAGE}" --wp=1 --sys_props 0,0x10001,1 --mode=factory
 
 test_update "Factory mode update (WP=1)" \
-	"${FROM_IMAGE}" "!needs WP disabled" \
+	"${FROM_IMAGE}" "!remove write protection for factory mode" \
 	--factory -i "${TO_IMAGE}" --wp=1 --sys_props 0,0x10001,1
 
 test_update "Factory mode update (GBB=0 -> 39)" \