c18n: Fix rtld_bind reentry bug #2134
Conversation
dpgao
changed the title
| * stack be used because the signal handler uses the recorded size of | ||
| * the stack to determine whether it has been allocated. | ||
| */ | ||
| static char dummy_stk; |
There was a problem hiding this comment.
This is ugly, surely there's a better way. Where does this end up being used to determine that the compartment is RTLD, anyway? Isn't the trusted stack tracking the compartment ID?
There was a problem hiding this comment.
Isn't the trusted stack tracking the compartment ID?
This is generally true except at certain points in the trampoline where the trusted stack becomes temporarily out-of-sync with the currently installed untrusted stack.
The signal handler needs to handle such situations and makes a few guesses about the actual compartment that the current untrusted stack belongs to. It does so by checking whether the current untrusted stack is a subset of one of the candidate compartments' stack. Hence the dummy stack must be tagged and have positive length to prevent compartments from impersonating as RTLD.
There was a problem hiding this comment.
"makes a few guesses about the actual compartment that the current untrusted stack belongs to" doesn't fill me with confidence as to the security of this mechanism
There was a problem hiding this comment.
The security argument definitely has lots of room for improvement. But at least signal handling should be functionally correct now.
| static inline struct trusted_frame * | ||
| pop_dummy_rtld_trusted_frame(struct trusted_frame *tf) | ||
| { | ||
| assert(get_trusted_stk() == tf); |
There was a problem hiding this comment.
So why not just make it implicit?...
There was a problem hiding this comment.
I want to retain the assertion because it would be very hard to debug if something goes wrong here.
There was a problem hiding this comment.
What does asserting that the input is the one valid value achieve? The function can just use get_trusted_stk() itself, it doesn't need an argument.
There was a problem hiding this comment.
My main worry is that somebody pushed to the trusted stack and forgot to pop. The assertion helps to catch that.
There was a problem hiding this comment.
And how does this code help catch that?
There was a problem hiding this comment.
For example, if somebody pushed but forgot to pop, then we will have get_trusted_stk() < tf.
There was a problem hiding this comment.
As in, this assertion checks that nobody's made unbalanced push/pops to the trusted stack between the pair of push and pop that I am doing right now.
| * current compartment is RTLD must be pushed. | ||
| */ | ||
| static inline struct trusted_frame * | ||
| push_dummy_rtld_trusted_frame(struct trusted_frame *tf) |
There was a problem hiding this comment.
The only case when this isn't get_trusted_stk() is when you first create the stack. Implicitly use get_trusted_stk(), and then upon creation you just need to set_trusted_stk() before calling this.
|
Please also add a regression test to cheribsdtest_ifunc.c |
dpgao
force-pushed
the
dpgao-patch-1
branch
2 times, most recently
from
8dab7ed to
84897f7
Compare
|
@jrtc27 I've added a regression test. Regarding the use of implicit arguments, I'm still in favour of explicit arguments because then we are able to track canonical state of the trusted stack instead of relying on a special register, which might be corrupted between a push and a pop. |
The check for benchmark ABI was erroneously added and conflicts with benchmark ABI-only code below.
This is a common pattern and warrants shared code.
The dummy frame indicates that the current compartment is RTLD. Because _rtld_bind may cause domain switches (e.g., via a ifunc resolver which calls a lazily-bound symbol), a frame that correctly identifies the current compartment as RTLD is necessary. The signal handler has also been updated to handle the temporary inconsistency between the untrusted stack and the callee field of the topmost truted frame that result from the changes above.
The current method of popping the topmost trusted frame and restoring it at the end of the function does not correctly identify the current compartment as RTLD. Instead, push a dummy frame that does so.
This prevents a compartment from being able to set its stack to NULL and then trigger a signal that would cause it to be mis-identified as RTLD.
The dummy frame indicates that the current compartment is RTLD. Because _rtld_tlsdesc_dynamic may cause domain transitions (e.g., locking), a frame that correctly identifies the current compartment as RTLD is necessary.
Just for CI. Do not merge yet.This should fix #2130.