[PATCH] malloc: check tcache mem size in tcache_get_n to avoid arbitrary mem allocation
dbgbgtf
dudududumaxver@gmail.com
Thu May 8 13:41:06 GMT 2025
Hi,
>> I understood, but checks in `tcache_put` is too early.
> We need checks on both put and get, depending on the type of exploit
> we're guarding against.
Arbitrary mem allocation `always` comes from tcache_get_n, that's the best way to prevent arbitrary mem allocation.
If you really want to add check in tcache_put, please ensure my testcase passes correctly.
> At some point, we have to assume if a hacker can do XYZ, they can do far
> worse things without bothering to corrupt memory first. Our protection
> in tcache needs to balance risk vs performance, even more so than the
> main malloc code. By that I mean if we make tcache so paranoid that
> it's slower than not using tcache, we've lost.
Perhaps you consider tcache_get_n a hot path and are hesitant to add
checks there due to potential performance impact.
But take a look at my benchmark results, I'd say the cost is acceptable.
If possible, I would like to see your test results as well.
Here are results based on my patch
```
❯ cat benchtests/bench-malloc-simple-128.out
{
"timing_type": "hp_timing",
"functions": {
"malloc": {
"": {
"malloc_block_size": 128,
"max_rss": 5644,
"main_arena_st_allocs_0025_time": 46.8032,
"main_arena_st_allocs_0100_time": 49.2103,
"main_arena_st_allocs_0400_time": 49.7546,
"main_arena_st_allocs_1600_time": 94.4427,
"main_arena_mt_allocs_0025_time": 71.1103,
"main_arena_mt_allocs_0100_time": 87.8427,
"main_arena_mt_allocs_0400_time": 92.6274,
"main_arena_mt_allocs_1600_time": 137.929,
"thread_arena__allocs_0025_time": 71.4449,
"thread_arena__allocs_0100_time": 86.8768,
"thread_arena__allocs_0400_time": 90.9421,
"thread_arena__allocs_1600_time": 135.167
}
}
}
}%
❯ cat benchtests/bench-malloc-thread-8.out
{
"timing_type": "hp_timing",
"functions": {
"malloc": {
"": {
"duration": 2.33475e+11,
"iterations": 8.90818e+09,
"time_per_iteration": 26.2091,
"max_rss": 5340,
"threads": 8,
"min_size": 4,
"max_size": 32768,
"random_seed": 88
}
}
}
}%
```
Here are the results based on origin/master
```
❯ cat benchtests/bench-malloc-simple-128.out
{
"timing_type": "hp_timing",
"functions": {
"malloc": {
"": {
"malloc_block_size": 128,
"max_rss": 5644,
"main_arena_st_allocs_0025_time": 49.0039,
"main_arena_st_allocs_0100_time": 50.4858,
"main_arena_st_allocs_0400_time": 49.2787,
"main_arena_st_allocs_1600_time": 96.8805,
"main_arena_mt_allocs_0025_time": 71.2422,
"main_arena_mt_allocs_0100_time": 87.7239,
"main_arena_mt_allocs_0400_time": 92.2462,
"main_arena_mt_allocs_1600_time": 142.958,
"thread_arena__allocs_0025_time": 71.1473,
"thread_arena__allocs_0100_time": 87.3973,
"thread_arena__allocs_0400_time": 91.4813,
"thread_arena__allocs_1600_time": 135.501
}
}
}
}%
❯ cat benchtests/bench-malloc-thread-8.out
{
"timing_type": "hp_timing",
"functions": {
"malloc": {
"": {
"duration": 2.33469e+11,
"iterations": 9.28604e+09,
"time_per_iteration": 25.142,
"max_rss": 5588,
"threads": 8,
"min_size": 4,
"max_size": 32768,
"random_seed": 88
}
}
}
}%
```
Thanks,
dbgbgtf
Signed-off-by: dbgbgtf <dudududuMaxVer@gmail.com>
---
malloc/malloc.c | 7 +--
malloc/tst-tcache-arbitrary-alloc.c | 90 +++++++++++++++++++++++++++++
2 files changed, 92 insertions(+), 5 deletions(-)
create mode 100644 malloc/tst-tcache-arbitrary-alloc.c
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 9d860eac9c..6ad2f212dc 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3186,6 +3186,8 @@ tcache_get_n (size_t tc_idx, tcache_entry **ep)
if (__glibc_unlikely (!aligned_OK (e)))
malloc_printerr ("malloc(): unaligned tcache chunk detected");
+ if (__glibc_unlikely (tc_idx != csize2tidx( chunksize_nomask (mem2chunk(e)))))
+ malloc_printerr ("malloc(): tcache mem size vs request size");
if (ep == &(tcache->entries[tc_idx]))
*ep = REVEAL_PTR (e->next);
@@ -3956,11 +3958,6 @@ _int_malloc (mstate av, size_t bytes)
while (tcache->counts[tc_idx] < mp_.tcache_count
&& (tc_victim = *fb) != NULL)
{
- if (__glibc_unlikely (misaligned_chunk (tc_victim)))
- malloc_printerr ("malloc(): unaligned fastbin chunk detected 3");
- size_t victim_tc_idx = csize2tidx (chunksize (tc_victim));
- if (__glibc_unlikely (tc_idx != victim_tc_idx))
- malloc_printerr ("malloc(): chunk size mismatch in fastbin");
if (SINGLE_THREAD_P)
*fb = REVEAL_PTR (tc_victim->fd);
else
diff --git a/malloc/tst-tcache-arbitrary-alloc.c b/malloc/tst-tcache-arbitrary-alloc.c
new file mode 100644
index 0000000000..b785006984
--- /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()
+{
+ printf("here are some danger instructions\n");
+ 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()
+{
+ printf("here are some safe instructions\n");
+ void *ptr[4];
+ ptr[0] = malloc(0x100);
+ ptr[1] = malloc(0x100);
+
+ free (ptr[1]);
+ free (ptr[0]);
+}
+
+int
+main ()
+{
+ safe();
+
+ danger();
+}
--
2.49.0
More information about the Libc-alpha
mailing list