system: Ensure space for panic and jump data

Ensure space is available for end of ram data. End of ram data must be
located at the very end of noinit ram. This currently includes
panic_data followed by jump_data.

This is being enforced with linker asserts and build asserts rather
than allocating the space directly so RAM utiliztion reports are still
relevant.

Introduce PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE config option and
default it to 1KB. This can be adjusted on boards that are
constrained.

BUG=b:246778588,b:246798928
BRANCH=None
TEST=./twister -c -s zephyr/test/jump_tags/jump_tags.default
&& make run-kb_8042

Change-Id: I444bbe3a583396b4f9b104bb6999e78ae3ff6f2f
Signed-off-by: Rob Barnes <robbarnes@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3896272
Reviewed-by: Aaron Massey <aaronmassey@google.com>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com>
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>

15 files changed, 360 insertions, 9 deletions

diff --git a/common/panic_output.c b/common/panic_output.c
index 2f92e65514..a5bba9d184 100644
--- a/common/panic_output.c
+++ b/common/panic_output.c
@@ -208,7 +208,7 @@ struct panic_data *get_panic_data_write(void)
 	 * end of RAM.
 	 */
 	struct panic_data *const pdata_ptr = PANIC_DATA_PTR;
-	const struct jump_data *jdata_ptr;
+	struct jump_data *jdata_ptr;
 	uintptr_t data_begin;
 	size_t move_size;
 	int delta;
@@ -264,6 +264,16 @@ struct panic_data *get_panic_data_write(void)
 		move_size = 0;
 	}
 
+	/* Check if there's enough space for jump tags after move */
+	if (data_begin - move_size < JUMP_DATA_MIN_ADDRESS) {
+		/* Not enough room for jump tags, clear tags.
+		 * TODO(b/251190975): This failure should be reported
+		 * in the panic data structure for more visibility.
+		 */
+		move_size -= jdata_ptr->jump_tag_total;
+		jdata_ptr->jump_tag_total = 0;
+	}
+
 	data_begin -= move_size;
 
 	if (move_size != 0) {
diff --git a/common/system.c b/common/system.c
index 375cca2882..af5d670216 100644
--- a/common/system.c
+++ b/common/system.c
@@ -67,6 +67,11 @@ static enum ec_reboot_cmd reboot_at_shutdown;
 
 static enum sysinfo_flags system_info_flags;
 
+/* Ensure enough space for panic_data, jump_data and at least one jump tag */
+BUILD_ASSERT((sizeof(struct panic_data) + sizeof(struct jump_data) +
+	      JUMP_TAG_MAX_SIZE) <= CONFIG_PRESERVED_END_OF_RAM_SIZE,
+	     "End of ram data size is too small for panic and jump data");
+
 STATIC_IF(CONFIG_HIBERNATE) uint32_t hibernate_seconds;
 STATIC_IF(CONFIG_HIBERNATE) uint32_t hibernate_microseconds;
 
@@ -349,15 +354,24 @@ test_mockable int system_jumped_late(void)
 int system_add_jump_tag(uint16_t tag, int version, int size, const void *data)
 {
 	struct jump_tag *t;
+	size_t new_entry_size;
 
 	/* Only allowed during a sysjump */
 	if (!jdata || jdata->magic != JUMP_DATA_MAGIC)
 		return EC_ERROR_UNKNOWN;
 
 	/* Make room for the new tag */
-	if (size > 255)
+	if (size > JUMP_TAG_MAX_SIZE)
 		return EC_ERROR_INVAL;
-	jdata->jump_tag_total += ROUNDUP4(size) + sizeof(struct jump_tag);
+
+	new_entry_size = ROUNDUP4(size) + sizeof(struct jump_tag);
+
+	if (system_usable_ram_end() - new_entry_size < JUMP_DATA_MIN_ADDRESS) {
+		ccprintf("ERROR: out of space for jump tags\n");
+		return EC_ERROR_INVAL;
+	}
+
+	jdata->jump_tag_total += new_entry_size;
 
 	t = (struct jump_tag *)system_usable_ram_end();
 	t->tag = tag;
@@ -378,6 +392,10 @@ test_mockable const uint8_t *system_get_jump_tag(uint16_t tag, int *version,
 	if (!jdata)
 		return NULL;
 
+	/* Ensure system_usable_ram_end() is within bounds */
+	if (system_usable_ram_end() < JUMP_DATA_MIN_ADDRESS)
+		return NULL;
+
 	/* Search through tag data for a match */
 	while (used < jdata->jump_tag_total) {
 		/* Check the next tag */
@@ -918,6 +936,18 @@ void system_common_pre_init(void)
 		else
 			delta = sizeof(struct jump_data) - jdata->struct_size;
 
+		/*
+		 * Check if enough space for jump data.
+		 * Clear jump data and return if not.
+		 */
+		if (system_usable_ram_end() < JUMP_DATA_MIN_ADDRESS) {
+			/* TODO(b/251190975): This failure should be reported
+			 * in the panic data structure for more visibility.
+			 */
+			memset(jdata, 0, sizeof(struct jump_data));
+			return;
+		}
+
 		if (delta && jdata->jump_tag_total) {
 			uint8_t *d = (uint8_t *)system_usable_ram_end();
 			memmove(d, d + delta, jdata->jump_tag_total);
diff --git a/include/config.h b/include/config.h
index baf86db418..e8bcf8b604 100644
--- a/include/config.h
+++ b/include/config.h
@@ -6944,4 +6944,9 @@
 #define HAS_GPU_DRIVER
 #endif
 
+/* Default to 1024 for end of ram data (panic and jump data) */
+#ifndef CONFIG_PRESERVED_END_OF_RAM_SIZE
+#define CONFIG_PRESERVED_END_OF_RAM_SIZE 1024
+#endif
+
 #endif /* __CROS_EC_CONFIG_H */
diff --git a/include/sysjump.h b/include/sysjump.h
index 7d86df2e61..b7779b9c86 100644
--- a/include/sysjump.h
+++ b/include/sysjump.h
@@ -21,6 +21,16 @@
 #define JUMP_DATA_SIZE_V1 12 /* Size of version 1 jump data struct */
 #define JUMP_DATA_SIZE_V2 16 /* Size of version 2 jump data struct */
 
+#define JUMP_TAG_MAX_SIZE 255
+
+#if !defined(CONFIG_RAM_SIZE) || !(CONFIG_RAM_SIZE > 0)
+/* Disable check by setting jump data min address to zero */
+#define JUMP_DATA_MIN_ADDRESS 0
+#else
+#define JUMP_DATA_MIN_ADDRESS \
+	(CONFIG_RAM_BASE + CONFIG_RAM_SIZE - CONFIG_PRESERVED_END_OF_RAM_SIZE)
+#endif
+
 struct jump_data {
 	/*
 	 * Add new fields to the _start_ of the struct, since we copy it to the
diff --git a/include/system.h b/include/system.h
index ed811e9626..4d98136da5 100644
--- a/include/system.h
+++ b/include/system.h
@@ -214,7 +214,7 @@ int system_jumped_late(void);
  * This may ONLY be called from within a HOOK_SYSJUMP handler.
  *
  * @param tag		Data type
- * @param size          Size of data; must be less than 255 bytes.
+ * @param size          Size of data; must be less than JUMP_TAG_MAX_SIZE bytes.
  * @param version       Data version, so that tag data can evolve as firmware
  *			is updated.
  * @param data		Pointer to data to save
diff --git a/zephyr/Kconfig.panic b/zephyr/Kconfig.panic
index c402fc1e70..4bffa64b39 100644
--- a/zephyr/Kconfig.panic
+++ b/zephyr/Kconfig.panic
@@ -2,6 +2,14 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+config PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE
+	int "Size of preserved ram for panic and jump data"
+	default 1024
+	help
+	  Size of preserved non-initialized memory at end of ram for panic and
+	  jump data. The linker will ensure at least this much space is
+	  unallocated.
+
 if PLATFORM_EC_PANIC
 
 config PLATFORM_EC_SOFTWARE_PANIC
diff --git a/zephyr/linker/CMakeLists.txt b/zephyr/linker/CMakeLists.txt
index 94544d454b..adffc2246f 100644
--- a/zephyr/linker/CMakeLists.txt
+++ b/zephyr/linker/CMakeLists.txt
@@ -21,3 +21,7 @@ zephyr_linker_sources_ifdef(CONFIG_SOC_FAMILY_MEC ROM_START SORT_KEY 1
 
 zephyr_linker_sources(DATA_SECTIONS iterables-ram.ld)
 zephyr_linker_sources(SECTIONS iterables-rom.ld)
+
+# Ensure there's space for panic and jump data at the end of ram
+# Must be added to "SECTIONS" because this is applied last
+zephyr_linker_sources(SECTIONS end-of-ram.ld)
diff --git a/zephyr/linker/end-of-ram.ld b/zephyr/linker/end-of-ram.ld
new file mode 100644
index 0000000000..e03de7481d
--- /dev/null
+++ b/zephyr/linker/end-of-ram.ld
@@ -0,0 +1,29 @@
+/* Copyright 2022 The Chromium OS Authors.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+/* This section simply ensures there's enough unused space at the end of ram to
+ * hold panic and jump data. This space isn't directly allocated because it
+ * would result in the SRAM utilization always being reported as 100%.
+ */
+#if !defined(CONFIG_BOARD_NATIVE_POSIX)
+SECTION_PROLOGUE(.end_of_ram_info, 0, )
+{
+	ASSERT(DEFINED(_image_ram_end) && _image_ram_end > 0, "Error: _image_ram_end is not defined");
+
+#if defined(RAM_ADDR) && defined(RAM_SIZE)
+	PROVIDE(__unused_ram_start = _image_ram_end);
+	PROVIDE(__unused_ram_end = RAM_ADDR + RAM_SIZE);
+	PROVIDE(__unused_ram_size = __unused_ram_end - __unused_ram_start);
+	ASSERT(__unused_ram_size >= CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE,
+	       "ERROR: Not enough space for preserved end of ram data (see PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE)");
+#endif
+
+#if defined(CONFIG_SHAREDMEM_MINIMUM_SIZE)
+	ASSERT(CONFIG_SHAREDMEM_MINIMUM_SIZE >= CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE,
+	       "ERROR: Sharedmem must be at least large enough for preserved end of ram data");
+#endif
+
+}
+#endif
diff --git a/zephyr/shim/include/config_chip.h b/zephyr/shim/include/config_chip.h
index f8e69fe55d..3caa3db634 100644
--- a/zephyr/shim/include/config_chip.h
+++ b/zephyr/shim/include/config_chip.h
@@ -495,17 +495,23 @@
 /* The jump data goes at the end of data ram, so for posix, the end of ram is
  * wherever the jump data ended up.
  */
-#include "sysjump.h"
-extern char mock_jump_data[sizeof(struct jump_data) + 256];
+extern char mock_jump_data[CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE];
 #define CONFIG_RAM_BASE 0x0
-#define CONFIG_DATA_RAM_SIZE \
-	(((uintptr_t)&mock_jump_data) + sizeof(mock_jump_data))
+#define CONFIG_DATA_RAM_SIZE            \
+	(((uintptr_t)&mock_jump_data) + \
+	 CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE)
 #else
 #error "A zephyr,sram device must be chosen in the device tree"
 #endif
 
 #define CONFIG_RAM_SIZE CONFIG_DATA_RAM_SIZE
 
+#undef CONFIG_PRESERVED_END_OF_RAM_SIZE
+#ifdef CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE
+#define CONFIG_PRESERVED_END_OF_RAM_SIZE \
+	CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE
+#endif
+
 #define CONFIG_RO_MEM_OFF CONFIG_CROS_EC_RO_MEM_OFF
 #define CONFIG_RO_MEM_SIZE CONFIG_CROS_EC_RO_MEM_SIZE
 #define CONFIG_RW_MEM_OFF CONFIG_CROS_EC_RW_MEM_OFF
diff --git a/zephyr/shim/src/ztest_system.c b/zephyr/shim/src/ztest_system.c
index 5933f18f05..13614ec8f0 100644
--- a/zephyr/shim/src/ztest_system.c
+++ b/zephyr/shim/src/ztest_system.c
@@ -12,7 +12,7 @@
 
 #define CPRINTS(format, args...) cprints(CC_SYSTEM, format, ##args)
 
-char mock_jump_data[sizeof(struct jump_data) + 256];
+char mock_jump_data[CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE];
 
 /* When CONFIG_RAM_SIZE is defined, this is provided by common/system.c */
 #ifndef CONFIG_RAM_SIZE
diff --git a/zephyr/test/jump_tags/CMakeLists.txt b/zephyr/test/jump_tags/CMakeLists.txt
new file mode 100644
index 0000000000..710195275e
--- /dev/null
+++ b/zephyr/test/jump_tags/CMakeLists.txt
@@ -0,0 +1,13 @@
+# Copyright 2022 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+cmake_minimum_required(VERSION 3.20.0)
+find_package(Zephyr REQUIRED HINTS "${ZEPHYR_BASE}")
+project(jump_tags)
+
+# Include FFF fakes
+add_subdirectory(${PLATFORM_EC}/zephyr/test/test_utils test_utils)
+
+FILE(GLOB test_sources src/*.c)
+target_sources(app PRIVATE ${test_sources})
diff --git a/zephyr/test/jump_tags/boards/native_posix.overlay b/zephyr/test/jump_tags/boards/native_posix.overlay
new file mode 100644
index 0000000000..9f3238d076
--- /dev/null
+++ b/zephyr/test/jump_tags/boards/native_posix.overlay
@@ -0,0 +1,33 @@
+/* Copyright 2022 The ChromiumOS Authors
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include <board-overlays/native_posix.dts>
+#include <cros/binman.dtsi>
+
+/ {
+	chosen {
+		cros-ec,flash = &flash1;
+		cros-ec,flash-controller = &cros_flash;
+	};
+	aliases {
+		gpio-wp = &gpio_wp_l;
+	};
+	named-gpios {
+		compatible = "named-gpios";
+		gpio_wp_l: wp_l {
+			gpios = <&gpio0 3 (GPIO_INPUT | GPIO_ACTIVE_LOW)>;
+		};
+	};
+	cros_flash: cros-flash {
+		compatible = "cros-ec,flash-emul";
+	};
+	flash1: flash@64000000 {
+		reg = <0x64000000 DT_SIZE_K(512)>;
+	};
+};
+
+&gpio0 {
+	ngpios = <4>;
+};
diff --git a/zephyr/test/jump_tags/prj.conf b/zephyr/test/jump_tags/prj.conf
new file mode 100644
index 0000000000..9a2a537b89
--- /dev/null
+++ b/zephyr/test/jump_tags/prj.conf
@@ -0,0 +1,15 @@
+# Copyright 2022 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+CONFIG_CROS_EC=y
+CONFIG_EMUL_CROS_FLASH=y
+CONFIG_FLASH=y
+CONFIG_PLATFORM_EC_CONSOLE_CMD_CHARGEN=n
+CONFIG_PLATFORM_EC=y
+CONFIG_SERIAL=y
+CONFIG_SHELL_BACKEND_DUMMY=y
+CONFIG_SHELL_BACKEND_SERIAL=n
+CONFIG_SHIMMED_TASKS=y
+CONFIG_ZTEST_NEW_API=y
+CONFIG_ZTEST=y
diff --git a/zephyr/test/jump_tags/src/jump_tags.c b/zephyr/test/jump_tags/src/jump_tags.c
new file mode 100644
index 0000000000..5811f11648
--- /dev/null
+++ b/zephyr/test/jump_tags/src/jump_tags.c
@@ -0,0 +1,175 @@
+/* Copyright 2022 The ChromiumOS Authors
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include <zephyr/kernel.h>
+#include <zephyr/ztest.h>
+#include <setjmp.h>
+
+#include "ec_commands.h"
+#include "hooks.h"
+#include "host_command.h"
+#include "sysjump.h"
+#include "system_fake.h"
+#include "system.h"
+
+#define TEST_BASIC_JUMP_TAG 0x9901
+#define TEST_MISSING_JUMP_TAG 0x9902
+#define TEST_MAX_JUMP_TAG 0x9903
+#define TEST_TOO_BIG_JUMP_TAG 0x9904
+
+#define TEST_JUMP_TAG_VERSION 1
+
+#define SOME_STR_VAL "JumpTagTest"
+
+void (*add_tag_func)(void);
+
+struct test_basic_jump_data_struct {
+	char some_str[32];
+};
+
+struct test_max_jump_data_struct {
+	char some_str[JUMP_TAG_MAX_SIZE];
+};
+
+struct test_too_big_jump_data_struct {
+	char some_str[JUMP_TAG_MAX_SIZE + 1];
+};
+
+static void system_before(void *data)
+{
+	add_tag_func = NULL;
+	system_common_pre_init();
+	system_set_shrspi_image_copy(EC_IMAGE_RO);
+}
+
+static void do_fake_sysjump(void)
+{
+	jmp_buf env;
+	enum ec_image target_image = system_get_image_copy() == EC_IMAGE_RO ?
+					     EC_IMAGE_RW :
+					     EC_IMAGE_RO;
+
+	if (!setjmp(env)) {
+		system_fake_setenv(&env);
+		system_run_image_copy(target_image);
+		zassert_unreachable();
+	}
+
+	system_set_shrspi_image_copy(target_image);
+	zassert_equal(system_get_image_copy(), target_image);
+}
+
+static void add_max_jump_tag(void)
+{
+	struct test_max_jump_data_struct max_tag = {
+		.some_str = SOME_STR_VAL,
+	};
+	zassert_ok(system_add_jump_tag(TEST_MAX_JUMP_TAG, TEST_JUMP_TAG_VERSION,
+				       sizeof(max_tag), &max_tag));
+}
+
+static void add_too_big_jump_tag(void)
+{
+	struct test_too_big_jump_data_struct too_big_tag = {
+		.some_str = SOME_STR_VAL,
+	};
+	zassert_equal(system_add_jump_tag(TEST_TOO_BIG_JUMP_TAG,
+					  TEST_JUMP_TAG_VERSION,
+					  sizeof(too_big_tag), &too_big_tag),
+		      EC_ERROR_INVAL);
+}
+
+static void add_too_many_jump_tags(void)
+{
+	int rv;
+	struct test_max_jump_data_struct max_tag = {
+		.some_str = SOME_STR_VAL,
+	};
+	/* Ensure at least one tag can be added, but not 10 */
+	for (int i = 0; i < 10; i++) {
+		rv = system_add_jump_tag(TEST_MAX_JUMP_TAG,
+					 TEST_JUMP_TAG_VERSION, sizeof(max_tag),
+					 &max_tag);
+		if (rv != 0) {
+			zassert_equal(rv, EC_ERROR_INVAL);
+			zassert_true(i > 0);
+			return;
+		}
+	}
+	zassert_unreachable(
+		"Adding too many jump tags failed to result in an error");
+}
+
+static void add_basic_jump_tag(void)
+{
+	struct test_basic_jump_data_struct basic_tag = {
+		.some_str = SOME_STR_VAL,
+	};
+	zassert_ok(system_add_jump_tag(TEST_BASIC_JUMP_TAG,
+				       TEST_JUMP_TAG_VERSION, sizeof(basic_tag),
+				       &basic_tag));
+}
+
+static void test_sysjump_hook(void)
+{
+	if (add_tag_func)
+		add_tag_func();
+}
+DECLARE_HOOK(HOOK_SYSJUMP, test_sysjump_hook, HOOK_PRIO_DEFAULT);
+
+static void check_for_jump_tag(int jump_tag, int expected_size)
+{
+	int version;
+	int size;
+	const unsigned char *data;
+
+	data = system_get_jump_tag(jump_tag, &version, &size);
+	zassert_equal(size, expected_size);
+	zassert_equal(version, TEST_JUMP_TAG_VERSION);
+	zassert_equal(strcmp(data, SOME_STR_VAL), 0);
+}
+
+ZTEST(jump_tags, test_get_missing_jump_tag)
+{
+	int version;
+	int size;
+	struct test_jump_data_struct *data;
+
+	data = (struct test_jump_data_struct *)system_get_jump_tag(
+		TEST_MISSING_JUMP_TAG, &version, &size);
+	zassert_equal(data, NULL);
+}
+
+ZTEST(jump_tags, test_add_max_jump_tag)
+{
+	add_tag_func = add_max_jump_tag;
+	do_fake_sysjump();
+	check_for_jump_tag(TEST_MAX_JUMP_TAG,
+			   sizeof(struct test_max_jump_data_struct));
+}
+
+ZTEST(jump_tags, test_too_big_jump_tag)
+{
+	add_tag_func = add_too_big_jump_tag;
+	do_fake_sysjump();
+}
+
+ZTEST(jump_tags, test_too_many_jump_tags)
+{
+	add_tag_func = add_too_many_jump_tags;
+	do_fake_sysjump();
+	check_for_jump_tag(TEST_MAX_JUMP_TAG,
+			   sizeof(struct test_max_jump_data_struct));
+}
+
+ZTEST(jump_tags, test_add_basic_jump_tag)
+{
+	add_tag_func = add_basic_jump_tag;
+	do_fake_sysjump();
+	check_for_jump_tag(TEST_BASIC_JUMP_TAG,
+			   sizeof(struct test_basic_jump_data_struct));
+}
+
+ZTEST_SUITE(jump_tags, NULL, NULL, system_before, NULL, NULL);
diff --git a/zephyr/test/jump_tags/testcase.yaml b/zephyr/test/jump_tags/testcase.yaml
new file mode 100644
index 0000000000..11ef8d73ec
--- /dev/null
+++ b/zephyr/test/jump_tags/testcase.yaml
@@ -0,0 +1,13 @@
+# Copyright 2022 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+common:
+  platform_allow: native_posix
+tests:
+  jump_tags.default:
+    extra_configs:
+      - CONFIG_PLATFORM_EC_PRESERVED_END_OF_RAM_SIZE=1024
+    tags:
+        common
+        system