[coreboot-gerrit] New patch to review for coreboot: libpayload: Move heap out of the BSS, make it self-initializing
Julius Werner (jwerner@chromium.org)
gerrit at coreboot.org
Sat Aug 6 06:41:37 CEST 2016
Julius Werner (jwerner at chromium.org) just uploaded a new patch set to gerrit, which you can find at https://review.coreboot.org/16090
-gerrit
commit b97cffe95e797ce1618698ececa481430fd9a225
Author: Julius Werner <jwerner at chromium.org>
Date: Fri Aug 5 20:54:45 2016 -0700
libpayload: Move heap out of the BSS, make it self-initializing
Some payloads like to have very large heaps for various nefarious
purposes. This isn't usually a problem, except that the amount of time
coreboot takes to clear many megabytes of heap when loading the payload
may have a noticeable effect on boot time. This is particularly silly
since code doesn't even expect heap memory to be cleared when requesting
it.
The solution, implemented by this patch, is to move the heap out of the
BSS and behind the actual "segment" known to and zeroed by coreboot. A
little complication with this is that our heap implementation currently
bootstraps itself by checking if the first byte of the heap (which
should contain a malloc header) is zero. If the heap doesn't get zeroed,
this may lead to libpayload "adopting" heap allocations left over from a
previous boot with all kinds of weird effects, so we have to slightly
redesign the heap initialization code to bootstrap of something else
(which is still in the actual BSS).
In addition, some payloads may want to intentionally zero out all of
memory for security reasons. In order to not leave a megabytes-wide hole
in that protection, this patch also offers functions to intetionally
zero all unused memory in the heap. Finally, payloads may (for the same
reason) want to be able to query the exact location of heap and DMA
heap, so move their controlling data structures into lib_sysinfo to
allow external access.
Also adds some much-needed console warnings for various error cases that
the allocator previously just silently ignored, and removes a 'volatile'
qualifier from a pointer in the main allocator function since I can find
no good reason why that should be there.
Change-Id: Ieaf656933528b5f3af41860ad9d123393561632e
Signed-off-by: Julius Werner <jwerner at chromium.org>
---
payloads/libpayload/arch/arm/libpayload.ldscript | 13 +-
payloads/libpayload/arch/arm64/libpayload.ldscript | 11 +-
payloads/libpayload/arch/mips/libpayload.ldscript | 16 ++-
payloads/libpayload/arch/x86/libpayload.ldscript | 13 +-
payloads/libpayload/include/stdlib.h | 2 +
payloads/libpayload/include/sysinfo.h | 14 +++
payloads/libpayload/libc/malloc.c | 132 +++++++++++++--------
7 files changed, 122 insertions(+), 79 deletions(-)
diff --git a/payloads/libpayload/arch/arm/libpayload.ldscript b/payloads/libpayload/arch/arm/libpayload.ldscript
index 492bd0c..00c87f7 100644
--- a/payloads/libpayload/arch/arm/libpayload.ldscript
+++ b/payloads/libpayload/arch/arm/libpayload.ldscript
@@ -67,19 +67,16 @@ SECTIONS
/* Stack and heap */
- . = ALIGN(16);
- _heap = .;
- . += CONFIG_LP_HEAP_SIZE;
- . = ALIGN(16);
- _eheap = .;
-
_estack = .;
. += CONFIG_LP_STACK_SIZE;
. = ALIGN(16);
_stack = .;
- }
- _end = .;
+ . = ALIGN(16);
+ _heap = .;
+ _eheap = . + ALIGN(CONFIG_LP_HEAP_SIZE, 16);
+ _end = _eheap;
+ }
/DISCARD/ : {
*(.comment)
diff --git a/payloads/libpayload/arch/arm64/libpayload.ldscript b/payloads/libpayload/arch/arm64/libpayload.ldscript
index 5c807ce..903034a 100644
--- a/payloads/libpayload/arch/arm64/libpayload.ldscript
+++ b/payloads/libpayload/arch/arm64/libpayload.ldscript
@@ -67,12 +67,6 @@ SECTIONS
/* Stack and heap */
- . = ALIGN(16);
- _heap = .;
- . += CONFIG_LP_HEAP_SIZE;
- . = ALIGN(16);
- _eheap = .;
-
_estack = .;
. += CONFIG_LP_STACK_SIZE;
. = ALIGN(16);
@@ -86,6 +80,11 @@ SECTIONS
. += CONFIG_LP_STACK_SIZE;
. = ALIGN(16);
_exc_stack = .;
+
+ . = ALIGN(16);
+ _heap = .;
+ _eheap = . + ALIGN(CONFIG_LP_HEAP_SIZE, 16);
+ _end = _eheap;
}
_end = .;
diff --git a/payloads/libpayload/arch/mips/libpayload.ldscript b/payloads/libpayload/arch/mips/libpayload.ldscript
index 351c225..1d4c7b2 100644
--- a/payloads/libpayload/arch/mips/libpayload.ldscript
+++ b/payloads/libpayload/arch/mips/libpayload.ldscript
@@ -62,20 +62,18 @@ SECTIONS
/* Stack and heap */
- . = ALIGN(16);
- _heap = .;
- . += CONFIG_LP_HEAP_SIZE;
- . = ALIGN(16);
- _eheap = .;
-
_estack = .;
. += CONFIG_LP_STACK_SIZE;
. = ALIGN(16);
_stack = .;
- }
- _ebss = .;
- _end = .;
+ _ebss = .;
+
+ . = ALIGN(16);
+ _heap = .;
+ _eheap = . + ALIGN(CONFIG_LP_HEAP_SIZE, 16);
+ _end = _eheap;
+ }
/DISCARD/ : {
*(.comment)
diff --git a/payloads/libpayload/arch/x86/libpayload.ldscript b/payloads/libpayload/arch/x86/libpayload.ldscript
index bcb2165..99aa8af 100644
--- a/payloads/libpayload/arch/x86/libpayload.ldscript
+++ b/payloads/libpayload/arch/x86/libpayload.ldscript
@@ -66,19 +66,16 @@ SECTIONS
/* Stack and heap */
- . = ALIGN(16);
- _heap = .;
- . += CONFIG_LP_HEAP_SIZE;
- . = ALIGN(16);
- _eheap = .;
-
_estack = .;
. += CONFIG_LP_STACK_SIZE;
. = ALIGN(16);
_stack = .;
- }
- _end = .;
+ . = ALIGN(16);
+ _heap = .;
+ _eheap = . + ALIGN(CONFIG_LP_HEAP_SIZE, 16);
+ _end = _eheap;
+ }
/DISCARD/ : {
*(.comment)
diff --git a/payloads/libpayload/include/stdlib.h b/payloads/libpayload/include/stdlib.h
index 689bf01..6674b32 100644
--- a/payloads/libpayload/include/stdlib.h
+++ b/payloads/libpayload/include/stdlib.h
@@ -154,6 +154,8 @@ void *dma_memalign(size_t align, size_t size);
void init_dma_memory(void *start, u32 size);
int dma_initialized(void);
int dma_coherent(void *ptr);
+void wipe_heap(void);
+void dma_wipe_heap(void);
static inline void *xmalloc_work(size_t size, const char *file,
const char *func, int line)
diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h
index 2110d88..bf5fddf 100644
--- a/payloads/libpayload/include/sysinfo.h
+++ b/payloads/libpayload/include/sysinfo.h
@@ -42,6 +42,17 @@
struct cb_serial;
+struct memory_type {
+ void *start;
+ void *end;
+ struct align_region_t* align_regions;
+#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
+ int magic_initialized;
+ size_t minimal_free;
+ const char *name;
+#endif
+};
+
/*
* All pointers in here shall be virtual.
*
@@ -124,6 +135,9 @@ struct sysinfo_t {
uint64_t boot_media_size;
uint64_t mtc_start;
uint32_t mtc_size;
+
+ struct memory_type heap;
+ struct memory_type dma_heap;
};
extern struct sysinfo_t lib_sysinfo;
diff --git a/payloads/libpayload/libc/malloc.c b/payloads/libpayload/libc/malloc.c
index b7ac1a7..444c3a9 100644
--- a/payloads/libpayload/libc/malloc.c
+++ b/payloads/libpayload/libc/malloc.c
@@ -43,27 +43,9 @@
#include <libpayload.h>
#include <stdint.h>
-struct memory_type {
- void *start;
- void *end;
- struct align_region_t* align_regions;
-#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
- int magic_initialized;
- size_t minimal_free;
- const char *name;
-#endif
-};
-
extern char _heap, _eheap; /* Defined in the ldscript. */
-
-static struct memory_type default_type =
- { (void *)&_heap, (void *)&_eheap, NULL
-#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
- , 0, 0, "HEAP"
-#endif
- };
-static struct memory_type *const heap = &default_type;
-static struct memory_type *dma = &default_type;
+static struct memory_type *const heap = &lib_sysinfo.heap;
+static struct memory_type *dma = &lib_sysinfo.heap; /* NOT dma_heap! */
typedef u64 hdrtype_t;
#define HDRSIZE (sizeof(hdrtype_t))
@@ -93,26 +75,47 @@ void init_dma_memory(void *start, u32 size)
return;
}
- /*
- * DMA memory might not be zeroed by Coreboot on stage loading, so make
- * sure we clear the magic cookie from last boot.
- */
- *(hdrtype_t *)start = 0;
-
- dma = malloc(sizeof(*dma));
+ dma = &lib_sysinfo.dma_heap;
dma->start = start;
dma->end = start + size;
dma->align_regions = NULL;
+ hdrtype_t *ptr = (hdrtype_t *)dma->start;
+ *ptr = FREE_BLOCK(dma->end - dma->start - HDRSIZE);
+
#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
- dma->minimal_free = 0;
dma->magic_initialized = 0;
+ dma->minimal_free = dma->end - dma->start - HDRSIZE;
dma->name = "DMA";
printf("Initialized cache-coherent DMA memory at [%p:%p]\n", start, start + size);
#endif
}
+/* This should be called before every access to *type to ensure it has been
+ * initialized. It will only trigger for the normal heap (since both start and
+ * size get initialized 0 as part of the global lib_sysinfo). The DMA type will
+ * not get set up until an explicit call to init_dma_memory(). */
+static void ensure_initialized(struct memory_type *type)
+{
+ if (type->start == type->end) {
+ type->start = &_heap;
+ type->end = &_eheap;
+ type->align_regions = NULL;
+
+ hdrtype_t *ptr = (hdrtype_t *)type->start;
+ *ptr = FREE_BLOCK(type->end - type->start - HDRSIZE);
+
+#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
+ type->magic_initialized = 1;
+ type->minimal_free = type->end - type->start - HDRSIZE;
+ type->name = "HEAP";
+ printf("Initialized heap memory at [%p:%p]\n", type->start,
+ type->end);
+#endif
+ }
+}
+
int dma_initialized()
{
return dma != heap;
@@ -126,28 +129,18 @@ int dma_coherent(void *ptr)
static void *alloc(int len, struct memory_type *type)
{
- hdrtype_t header;
- hdrtype_t volatile *ptr = (hdrtype_t volatile *)type->start;
-
/* Align the size. */
len = ALIGN_UP(len, HDRSIZE);
if (!len || len > MAX_SIZE)
return (void *)NULL;
- /* Make sure the region is setup correctly. */
- if (!HAS_MAGIC(*ptr)) {
- size_t size = (type->end - type->start) - HDRSIZE;
- *ptr = FREE_BLOCK(size);
-#if IS_ENABLED(CONFIG_LP_DEBUG_MALLOC)
- type->magic_initialized = 1;
- type->minimal_free = size;
-#endif
- }
+ ensure_initialized(type);
+ hdrtype_t *ptr = (hdrtype_t *)type->start;
/* Find some free space. */
do {
- header = *ptr;
+ hdrtype_t header = *ptr;
int size = SIZE(header);
if (!HAS_MAGIC(header) || size == 0) {
@@ -159,7 +152,8 @@ static void *alloc(int len, struct memory_type *type)
if (header & FLAG_FREE) {
if (len <= size) {
- hdrtype_t volatile *nptr = (hdrtype_t volatile *)((uintptr_t)ptr + HDRSIZE + len);
+ hdrtype_t *nptr = (hdrtype_t *)((uintptr_t)ptr
+ + HDRSIZE + len);
int nsize = size - (HDRSIZE + len);
/* If there is still room in this block,
@@ -182,7 +176,7 @@ static void *alloc(int len, struct memory_type *type)
}
}
- ptr = (hdrtype_t volatile *)((uintptr_t)ptr + HDRSIZE + size);
+ ptr = (hdrtype_t *)((uintptr_t)ptr + HDRSIZE + size);
} while (ptr < (hdrtype_t *) type->end);
@@ -225,16 +219,39 @@ static void _consolidate(struct memory_type *type)
}
}
+static void wipe(struct memory_type *type)
+{
+ ensure_initialized(type);
+ hdrtype_t *ptr = type->start;
+
+ do {
+ hdrtype_t header = *ptr;
+ int size = SIZE(header);
+
+ if (!HAS_MAGIC(header) || size == 0)
+ die("memory allocator panic on wipe");
+
+ if (header & FLAG_FREE)
+ memset((void *)ptr + HDRSIZE, 0, size);
+
+ ptr = (hdrtype_t *)((void *)ptr + HDRSIZE + size);
+ } while ((void *)ptr < type->end);
+}
+
void free(void *ptr)
{
hdrtype_t hdr;
struct memory_type *type = heap;
- /* Sanity check. */
+ /* Sanity check (will fail if heap is not yet initialized). */
if (ptr < type->start || ptr >= type->end) {
type = dma;
- if (ptr < type->start || ptr >= type->end)
+ if (ptr < type->start || ptr >= type->end) {
+ if (ptr != NULL)
+ printf("ERROR: free(%p) outside the heap!\n",
+ ptr);
return;
+ }
}
if (free_aligned(ptr, type)) return;
@@ -243,12 +260,16 @@ void free(void *ptr)
hdr = *((hdrtype_t *) ptr);
/* Not our header (we're probably poisoned). */
- if (!HAS_MAGIC(hdr))
+ if (!HAS_MAGIC(hdr)) {
+ printf("ERROR: free(%p) was not previously allocated!\n", ptr);
return;
+ }
/* Double free. */
- if (hdr & FLAG_FREE)
+ if (hdr & FLAG_FREE) {
+ printf("ERROR: free(%p) had already been freed before!\n", ptr);
return;
+ }
*((hdrtype_t *) ptr) = FREE_BLOCK(SIZE(hdr));
_consolidate(type);
@@ -286,8 +307,11 @@ void *realloc(void *ptr, size_t size)
pptr = ptr - HDRSIZE;
- if (!HAS_MAGIC(*((hdrtype_t *) pptr)))
+ if (!HAS_MAGIC(*((hdrtype_t *) pptr))) {
+ printf("ERROR: realloc(%p) was not previously allocated!\n",
+ ptr);
return NULL;
+ }
if (ptr < type->start || ptr >= type->end)
type = dma;
@@ -316,6 +340,16 @@ void *realloc(void *ptr, size_t size)
return ret;
}
+void wipe_heap(void)
+{
+ wipe(heap);
+}
+
+void dma_wipe_heap(void)
+{
+ wipe(dma);
+}
+
struct align_region_t
{
/* If alignment is 0 then the region reqpresents a large region which
@@ -459,6 +493,7 @@ static void *alloc_aligned(size_t align, size_t size, struct memory_type *type)
const size_t large_request = 1024;
if (size == 0) return 0;
+ ensure_initialized(type);
if (type->align_regions == 0) {
type->align_regions = malloc(sizeof(struct align_region_t));
if (type->align_regions == NULL)
@@ -547,6 +582,7 @@ void print_malloc_map(void)
int free_memory;
again:
+ ensure_initialized(type);
ptr = type->start;
free_memory = 0;
More information about the coreboot-gerrit
mailing list