With this patch on -current and
ln -sf CJ /etc/malloc.conf
a lot of things stopped working, like "man ls" and "tmux".
With only C or only J, the system seems to work ok, but with both it
doesn't.
Will check the resulting core files.
ntpd and tmux both bombed out on memset inside realloc if I read the dumps
ok:
Program terminated with signal 11, Segmentation fault.
(no debugging symbols found)
Loaded symbols for /usr/sbin/ntpd
Reading symbols from /usr/lib/libutil.so.12.1...done.
Loaded symbols for /usr/lib/libutil.so.12.1
Reading symbols from /usr/lib/libtls.so.9.0...done.
Loaded symbols for /usr/lib/libtls.so.9.0
Reading symbols from /usr/lib/libssl.so.37.0...done.
Loaded symbols for /usr/lib/libssl.so.37.0
Reading symbols from /usr/lib/libcrypto.so.36.0...done.
Loaded symbols for /usr/lib/libcrypto.so.36.0
Reading symbols from /usr/lib/libc.so.84.0...done.
Loaded symbols for /usr/lib/libc.so.84.0
Reading symbols from /usr/libexec/ld.so...done.
Loaded symbols for /usr/libexec/ld.so
#0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:51
51 L1: rep
(gdb) bt full
#0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:51
No locals.
#1 0x000016e1a67748f2 in realloc (ptr=0x16e22de6fd20, size=28)
at /usr/src/lib/libc/stdlib/malloc.c:1420
r = Variable "r" is not available.
2015-10-23 7:18 GMT+02:00 Daniel Micay <danielmi...@gmail.com>:
>
> 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();
>
--
May the most significant bit of your life be positive.
