ec_commands: Add ec_response_get_version_v1

A field (cros_fwid_rw) was added to ec_response_get_version and the
version was updated to v1. Some system components that still use v0
of the version host command fail because the size of the response
does not match the updated ec_response_get_version struct.

Restore ec_response_get_version to match v0. Create a new
ec_response_get_version_v1 structure with the added v1 fields.
This allows legacy code to continue using ec_response_get_version,
which matches the expected response size for the v0 command.

BUG=b:188073399,b:200075921
TEST=EC console 'version' works
Legacy 'ectool version' works with old an new EC firmware.
New 'ectool version' works with old and new EC firmware.
BRANCH=None

Change-Id: I51a052a550c2460f2604da8e04fc43c36acba4d5
Signed-off-by: Rob Barnes <robbarnes@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3169100
Reviewed-by: caveh jalali <caveh@chromium.org>
Reviewed-by: Patryk Duda <patrykd@google.com>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>

3 files changed, 51 insertions, 21 deletions

diff --git a/common/system.c b/common/system.c
index 985d64752e..1670857241 100644
--- a/common/system.c
+++ b/common/system.c
@@ -1568,16 +1568,12 @@ DECLARE_CONSOLE_COMMAND(rflags, command_rflags,
 static enum ec_status
 host_command_get_version(struct host_cmd_handler_args *args)
 {
-	struct ec_response_get_version *r = args->response;
+	struct ec_response_get_version_v1 *r = args->response;
 	enum ec_image active_slot = system_get_active_copy();
 
-	/* Clear optional fields (i.e. cros_fwid). */
-	memset(r, 0, sizeof(*r));
-
 	strzcpy(r->version_string_ro, system_get_version(EC_IMAGE_RO),
 		sizeof(r->version_string_ro));
-	strzcpy(r->version_string_rw,
-		system_get_version(active_slot),
+	strzcpy(r->version_string_rw, system_get_version(active_slot),
 		sizeof(r->version_string_rw));
 
 	switch (system_get_image_copy()) {
@@ -1593,18 +1589,30 @@ host_command_get_version(struct host_cmd_handler_args *args)
 		break;
 	}
 
+	/*
+	 * Assuming args->response is zero'd in host_command_process, so no need
+	 * to zero uninitialized fields here.
+	 */
 	if (args->version > 0 && IS_ENABLED(CONFIG_CROS_FWID_VERSION)) {
 		strzcpy(r->cros_fwid_ro, system_get_cros_fwid(EC_IMAGE_RO),
 			sizeof(r->cros_fwid_ro));
 		strzcpy(r->cros_fwid_rw, system_get_cros_fwid(EC_IMAGE_RW),
 			sizeof(r->cros_fwid_rw));
 	}
+
+	/*
+	 * By convention, ec_response_get_version_v1 is a strict superset of
+	 * ec_response_get_version(v0). The v1 response changes the semantics
+	 * of one field (reserved to cros_fwid_ro) and adds one additional field
+	 * (cros_fwid_rw). So simply adjusting the response size here is safe.
+	 */
 	if (args->version == 0)
-		/* cros_fwid_rw[32] is not present in version 0 */
-		args->response_size =
-			offsetof(struct ec_response_get_version, cros_fwid_rw);
-	else
 		args->response_size = sizeof(struct ec_response_get_version);
+	else if (args->version == 1)
+		args->response_size = sizeof(struct ec_response_get_version_v1);
+	else
+		/* Shouldn't happen because of EC_VER_MASK */
+		return EC_RES_INVALID_VERSION;
 
 	return EC_RES_SUCCESS;
 }
diff --git a/include/ec_commands.h b/include/ec_commands.h
index 6e9c538e5e..95431babad 100644
--- a/include/ec_commands.h
+++ b/include/ec_commands.h
@@ -1149,14 +1149,33 @@ enum ec_image {
 };
 
 /**
- * struct ec_response_get_version - Response to the get version command.
+ * struct ec_response_get_version - Response to the v0 get version command.
+ * @version_string_ro: Null-terminated RO firmware version string.
+ * @version_string_rw: Null-terminated RW firmware version string.
+ * @reserved: Unused bytes; was previously RW-B firmware version string.
+ * @current_image: One of ec_image.
+ */
+struct ec_response_get_version {
+	char version_string_ro[32];
+	char version_string_rw[32];
+	char reserved[32];  /* Changed to cros_fwid_ro in version 1 */
+	uint32_t current_image;
+} __ec_align4;
+
+/**
+ * struct ec_response_get_version_v1 - Response to the v1 get version command.
+ *
+ * ec_response_get_version_v1 is a strict superset of ec_response_get_version.
+ * The v1 response changes the semantics of one field (reserved to cros_fwid_ro)
+ * and adds one additional field (cros_fwid_rw).
+ *
  * @version_string_ro: Null-terminated RO firmware version string.
  * @version_string_rw: Null-terminated RW firmware version string.
  * @cros_fwid_ro: Null-terminated RO CrOS FWID string.
  * @current_image: One of ec_image.
  * @cros_fwid_rw: Null-terminated RW CrOS FWID string.
  */
-struct ec_response_get_version {
+struct ec_response_get_version_v1 {
 	char version_string_ro[32];
 	char version_string_rw[32];
 	char cros_fwid_ro[32];  /* Added in version 1 (Used to be reserved) */
diff --git a/util/ectool.c b/util/ectool.c
index 4f983e24f1..79e496f262 100644
--- a/util/ectool.c
+++ b/util/ectool.c
@@ -1069,23 +1069,26 @@ int cmd_uptimeinfo(int argc, char *argv[])
 
 int cmd_version(int argc, char *argv[])
 {
-	struct ec_response_get_version r;
+	struct ec_response_get_version_v1 r;
 	char *build_string = (char *)ec_inbuf;
-	int cmdver = 1;
 	int rv;
 
-	if (!ec_cmd_version_supported(EC_CMD_GET_VERSION, 1)) {
-		cmdver = 0;
-		/* CrOS FWID is not supported. Set it to empty string. */
+	if (ec_cmd_version_supported(EC_CMD_GET_VERSION, 1)) {
+		rv = ec_command(EC_CMD_GET_VERSION, 1, NULL, 0, &r,
+				sizeof(struct ec_response_get_version_v1));
+	} else {
+		/* Fall-back to version 0 if version 1 is not supported */
+		rv = ec_command(EC_CMD_GET_VERSION, 0, NULL, 0, &r,
+				sizeof(struct ec_response_get_version));
+		/* These fields are not supported in version 0, ensure empty */
 		r.cros_fwid_ro[0] = '\0';
 		r.cros_fwid_rw[0] = '\0';
 	}
-
-	rv = ec_command(EC_CMD_GET_VERSION, cmdver, NULL, 0, &r, sizeof(r));
 	if (rv < 0) {
 		fprintf(stderr, "ERROR: EC_CMD_GET_VERSION failed: %d\n", rv);
 		goto exit;
 	}
+
 	rv = ec_command(EC_CMD_GET_BUILD_INFO, 0,
 			NULL, 0, ec_inbuf, ec_max_insize);
 	if (rv < 0) {
@@ -1104,10 +1107,10 @@ int cmd_version(int argc, char *argv[])
 	r.cros_fwid_rw[sizeof(r.cros_fwid_rw) - 1] = '\0';
 	/* Print versions */
 	printf("RO version:    %s\n", r.version_string_ro);
-	if (cmdver > 0 && strlen(r.cros_fwid_ro))
+	if (strlen(r.cros_fwid_ro))
 		printf("RO cros fwid:  %s\n", r.cros_fwid_ro);
 	printf("RW version:    %s\n", r.version_string_rw);
-	if (cmdver > 0 && strlen(r.cros_fwid_rw))
+	if (strlen(r.cros_fwid_rw))
 		printf("RW cros fwid:  %s\n", r.cros_fwid_rw);
 	printf("Firmware copy: %s\n",
 	       (r.current_image < ARRAY_SIZE(image_names) ?