Hi,
This patch adds an opt-in malloc configuration option placing canaries after
small allocations to detect heap overflows on free(...). It's intended to be
used alongside guard pages for large allocations. Since it's essentially
adding extra padding to all small allocations, a small heap overflow will be
rendered harmless.
The current implementation uses pointer-size canaries, but it could be easily
extended to allow bumping up the size of the canaries by passing the option
multiple times. The entry points into malloc account for the canary size when
it's enabled and then it's generated on allocation and checked on free. Small
allocations without room for a canary are simply turned into large
allocations. Some care needs to be taken to avoid clobbering the canary in the
junk filling code and realloc copying.
The canary is placed at the very end of the memory allocations so there will
often be slack space in between the real allocation and the canary preventing
small overflows from being detected. It would be much better at detecting
corruption with finer-grained size classes. The extreme would be every
multiple of the alignment, but logarithmic growth would be more realistic (see
jemalloc's size classes). Finer-grained size classes would also reduce the
memory overhead caused by allocations being pushed into the next size class by
the canary.
The canaries are currently generated with canary_value ^ hash(canary_address).
It would be best to avoid involving addresses to avoid introducing address
leaks via read overflows where there were none before, but it's the easiest
way to get unique canaries and is a minor issue to improve down the road.
I implemented this feature after porting OpenBSD malloc to Android (in
CopperheadOS) and it has found a few bugs in the app ecosystem. Note that I've
only heavily tested it there, not on OpenBSD itself. I'm not sure if you want
this feature but it seemed worth submitting.
Hopefully you don't mind a patch generated with Git. :)
diff --git a/stdlib/malloc.c b/stdlib/malloc.c
index 424dd77..65b5027 100644
--- a/stdlib/malloc.c
+++ b/stdlib/malloc.c
@@ -185,12 +185,14 @@ struct malloc_readonly {
int malloc_move; /* move allocations to end of page? */
int malloc_realloc; /* always realloc? */
int malloc_xmalloc; /* xmalloc behaviour? */
+ size_t malloc_canaries; /* use canaries after chunks? */
size_t malloc_guard; /* use guard pages after allocations? */
u_int malloc_cache; /* free pages we cache */
#ifdef MALLOC_STATS
int malloc_stats; /* dump statistics at end */
#endif
u_int32_t malloc_canary; /* Matched against ones in malloc_pool
*/
+ uintptr_t malloc_chunk_canary;
};
/* This object is mapped PROT_READ after initialisation to prevent tampering */
@@ -526,6 +528,12 @@ omalloc_init(struct dir_info **dp)
case 'A':
mopts.malloc_abort = 1;
break;
+ case 'c':
+ mopts.malloc_canaries = 0;
+ break;
+ case 'C':
+ mopts.malloc_canaries = sizeof(void *);
+ break;
#ifdef MALLOC_STATS
case 'd':
mopts.malloc_stats = 0;
@@ -619,6 +627,9 @@ omalloc_init(struct dir_info **dp)
while ((mopts.malloc_canary = arc4random()) == 0)
;
+ arc4random_buf(&mopts.malloc_chunk_canary,
+ sizeof(mopts.malloc_chunk_canary));
+
/*
* Allocate dir_info with a guard page on either side. Also
* randomise offset inside the page at which the dir_info
@@ -984,8 +995,15 @@ malloc_bytes(struct dir_info *d, size_t size, void *f)
k += (lp - bp->bits) * MALLOC_BITS;
k <<= bp->shift;
+ if (mopts.malloc_canaries && bp->size > 0) {
+ char *end = (char *)bp->page + k + bp->size;
+ uintptr_t *canary = (uintptr_t *)(end - mopts.malloc_canaries);
+ *canary = mopts.malloc_chunk_canary ^ hash(canary);
+ }
+
if (mopts.malloc_junk == 2 && bp->size > 0)
- memset((char *)bp->page + k, SOME_JUNK, bp->size);
+ memset((char *)bp->page + k, SOME_JUNK,
+ bp->size - mopts.malloc_canaries);
return ((char *)bp->page + k);
}
@@ -999,6 +1017,13 @@ find_chunknum(struct dir_info *d, struct region_info *r,
void *ptr)
if (info->canary != d->canary1)
wrterror("chunk info corrupted", NULL);
+ if (mopts.malloc_canaries && info->size > 0) {
+ char *end = (char *)ptr + info->size;
+ uintptr_t *canary = (uintptr_t *)(end - mopts.malloc_canaries);
+ if (*canary != (mopts.malloc_chunk_canary ^ hash(canary)))
+ wrterror("chunk canary corrupted", ptr);
+ }
+
/* Find the chunk number on the page */
chunknum = ((uintptr_t)ptr & MALLOC_PAGEMASK) >> info->shift;
@@ -1121,7 +1146,7 @@ omalloc(size_t sz, int zero_fill, void *f)
/* takes care of SOME_JUNK */
p = malloc_bytes(pool, sz, f);
if (zero_fill && p != NULL && sz > 0)
- memset(p, 0, sz);
+ memset(p, 0, sz - mopts.malloc_canaries);
}
return p;
@@ -1176,6 +1201,8 @@ malloc(size_t size)
malloc_recurse();
return NULL;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omalloc(size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
@@ -1242,7 +1269,7 @@ ofree(void *p)
int i;
if (mopts.malloc_junk && sz > 0)
- memset(p, SOME_FREEJUNK, sz);
+ memset(p, SOME_FREEJUNK, sz - mopts.malloc_canaries);
if (!mopts.malloc_freenow) {
if (find_chunknum(pool, r, p) == -1)
return;
@@ -1386,16 +1413,24 @@ gotit:
}
}
if (newsz <= oldsz && newsz > oldsz / 2 && !mopts.malloc_realloc) {
- if (mopts.malloc_junk == 2 && newsz > 0)
- memset((char *)p + newsz, SOME_JUNK, oldsz - newsz);
+ if (mopts.malloc_junk == 2 && newsz > 0 && newsz < oldsz) {
+ size_t clearsz = oldsz - newsz;
+ if (oldsz <= MALLOC_MAXCHUNK)
+ clearsz -= mopts.malloc_canaries;
+ memset((char *)p + newsz, SOME_JUNK, clearsz);
+ }
STATS_SETF(r, f);
return p;
} else if (newsz != oldsz || mopts.malloc_realloc) {
q = omalloc(newsz, 0, f);
if (q == NULL)
return NULL;
- if (newsz != 0 && oldsz != 0)
- memcpy(q, p, oldsz < newsz ? oldsz : newsz);
+ if (newsz != 0 && oldsz != 0) {
+ size_t copysz = oldsz < newsz ? oldsz : newsz;
+ if (copysz <= MALLOC_MAXCHUNK)
+ copysz -= mopts.malloc_canaries;
+ memcpy(q, p, copysz);
+ }
ofree(p);
return q;
} else {
@@ -1420,6 +1455,8 @@ realloc(void *ptr, size_t size)
malloc_recurse();
return NULL;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = orealloc(ptr, size, CALLER);
malloc_active--;
@@ -1468,6 +1505,8 @@ calloc(size_t nmemb, size_t size)
}
size *= nmemb;
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omalloc(size, 1, CALLER);
malloc_active--;
@@ -1595,6 +1634,8 @@ posix_memalign(void **memptr, size_t alignment, size_t
size)
malloc_recurse();
goto err;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omemalign(alignment, size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
