[PATCH 0/1] Improved double free detection in the tcache.

[Wilco Dijkstra]

Hi David,

The new version looks good in terms of correctness. A few minor things:

- it needs rebasing to latest trunk (there were changes to tcache_double_free_verify)
- there are a few code formatting issues (see below)
- the commit message could be improved to describe the actual changes made
  (it discusses alternatives but not that it now scans through all sizes)
- please include libc-alpha so that it gets posted to the mailing list and tested
- the attachment you used does not get recognized and thus none of the tests run,
  so could you try just copying the patch into the email body?

+tcache_double_free_verify (tcache_entry *e)
+    for (size_t tc_idx = 0; tc_idx < TCACHE_MAX_BINS; ++tc_idx) {

The '{' should be on the next line indented by 2 more spaces, and the
body then by another 2. The 'for' seems to be indented by 4 rather than 2.

+  	size_t cnt = 0;
+  	LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);
+      for (tmp = tcache->entries[tc_idx];
+           tmp;
+           tmp = REVEAL_PTR (tmp->next), ++cnt)
+        {
+          if (cnt >= mp_.tcache_count)
+    	malloc_printerr ("free(): too many chunks detected in tcache");
+          if (__glibc_unlikely (!aligned_OK (tmp)))
+    	malloc_printerr ("free(): unaligned chunk detected in tcache 2");
+          if (tmp == e)
+    	malloc_printerr ("free(): double free detected in tcache 2");
+        }

Odd indentation, have all tabs been removed?

+++ b/malloc/tst-tcfree4.c
@@ -0,0 +1,59 @@
+/* Test that malloc tcache catches double free.
+   Copyright (C) 2018-2025 Free Software Foundation, Inc.

Should be just 2025.

> - With "poison null byte" I meant using a terminating null byte and an
> off by one error
>    to change the byte behind the buffer.
> - I added a testcase for this exact scenario (malloc/tst-tcfree4.c)

Thanks, that makes it clearer what case is being tested for.

> If there is a buffer overrun that overwrites more than the size field,
> the attacker would be able to overwrite the tcache_key and also the
> encrypted next pointer,
> thus making the check for the tcache_key useless.

Sure, if you somehow worked around the pointer swizzle, you could do
an overwrite that remains undetected.

> Yes, i think that in general sacrificing 8 byte of memory
> (or even better as you have suggested for 64 bit Systems truncating the
> tcache_key to 32 bytes
> and utilizing the 32 bit of the size field)
> to detect double frees would be a good tradeoff.

The main goal would not be to detect double frees (we need 1 bit for that),
but to reliably detect overwrites from the previous chunk. Storing more data
in the size field means we only need 1 load to get it all, which would make
the various checks simpler and cheaper.

> Also it would make leaking the tcahce_key harder.
> Because then the tchache_key would be before the pointer, that gets
> returned by malloc,
> thus a normal use after free would not leak the tcache_key.

True, however we don't consider it a secret - it could even be a constant value
in current usage. We should avoid using the word 'key' to avoid people thinking
it has to be cryptographically safe and protected from leaking...

> But I myself am totally new to the whole development of the glibc and
> therefor do not trust myself to
> implement this correctly.

Yes, I agree this would be complex since the current code is quite a mess.

Cheers,
Wilco