Make jemalloc bound returned pointers to their usable size

[markjdb]

This allows MRS to handle realloc() loops more efficiently, since it can avoid going back to the allocator every time the allocation size increases. See #2133. It is also consistent with snmalloc. However, tight bounds might be desirable in some cases, particularly when giving demos. Thus, this PR also adds a new option to MRS to tighten bounds on pointers returned by the backing memory allocator. This option is enabled by default, so default behaviour doesn't change, but this should be reconsidered in the future. In particular, setting _RUNTIME_NOBOUND_CHERI_POINTERS=1 in the environment causes MRS to stop applying bounds, so one gets bounds corresponding to the allocation's usable size.

I also changed jemalloc's malloc_usable_size() implementation to avoid clamping to the capability bounds if the tag bit is clear. This made it possible to compare bounds set by jemalloc with the underlying allocation size, e.g., by comparing cheri_getlen(p) with malloc_usable_size(cheri_cleartag(p)). I used this to write a test program which checks that these values are equal for pointers returned by malloc(), realloc() and posix_memalign() for various allocation sizes and alignments. It's not required otherwise though, so may be dropped.

[bsdjhb]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default. I also think it's a bit odd that in the case where revocation is disabled, MRS is no longer a stub since it narrows the bounds of the underlying allocator. I kind of think the !quarantining cases should probably still pass through to the underlying allocator directly.

[markjdb]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default.

No objection from me! The main problem with MRS not bounding pointers is that it may affect demos and so on in a negatively surprising way.

I also think it's a bit odd that in the case where revocation is disabled, MRS is no longer a stub since it narrows the bounds of the underlying allocator. I kind of think the !quarantining cases should probably still pass through to the underlying allocator directly.

It is odd - MRS is not really the right place to implement this, it's just more convenient than doing this directly in jemalloc and snmalloc. I made this option orthogonal to the quarantining condition since it's logically unrelated. For testing purposes it is convenient to be able to enable or disable bounding independent of whether revocation is enabled (I made use of this when testing the change); to me this usefulness kind of outweighs the layering violation. I could make it conditional on quarantining though if we really think that that's important.

[bsdjhb]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default.

No objection from me! The main problem with MRS not bounding pointers is that it may affect demos and so on in a negatively surprising way.

Yes, I guess my thought here is that if we had stuck with snmalloc instead of switching back to jemalloc then those demos would already have the same issues? Perhaps though this is a way of "fixing" snmalloc as another way of thinking about it.

I also think it's a bit odd that in the case where revocation is disabled, MRS is no longer a stub since it narrows the bounds of the underlying allocator. I kind of think the !quarantining cases should probably still pass through to the underlying allocator directly.

It is odd - MRS is not really the right place to implement this, it's just more convenient than doing this directly in jemalloc and snmalloc. I made this option orthogonal to the quarantining condition since it's logically unrelated. For testing purposes it is convenient to be able to enable or disable bounding independent of whether revocation is enabled (I made use of this when testing the change); to me this usefulness kind of outweighs the layering violation. I could make it conditional on quarantining though if we really think that that's important.

Yeah, it's really just the layering violation I'm not a fan of I guess, but I see why it is a convenient place to enforce this.

[markjdb]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default.

No objection from me! The main problem with MRS not bounding pointers is that it may affect demos and so on in a negatively surprising way.

Yes, I guess my thought here is that if we had stuck with snmalloc instead of switching back to jemalloc then those demos would already have the same issues? Perhaps though this is a way of "fixing" snmalloc as another way of thinking about it.

I wasn't aware of that history. I'd tend to think that we prefer a more performance-friendly setting out of the box, so MRS shouldn't be bounding pointers by default. But, maybe there is some kind of demo checklist that I should update as a part of that change?

[brooksdavis]

I think these should be a macro or inline function to reduce explicit instances of cheri_setbounds. Philosophically that is because each setbounds is a point potentially requiring audit when you bring in foreign code.

[markjdb]

Done in the latest push. I also added a comment above the new function to explain the intent behind the flag.

[brooksdavis]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default.

No objection from me! The main problem with MRS not bounding pointers is that it may affect demos and so on in a negatively surprising way.

Yes, I guess my thought here is that if we had stuck with snmalloc instead of switching back to jemalloc then those demos would already have the same issues? Perhaps though this is a way of "fixing" snmalloc as another way of thinking about it.

I wasn't aware of that history. I'd tend to think that we prefer a more performance-friendly setting out of the box, so MRS shouldn't be bounding pointers by default. But, maybe there is some kind of demo checklist that I should update as a part of that change?

Technically what you've got MRS doing is UB since you must past exactly the same pointer to free that malloc returned (I've literally implemented that requirement in a cherified dlmalloc where there was space in the out of bounds metadata to stash a copy of the returned pointer and it made that check extremely cheap (just __builtin_cheri_equal_exact against the stashed copy)).

[markjdb]

Given that snmalloc already did what jemalloc now does, I actually think it's more useful for MRS to not do its own bounds by default.

No objection from me! The main problem with MRS not bounding pointers is that it may affect demos and so on in a negatively surprising way.

Yes, I guess my thought here is that if we had stuck with snmalloc instead of switching back to jemalloc then those demos would already have the same issues? Perhaps though this is a way of "fixing" snmalloc as another way of thinking about it.

I wasn't aware of that history. I'd tend to think that we prefer a more performance-friendly setting out of the box, so MRS shouldn't be bounding pointers by default. But, maybe there is some kind of demo checklist that I should update as a part of that change?

Technically what you've got MRS doing is UB since you must past exactly the same pointer to free that malloc returned (I've literally implemented that requirement in a cherified dlmalloc where there was space in the out of bounds metadata to stash a copy of the returned pointer and it made that check extremely cheap (just __builtin_cheri_equal_exact against the stashed copy)).

That seems like another argument for making MRS not bound pointers by default. Do you think this particular UB should prevent us from making it an option at all?

[brooksdavis]

That seems like another argument for making MRS not bound pointers by default. Do you think this particular UB should prevent us from making it an option at all?

I think current allocators are OK with this in practice so it's ok as an option for now.

[markjdb]

That seems like another argument for making MRS not bound pointers by default. Do you think this particular UB should prevent us from making it an option at all?

I think current allocators are OK with this in practice so it's ok as an option for now.

Ok. I did however switch the default, so now MRS does not touch the bounds unless you ask it to.

[bsdjhb]

Given Brooks comment about UB, it seems like the cleanest approach would be for each backing allocator (jemalloc, snmalloc) to have a flag to determine if bounds are tight or not rather than doing this in mrs. This would avoid the UB of malloc vs free mismatches and keep mrs as a revocation-only wrapper. We could use a consistent name across both allocators for this setting at least.

[brooksdavis]

I think we need to make sure that size is at least 1.

It's possible for an allocator to actually support zero sized allocations, but it needs a special pool so they can't be confused with an adjacent allocation manipulated to point to one past the end with bounds of 0. No current allocator does this (maybe out dlmalloc port does by accident).

[markjdb]

I modified the routine to only bound the pointer if size > 0.

[bsdjhb]

I still think that the bounding option belongs in the backing allocator instead of mrs, but I do think the jemalloc fixes are good and would be fine with moving the bounding option down into the backing allocators later.

[markjdb]

This would avoid the UB of malloc vs free mismatches and keep mrs as a revocation-only wrapper

I don't really agree with the "revocation-only" remark, because the problem which motivates this change is entirely caused by MRS and revocation in general. That's the main reason I've been a bit reluctant to handle this within individual allocators.