Bind free list shadow pointers to the slot holding them

[jvoisin]

The shadow of a free list pointer is currently BSWAP(next) ^ shadow_key, which does not depend on where it is stored, meaning that:

• zend_mm_free_small() encodes whatever heap->free_slot[bin] happens to be, including NULL when the bin has been drained, meaning the slot ends up holding shadow_key verbatim.
• A (next, shadow) pair harvested from one free slot is valid in every other slot of every bin. An attacker who can read one free slot can therefore forge a link anywhere in the heap without needing the key.

This commit adds the address of the slot that holds the shadow into the mix:

shadow = BSWAP(next) ^ shadow_key ^ (uintptr_t)holder

The holder term cancels on decode, so this is one extra xor on a register that is already live, with no branch. Encoding NULL now yields shadow_key ^ holder rather than the key, and a shadow only verifies in the slot it was written for.

This was verified under GDB: Freeing into a drained bin used to store shadow_key exactly; it now stores shadow_key ^ holder (xoring the two back gives the slot address). Naïvely replaying a valid (next, shadow) pair from one slot into another and traversing from it is accepted before this change and aborts with "zend_mm_heap corrupted" after.

Performance-wise, the impact is in the noise level, which is expected as it more or less adds a single xor instruction per zend_mm_set_next_free_slot().

This commit is a follow up on 25360ef and c561f7d.

[jvoisin]

Part of #14083

[arnaud-lb]

This looks good to me, but can I suggest that we use the address of the shadow as holder? This reduces the number of alive variables in zend_mm_get_next_free_slot() as slot is not needed after fetching next:

diff --git a/Zend/zend_alloc.c b/Zend/zend_alloc.c
index 5824bd97136..ff7d828f0bc 100644
--- a/Zend/zend_alloc.c
+++ b/Zend/zend_alloc.c
@@ -1284,10 +1284,10 @@ static zend_always_inline int zend_mm_small_size_to_bin(size_t size)
  * prevents a valid shadow from being naïvely replayed into another slot.
  */
 
-#define ZEND_MM_FREE_SLOT_PTR_SHADOW(free_slot, bin_num) \
-	*((zend_mm_free_slot**)((char*)(free_slot) + bin_data_size[(bin_num)] - sizeof(zend_mm_free_slot*)))
+#define ZEND_MM_FREE_SLOT_PTR_SHADOW_ADDR(free_slot, bin_num) \
+	((zend_mm_free_slot**)((char*)(free_slot) + bin_data_size[(bin_num)] - sizeof(zend_mm_free_slot*)))
 
-static zend_always_inline zend_mm_free_slot* zend_mm_encode_free_slot(const zend_mm_heap *heap, const zend_mm_free_slot *holder, const zend_mm_free_slot *next)
+static zend_always_inline zend_mm_free_slot* zend_mm_encode_free_slot(const zend_mm_heap *heap, void *holder, const zend_mm_free_slot *next)
 {
 #ifdef WORDS_BIGENDIAN
 	return (zend_mm_free_slot*)((uintptr_t)next ^ heap->shadow_key ^ (uintptr_t)holder);
@@ -1296,7 +1296,7 @@ static zend_always_inline zend_mm_free_slot* zend_mm_encode_free_slot(const zend
 #endif
 }
 
-static zend_always_inline zend_mm_free_slot* zend_mm_decode_free_slot_key(uintptr_t shadow_key, const zend_mm_free_slot *holder, zend_mm_free_slot *shadow)
+static zend_always_inline zend_mm_free_slot* zend_mm_decode_free_slot_key(uintptr_t shadow_key, void *holder, zend_mm_free_slot *shadow)
 {
 #ifdef WORDS_BIGENDIAN
 	return (zend_mm_free_slot*)((uintptr_t)shadow ^ shadow_key ^ (uintptr_t)holder);
@@ -1305,7 +1305,7 @@ static zend_always_inline zend_mm_free_slot* zend_mm_decode_free_slot_key(uintpt
 #endif
 }
 
-static zend_always_inline zend_mm_free_slot* zend_mm_decode_free_slot(zend_mm_heap *heap, const zend_mm_free_slot *holder, zend_mm_free_slot *shadow)
+static zend_always_inline zend_mm_free_slot* zend_mm_decode_free_slot(zend_mm_heap *heap, void *holder, zend_mm_free_slot *shadow)
 {
 	return zend_mm_decode_free_slot_key(heap->shadow_key, holder, shadow);
 }
@@ -1315,15 +1315,16 @@ static zend_always_inline void zend_mm_set_next_free_slot(zend_mm_heap *heap, ui
 	ZEND_ASSERT(bin_data_size[bin_num] >= ZEND_MM_MIN_USEABLE_BIN_SIZE);
 
 	slot->next_free_slot = next;
-	ZEND_MM_FREE_SLOT_PTR_SHADOW(slot, bin_num) = zend_mm_encode_free_slot(heap, slot, next);
+	zend_mm_free_slot **shadow_addr = ZEND_MM_FREE_SLOT_PTR_SHADOW_ADDR(slot, bin_num);
+	*shadow_addr = zend_mm_encode_free_slot(heap, shadow_addr, next);
 }
 
 static zend_always_inline zend_mm_free_slot *zend_mm_get_next_free_slot(zend_mm_heap *heap, uint32_t bin_num, zend_mm_free_slot* slot)
 {
 	zend_mm_free_slot *next = slot->next_free_slot;
 	if (EXPECTED(next != NULL)) {
-		zend_mm_free_slot *shadow = ZEND_MM_FREE_SLOT_PTR_SHADOW(slot, bin_num);
-		if (UNEXPECTED(next != zend_mm_decode_free_slot(heap, slot, shadow))) {
+		zend_mm_free_slot **shadow_addr = ZEND_MM_FREE_SLOT_PTR_SHADOW_ADDR(slot, bin_num);
+		if (UNEXPECTED(next != zend_mm_decode_free_slot(heap, shadow_addr, *shadow_addr))) {
 			zend_mm_panic("zend_mm_heap corrupted");
 		}
 	}
@@ -2031,8 +2032,8 @@ ZEND_API void zend_mm_refresh_key_child(zend_mm_heap *heap)
 		}
 		zend_mm_free_slot *next;
 		while ((next = slot->next_free_slot)) {
-			zend_mm_free_slot *shadow = ZEND_MM_FREE_SLOT_PTR_SHADOW(slot, i);
-			if (UNEXPECTED(next != zend_mm_decode_free_slot_key(old_key, slot, shadow))) {
+			zend_mm_free_slot **shadow_addr = ZEND_MM_FREE_SLOT_PTR_SHADOW_ADDR(slot, i);
+			if (UNEXPECTED(next != zend_mm_decode_free_slot_key(old_key, shadow_addr, *shadow_addr))) {
 				zend_mm_panic("zend_mm_heap corrupted");
 			}
 			zend_mm_set_next_free_slot(heap, i, slot, next);

The overhead was already very small (<0.20%), but this patch eliminates it entirely in my benchmark.

Edit: simplified patch

[jvoisin]

Smart, thanks

[arnaud-lb]

Thank you!