On 02/11/15 06:40 AM, Theo Buehler wrote:
> Sorry for this rather long mail:
>
> I have three small comments on the patch itself
> (starting 80 lines below).
>
> For those who want to try both new features, I attached a patch against
> -current that merges the three parts of Daniel's diff (plus the trivial
> two of the nits below) at the very end of this mail.
>
> In addition to Daniel's test program, here's a small program that
> in 1000 test runs on amd64 always triggered an abort at the second
> free().
>
>
> $ cat heap.c
> #include <err.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <strings.h>
>
> int
> main(void)
> {
> char *p;
>
> p = malloc(1);
> if (p == NULL)
> err(1, "malloc");
>
> p = strcpy(p, "1234567");
> printf("%s\n", p);
> free(p);
>
> p = malloc(1);
> p = strcpy(p, "12345678");
> printf("%s\n", p);
> free(p);
>
> return 0;
> }
> $ cc -o heap heap.c
> /tmp//ccHH1E1h.o: In function `main': heap.c:(.text+0x6b): warning: warning:
> strcpy() is almost always
> misused, please use strlcpy()
> $ ./heap
> 1234567
> 12345678
> heap(5992) in free(): error: chunk canary corrupted 0x1f4619f0e9c0
> Abort trap (core dumped)
> $
>
>
> The performance impact of MALLOC_OPTIONS='CSV' is very noticeable,
> but 'CV' is negligible and 'CJV' makes the system feel a bit less
> snappy on my machine, but the actual effect seems almost negligible.
>
> Here's a rough 'benchmark':
>
> Building the base system on my i7 Thinkpad T420 with 4G RAM,
> with the various malloc options each with a freshly booted system.
>
> defaults: 31m04
> CV: 33m16
> CJV: 33m46
> CSV: 53m32
>
> Unfortunately, I have yet to hit an actual bug with this, one that isn't
> triggered by simple test programs. Casual browsing with firefox, even
> watching movies with mplayer is possible with MALLOC_OPTIONS='CSV'.
It would help quite a bit to bump up the size of the quarantine. The
default 16 large one doesn't keep around the allocations for very long.
A more expensive version of the feature could also check for either zero
or junk data before allocating anything. Haven't decided how to expose
that via the option though.
Most bugs triggering in fairly normal situations with Firefox, etc. have
probably already been found by Valgrind though. The strength of features
like this in malloc is that they can enabled all the time with little
performance loss, so eventually they'll find new bugs (which has been
the case for us on Android, but perhaps that's mostly because many apps
were never tested at all with ASAN/Valgrind).
It may also be viable as a security feature in some situations, not just
a way of finding bugs. Depends on how far away the UAF is from the free
call since one other free is all that's needed to lose reliable
detection. It might work better with a FIFO ring buffer rather than the
current fully randomized array (perhaps a mix? dunno).
> On Sun, Nov 01, 2015 at 02:56:11PM -0500, Daniel Micay wrote:
>> @@ -560,6 +563,12 @@ omalloc_init(struct dir_info **dp)
>> case 'J':
>> mopts.malloc_junk = 2;
>> break;
>> + case 'v':
>> + mopts.malloc_validate = 0;
>> + break;
>> + case 'V':
>> + mopts.malloc_validate = 1;
>> + break;
>> case 'n':
>> case 'N':
>> break;
>
> I'd keep the alphabetical order in the switch.
>
>> @@ -608,6 +617,9 @@ omalloc_init(struct dir_info **dp)
>> }
>> }
>>
>> + if (!mopts.malloc_junk)
>> + mopts.malloc_validate = 0;
>> +
>
> Wouldn't it make sense to just let 'V' imply 'J' (and 'j' imply 'v') as
> is already done with 'F' and 'U', respectively?
The V option doesn't benefit from J since it's only able to check for
use-after-frees in small allocations. I don't think there's a good
reason to tie them together.
A side note is that it would be nice if junk freeing was split up into
the security feature (full junk fill on free) and the debugging feature
(full junk fill on init). I ended up doing that in CopperheadOS because
it makes J significantly cheaper without giving up the nice security
properties gained by sanitizing everything on free. Might be too late to
change the meaning in OpenBSD though.
>> @@ -1190,6 +1208,35 @@ malloc(size_t size)
>> /*DEF_STRONG(malloc);*/
>>
>> static void
>> +validate_junk(void *p) {
>> + struct region_info *r;
>> + struct dir_info *pool = getpool();
>> + size_t byte, sz;
>> + if (p == NULL)
>> + return;
>> + r = find(pool, p);
>> + if (r == NULL)
>> + wrterror("bogus pointer in validate_junk", p);
>> + REALSIZE(sz, r);
>> + for (byte = 0; byte < sz; byte++) {
>> + if (((char *)p)[byte] != SOME_FREEJUNK) {
>
> This cast should be an (unsigned char *):
> we have SOME_FREEJUNK == 0xdf, so '((char *)p)[byte] != SOME_FREEJUNK'
> will always be true (this is pointed out when compiling with both clang
> and gcc on amd64).
Ah, I didn't run into this problem on Android because char is unsigned
for the Linux ABI on ARM. :)
signature.asc
Description: OpenPGP digital signature
