On Wed, Sep 07, 2016 at 06:29:07PM -0400, Ted Unangst wrote:
> Instead of always using a fixed byte pattern, I think malloc should use a
> random pattern. Now, this sometimes means it's harder to identify exactly
> what's used after free, so we should provide a means to get the old 0xdf
> pattern back.
>
> Since we already have two junk modes, I thought I'd carry on along those
> lines. The default junk behavior, for free chunks only, is more of a security
> measure. I think this means we want random junk. The second level 'J' junk is
> more of a debugging tool, so that retains 0xdf.
>
> There's some overlap here with canaries, but nothing wrong with that. :)
Like it, though I am a bit worried about the costs. Any measurements?
Should be able to look more closely the coming days.
BTW, we should revisit canaries and work further on moving them
closer to requested size. There's a chance this diff wil complicate
that.
-Otto
>
> Index: malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.195
> diff -u -p -r1.195 malloc.c
> --- malloc.c 1 Sep 2016 10:41:02 -0000 1.195
> +++ malloc.c 7 Sep 2016 22:21:37 -0000
> @@ -186,6 +186,7 @@ struct malloc_readonly {
> #endif
> u_int32_t malloc_canary; /* Matched against ones in malloc_pool
> */
> uintptr_t malloc_chunk_canary;
> + u_char malloc_junkbytes[256];
> };
>
> /* This object is mapped PROT_READ after initialisation to prevent tampering
> */
> @@ -597,6 +598,8 @@ omalloc_init(void)
> mopts.malloc_move = 1;
> mopts.malloc_cache = MALLOC_DEFAULT_CACHE;
>
> + arc4random_buf(mopts.malloc_junkbytes, sizeof(mopts.malloc_junkbytes));
> +
> for (i = 0; i < 3; i++) {
> switch (i) {
> case 0:
> @@ -1260,7 +1263,29 @@ malloc(size_t size)
> /*DEF_STRONG(malloc);*/
>
> static void
> -validate_junk(struct dir_info *pool, void *p) {
> +random_junk(void *p, size_t amt)
> +{
> + u_long offset = ((u_long)p) & (sizeof(mopts.malloc_junkbytes) - 1);
> +
> + if (amt < sizeof(mopts.malloc_junkbytes) - offset) {
> + memcpy(p, mopts.malloc_junkbytes + offset, amt);
> + } else {
> + memcpy(p, mopts.malloc_junkbytes + offset,
> + sizeof(mopts.malloc_junkbytes) - offset);
> + amt -= sizeof(mopts.malloc_junkbytes) - offset;
> + while (amt > 0) {
> + size_t x = amt > sizeof(mopts.malloc_junkbytes) ?
> + sizeof(mopts.malloc_junkbytes) : amt;
> + memcpy(p, mopts.malloc_junkbytes, x);
> + amt -= x;
> + }
> + }
> +}
> +
> +
> +static void
> +validate_junk(struct dir_info *pool, void *p)
> +{
> struct region_info *r;
> size_t byte, sz;
>
> @@ -1274,9 +1299,15 @@ validate_junk(struct dir_info *pool, voi
> sz -= mopts.malloc_canaries;
> if (sz > 32)
> sz = 32;
> - for (byte = 0; byte < sz; byte++) {
> - if (((unsigned char *)p)[byte] != SOME_FREEJUNK)
> + if (mopts.malloc_junk == 1) {
> + u_long offset = ((u_long)p) & (sizeof(mopts.malloc_junkbytes) -
> 1);
> + if (memcmp(p, mopts.malloc_junkbytes + offset, sz) != 0)
> wrterror(pool, "use after free", p);
> + } else {
> + for (byte = 0; byte < sz; byte++) {
> + if (((unsigned char *)p)[byte] != SOME_FREEJUNK)
> + wrterror(pool, "use after free", p);
> + }
> }
> }
>
> @@ -1336,10 +1367,11 @@ ofree(struct dir_info *argpool, void *p)
> }
> STATS_SUB(pool->malloc_guarded, mopts.malloc_guard);
> }
> - if (mopts.malloc_junk && !mopts.malloc_freeunmap) {
> - size_t amt = mopts.malloc_junk == 1 ? MALLOC_MAXCHUNK :
> - PAGEROUND(sz) - mopts.malloc_guard;
> - memset(p, SOME_FREEJUNK, amt);
> + if (mopts.malloc_junk == 2 && !mopts.malloc_freeunmap) {
> + memset(p, SOME_FREEJUNK,
> + PAGEROUND(sz) - mopts.malloc_guard);
> + } else if (mopts.malloc_junk == 1 && !mopts.malloc_freeunmap) {
> + random_junk(p, MALLOC_MAXCHUNK);
> }
> unmap(pool, p, PAGEROUND(sz));
> delete(pool, r);
> @@ -1347,8 +1379,10 @@ ofree(struct dir_info *argpool, void *p)
> void *tmp;
> int i;
>
> - if (mopts.malloc_junk && sz > 0)
> + if (mopts.malloc_junk == 2 && sz > 0)
> memset(p, SOME_FREEJUNK, sz - mopts.malloc_canaries);
> + else if (mopts.malloc_junk == 1 && sz > 0)
> + random_junk(p, sz);
> if (!mopts.malloc_freenow) {
> if (find_chunknum(pool, r, p) == -1)
> goto done;
