On Thu, Sep 08, 2016 at 08:15:42AM +0200, Otto Moerbeek wrote:
> 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.
As often, real life came in between. Did anybody do measurements? I
really would like to to see hard data.
-Otto
>
> 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;
