Re: [musl] [PATCH v2] Add a safe dequeue integrity check for mallocng

[Rich Felker <dalias@libc.org>]

On Sat, Sep 16, 2023 at 04:58:45PM +0200, jvoisin wrote:
> On 16/09/2023 09:08, James Raphael Tiovalen wrote:
> > This commit adds an integrity check to allow for safer dequeuing of the
> > out-of-band meta structs in mallocng. If the unlikely condition is true
> > due to some sort of heap metadata corruption, we abort.
> 
> Since asserts aren't present in production code[1], I don't think that
> this change is useful.

Did you read the previous (v1 patch) thread or the code? In mallocng,
assert expands to a lightweight assertion check.

> > This approach is similar to the safe unlinking check performed by glibc.
> 
> The metadata in musl's heap implementation are stored out-of-bound.
> Should an attacker be able to locate and modify them, it's already game
> over. Adding a `m->prev->next == m && m->next->prev == m` would only
> impede attackers if they only have an arbitrary read and a one-shot
> arbitrary write that can only overwrite one `meta` instead. This seems
> pretty contrived (as in "unlikely) to me. Do you have any particular
> scenario in mind?
> 
> 
> 1. https://git.musl-libc.org/cgit/musl/tree/include/assert.h

Indeed, I'm still mildly unsure of the usefulness of this check, but
not really opposed to it unless it turns out to be costly. (Note: many
of the others already there *are* moderately costly, but are based on
very plausible attack models where the data being checked is easily
reachable via application bugs.) Has anyone checked if this measurably
affects performance?

Rich