[PATCH] malloc: check tcache mem size in tcache_get_n to avoid arbitrary mem allocation

[Wilco Dijkstra]

Hi,

Sorry for the delay - there has been a lot of malloc activity recently...

>> If the malloc succeeds despite corruption, it will likely fail if that block is freed
>> again, or on the next malloc that uses the now corrupted freelist. Adding free(ptr[0])
>> at the end of your test causes "double free or corruption".
>
> `ptr[0]` is the address of `ptr[2]` at the end of the test.
> At this time, attackers already got an arbitrary mem alloc, we shouldn't
> leave it to `free` or another `malloc`.

On AArch64 the alignment check does usually trigger on that final malloc in the test
without your patch, so it is capable of detecting this particular corruption. That seems
to be an issue with the testcase, however it's true the existing checks aren't consistent.

One thing that looks wrong in the existing checks is that we don't even try to maintain
simple invariants. Currently tcache_put doesn't always guarantee the chunk is aligned,
but we could trivially ensure that by moving the alignment check inside it. After that
the alignment check in tcache_get_n becomes redundant, and instead we could check
the *next* pointer is aligned and that way maintain the invariant as well as detect heap
corruption earlier.

Changing tcache_get_n to check the next pointer ensures your testcase passes without
needing additional checks. I believe that this is the best way forward - rather than just
adding random checks on some paths, we should be maintaining clear invariants on
all paths.

Additionally it would be great to have a generic testcase as part of the patch - to make
it work in GLIBC, you'll need to ensure it works with the default -O2 flag (which means
adding volatile in a few places). Also the ptr array needs to be suitably aligned so that
&ptr[2] has MALLOC_ALIGNMENT as otherwise existing checks trigger. And the test
may need to be excluded from the check and mcheck cases.

>> You can statically link with GLIBC, but that's not needed here - "make bench" will generate
>> benchmarks which will always dynamically link with your local GLIBC build.
>
> Is that true?

Yes, it overrides the default dynamic linker and thus always links with the latest GLIBC.

Cheers,
Wilco