malloc: Improved double free detection in the tcache

The previous double free detection did not account for an attacker to
use a terminating null byte overflowing from the previous
chunk to change the size of a memory chunk is being sorted into.
So that the check in 'tcache_double_free_verify' would pass
even though it is a double free.

Solution:
Let 'tcache_double_free_verify' iterate over all tcache entries to
detect double frees.

This patch only protects from buffer overflows by one byte.
But I would argue that off by one errors are the most common
errors to be made.

Alternatives Considered:
  Store the size of a memory chunk in big endian and thus
  the chunk size would not get overwritten because entries in the
  tcache are not that big.

  Move the tcache_key before the actual memory chunk so that it
  does not have to be checked at all, this would work better in general
  but also it would increase the memory usage.

Signed-off-by: David Lau  <david.lau@fau.de>
Reviewed-by: Wilco Dijkstra  <Wilco.Dijkstra@arm.com>

diff --git a/malloc/Makefile b/malloc/Makefile
index e2b2c1ae1bfe9bbf7cbcdbd3bacba5a0a28e04fd..3d3822db31000d60f15827ae7d3a08b545d3799e 100644 (file)
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -63,7 +63,7 @@ tests := \
   tst-realloc \
   tst-reallocarray \
   tst-safe-linking \
-  tst-tcfree1 tst-tcfree2 tst-tcfree3 \
+  tst-tcfree1 tst-tcfree2 tst-tcfree3 tst-tcfree4 \
   tst-trim1 \
   tst-valloc \
 # tests

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 9d860eac9cc923ef8f20218eb37f35b926f3ef82..9f44f5ab075faf99d427ac30c6ec8859c39a4a9b 100644 (file)
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3226,21 +3226,24 @@ tcache_available (size_t tc_idx)
 /* Verify if the suspicious tcache_entry is double free.
    It's not expected to execute very often, mark it as noinline.  */
 static __attribute__ ((noinline)) void
-tcache_double_free_verify (tcache_entry *e, size_t tc_idx)
+tcache_double_free_verify (tcache_entry *e)
 {
   tcache_entry *tmp;
-  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)
+  for (size_t tc_idx = 0; tc_idx < TCACHE_MAX_BINS; ++tc_idx)
     {
-      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");
+      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");
+       }
     }
   /* No double free detected - it might be in a tcache of another thread,
      or user data that happens to match the key.  Since we are not sure,
@@ -3428,7 +3431,7 @@ __libc_free (void *mem)
 
       /* Check for double free - verify if the key matches.  */
       if (__glibc_unlikely (e->key == tcache_key))
-        return tcache_double_free_verify (e, tc_idx);
+        return tcache_double_free_verify (e);
 
       if (__glibc_likely (tcache->counts[tc_idx] < mp_.tcache_count))
         return tcache_put (p, tc_idx);

diff --git a/malloc/tst-tcfree4.c b/malloc/tst-tcfree4.c
new file mode 100644 (file)
index 0000000..03850dd
--- /dev/null
+++ b/malloc/tst-tcfree4.c
@@ -0,0 +1,59 @@
+/* Test that malloc tcache catches double free.
+   Copyright (C) 2025 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <error.h>
+#include <limits.h>
+#include <malloc.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/signal.h>
+
+/* Test for a double free where the size information gets overwritten by a
+ * terminating null byte.  */
+static int
+do_test (void)
+{
+  /* The payload is exactly 0x19 Bytes long:
+   * 0x18 bytes 'B' and one terminating null byte
+   */
+  const char *payload = "BBBBBBBBBBBBBBBBBBBBBBBB";
+
+  char *volatile first_chunk
+      = malloc (strlen (payload)); // <-- off by one error
+  char *volatile second_chunk = malloc (0x118);
+
+  // free the second chunk the first time now it is in the tcache with tc_idx =
+  free (second_chunk);
+
+  // change the the size of the second_chunk using the terminating null byte if
+  // the PAYLOAD
+  strcpy (first_chunk, payload);
+
+  // now the second_chunk has a new size
+  // calling free a second time will not trigger the double free detection
+  free (second_chunk);
+
+  printf ("FAIL: tcache double free not detected\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>