diff --git a/Android.bp b/Android.bp index 3926aac..a6c7362 100644 --- a/Android.bp +++ b/Android.bp @@ -14,8 +14,7 @@ common_cflags = [ "-DWRITE_AFTER_FREE_CHECK=true", "-DSLOT_RANDOMIZE=true", "-DSLAB_CANARY=true", - "-DSLAB_QUARANTINE_RANDOM_LENGTH=1", - "-DSLAB_QUARANTINE_QUEUE_LENGTH=1", + "-DSLAB_QUARANTINE_LENGTH=2", "-DCONFIG_EXTENDED_SIZE_CLASSES=true", "-DCONFIG_LARGE_SIZE_CLASSES=true", "-DGUARD_SLABS_INTERVAL=1", diff --git a/Makefile b/Makefile index de8d82c..c3b7670 100644 --- a/Makefile +++ b/Makefile @@ -99,8 +99,7 @@ CPPFLAGS += \ -DWRITE_AFTER_FREE_CHECK=$(CONFIG_WRITE_AFTER_FREE_CHECK) \ -DSLOT_RANDOMIZE=$(CONFIG_SLOT_RANDOMIZE) \ -DSLAB_CANARY=$(CONFIG_SLAB_CANARY) \ - -DSLAB_QUARANTINE_RANDOM_LENGTH=$(CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH) \ - -DSLAB_QUARANTINE_QUEUE_LENGTH=$(CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH) \ + -DSLAB_QUARANTINE_LENGTH=$(CONFIG_SLAB_QUARANTINE_LENGTH) \ -DCONFIG_EXTENDED_SIZE_CLASSES=$(CONFIG_EXTENDED_SIZE_CLASSES) \ -DCONFIG_LARGE_SIZE_CLASSES=$(CONFIG_LARGE_SIZE_CLASSES) \ -DGUARD_SLABS_INTERVAL=$(CONFIG_GUARD_SLABS_INTERVAL) \ diff --git a/README.md b/README.md index 089dd1a..93895e2 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ address space. There are lots of smaller differences in the implementation approach. It incorporates the previous extensions made to OpenBSD malloc including adding padding to allocations for canaries (distinct from the current OpenBSD malloc canaries), write-after-free detection tied to the existing -clearing on free, queues alongside the existing randomized arrays for +clearing on free, unified randomized quarantine arrays for quarantining allocations and proper double-free detection for quarantined allocations. The per-size-class memory regions with their own random bases were loosely inspired by the size and type-based partitioning in PartitionAlloc. The @@ -282,20 +282,13 @@ The following boolean configuration options are available: The following integer configuration options are available: -* `CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH`: `1` (default) to control the number - of slots in the random array used to randomize reuse for small memory +* `CONFIG_SLAB_QUARANTINE_LENGTH`: `2` (default) to control the number of + slots in the quarantine used to randomize and delay reuse for small memory allocations. This sets the length for the largest size class (either 16kiB or 128kiB based on `CONFIG_EXTENDED_SIZE_CLASSES`) and the quarantine length for smaller size classes is scaled to match the total memory of the - quarantined allocations (1 becomes 1024 for 16 byte allocations with 16kiB - as the largest size class, or 8192 with 128kiB as the largest). -* `CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH`: `1` (default) to control the number of - slots in the queue used to delay reuse for small memory allocations. This - sets the length for the largest size class (either 16kiB or 128kiB based on - `CONFIG_EXTENDED_SIZE_CLASSES`) and the quarantine length for smaller size - classes is scaled to match the total memory of the quarantined allocations (1 - becomes 1024 for 16 byte allocations with 16kiB as the largest size class, or - 8192 with 128kiB as the largest). + quarantined allocations (2 becomes 2048 for 16 byte allocations with 16kiB + as the largest size class, or 16384 with 128kiB as the largest). * `CONFIG_GUARD_SLABS_INTERVAL`: `1` (default) to control the number of slabs before a slab is skipped and left as an unused memory protected guard slab. The default of `1` leaves a guard slab between every slab. This feature does @@ -445,7 +438,7 @@ was a bit less important and if a core goal was finding latent bugs. * Slab allocations are zeroed on free * Detection of write-after-free for slab allocations by verifying zero filling is intact at allocation time -* Delayed free via a combination of FIFO and randomization for slab allocations +* Delayed free via randomized quarantine arrays for slab allocations * Large allocations are purged and memory protected on free with the memory mapping kept reserved in a quarantine to detect use-after-free * The quarantine is primarily based on a FIFO ring buffer, with the oldest @@ -741,8 +734,8 @@ This ensures the following properties: - Linear overflows are deterministically detected. - Use-after-free are deterministically detected until the freed slot goes through - both the random and FIFO quarantines, gets allocated again, goes through both - quarantines again and then finally gets allocated again for a 2nd time. + the quarantine, gets allocated again, goes through the quarantine again and + then finally gets allocated again for a 2nd time. - Since the default `0` tag is reserved, untagged pointers can't access slab allocations and vice versa. diff --git a/config/default.mk b/config/default.mk index d8f03a7..c6c9f89 100644 --- a/config/default.mk +++ b/config/default.mk @@ -7,8 +7,7 @@ CONFIG_ZERO_ON_FREE := true CONFIG_WRITE_AFTER_FREE_CHECK := true CONFIG_SLOT_RANDOMIZE := true CONFIG_SLAB_CANARY := true -CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH := 1 -CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH := 1 +CONFIG_SLAB_QUARANTINE_LENGTH := 2 CONFIG_EXTENDED_SIZE_CLASSES := true CONFIG_LARGE_SIZE_CLASSES := true CONFIG_GUARD_SLABS_INTERVAL := 1 diff --git a/config/light.mk b/config/light.mk index da8676d..5849a39 100644 --- a/config/light.mk +++ b/config/light.mk @@ -7,8 +7,7 @@ CONFIG_ZERO_ON_FREE := true CONFIG_WRITE_AFTER_FREE_CHECK := false CONFIG_SLOT_RANDOMIZE := false CONFIG_SLAB_CANARY := true -CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH := 0 -CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH := 0 +CONFIG_SLAB_QUARANTINE_LENGTH := 0 CONFIG_EXTENDED_SIZE_CLASSES := true CONFIG_LARGE_SIZE_CLASSES := true CONFIG_GUARD_SLABS_INTERVAL := 8 diff --git a/h_malloc.c b/h_malloc.c index c6c58e0..b0912f7 100644 --- a/h_malloc.c +++ b/h_malloc.c @@ -24,7 +24,7 @@ #include #endif -#define SLAB_QUARANTINE (SLAB_QUARANTINE_RANDOM_LENGTH > 0 || SLAB_QUARANTINE_QUEUE_LENGTH > 0) +#define SLAB_QUARANTINE (SLAB_QUARANTINE_LENGTH > 0) #define REGION_QUARANTINE (REGION_QUARANTINE_RANDOM_LENGTH > 0 || REGION_QUARANTINE_QUEUE_LENGTH > 0) #define MREMAP_MOVE_THRESHOLD ((size_t)32 * 1024 * 1024) @@ -32,10 +32,8 @@ static_assert(sizeof(void *) == 8, "64-bit only"); static_assert(!WRITE_AFTER_FREE_CHECK || ZERO_ON_FREE, "WRITE_AFTER_FREE_CHECK depends on ZERO_ON_FREE"); -static_assert(SLAB_QUARANTINE_RANDOM_LENGTH >= 0 && SLAB_QUARANTINE_RANDOM_LENGTH <= 65536, - "invalid slab quarantine random length"); -static_assert(SLAB_QUARANTINE_QUEUE_LENGTH >= 0 && SLAB_QUARANTINE_QUEUE_LENGTH <= 65536, - "invalid slab quarantine queue length"); +static_assert(SLAB_QUARANTINE_LENGTH >= 0 && SLAB_QUARANTINE_LENGTH <= 65536, + "invalid slab quarantine length"); static_assert(REGION_QUARANTINE_RANDOM_LENGTH >= 0 && REGION_QUARANTINE_RANDOM_LENGTH <= 65536, "invalid region quarantine random length"); static_assert(REGION_QUARANTINE_QUEUE_LENGTH >= 0 && REGION_QUARANTINE_QUEUE_LENGTH <= 65536, @@ -309,13 +307,8 @@ struct __attribute__((aligned(CACHELINE_SIZE))) size_class { size_t metadata_count; size_t metadata_count_unguarded; -#if SLAB_QUARANTINE_QUEUE_LENGTH > 0 - size_t quarantine_queue_index; - void *quarantine_queue[SLAB_QUARANTINE_QUEUE_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)]; -#endif - -#if SLAB_QUARANTINE_RANDOM_LENGTH > 0 - void *quarantine_random[SLAB_QUARANTINE_RANDOM_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)]; +#if SLAB_QUARANTINE_LENGTH > 0 + void *quarantine[SLAB_QUARANTINE_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)]; #endif }; @@ -856,12 +849,12 @@ static inline void deallocate_small(void *p, const size_t *expected_size) { size_t quarantine_shift = clz64(size) - (63 - MAX_SLAB_SIZE_CLASS_SHIFT); -#if SLAB_QUARANTINE_RANDOM_LENGTH > 0 - size_t slab_quarantine_random_length = SLAB_QUARANTINE_RANDOM_LENGTH << quarantine_shift; +#if SLAB_QUARANTINE_LENGTH > 0 + size_t slab_quarantine_length = SLAB_QUARANTINE_LENGTH << quarantine_shift; - size_t random_index = get_random_u16_uniform(&c->rng, slab_quarantine_random_length); - void *random_substitute = c->quarantine_random[random_index]; - c->quarantine_random[random_index] = p; + size_t random_index = get_random_u16_uniform(&c->rng, slab_quarantine_length); + void *random_substitute = c->quarantine[random_index]; + c->quarantine[random_index] = p; if (random_substitute == NULL) { mutex_unlock(&c->lock); @@ -871,24 +864,6 @@ static inline void deallocate_small(void *p, const size_t *expected_size) { p = random_substitute; #endif -#if SLAB_QUARANTINE_QUEUE_LENGTH > 0 - size_t slab_quarantine_queue_length = SLAB_QUARANTINE_QUEUE_LENGTH << quarantine_shift; - - void *queue_substitute = c->quarantine_queue[c->quarantine_queue_index]; - c->quarantine_queue[c->quarantine_queue_index] = p; - - // Modulo here is costly so we're using an increment and an if instead. - size_t next_queue_index = c->quarantine_queue_index + 1; - c->quarantine_queue_index = next_queue_index < slab_quarantine_queue_length ? next_queue_index : 0; - - if (queue_substitute == NULL) { - mutex_unlock(&c->lock); - return; - } - - p = queue_substitute; -#endif - metadata = get_metadata(c, p); slab = get_slab(c, slab_size, metadata); slot = libdivide_u32_do((char *)p - (char *)slab, &c->size_divisor); @@ -2002,20 +1977,10 @@ EXPORT int h_malloc_trim(UNUSED size_t pad) { if (size >= min_extended_size_class) { size_t quarantine_shift = clz64(size) - (63 - MAX_SLAB_SIZE_CLASS_SHIFT); -#if SLAB_QUARANTINE_RANDOM_LENGTH > 0 - size_t slab_quarantine_random_length = SLAB_QUARANTINE_RANDOM_LENGTH << quarantine_shift; - for (size_t i = 0; i < slab_quarantine_random_length; i++) { - void *p = c->quarantine_random[i]; - if (p != NULL) { - memory_purge(p, size); - } - } -#endif - -#if SLAB_QUARANTINE_QUEUE_LENGTH > 0 - size_t slab_quarantine_queue_length = SLAB_QUARANTINE_QUEUE_LENGTH << quarantine_shift; - for (size_t i = 0; i < slab_quarantine_queue_length; i++) { - void *p = c->quarantine_queue[i]; +#if SLAB_QUARANTINE_LENGTH > 0 + size_t slab_quarantine_length = SLAB_QUARANTINE_LENGTH << quarantine_shift; + for (size_t i = 0; i < slab_quarantine_length; i++) { + void *p = c->quarantine[i]; if (p != NULL) { memory_purge(p, size); } diff --git a/test/Makefile b/test/Makefile index 41b48a4..e9274b6 100644 --- a/test/Makefile +++ b/test/Makefile @@ -75,7 +75,12 @@ EXECUTABLES := \ impossibly_large_malloc \ realloc_init \ malloc_zero_different \ - malloc_noreuse + malloc_noreuse \ + quarantine_double_free_extended \ + quarantine_double_free_extended_delayed \ + quarantine_invalid_malloc_usable_size_extended \ + quarantine_invalid_malloc_object_size_extended \ + quarantine_write_after_free_extended_reuse all: $(EXECUTABLES) diff --git a/test/quarantine_double_free_extended.c b/test/quarantine_double_free_extended.c new file mode 100644 index 0000000..15b81f9 --- /dev/null +++ b/test/quarantine_double_free_extended.c @@ -0,0 +1,13 @@ +#include + +#include "test_util.h" + +OPTNONE int main(void) { + void *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + free(p); + return 0; +} diff --git a/test/quarantine_double_free_extended_delayed.c b/test/quarantine_double_free_extended_delayed.c new file mode 100644 index 0000000..0330e5b --- /dev/null +++ b/test/quarantine_double_free_extended_delayed.c @@ -0,0 +1,23 @@ +#include + +#include "test_util.h" + +OPTNONE int main(void) { + void *p = malloc(65536); + if (!p) { + return 1; + } + void *allocs[100]; + for (int i = 0; i < 100; i++) { + allocs[i] = malloc(65536); + if (!allocs[i]) { + return 1; + } + } + free(p); + for (int i = 0; i < 100; i++) { + free(allocs[i]); + } + free(p); + return 0; +} diff --git a/test/quarantine_invalid_malloc_object_size_extended.c b/test/quarantine_invalid_malloc_object_size_extended.c new file mode 100644 index 0000000..7a4a03d --- /dev/null +++ b/test/quarantine_invalid_malloc_object_size_extended.c @@ -0,0 +1,14 @@ +#include + +#include "test_util.h" +#include "../include/h_malloc.h" + +OPTNONE int main(void) { + void *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + h_malloc_object_size(p); + return 0; +} diff --git a/test/quarantine_invalid_malloc_usable_size_extended.c b/test/quarantine_invalid_malloc_usable_size_extended.c new file mode 100644 index 0000000..a755d16 --- /dev/null +++ b/test/quarantine_invalid_malloc_usable_size_extended.c @@ -0,0 +1,14 @@ +#include +#include + +#include "test_util.h" + +OPTNONE int main(void) { + void *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + malloc_usable_size(p); + return 0; +} diff --git a/test/quarantine_write_after_free_extended_reuse.c b/test/quarantine_write_after_free_extended_reuse.c new file mode 100644 index 0000000..c6fe661 --- /dev/null +++ b/test/quarantine_write_after_free_extended_reuse.c @@ -0,0 +1,17 @@ +#include + +#include "test_util.h" +#include "../util.h" + +OPTNONE int main(void) { + char *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + p[100] = 'a'; + for (size_t i = 0; i < 10000; i++) { + free(malloc(32768)); + } + return 0; +} diff --git a/test/test_smc.py b/test/test_smc.py index 0d5665b..d00ac42 100644 --- a/test/test_smc.py +++ b/test/test_smc.py @@ -287,5 +287,43 @@ def test_malloc_noreuse(self): "malloc_noreuse") self.assertEqual(returncode, 0) + def test_quarantine_double_free_extended(self): + _stdout, stderr, returncode = self.run_test( + "quarantine_double_free_extended") + self.assertEqual(returncode, -6) + self.assertEqual(stderr.decode("utf-8"), + "fatal allocator error: double free (quarantine)\n") + + def test_quarantine_double_free_extended_delayed(self): + _stdout, stderr, returncode = self.run_test( + "quarantine_double_free_extended_delayed") + self.assertEqual(returncode, -6) + # with quarantine length 4 for 65536 bytes (default L=2, shift=1), freeing 100 allocations + # almost certainly displaces the first pointer from quarantine + self.assertIn(stderr.decode("utf-8"), + ["fatal allocator error: double free\n", + "fatal allocator error: double free (quarantine)\n"]) + + def test_quarantine_invalid_malloc_usable_size_extended(self): + _stdout, stderr, returncode = self.run_test( + "quarantine_invalid_malloc_usable_size_extended") + self.assertEqual(returncode, -6) + self.assertEqual(stderr.decode("utf-8"), + "fatal allocator error: invalid malloc_usable_size (quarantine)\n") + + def test_quarantine_invalid_malloc_object_size_extended(self): + _stdout, stderr, returncode = self.run_test( + "quarantine_invalid_malloc_object_size_extended") + self.assertEqual(returncode, -6) + self.assertEqual(stderr.decode("utf-8"), + "fatal allocator error: invalid malloc_object_size (quarantine)\n") + + def test_quarantine_write_after_free_extended_reuse(self): + _stdout, stderr, returncode = self.run_test( + "quarantine_write_after_free_extended_reuse") + self.assertEqual(returncode, -6) + self.assertEqual(stderr.decode("utf-8"), + "fatal allocator error: detected write after free\n") + if __name__ == '__main__': unittest.main()