diff options
author | Stewart Smith <stewart@linux.vnet.ibm.com> | 2014-10-16 18:05:07 +1100 |
---|---|---|
committer | Stewart Smith <stewart@linux.vnet.ibm.com> | 2014-10-17 11:55:16 +1100 |
commit | 78237cccad984a0d757e578ec2a9745de4fcfd87 (patch) | |
tree | af20630ea7da3c7093067a9683742986a152a347 /core | |
parent | 3234c68da5688069758697a0b134a14eb29ee874 (diff) | |
download | skiboot-78237cccad984a0d757e578ec2a9745de4fcfd87.zip skiboot-78237cccad984a0d757e578ec2a9745de4fcfd87.tar.gz skiboot-78237cccad984a0d757e578ec2a9745de4fcfd87.tar.bz2 |
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>
Diffstat (limited to 'core')
-rw-r--r-- | core/mem_region.c | 2 | ||||
-rw-r--r-- | core/test/run-malloc.c | 25 |
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; } |