***This mail has been sent from an external source***

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

Interesting. From the logging I’m seeing (admittedly with an old fork in use with seL4) ‘mem’ is not guaranteed to be a multiple of SIZE_ALIGN as you are suggesting above.

The following was generated with logging inside of the __memalign routine to show the values of ’new’ and ‘mem’:

Stpehen