jemalloc: resolve sub-capabilities in je_malloc_underlying_allocation

[MauriceDHanisch]

Fixes #2446

Summary

je_malloc_underlying_allocation() previously relied only on the caller-supplied pointer properties. Sub-capabilities (with modified bounds) could be passed in and were implicitly accepted, violating heap spatial and temporal safety.

This change reconstructs the canonical allocation capability from allocator metadata and compares it against the caller-supplied pointer using cheri_equal_exact. Only if they match is the allocation considered valid. This enforces that the capability corresponds exactly to the one handed out by malloc(3), modulo SW_VMEM stripping.

Rationale

• Aligns with the C standard requirement that free(3) must be passed the exact pointer returned by allocation functions.
• Prevents sub-capabilities from being mistaken as canonical allocations, fixing both spatial and temporal safety.
• Retains practical compatibility: the allocator uses its own metadata (rtree, size class) to construct the authoritative capability, then validates.

Design

• Use rtree + size-class arithmetic to reconstruct the allocation’s canonical capability.
• Normalize permissions by masking out CHERI_PERM_SW_VMEM, which is not retained by the allocator.
• Apply cheri_equal_exact() to check equivalence between reconstructed capability and the user-supplied pointer.
• If they differ, log an error and abort.

Testing

• Ran CheriBSD Kyua suite: kyua test -k /usr/tests/Kyuafile lib/libc/stdlib.
• Results unchanged relative to current tree: all tests pass except the failures posix_memalign_test:aligned_alloc_basic and posix_memalign_basic.

[brooksdavis]

Looks good over all.

I've made a number of comments, many of the nit-picky style variety.

One additional style issue is that jemalloc should be indented with tabs and multi line expressions should be indented an additional 4 spaces.

[MauriceDHanisch]

Thanks for taking a look. I committed minor fixes. Lmk if I need to expand on the comment regarding the length=0 check.

[jrtc27]

Shouldn't all the print+aborts be just return NULL?

[brooksdavis]

Shouldn't all the print+aborts be just return NULL?

Hmm, now that you point it out, that's the intended contract in the original design. I somewhat prefer more verbose diagnostics, but there's something to be said for keeping it simple and letting mrs decide what to do with bad values passed to free. @MauriceDHanisch Sorry for the churn, but let's go with returning NULL and let mrs decide.

[MauriceDHanisch]

Okay, I changed it to returning NULL

[MauriceDHanisch]

@brooksdavis?

[brooksdavis]

Functionally I think this is ready to go, a couple minor things.

[brooksdavis]

I'm not sure what this comment is trying to say (or at least as I'm reading it literally it's wrong). The point of using an exact equality is that it ensures all capability state is as expected. E.g., no additional permission bits are stripped, no flags altered (on RISC-V), and broadly speaking, no state corresponds got new functionality changed has been changed.

[MauriceDHanisch]

This was related to what we discussed with the bound_pointers flag in #2446. If enabled, the output pointer is bounded to the requested size instead of jemalloc's corresponding size class. Thus, the user-provided pointer to free will have a size that we cannot verify.

Currently, to be compatible with that flag, we "trust" the length of the user-provided pointer and set it to be the length of ret_check. This is why I tried to say that it effectively only checks the address and the permissions.

Because we continue with the resolved allocation pointer, this will not cause any vulnerability.

I could either try to explain this better in the comment or remove it altogether with the length setting of ret_check (although this would break if bound_pointers were to be enabled).

[brooksdavis]

Ah right. I think we should revert e904b1d to remove that functionality. I think it's more important that we have a defensive malloc and if we want a malloc with tight bounds we should implemented one with that goal in mind.

[MauriceDHanisch]

So the best would be to remove the length setting of ret_check and revert the bound_pointers functionality if it causes issues down the road.

[MauriceDHanisch]

What do you think @brooksdavis?

[brooksdavis]

Yes, we should drop the length setting revert build_pointers as required.

Sorry for the delay, lots of work and personal travel and too much to do when not traveling :(