[PATCH] malloc tcache: Debugger now sees the address of the corrupted chunk.

[Adder]

On Sat, Dec 12, 2020 at 5:17 AM DJ Delorie <dj@redhat.com> wrote:

> Hmm... OK, I think I get it. It's not the 'e' we know, its the 'e' from
> the previous call to tcache_get().

Yes ! I am so happy !


> So basically, when we remove a chunk from the tcache, we want to
> validate the pointer we're leaving behind?

Correct ! And the result is crash on the first malloc, not the second one.

This has the additional benefit of capturing (in the core file)
the illegally-overwritten-via-p1 contents of the chunk,
without further (legal) write results via p7 (which is equal to p1),
which may occur before the second malloc (which currently crashes).


>  static __always_inline void *
>  tcache_get (size_t tc_idx)
>  {
>    tcache_entry *e = tcache->entries[tc_idx];
>    if (__glibc_unlikely (!aligned_OK (e)))
>      malloc_printerr ("malloc(): unaligned tcache chunk detected");
>    tcache->entries[tc_idx] = REVEAL_PTR (e->next);
> +  /* Validate the pointer we're leaving behind, while we still know
> +     where it came from, in case a use-after-free corrupted it.  */
> +  if (tcache->entries[tc_idx])
> +    * (volatile char **) tcache->entries[tc_idx];
>    --(tcache->counts[tc_idx]);
>    e->key = NULL;
>    return (void *) e;
>  }

This looks cleaner and more efficient than my attempt.
Thank you for the "volatile" tip !

But I am getting:

  malloc.c:2961:5: error: statement with no effect [-Werror=unused-value]
       * (volatile char **) tcache->entries[tc_idx];

Even if I #pragma out the warning, no machine code seems to be generated
(at least for x86_64).

I would like to suggest:

static __always_inline void *
tcache_get (size_t tc_idx)
{
  tcache_entry *e = tcache->entries[tc_idx];
  if (__glibc_unlikely (!aligned_OK (e)))
    malloc_printerr ("malloc(): unaligned tcache chunk detected");

-    tcache->entries[tc_idx] = REVEAL_PTR (e->next);
+  tcache_entry *const e_next = REVEAL_PTR (e->next);
+
+  /* Validate the pointer we're leaving behind, while we still know
+     where it came from, in case a write-after-free has corrupted it.
+     The write operation (via pointer-to-volatile) serves two purposes:
+     it prevents the code from being optimized out
+     and it validates the pointer for writing (not just for reading). */
+  if (e_next)
+    ((volatile tcache_entry *) e_next)->next = e_next->next;

  tcache->entries[tc_idx] = e_next;
  --(tcache->counts[tc_idx]);
  e->key = NULL;
  return (void *) e;
}



It crashes as desired, and we can see the value of e in the core file.

The extra generated machine code is, again,
much simpler than my original version:

  cmp r9, rdi
  je @@new_label_1
  mov rdi, [rax]
  mov [rax], rdi
 @@ new_label_1:
  ...