Re: [PATCH] Fix tcache count maximum

[Wilco Dijkstra <Wilco dot Dijkstra at arm dot com>]

Hi DJ,

>Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
>> Well I already mentioned that all calls to tcache_get ensure there
>> is an entry:
>>
>>       && tcache->entries[tc_idx] != NULL)
>
> This is not a valid assumption.  Since the t->next entry in each chunk
> is part of the user data, it might be corrupted by the application.
> There's been a test case posted, too, I think - but it's a simple
> "modify after free" scenario.  The assert in tcache_get() is a double
> check that the linked list and the counts are kept in sync, or at least,
> if one is corrupted the other can detect it.

Well it's just one of the many possible corruptions that can make the list and
count go out of sync. We could change the above to check the count instead:

   && tcache->counts[tc_idx] > 0

That ensures we never return more blocks than were added, even when the list
gets completely corrupted.

Note if we care about list corruption, using an array of pointers to the free blocks
would be much better rather than storing critical pointers in the blocks themselves.
This can also give performance gains due to fewer TLB and cache misses.

>> Now it is of course feasible to overwrite the tcace count or the entries or corrupt
>> the blocks held in the tcache list. If that happens then all bets are off, since any
>> targeted corruption can be made to look like a valid entry. This is true for all the
>> malloc datastructures - you need to encrypt all the fields to reduce the chances
>> of being able to spoof the fields, but that is way too much overhead.
>
> Yes, and we have lots of double-checks for exactly that reason.  We've
> actually talked about encrypting the chunk headers too.

Yes the chunk headers are also easily corruptible. At least for small blocks it is
feasible to avoid using headers altogether so corrupting/spoofing the heap data
structure becomes much harder.

> But even so, the assert is unrelated to the overflow changes.  The rest
> of your patch is OK.

Sure I'll commit it with the assert for now, and create a separate path for the above
change to remove the asserts.

Wilco