From 3842121022bc7a9f30a879f9715ff9d416eae7ba Mon Sep 17 00:00:00 2001 From: Jett Rink Date: Fri, 22 Nov 2019 08:52:23 -0700 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1930931 --- common/host_command.c | 14 ++++++ include/host_command.h | 7 ++- test/host_command.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++- 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(); } -- cgit v1.2.1