diff mbox series

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

Message ID 20250404040119.121238-1-dudududuMaxVer@gmail.com (mailing list archive)
State New
Headers
Series malloc: check tcache mem size in tcache_get_n to avoid arbitrary mem allocation |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

dbgbgtf April 4, 2025, 4:01 a.m. UTC 
  Thanks for your reply.

I add a test, which will prove this check does avoid tcache arbitrary
alloc.Use `gcc malloc/tst-tcache-arbitrary-alloc.c` and then`./a.out` is
fine.

those results were produced on my x86_64 laptop.
I got these results by the follows commands, is that what you do?
I did those command in mybuild dir and build dir both, and compare the
result.

❯ make -O -j18
❯ make bench-clean
❯ make BENCH_DURATION=1 USE_CLOCK_GETTIME=1 USE_RDTSCP=1 bench
BENCHSET="malloc-simple malloc-thread"
❯ cat benchtests/bench-malloc-thread-1.out
❯ cat benchtests/bench-malloc-simple-16.out

Use`tc_idx != chunksize_nomask(mem2chunk(e))` is not correct.
`tc_idx` is obviously not size, so I need to convert tc_idx to size.

Using `chunksize_nomask` will return size with flags(0x91 for example).
while tc_idx can not provide chunk flags, so I have to use
`tidx2csize(tc_idx) != chunksize(mem2chunk(e))`.

Also I notice that `libc_malloc2` is noinline, that might be a waste
when USE_TCACHE is not on.Maybe alias malloc to __libc_malloc2 when
USE_TCACHE is not on?

Best,
dbgbgtf

Signed-off-by: dbgbgtf <dudududuMaxVer@gmail.com>
---
 malloc/tst-tcache-arbitrary-alloc.c | 52 +++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 malloc/tst-tcache-arbitrary-alloc.c

Comments

Wilco Dijkstra April 7, 2025, 9:20 a.m. UTC | #1 
Hi,

> I add a test, which will prove this check does avoid tcache arbitrary
> alloc.Use `gcc malloc/tst-tcache-arbitrary-alloc.c` and then`./a.out` is
> fine.

Thanks, having a test is always useful. This basically overwrites the freelist
with a correctly swizzled pointer to a fake block. Once an attacker can do that,
they can just as easily fake a correct chunk header and pass the extra check...
Note that it should trigger a check in free() since the size is corrupted.
If it is feasible for an attacker to break the swizzling, it would be best to improve
that in some way.

> those results were produced on my x86_64 laptop.
> I got these results by the follows commands, is that what you do?

I normally just build glibc and benchtests and then run each benchmark
a few times like:

time ./build/glibc/benchtests/bench-malloc-thread 1

> Use`tc_idx != chunksize_nomask(mem2chunk(e))` is not correct.
> `tc_idx` is obviously not size, so I need to convert tc_idx to size.

Sorry, I meant csize2tidx (chunksize_nomask(mem2chunk(e)).
It's a bit long but it reuses existing macros and needs fewer instructions.

> Using `chunksize_nomask` will return size with flags(0x91 for example).
> while tc_idx can not provide chunk flags, so I have to use
> `tidx2csize(tc_idx) != chunksize(mem2chunk(e))`.

csize2tidx will shift out those flags, so it doesn't matter.

> Also I notice that `libc_malloc2` is noinline, that might be a waste
> when USE_TCACHE is not on.Maybe alias malloc to __libc_malloc2 when
> USE_TCACHE is not on?

This is not an issue since we don't build GLIBC without tcache. I'm actually
thinking of removing all the USE_TCACHE since it's quite messy...

Cheers,
Wilco
diff mbox series

Patch

diff --git a/malloc/tst-tcache-arbitrary-alloc.c b/malloc/tst-tcache-arbitrary-alloc.c
new file mode 100644
index 0000000000..f8a877c194
--- /dev/null
+++ b/malloc/tst-tcache-arbitrary-alloc.c
@@ -0,0 +1,52 @@ 
+# include <stddef.h>
+# include <stdlib.h>
+# include <stdio.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;
+
+int main()
+{
+    void* ptr[3];
+    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]);
+    ((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] address as example
+
+    ptr[1] = malloc(0x100);
+    ptr[0] = malloc(0x100);
+    // and the malloc them, take the tcachebin out
+    // to see if we have ptr[2] address
+
+    printf("%ld", ptr[0]);
+    // because malloc set the tcacheptr->key = 0
+    // on my machine, canary was set = 0
+    // so there is also a stack-smashing warning
+}