[Skiboot] [PATCH] fast-reboot: improve integrity of fast reboots
Nicholas Piggin
npiggin at gmail.com
Fri Feb 23 07:01:39 AEDT 2018
This is a start on some basic checks that might detect firmware
problems that call for a full IPL. The patch is pretty rough at
the moment but it's just for comments.
- Checksum text and most read-only data, and verify that against
checksum taken at IPL.
- Check integrity of memory allocation structures and freelists.
- Zeroes most OS memory (should be parallelised).
There's probably lots more software (and harware) checks we should
do here. But this is a first hack at it. What do people think, is
this a good idea?
Thanks,
Nick
---
core/fast-reboot.c | 24 +++++++++++++++--
core/init.c | 59 ++++++++++++++++++++++++++++++++++++++++--
core/mem_region.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---
include/mem_region.h | 4 +++
skiboot.lds.S | 35 +++++++++++++------------
5 files changed, 171 insertions(+), 24 deletions(-)
diff --git a/core/fast-reboot.c b/core/fast-reboot.c
index 382e781a..0f2cd8bc 100644
--- a/core/fast-reboot.c
+++ b/core/fast-reboot.c
@@ -19,6 +19,7 @@
#include <fsp.h>
#include <psi.h>
#include <opal.h>
+#include <mem_region.h>
#include <xscom.h>
#include <interrupts.h>
#include <cec.h>
@@ -70,6 +71,8 @@ void disable_fast_reboot(const char *reason)
fast_reboot_disabled = reason;
}
+bool verify_stuff(void);
+
void fast_reboot(void)
{
struct cpu_thread *cpu;
@@ -108,8 +111,6 @@ void fast_reboot(void)
return;
}
- /* Should mem_check() all regions before allowing fast reboot? */
-
prlog(PR_NOTICE, "RESET: Initiating fast reboot %d...\n", ++fast_reboot_count);
fast_boot_release = false;
sync();
@@ -122,6 +123,23 @@ void fast_reboot(void)
return;
}
+ /* Do basic sanity checks of our data structures after all other CPUs
+ * have been stopped.
+ */
+ if (!mem_check_all()) {
+ prlog(PR_NOTICE, "REST: Fast reboot failed due to inconsistent "
+ "firmware data\n");
+ opal_quiesce(QUIESCE_RESUME, -1);
+ return;
+ }
+ if (!verify_stuff()) {
+ prlog(PR_NOTICE, "REST: Fast reboot failed due to inconsistent "
+ "firmware checksum\n");
+ opal_quiesce(QUIESCE_RESUME, -1);
+ return;
+ }
+ /* XXX: add more checks here. */
+
/*
* There is no point clearing special wakeup or un-quiesce due to
* failure after this point, because we will be going to full IPL.
@@ -321,6 +339,8 @@ void __noreturn fast_reboot_entry(void)
/* Wait for them to respond */
cpu_state_wait_all_others(cpu_state_active, 0);
+ mem_region_clear_unused();
+
sync();
prlog(PR_INFO, "RESET: All done, cleaning up...\n");
diff --git a/core/init.c b/core/init.c
index 0bfdac55..03ba28bd 100644
--- a/core/init.c
+++ b/core/init.c
@@ -340,6 +340,9 @@ bool start_preload_kernel(void)
return true;
}
+extern uint64_t kstart;
+extern uint64_t klength;
+
static bool load_kernel(void)
{
void *stb_container = NULL;
@@ -397,6 +400,7 @@ static bool load_kernel(void)
} else
kh = (struct elf_hdr *) (KERNEL_LOAD_BASE);
+ kstart = (unsigned long)KERNEL_LOAD_BASE;
}
prlog(PR_DEBUG,
@@ -419,6 +423,9 @@ static bool load_kernel(void)
return false;
}
+ if (kstart)
+ klength = kernel_size;
+
if (chip_quirk(QUIRK_MAMBO_CALLOUTS)) {
secureboot_verify(RESOURCE_ID_KERNEL,
stb_container,
@@ -454,8 +461,6 @@ static void load_initramfs(void)
(uint64_t)INITRAMFS_LOAD_BASE + initramfs_size);
}
-int64_t mem_dump_free(void);
-
void *fdt;
void __noreturn load_and_boot_kernel(bool is_reboot)
@@ -783,6 +788,54 @@ static void pci_nvram_init(void)
}
}
+static uint32_t csum(void *_p, void *_e)
+{
+ size_t len = _e - _p;
+ uint32_t *p = _p;
+ uint32_t v1 = 0, v2 = 0;
+ unsigned int i;
+
+ for (i = 0; i < len; i += 4) {
+ uint32_t v = *p++;
+ v1 += v;
+ v2 += v1;
+ }
+
+ return v1 ^ v2;
+}
+
+extern char _start[];
+extern char _stext[];
+extern char __sym_map_end[];
+
+static uint32_t stuff_csum;
+
+static void checksum_stuff(void)
+{
+ /*
+ * Don't need to checksum 0-0x100 because that's IPL entry
+ * Interrupt vectors because those get overwritten
+ * XXX: what's between 0x2000 and SPIRA_OFF that changes for me?
+ * XXX: real hardware might change other sections?
+ * XXX: make proper linker script delimiters for these regions.
+ */
+ stuff_csum = csum(_start + SPIRA_OFF, _stext);
+ stuff_csum += csum(_stext, __sym_map_end);
+ stuff_csum += csum(__builtin_kernel_start, __builtin_kernel_end);
+}
+
+bool verify_stuff(void);
+bool verify_stuff(void)
+{
+ uint32_t old = stuff_csum;
+ checksum_stuff();
+ if (old != stuff_csum) {
+ prlog(PR_NOTICE, "OPAL checksums did not match\n");
+ return false;
+ }
+ return true;
+}
+
/* Called from head.S, thus no prototype. */
void main_cpu_entry(const void *fdt);
@@ -849,6 +902,8 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
skiboot_gcov_done();
#endif
+ checksum_stuff();
+
/* Initialize boot cpu's cpu_thread struct */
init_boot_cpu();
diff --git a/core/mem_region.c b/core/mem_region.c
index 18649174..aebe4b4e 100644
--- a/core/mem_region.c
+++ b/core/mem_region.c
@@ -26,9 +26,6 @@
#include <mem_region.h>
#include <mem_region-malloc.h>
-int64_t mem_dump_free(void);
-void mem_dump_allocs(void);
-
/* Memory poisoning on free (if POISON_MEM_REGION set to 1) */
#ifdef DEBUG
#define POISON_MEM_REGION 1
@@ -622,6 +619,18 @@ bool mem_check(const struct mem_region *region)
return true;
}
+bool mem_check_all(void)
+{
+ struct mem_region *r;
+
+ list_for_each(®ions, r, list) {
+ if (!mem_check(r))
+ return false;
+ }
+
+ return true;
+}
+
static struct mem_region *new_region(const char *name,
uint64_t start, uint64_t len,
struct dt_node *node,
@@ -1157,6 +1166,64 @@ void mem_region_release_unused(void)
unlock(&mem_region_lock);
}
+uint64_t kstart = 0;
+uint64_t klength = 0;
+
+void mem_region_clear_unused(void)
+{
+ struct mem_region *r;
+
+ lock(&mem_region_lock);
+ assert(mem_regions_finalised);
+
+ prlog(PR_NOTICE, "Clearing unused memory:\n");
+ list_for_each(®ions, r, list) {
+ unsigned long s, e, l;
+
+ /* If it's not unused, ignore it. */
+ if (!(r->type == REGION_OS))
+ continue;
+
+ assert(r != &skiboot_heap);
+
+ s = r->start;
+ l = r->len;
+ e = s + l;
+
+ if (s < 0x2000) { /* skip low vectors */
+ if (e <= 0x2000)
+ continue;
+ s = 0x2000;
+ l = e - s;
+ }
+
+ /*
+ * XXX: this is a hack for mambo kernel in memory. The OS
+ * might have overwritten this so it needs to really be
+ * captured as a region to work properly.
+ */
+ if (kstart) { /* skip in-memory kernel */
+ uint64_t kend = kstart + klength;
+ if (s < kend && e > kstart) {
+ if (s < kstart) {
+ prlog(PR_NOTICE, "Clearing region %lx-%llx\n", s, kstart);
+ memset((void *)s, 0, kstart - s);
+ }
+ if (e > kend) {
+ prlog(PR_NOTICE, "Clearing region %llx-%lx\n", kend, e);
+ memset((void *)kend, 0, e - kend);
+ }
+ continue;
+ }
+ }
+
+ prlog(PR_NOTICE, "Clearing region %lx-%lx\n", s, s + l);
+ memset((void *)s, 0, l);
+ }
+ unlock(&mem_region_lock);
+}
+
+
static void mem_region_add_dt_reserved_node(struct dt_node *parent,
struct mem_region *region)
{
diff --git a/include/mem_region.h b/include/mem_region.h
index 4da3b6ea..9e288dac 100644
--- a/include/mem_region.h
+++ b/include/mem_region.h
@@ -63,7 +63,11 @@ bool mem_resize(struct mem_region *region, void *mem, size_t len,
const char *location);
size_t mem_allocated_size(const void *ptr);
bool mem_check(const struct mem_region *region);
+bool mem_check_all(void);
void mem_region_release_unused(void);
+void mem_region_clear_unused(void);
+int64_t mem_dump_free(void);
+void mem_dump_allocs(void);
/* Specifically for working on the heap. */
extern struct mem_region skiboot_heap;
diff --git a/skiboot.lds.S b/skiboot.lds.S
index 7f71d5cd..b64a46fa 100644
--- a/skiboot.lds.S
+++ b/skiboot.lds.S
@@ -20,6 +20,7 @@
ENTRY(boot_entry);
SECTIONS
{
+ _start = .;
. = 0;
.head : {
@@ -65,23 +66,6 @@ SECTIONS
__rodata_end = .;
}
- .data : {
- /*
- * A couple of things that need to be 4K aligned and
- * to reside in their own pages for the sake of TCE
- * mappings
- */
- . = ALIGN(0x1000);
- *(.data.memcons);
- . = ALIGN(0x1000);
- *(.data.boot_trace);
- . = ALIGN(0x1000);
- *(.data*)
- *(.force.data)
- *(.toc1)
- *(.branch_lt)
- }
-
. = ALIGN(0x10);
.init : {
__ctors_start = .;
@@ -145,6 +129,23 @@ SECTIONS
__sym_map_end = . ;
}
+ .data : {
+ /*
+ * A couple of things that need to be 4K aligned and
+ * to reside in their own pages for the sake of TCE
+ * mappings
+ */
+ . = ALIGN(0x1000);
+ *(.data.memcons);
+ . = ALIGN(0x1000);
+ *(.data.boot_trace);
+ . = ALIGN(0x1000);
+ *(.data*)
+ *(.force.data)
+ *(.toc1)
+ *(.branch_lt)
+ }
+
/* We locate the BSS at 3M to leave room for the symbol map */
. = 0x300000;
--
2.16.1
More information about the Skiboot
mailing list