host_command: clear any leftover data

We want to ensure that the entire buffer we may be sending back to the
host from the EC does not contain any data from previous host command
responses. Clear the data in common code so all chips do not have to
implement this functionality.

BRANCH=none
BUG=b:144878983,chromium:1026994
TEST=new unit test shows cleared data

Change-Id: I93ad4d36923ba1bf171f740e94830640d3fde3b0
Signed-off-by: Jett Rink <jettrink@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1930931

3 files changed, 153 insertions, 2 deletions

diff --git a/common/host_command.c b/common/host_command.c
index eff1fac56b..c3a207937e 100644
--- a/common/host_command.c
+++ b/common/host_command.c
@@ -681,6 +681,20 @@ uint16_t host_command_process(struct host_cmd_handler_args *args)
 	if (hcdebug)
 		host_command_debug_request(args);
 
+	/*
+	 * Pre-emptively clear the entire response buffer so we do not
+	 * have any left over contents from previous host commands.
+	 * For example, this prevents the last portion of a char array buffer
+	 * from containing data from the last host command if the string does
+	 * not take the entire width of the char array buffer.
+	 *
+	 * Note that if request and response buffers pointed to the same memory
+	 * location, then the chip implementation already needed to provide a
+	 * request_temp buffer in which the request data was already copied
+	 * by this point (see host_packet_receive function).
+	 */
+	memset(args->response, 0, args->response_max);
+
 #ifdef CONFIG_HOSTCMD_PD
 	if (args->command >= EC_CMD_PASSTHRU_OFFSET(1) &&
 	    args->command <= EC_CMD_PASSTHRU_MAX(1)) {
diff --git a/include/host_command.h b/include/host_command.h
index 10d7fc37ea..8ace629505 100644
--- a/include/host_command.h
+++ b/include/host_command.h
@@ -67,7 +67,12 @@ struct host_packet {
 	 */
 	void (*send_response)(struct host_packet *pkt);
 
-	/* Input request data */
+	/*
+	 * Input request data. If request and response buffers overlap,
+	 * then request_temp must be non-null and be large enough to store the
+	 * entire request buffer. The request_temp buffer will then be used
+	 * as the buffer passed into the command handlers.
+	 */
 	const void *request;
 
 	/*
diff --git a/test/host_command.c b/test/host_command.c
index 35b6728ea6..4b51a6f8fc 100644
--- a/test/host_command.c
+++ b/test/host_command.c
@@ -24,6 +24,8 @@ struct ec_params_hello *p = (struct ec_params_hello *)(req_buf + sizeof(*req));
 struct ec_host_response *resp = (struct ec_host_response *)resp_buf;
 struct ec_response_hello *r =
 	(struct ec_response_hello *)(resp_buf + sizeof(*resp));
+struct ec_response_get_chip_info *chip_info_r =
+	(struct ec_response_get_chip_info *)(resp_buf + sizeof(*resp));
 
 static void hostcmd_respond(struct host_packet *pkt)
 {
@@ -58,9 +60,9 @@ static void hostcmd_fill_in_default(void)
 	req->data_len = 4;
 	p->in_data = 0x11223344;
 
-	pkt.request_size = 0;
 	pkt.send_response = hostcmd_respond;
 	pkt.request = (const void *)req_buf;
+	pkt.request_temp = NULL;
 	pkt.request_max = BUFFER_SIZE;
 	pkt.request_size = sizeof(*req) + sizeof(*p);
 	pkt.response = (void *)resp_buf;
@@ -170,6 +172,134 @@ static int test_hostcmd_invalid_checksum(void)
 	return EC_SUCCESS;
 }
 
+static int test_hostcmd_reuse_response_buffer(void)
+{
+	struct ec_host_request *h = (struct ec_host_request *)resp_buf;
+	struct ec_params_hello *d =
+		(struct ec_params_hello *)(resp_buf + sizeof(*h));
+
+	h->struct_version = 3;
+	h->checksum = 0;
+	h->command = EC_CMD_HELLO;
+	h->command_version = 0;
+	h->reserved = 0;
+	h->data_len = 4;
+	d->in_data = 0x11223344;
+
+	pkt.send_response = hostcmd_respond;
+	/*
+	 * The original request buffer is shared with the response and the
+	 * request buffer is used as the temporary buffer
+	 */
+	pkt.request = (const void *)resp_buf;
+	pkt.request_temp = req_buf;
+	pkt.request_max = BUFFER_SIZE;
+	pkt.request_size = sizeof(*h) + sizeof(*d);
+	pkt.response = (void *)resp_buf;
+	pkt.response_max = BUFFER_SIZE;
+	pkt.driver_result = 0;
+
+	h->checksum = calculate_checksum(resp_buf, pkt.request_size);
+
+	ccprintf("\nBuffer contents before process 0x%ph\n",
+			HEX_BUF(resp_buf, BUFFER_SIZE));
+	host_packet_receive(&pkt);
+	task_wait_event(-1);
+
+	ccprintf("\nBuffer contents after process 0x%ph\n",
+			HEX_BUF(resp_buf, BUFFER_SIZE));
+
+	TEST_EQ(calculate_checksum(resp_buf,
+				sizeof(*resp) + resp->data_len), 0, "%d");
+	TEST_EQ(resp->result, EC_RES_SUCCESS, "%d");
+	TEST_EQ(r->out_data, 0x12243648, "0x%x");
+
+	return EC_SUCCESS;
+}
+
+static void hostcmd_fill_chip_info(void)
+{
+	req->struct_version = 3;
+	req->checksum = 0;
+	req->command = EC_CMD_GET_CHIP_INFO;
+	req->command_version = 0;
+	req->reserved = 0;
+	req->data_len = 0;
+
+	pkt.send_response = hostcmd_respond;
+	pkt.request = (const void *)req_buf;
+	pkt.request_temp = NULL;
+	pkt.request_max = BUFFER_SIZE;
+	pkt.request_size = sizeof(*req);
+	pkt.response = (void *)resp_buf;
+	pkt.response_max = BUFFER_SIZE;
+	pkt.driver_result = 0;
+}
+
+static int test_hostcmd_clears_unused_data(void)
+{
+	int i, found_null;
+
+	/* Set the buffer to junk and ensure that is gets cleared */
+	memset(resp_buf, 0xAA, BUFFER_SIZE);
+	hostcmd_fill_chip_info();
+
+	hostcmd_send();
+
+	ccprintf("\nBuffer contents 0x%ph\n",
+			HEX_BUF(resp_buf, BUFFER_SIZE));
+
+	TEST_EQ(calculate_checksum(resp_buf,
+				sizeof(*resp) + resp->data_len), 0, "%d");
+	TEST_EQ(resp->result, EC_RES_SUCCESS, "%d");
+
+	/* Ensure partial strings have 0s after the NULL byte */
+	found_null = 0;
+	for (i = 0; i < sizeof(chip_info_r->name); ++i) {
+		if (!chip_info_r->name[i])
+			found_null = 1;
+
+		if (found_null) {
+			if (chip_info_r->name[i])
+				ccprintf("\nByte %d is not zero!\n", i);
+			TEST_EQ(chip_info_r->name[i], 0, "0x%x");
+		}
+	}
+
+	found_null = 0;
+	for (i = 0; i < sizeof(chip_info_r->revision); ++i) {
+		if (!chip_info_r->revision[i])
+			found_null = 1;
+
+		if (found_null) {
+			if (chip_info_r->revision[i])
+				ccprintf("\nByte %d is not zero!\n", i);
+			TEST_EQ(chip_info_r->revision[i], 0, "0x%x");
+		}
+	}
+
+	found_null = 0;
+	for (i = 0; i < sizeof(chip_info_r->vendor); ++i) {
+		if (!chip_info_r->vendor[i])
+			found_null = 1;
+
+		if (found_null) {
+			if (chip_info_r->vendor[i])
+				ccprintf("\nByte %d is not zero!\n", i);
+			TEST_EQ(chip_info_r->vendor[i], 0, "0x%x");
+		}
+	}
+
+	/* Ensure rest of buffer after valid response is also 0 */
+	for (i = resp->data_len + sizeof(*resp) + 1; i < BUFFER_SIZE; ++i) {
+		if (resp_buf[i])
+			ccprintf("\nByte %d is not zero!\n", i);
+		TEST_EQ(resp_buf[i], 0, "0x%x");
+	}
+
+	return EC_SUCCESS;
+}
+
 void run_test(void)
 {
 	wait_for_task_started();
@@ -183,6 +313,8 @@ void run_test(void)
 	RUN_TEST(test_hostcmd_wrong_command_version);
 	RUN_TEST(test_hostcmd_wrong_struct_version);
 	RUN_TEST(test_hostcmd_invalid_checksum);
+	RUN_TEST(test_hostcmd_reuse_response_buffer);
+	RUN_TEST(test_hostcmd_clears_unused_data);
 
 	test_print_result();
 }