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

[dbgbgtf]

Hi

> The goal is to detect heap corruption cheaply and as early as possible.

I understood, but checks in `tcache_put` is too early.
And hackers can break the pointer swizzing after `tcache_put` easily.

> however if we wanted to make malloc
> safer, we'd have to stop using data structures that are stored in chunks.

I agree. But that is not up to me, I just want to add a tiny check.

> Tcache_put is used in several places on blocks that have been freed for a while
> and were stored in different lists, so they could have been corrupted at that point.

I don't want to rely on such a possibility, it's not safe.
Adding a check that can likely be bypassed is worse than having none at all.

Checks in `tcache_get` is effective and the cost is acceptable,
while checks in tcache_put costs and failed to provide meaningful validation.

> tcache_get already does a check - what I'm saying is that we can improve it.

Yeah, I mean why not put my check and align check in `tcache_put` together.

> This works, is cheap and triggers in your testcase.

If you look closely, your check can incorrectly terminate legitimate free operations.
(the malloc diff shows how I implemented your suggested patch)
In my new tst, your check leads to `malloc(): unaligned tcache chunk
detected` before dangerous instructions.

Anyway, I really don’t want to keep arguing about where my check should be placed.
This simple patch has already taken up too much of my time.

If your only concern about my patch is where the check should be
placed now, I suggest merging my check into the mainline first.
If you agree, I'll send the final version of my patch in the next email.

And then you can open a separate commit to discuss whether my new check—or any other checks—should be relocated.
I’ll continue to follow up and try to provide feedback.

Thanks,
dbgbgtf

Signed-off-by: dbgbgtf <dudududuMaxVer@gmail.com>
---
 malloc/malloc.c                     |  3 +
 malloc/tst-tcache-arbitrary-alloc.c | 90 +++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 malloc/tst-tcache-arbitrary-alloc.c

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 9d860eac9c..050fc00cd7 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3163,6 +3163,9 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
 {
   tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
 
+  if (__glibc_unlikely (!aligned_OK (REVEAL_PTR (e->next))))
+    malloc_printerr ("malloc(): unaligned tcache chunk detected");
+
   /* Mark this chunk as "in the tcache" so the test in __libc_free will
      detect a double free.  */
   e->key = tcache_key;
diff --git a/malloc/tst-tcache-arbitrary-alloc.c b/malloc/tst-tcache-arbitrary-alloc.c
new file mode 100644
index 0000000000..7941040d92
--- /dev/null
+++ b/malloc/tst-tcache-arbitrary-alloc.c
@@ -0,0 +1,90 @@
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/cdefs.h>
+#define INTERNAL_SIZE_T size_t
+
+#define mem2chunk(p) ((void *)((char *)(p) - CHUNK_HDR_SZ))
+#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
+#define CHUNK_HDR_SZ (2 * SIZE_SZ)
+
+struct malloc_chunk
+{
+
+  INTERNAL_SIZE_T mchunk_prev_size; /* Size of previous chunk (if free).  */
+  INTERNAL_SIZE_T mchunk_size;      /* Size in bytes, including overhead. */
+
+  struct malloc_chunk *fd; /* double links -- used only if free. */
+  struct malloc_chunk *bk;
+
+  /* Only used for large blocks: pointer to next larger size.  */
+  struct malloc_chunk *fd_nextsize; /* double links -- used only if free. */
+  struct malloc_chunk *bk_nextsize;
+};
+
+typedef struct malloc_chunk *mchunkptr;
+
+void danger()
+{
+  void *ptr[4];
+  ptr[0] = malloc (0x100);
+  ptr[1] = malloc (0x100);
+  // malloc aleast two chunk at the same size
+
+  free (ptr[1]);
+  free (ptr[0]);
+  // free the second one, and the the first one
+  // the tcachebin will be tcache->ptr[0]->ptr[1]
+
+  ptr[0] = mem2chunk (ptr[0]);
+
+  // checking if ptr[2] address is correctly aligned like 0x7fff ffff fff0
+  // if ptr[2] is not correctly aligned, use ptr[3] instead
+  // which should be a correctly aligned address
+  bool is_wrong_align = (INTERNAL_SIZE_T)&ptr[2] & 0x8;
+  if (is_wrong_align)
+    {
+      printf ("target address at: %p\n", &ptr[3]);
+      ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12)
+                                            ^ (INTERNAL_SIZE_T)(&ptr[3]));
+    }
+  else
+    {
+      printf ("target address at: %p\n", &ptr[2]);
+      ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12)
+                                            ^ (INTERNAL_SIZE_T)(&ptr[2]));
+    }
+  // if we change the ptr[0]->fd
+  // the bin will be tcache->ptr[0]->anywhere
+  // I use ptr[2] or ptr[3] address as example
+
+  ptr[1] = malloc (0x100);
+  ptr[0] = malloc (0x100);
+  // and the malloc them, take the tcachebin out
+  // to see if we have target address
+
+  printf ("ptr[0] point at: %p", ptr[0]);
+  // so we get target address now
+}
+
+void safe()
+{
+  void *ptr[4];
+  ptr[0] = malloc(0x100);
+  ptr[1] = malloc(0x100);
+
+  free (ptr[1]);
+  free (ptr[0]);
+}
+
+int
+main ()
+{
+  printf("here are some safe instructions\n");
+  safe();
+
+  printf("here are some danger instructions\n");
+  danger();
+}
-- 
2.49.0