[Skiboot] [PATCH v2] nvram: Flag dangerous NVRAM options

Michael Neuling mikey at neuling.org
Mon May 13 17:09:39 AEST 2019


Most nvram options used by skiboot are just for debug or testing for
regressions. They should never be used long term.

We've hit a number of issues in testing and the field where nvram
options have been set "temporarily" but haven't been properly cleared
after, resulting in crashes or real bugs being masked.

This patch marks most nvram options used by skiboot as dangerous and
prints a chicken to remind users of the problem.

Signed-off-by: Michael Neuling <mikey at neuling.org>
Reviewed-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
Acked-By: Alistair Popple <alistair at popple.id.au>
--
v2:
  Updated to upstream skiboot
  Review comments from oohal:
  - Changed a safe <-> dangerous on a few items
  - Change parameter to function name _safe/_dangerous
---
 core/hmi.c                   |  2 +-
 core/init.c                  |  8 +++---
 core/nvram-format.c          | 55 ++++++++++++++++++++++++++++++++----
 core/platform.c              |  4 +--
 core/test/run-nvram-format.c | 10 +++----
 hw/lpc-uart.c                |  2 +-
 hw/npu2-common.c             |  2 +-
 hw/npu2-opencapi.c           |  2 +-
 hw/npu2.c                    |  2 +-
 hw/phb4.c                    | 11 ++++----
 hw/slw.c                     |  2 +-
 hw/xscom.c                   |  2 +-
 include/nvram.h              |  6 ++--
 libstb/secureboot.c          |  2 +-
 libstb/trustedboot.c         |  2 +-
 15 files changed, 78 insertions(+), 34 deletions(-)

diff --git a/core/hmi.c b/core/hmi.c
index e813286009..af3f7fe491 100644
--- a/core/hmi.c
+++ b/core/hmi.c
@@ -672,7 +672,7 @@ static void find_npu2_checkstop_reason(int flat_chip_id,
 	if (!total_errors)
 		return;
 
-	npu2_hmi_verbose = nvram_query_eq("npu2-hmi-verbose", "true");
+	npu2_hmi_verbose = nvram_query_eq_safe("npu2-hmi-verbose", "true");
 	/* Force this for now until we sort out something better */
 	npu2_hmi_verbose = true;
 
diff --git a/core/init.c b/core/init.c
index bca12dfc66..c066163824 100644
--- a/core/init.c
+++ b/core/init.c
@@ -582,7 +582,7 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
 	fsp_console_select_stdout();
 
 	/* Use nvram bootargs over device tree */
-	cmdline = nvram_query("bootargs");
+	cmdline = nvram_query_safe("bootargs");
 	if (cmdline) {
 		dt_check_del_prop(dt_chosen, "bootargs");
 		dt_add_property_string(dt_chosen, "bootargs", cmdline);
@@ -740,7 +740,7 @@ static void console_log_level(void)
 
 	/* console log level:
 	 *   high 4 bits in memory, low 4 bits driver (e.g. uart). */
-	s = nvram_query("log-level-driver");
+	s = nvram_query_safe("log-level-driver");
 	if (s) {
 		level = console_get_level(s);
 		debug_descriptor.console_log_levels =
@@ -749,7 +749,7 @@ static void console_log_level(void)
 		prlog(PR_NOTICE, "console: Setting driver log level to %i\n",
 		      level & 0x0f);
 	}
-	s = nvram_query("log-level-memory");
+	s = nvram_query_safe("log-level-memory");
 	if (s) {
 		level = console_get_level(s);
 		debug_descriptor.console_log_levels =
@@ -867,7 +867,7 @@ static void pci_nvram_init(void)
 
 	pcie_max_link_speed = 0;
 
-	nvram_speed = nvram_query("pcie-max-link-speed");
+	nvram_speed = nvram_query_dangerous("pcie-max-link-speed");
 	if (nvram_speed) {
 		pcie_max_link_speed = atoi(nvram_speed);
 		prlog(PR_NOTICE, "PHB: NVRAM set max link speed to GEN%i\n",
diff --git a/core/nvram-format.c b/core/nvram-format.c
index b2fc960b33..e34add3223 100644
--- a/core/nvram-format.c
+++ b/core/nvram-format.c
@@ -206,13 +206,31 @@ static const char *find_next_key(const char *start, const char *end)
 	return NULL;
 }
 
+static void nvram_dangerous(const char *key)
+{
+	prlog(PR_ERR, " ___________________________________________________________\n");
+	prlog(PR_ERR, "<  Dangerous NVRAM option: %s\n", key);
+	prlog(PR_ERR, " -----------------------------------------------------------\n");
+	prlog(PR_ERR, "                  \\                         \n");
+	prlog(PR_ERR, "                   \\   WW                   \n");
+	prlog(PR_ERR, "                      <^ \\___/|             \n");
+	prlog(PR_ERR, "                       \\      /             \n");
+	prlog(PR_ERR, "                        \\_  _/              \n");
+	prlog(PR_ERR, "                          }{                 \n");
+}
+
+
 /*
- * nvram_query() - Searches skiboot NVRAM partition for a key=value pair.
+ * nvram_query_safe/dangerous() - Searches skiboot NVRAM partition
+ * for a key=value pair.
+ *
+ * Dangerous means it should only be used for testing as it may
+ * mask issues. Safe is ok for long term use.
  *
  * Returns a pointer to a NUL terminated string that contains the value
  * associated with the given key.
  */
-const char *nvram_query(const char *key)
+static const char *__nvram_query(const char *key, bool dangerous)
 {
 	const char *part_end, *start;
 	int key_len = strlen(key);
@@ -269,6 +287,8 @@ const char *nvram_query(const char *key)
 			prlog(PR_DEBUG, "NVRAM: Searched for '%s' found '%s'\n",
 				key, value);
 
+			if (dangerous)
+				nvram_dangerous(start);
 			return value;
 		}
 
@@ -280,18 +300,30 @@ const char *nvram_query(const char *key)
 	return NULL;
 }
 
+const char *nvram_query_safe(const char *key)
+{
+	return __nvram_query(key, false);
+}
+
+const char *nvram_query_dangerous(const char *key)
+{
+	return __nvram_query(key, true);
+}
 
 /*
- * nvram_query_eq() - Check if the given 'key' exists and
- * is set to 'value'.
+ * nvram_query_eq_safe/dangerous() - Check if the given 'key' exists
+ * and is set to 'value'.
+ *
+ * Dangerous means it should only be used for testing as it may
+ * mask issues. Safe is ok for long term use.
  *
  * Note: Its an error to check for non-existence of a key
  * by passing 'value == NULL' as a key's value can never be
  * NULL in nvram.
  */
-bool nvram_query_eq(const char *key, const char *value)
+static bool __nvram_query_eq(const char *key, const char *value, bool dangerous)
 {
-	const char *s = nvram_query(key);
+	const char *s = __nvram_query(key, dangerous);
 
 	if (!s)
 		return false;
@@ -299,3 +331,14 @@ bool nvram_query_eq(const char *key, const char *value)
 	assert(value != NULL);
 	return !strcmp(s, value);
 }
+
+bool nvram_query_eq_safe(const char *key, const char *value)
+{
+	return __nvram_query_eq(key, value, false);
+}
+
+bool nvram_query_eq_dangerous(const char *key, const char *value)
+{
+	return __nvram_query_eq(key, value, true);
+}
+
diff --git a/core/platform.c b/core/platform.c
index 62361f55c7..afa00adf71 100644
--- a/core/platform.c
+++ b/core/platform.c
@@ -59,13 +59,13 @@ static int64_t opal_cec_reboot(void)
 
 	opal_quiesce(QUIESCE_HOLD, -1);
 
-	if (proc_gen == proc_gen_p8 && nvram_query_eq("fast-reset","1")) {
+	if (proc_gen == proc_gen_p8 && nvram_query_eq_safe("fast-reset","1")) {
 		/*
 		 * Bugs in P8 mean fast reboot isn't 100% reliable when cores
 		 * are busy, so only attempt if explicitly *enabled*.
 		 */
 		fast_reboot();
-	} else if (!nvram_query_eq("fast-reset","0")) {
+	} else if (!nvram_query_eq_safe("fast-reset","0")) {
 		/* Try fast-reset unless explicitly disabled */
 		fast_reboot();
 	}
diff --git a/core/test/run-nvram-format.c b/core/test/run-nvram-format.c
index d86b1dcfa9..c8a75830af 100644
--- a/core/test/run-nvram-format.c
+++ b/core/test/run-nvram-format.c
@@ -144,23 +144,23 @@ int main(void)
 
 	/* does an empty partition break us? */
 	data = nvram_reset(nvram_image, 128*1024);
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does a zero length key break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = '=';
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does a missing = break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = 'a';
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does an empty value break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = 'a';
 	data[1] = '=';
-	result = nvram_query("a");
+	result = nvram_query_safe("a");
 	assert(result);
 	assert(strlen(result) == 0);
 
@@ -168,7 +168,7 @@ int main(void)
 	data = nvram_reset(nvram_image, 128*1024);
 #define TEST_1 "a\0a=\0test=test\0"
 	memcpy(data, TEST_1, sizeof(TEST_1));
-	result = nvram_query("test");
+	result = nvram_query_safe("test");
 	assert(result);
 	assert(strcmp(result, "test") == 0);
 
diff --git a/hw/lpc-uart.c b/hw/lpc-uart.c
index 365bf3e270..bca10e0e98 100644
--- a/hw/lpc-uart.c
+++ b/hw/lpc-uart.c
@@ -506,7 +506,7 @@ static void uart_init_opal_console(void)
 	/* Update the policy if the corresponding nvram variable
 	 * is present
 	 */
-	nv_policy = nvram_query("uart-con-policy");
+	nv_policy = nvram_query_dangerous("uart-con-policy");
 	if (nv_policy) {
 		if (!strcmp(nv_policy, "opal"))
 			uart_console_policy = UART_CONSOLE_OPAL;
diff --git a/hw/npu2-common.c b/hw/npu2-common.c
index 16b285d89b..f3f2f45a10 100644
--- a/hw/npu2-common.c
+++ b/hw/npu2-common.c
@@ -663,7 +663,7 @@ void probe_npu2(void)
 	}
 
 	/* Check for a zcal override */
-	zcal = nvram_query("nv_zcal_override");
+	zcal = nvram_query_dangerous("nv_zcal_override");
 	if (zcal) {
 		nv_zcal_nominal = atoi(zcal);
 		prlog(PR_WARNING, "NPU2: Using ZCAL impedance override = %d\n", nv_zcal_nominal);
diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
index 9df51b22ed..27dfc121fc 100644
--- a/hw/npu2-opencapi.c
+++ b/hw/npu2-opencapi.c
@@ -1727,7 +1727,7 @@ static void read_nvram_training_state(void)
 {
 	const char *state;
 
-	state = nvram_query("opencapi-link-training");
+	state = nvram_query_dangerous("opencapi-link-training");
 	if (state) {
 		if (!strcmp(state, "prbs31"))
 			npu2_ocapi_training_state = NPU2_TRAIN_PRBS31;
diff --git a/hw/npu2.c b/hw/npu2.c
index 0d79d8a508..97139dd8ba 100644
--- a/hw/npu2.c
+++ b/hw/npu2.c
@@ -1473,7 +1473,7 @@ int npu2_nvlink_init_npu(struct npu2 *npu)
 	 * it throws machine checkstop. Disabling snarfing fixes this so let's
 	 * disable it by default.
 	 */
-	if (nvram_query_eq("opal-npu2-snarf-cpm", "enable")) {
+	if (nvram_query_eq_dangerous("opal-npu2-snarf-cpm", "enable")) {
 		prlog(PR_WARNING, "NPU2#%d: enabling Probe.I.MO snarfing, a bad GPU driver may crash the system!\n",
 				npu->index);
 		val |= PPC_BIT(40); /* CONFIG_ENABLE_SNARF_CPM */
diff --git a/hw/phb4.c b/hw/phb4.c
index 3b0ebfba88..5565d94daf 100644
--- a/hw/phb4.c
+++ b/hw/phb4.c
@@ -5919,16 +5919,16 @@ void probe_phb4(void)
 	struct dt_node *np;
 	const char *s;
 
-	verbose_eeh = nvram_query_eq("pci-eeh-verbose", "true");
+	verbose_eeh = nvram_query_eq_safe("pci-eeh-verbose", "true");
 	/* REMOVEME: force this for now until we stabalise PCIe */
 	verbose_eeh = 1;
 	if (verbose_eeh)
 		prlog(PR_INFO, "PHB4: Verbose EEH enabled\n");
 
-	pci_tracing = nvram_query_eq("pci-tracing", "true");
-	pci_eeh_mmio = !nvram_query_eq("pci-eeh-mmio", "disabled");
-	pci_retry_all = nvram_query_eq("pci-retry-all", "true");
-	s = nvram_query("phb-rx-err-max");
+	pci_tracing = nvram_query_eq_safe("pci-tracing", "true");
+	pci_eeh_mmio = !nvram_query_eq_dangerous("pci-eeh-mmio", "disabled");
+	pci_retry_all = nvram_query_eq_dangerous("pci-retry-all", "true");
+	s = nvram_query_dangerous("phb-rx-err-max");
 	if (s) {
 		rx_err_max = atoi(s);
 
@@ -5937,7 +5937,6 @@ void probe_phb4(void)
 		rx_err_max = MIN(rx_err_max, 255);
 	}
 	prlog(PR_DEBUG, "PHB4: Maximum RX errors during training: %d\n", rx_err_max);
-
 	/* Look for PBCQ XSCOM nodes */
 	dt_for_each_compatible(dt_root, np, "ibm,power9-pbcq")
 		phb4_probe_pbcq(np);
diff --git a/hw/slw.c b/hw/slw.c
index adbfdce950..fcf4d5729b 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -883,7 +883,7 @@ void add_cpu_idle_state_properties(void)
 		if (wakeup_engine_state == WAKEUP_ENGINE_PRESENT)
 			supported_states_mask |= OPAL_PM_WINKLE_ENABLED;
 	}
-	nvram_disable_str = nvram_query("opal-stop-state-disable-mask");
+	nvram_disable_str = nvram_query_dangerous("opal-stop-state-disable-mask");
 	if (nvram_disable_str)
 		nvram_disabled_states_mask = strtol(nvram_disable_str, NULL, 0);
 	prlog(PR_DEBUG, "NVRAM stop disable mask: %x\n", nvram_disabled_states_mask);
diff --git a/hw/xscom.c b/hw/xscom.c
index b652e61702..30f4e831ab 100644
--- a/hw/xscom.c
+++ b/hw/xscom.c
@@ -833,7 +833,7 @@ int64_t xscom_trigger_xstop(void)
 	int rc = OPAL_UNSUPPORTED;
 	bool xstop_disabled = false;
 
-	if (nvram_query_eq("opal-sw-xstop", "disable"))
+	if (nvram_query_eq_dangerous("opal-sw-xstop", "disable"))
 		xstop_disabled = true;
 
 	if (xstop_disabled) {
diff --git a/include/nvram.h b/include/nvram.h
index 012c107f17..a018a11d15 100644
--- a/include/nvram.h
+++ b/include/nvram.h
@@ -24,7 +24,9 @@ bool nvram_validate(void);
 bool nvram_has_loaded(void);
 bool nvram_wait_for_load(void);
 
-const char *nvram_query(const char *name);
-bool nvram_query_eq(const char *key, const char *value);
+const char *nvram_query_safe(const char *name);
+const char *nvram_query_dangerous(const char *name);
+bool nvram_query_eq_safe(const char *key, const char *value);
+bool nvram_query_eq_dangerous(const char *key, const char *value);
 
 #endif /* __NVRAM_H */
diff --git a/libstb/secureboot.c b/libstb/secureboot.c
index 4f6a301d5e..1578f52ecd 100644
--- a/libstb/secureboot.c
+++ b/libstb/secureboot.c
@@ -104,7 +104,7 @@ void secureboot_init(void)
 
 	prlog(PR_DEBUG, "Found %s\n", compat);
 
-	if (nvram_query_eq("force-secure-mode", "always")) {
+	if (nvram_query_eq_dangerous("force-secure-mode", "always")) {
 		secure_mode = true;
 		prlog(PR_NOTICE, "secure mode on (FORCED by nvram)\n");
 	} else {
diff --git a/libstb/trustedboot.c b/libstb/trustedboot.c
index ae2cc55646..d9bacb2d3e 100644
--- a/libstb/trustedboot.c
+++ b/libstb/trustedboot.c
@@ -102,7 +102,7 @@ void trustedboot_init(void)
 		return;
 	}
 
-	if (nvram_query_eq("force-trusted-mode", "true")) {
+	if (nvram_query_eq_dangerous("force-trusted-mode", "true")) {
 		trusted_mode = true;
 		prlog(PR_NOTICE, "trusted mode on (FORCED by nvram)\n");
 	} else {
-- 
2.21.0



More information about the Skiboot mailing list