Re: [musl] BUG REPORT: Fault in src/malloc/oldmalloc/aligned_alloc.c leads to memory corruption

[Rich Felker <dalias@libc.org>]

On Tue, May 03, 2022 at 10:51:32AM +0000, WILLIAMS Stephen wrote:
> Fault detection / background:
> 
> This fault was detected whilst working with the seL4 microkernel
> which uses an old fork of musl libc (see
> https://github.com/seL4/musllibc/issues/17). Whilst the
> implementation of aligned_alloc has changed between the seL4 fork
> and the mainline musl libc the same underlying fault still appears
> to be present in oldmalloc branch of mainline musl libc.

Do you actually have a test case that demonstrates the corruption? I
tried the one from the linked thread at:

https://lists.sel4.systems/hyperkitty/list/devel@sel4.systems/thread/2K2S7OOZJCWD3Z22XGS36OIXD7HRXKNC

and with current musl built --with-malloc=oldmalloc, I get:

memalign: align = 0x40, size = 0x1000. Returned address = 0x579e6040
memalign: align = 0x40, size = 0x1000. Returned address = 0x579e8000

I suspect your fork of oldmalloc is breaking some invariant memalign
depends on.

It's also plausible that there was a bug in the very old version you
forked from, but commit 3c2cbbe7ba8b4486299ae0d5336ae01ab520d116 seems
like the only relevant change. There was an earlier bug vaguely
related to your report fixed in 2013 with commit
70a92bc968156155dd578f7fb1d4c4e3fceb32f8 but you seem to already have
that fix.

> Fault summary:
> 
> The correctness of the memory allocation bookkeeping system relies
> upon the constraint that the minimum size of a memory 'chunk' is
> SIZE_ALIGN, defined in malloc_impl.h as 4xsizeof(size_t). If this
> constraint is broken then bad things happen and the bookkeeping
> system becomes corrupted, specifically:
> 
> 1. Arithmetic wrap-around of x occurs in the routines bin_index and
> bin_index_up within malloc.c resulting in the maximum chunk index
> being used when the minimum index should have been used. This can
> lead to chunks below the minimum size limit to be considered to be
> large unallocated chunks of memory. Subsequent allocation of these
> unallocated chunks (considered to be large but in reality tiny)
> allows previously allocated chunks to be re-used / overwritten.
> 
> 2.The 'next' and 'prev' pointers held in an unallocated chunk (used
> to maintained a doubly linked list of unallocated chunks) that is
> below the minimum size limit may be overlayed with the bookkeeping
> of the following chunk.
> 
> The malloc routine enforces this minimum chunk size limit (through
> the adjust_size routine), however the code of the aligned_alloc
> routine within aligned_alloc.c can break this minimum size
> constraint and therefore lead to corruption of the bookkeeping.
> 
> The aligned_alloc routine works by malloc'ing sufficient memory to
> ensure the requested amount of memory is available, at the requested
> alignment, somewhere within the malloc'ed region. This means that
> there may be some unused memory allocated before the start of the
> aligned memory area. This can be handled by splitting the chunk
> allocated by malloc into two chunks, a chunk of memory prior to the
> start of the aligned memory followed by a chunk that starts at the
> requested alignment (see aligned_alloc.c lines 43:49). aligned_alloc
> then calls ‘__bin_chunk’ (line 51) on the first chunk which wasn't
> required. So far so good, however aligned_alloc fails to enforce the
> minimum chunk size constraint on either of the two split chunks.
> 
> Proposed fix:
> 
> 1. A minimum size limit on the ‘len’ parameter of aligned_alloc must
> be enforced to ensure that the resulting chunk returned by
> aligned_alloc meets the minimum chunk length limit, i.e. adjust the
> input ‘len’ value to be no less than SIZE_ALIGN.
> 
> 2. aligned_alloc must not call ‘__bin_chunk’ in the case where
> new-men < SIZE_ALIGN. In such a case rather than effectively
> ‘free’ing this small chunk (which is below the minimum length limit
> and therefore leads to corruption of the bookkeeping) the memory
> should be added to the end of the preceding chunk.

The splitting aligned_alloc does results in two chunks. The first one
necessarily has valid size because new-mem is a multiple of SIZE_ALIGN
(basic argument: new is rounded up to a multiple of some power of two
>=SIZE_ALIGN and mem already was already a multiple of SIZE_ALIGN by
virtue of having been returned by malloc).

The second chunk has size equal to whatever malloc(len+align-1)
produced (which is necessarily a multiple of SIZE_ALIGN and no less
than len+align-1), minus some multiple of SIZE_ALIGN (from the size of
the first, above) strictly less than align. This means it's >= len and
a multiple of SIZE_ALIGN. There is some concern to be had about what
happens when len==0, but because we used align-1 instead of
align-SIZE_ALIGN to get the extra space, we over-requested
SIZE_ALIGN-1 bytes already and there is no zero case.

If you think there's a gap in this reasoning, could you point it out
with a test case (doesn't need to run; just argument values and
possibly a made up address and chunk size malloc returns) that we can
work through by hand to see where something goes wrong?

Rich