in realloc(), memcpy() call would copy past end of allocation

or: Fix mem_size() to remove struct alloc_hdr from returned value

This bug was caught by switching test/run-malloc.c over to using
malloc/free (system malloc/free) to allocate the heap that we use
for testing our malloc and free.

Basically, when we did that, run-malloc.c test would get this
valgrind warning:
==3869== Invalid read of size 8
==3869==    at 0x4C2A706: memcpy (mc_replace_strmem.c:838)
==3869==    by 0x40323F: __realloc (malloc.c:69)
==3869==    by 0x405815: main (run-malloc.c:142)

Which was because in realloc(), when we have to relocate the allocated
bit of memory, we memcpy the contents of the old location into the new
one. The current mem_size() implementation *included* struct alloc_hdr
which mean that we were copying allocated size + sizeof(struct alloc_hdr)
from the returned pointer. This meant we read sizeof(struct alloc_hdr) past
the end of the allocation... which will pretty much always be harmless,
just get random junk in the realloc()ed space.

i.e. we would memcpy() 64+16 (80) bytes from the malloc(64) space to
the realloc(128) space, which is, obviously, 16 bytes more than we should.

IF we had some memory after a region that would make us explode if we read,
then we'd explode around the realloc() call... which would not be so good.
After a bit of a code audit I'm pretty sure this isn't going to actually
hurt us anywhere... or, at least, I hope not...

The fix is simple: fix mem_size() to subtract sizeof(struct alloc_hdr)
from the returned value. This should be okay with the other test case
that checks mem_size() result and there are no other mem_size() callers.

Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>
Acked-by: Rusty Russell <rusty@au1.ibm.com>

2 files changed, 25 insertions, 2 deletions

diff --git a/core/mem_region.c b/core/mem_region.c
index 8904a18..dd1f64f 100644
--- a/core/mem_region.c
+++ b/core/mem_region.c
@@ -399,7 +399,7 @@ void mem_free(struct mem_region *region, void *mem, const char *location)
 size_t mem_size(const struct mem_region *region __unused, const void *ptr)
 {
 	const struct alloc_hdr *hdr = ptr - sizeof(*hdr);
-	return hdr->num_longs * sizeof(long);
+	return hdr->num_longs * sizeof(long) - sizeof(struct alloc_hdr);
 }
 
 bool mem_resize(struct mem_region *region, void *mem, size_t len,
diff --git a/core/test/run-malloc.c b/core/test/run-malloc.c
index 226ce75..080e689 100644
--- a/core/test/run-malloc.c
+++ b/core/test/run-malloc.c
@@ -25,6 +25,23 @@ struct cpu_thread {
 	unsigned int			chip_id;
 };
 
+#include <stdlib.h>
+
+/* Use these before we undefine them below. */
+static inline void *real_malloc(size_t size)
+{
+	return malloc(size);
+}
+
+static inline void real_free(void *p)
+{
+	return free(p);
+}
+
+#undef malloc
+#undef free
+#undef realloc
+
 #include <skiboot.h>
 
 #define is_rodata(p) true
@@ -60,7 +77,8 @@ static bool heap_empty(void)
 
 int main(void)
 {
-	char test_heap[TEST_HEAP_SIZE], *p, *p2, *p3, *p4;
+	char *test_heap = real_malloc(TEST_HEAP_SIZE);
+	char *p, *p2, *p3, *p4;
 	size_t i;
 
 	/* Use malloc for the heap, so valgrind can find issues. */
@@ -116,13 +134,17 @@ int main(void)
 
 	/* Realloc with move. */
 	p2 = malloc(TEST_HEAP_SIZE - 64 - sizeof(struct alloc_hdr)*2);
+	memset(p2, 'a', TEST_HEAP_SIZE - 64 - sizeof(struct alloc_hdr)*2);
 	assert(p2);
 	p = malloc(64);
+	memset(p, 'b', 64);
+	p[63] = 'c';
 	assert(p);
 	free(p2);
 
 	p2 = realloc(p, 128);
 	assert(p2 != p);
+	assert(p2[63] == 'c');
 	free(p2);
 	assert(heap_empty());
 	assert(!mem_region_lock.lock_val);
@@ -140,5 +162,6 @@ int main(void)
 	assert(heap_empty());
 	assert(!mem_region_lock.lock_val);
 
+	real_free(test_heap);
 	return 0;
 }