[PATCH] Update tcache double-free check

[Carlos O'Donell]

On 7/25/20 6:39 AM, Eyal Itkin wrote:
> I've gone over the compiled binaries before/after the patch, and the
> change on my x64 binary is as follows: no change to tcache_get() or
> tcache_put(), and an addition of 2 asm instructions to _int_free(). If
> we would have only used ~tcache this would have been reduced to an
> addition of a single asm instruction in _int_free().
> 
> The patch's impact on benchtests/bench-malloc-(simple,thread) is
> negligible, and is less than the natural noise I get on my GCP
> instance between two consecutive benchmark executions.
> 
> Since the call to getrandom() is only on tcache_init(), hence once per
> thread, I'm fine with it being called for all threads, but it is
> really for you to decide.
> 
> There is a possible fix to make sure that the entropy we get from
> getrandom() will be accumulated, thus helping all future threads that
> might need it. We won't be able to update the keys of previous threads
> (if they lacked entropy), but we can enrich future threads with the
> entropy even if they failed to use getrandom(). This solution will
> also enable us to use all of the received entropy from getrandom()
> even when we get less than the wanted amount of bytes.
> 
> The solution for such an entropy accumulation is quite simple:
> 
> static __thread size_t tcache_key = 0;
> static size_t tcache_entropy = 0;
> 
> void tcache_init()
> {
>       ...
>       /* Attempt to get a random key for the double-free checks.  */
>       size_t local_entropy = 0;
>       getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK);
>       /* always accumulate it with the past seen entropy */
>       tcache_entropy ^= local_entropy;
>       /* always use the basic alternative of ~tcache in case we didn't
> have any entropy.  */
>       tcache_key = tcache_entropy ^ ~((size_t) tcache);
> }
> 
> As can be seen, in case that all calls to getrandom() failed, and we
> received nothing from it, we would still use ~tcache, just like
> before. In addition, this use of "~tcache" also adds the entropy from
> ASLR to the tcache_key, so we don't lose anything from using it.
> 
> Please advise me of the best way to proceed.

I like the accumulation of entropy. I'm a little worried about thread
startup and shutdown costs, but I haven't measured that yet. I'm also
busy getting the release out the door (I'm the release manager).
If you have thread startup/shutdown costs that would be interesting
for me to see as a reviewer.

I'd like to see Florian's opinion on this.

I fully expect that if you flesh this out you'll have to handle the
getrandom failure case, and the atomics required to avoid the data
race with multiple threads and global state. However, hold off on
that until we get a big more consensus if we really want to go to
this level of complexity. It would be good to keep the entropy we
have in the event the system is under some kind of entropy starvation
at a later point (I've seen at least one of these for TCP sequence
numbers, read and drop from /dev/random etc.)

-- 
Cheers,
Carlos.