Re: [musl] [PATCH] mallocng: Add MTE support for Aarch64

[Rich Felker <dalias@libc.org>]

On Tue, Jun 11, 2024 at 04:09:22PM +0200, Szabolcs Nagy wrote:
> * Stefan Jumarea <stefanjumarea02@gmail.com> [2024-06-10 15:36:25 +0300]:
> > @@ -102,17 +107,30 @@ void free(void *p)
> >  {
> >  	if (!p) return;
> >  
> > +#ifdef MEMTAG
> > +	void *untagged = (void *)((uint64_t)p & ~MTE_TAG_MASK);
> > +#else
> > +	void *untagged = p;
> > +#endif
> > +
> >  	struct meta *g = get_meta(p);
> ....
> >  static inline struct meta *get_meta(const unsigned char *p)
> >  {
> >  	assert(!((uintptr_t)p & 15));
> > -	int offset = *(const uint16_t *)(p - 2);
> > -	int index = get_slot_index(p);
> > -	if (p[-4]) {
> > +#ifdef MEMTAG
> > +	const unsigned char *untagged = (const unsigned char *)((uint64_t)p & ~MTE_TAG_MASK);
> > +#else
> > +	const unsigned char *untagged = p;
> > +#endif
> 
> if free just drops the tag, then incorrectly tagged pointer
> will not be detected. musl does some use-after-free checks,
> so i dont know how important this is, but i'd check that the
> passed pointer matches the memory tag (unless 0 sized and
> that the tag is non-0) otherwise a forged pointer may cause
> corruption.

Yes. Would just accessing a byte at the start fix this?

> i don't see where you enable tagged pointer abi and checks
> (prctl) or where you add PROT_MTE to the mmapped memory.
> https://www.kernel.org/doc/html/latest/arch/arm64/memory-tagging-extension.html
> https://www.kernel.org/doc/html/latest/arch/arm64/tagged-address-abi.html
> (i think we want the synchronous tag check option.)
> note there is software that assumes page granularity for
> memory protection e.g. does read access at p&-4096, and
> there may software that assumes the top bits of a valid
> pointer is 0 so unconditionally enabling tagging and tag
> checks can be an issue.

I don't think that's a problem. malloc has no contract to allow those
things.

> another potential issue is the early ALIGN_UP of the malloc
> size: this overflows malloc(-1) and i think changes
> malloc_usable_size(malloc(1)).

Yes, I saw that was incorrect and don't understand the motivation. If
it's to avoid tag mismatch at the final 12b, that could be done where
the size class is selected instead. But it would be better if we could
make it work with the tag mismatch (not sure how hard that is) so this
space is still usable (ignoring tag when accessing the header).

> iirc i changed IB when i tried out mte with mallocng.
> 
> i would avoid excessive ifdefs in the code, e.g. by using
> 'p = untag(p);' and define untag appropriately in a header.
> (this might as well do the tag checks when mte is enabled,

Yes.

> but might need special-casing 0 sized allocations.)

Zero-sized allocations could maybe be implemented as a wrong tag? But
then we'd need a way for free to let them pass untrapped.

> tagging large areas can be slow, just like 'dc zva,x0' in
> aarch64 memset, there is 'dc gva,x0' that tags an entire
> cacheline, alternatively it may be better to just rely on
> page protection for large allocations to avoid tagging
> overheads (don't even mmap them as PROT_MTE). i've never
> actually benchmarked what makes sense in practice, i'd at
> least put the hand written loop behind an abstraction.

That loses a lot of the benefit of MTE. Page protections don't work
because there would be too many VMAs if we put guard pages after each
mmap-serviced allocation. However, one thing you can do to protect
against overflow in mmap-serviced allocations without tagging the
whole thing with O(n) operation is just tagging the tail up to end of
page. This would make accesses past the end trap.

Rich