immunant / IA2-Phase2

5 stars 0 forks source link

`ia2_fn_ptr` shouldn't be shared between threads or compartments #226

Open fw-immunant opened 1 year ago

fw-immunant commented 1 year ago

Indirect calls (in the __IA2_CALL macro) write the function to call to the ia2_fn_ptr variable, and then call gates read it back.

From a threading perspective, this is trivially racy if multiple threads do indirect calls, which is UB.

If we didn't have threads, ia2_fn_ptr wouldn't need to be per-compartment because it only gets written/read in succession with no intervening non-IA2-runtime code executing. But in the presence of threads, if compartment A tries to do an indirect call into compartment B, a third compartment C running its own thread can overwrite ia2_fn_ptr and change what function compartment A calls.

Making ia2_fn_ptr thread-local doesn't solve the latter problem because it's still possible for a thread to determine the address of another thread's thread-local and write into it--we need to actually have per-compartment copies to pkey_mprotect them (unless there's a way to eliminate ia2_fn_ptr entirely).

ayrtonm commented 1 year ago

Alternatively we could avoid this issue altogether (i.e. remove ia2_fn_ptr) by having IA2_CALL expand to an asm statement inlining the callgates at each callsite. This would avoid the need to store the address of the expression we are going to call since we could more easily reference the fn ptr expression on the caller's stack, store it in a register and call it after switching stacks. The only downside is that it means inlining all indirect call callgates.

ayrtonm commented 1 year ago

Or we may be able to have IA2_CALL expand to an asm statement that references the fn ptr expression and stores it in a register (again removing the need for ia2_fn_ptr in the first place). Then have IA2_CALL call the callgate.

#define IA2_CALL(ptr)  ({               \
    asm(/* store ptr in a register */); \
    indirect_callgate();                \
})

the only issue is that we'd need to ensure that the compiler doesn't insert code that clobbers the register used to store ptr before the indirect callgate call.

fw-immunant commented 1 year ago

I like the latter idea here, but said clobbering seems like it could happen if the arguments to the are nontrivial expressions rather than variables/constants. I was curious if using explicit register variables (register void* fnptr asm("r12");) would be more robust, but the gcc docs here are pretty discouraging:

As with global register variables, it is recommended that you choose a register that is normally saved and restored by function calls on your machine, so that calls to library routines will not clobber it.

The only supported use for this feature is to specify registers for input and output operands when calling Extended asm (see Extended Asm - Assembler Instructions with C Expression Operands). This may be necessary if the constraints for a particular machine don’t provide sufficient control to select the desired register. To force an operand into a register, create a local variable and specify the register name after the variable’s declaration. Then use the local variable for the asm operand and specify any constraint letter that matches the register: [...]

What stops us from having the pointer be an argument to the indirect callgate? We would probably have to change uses from IA2_CALL(opaque)(arg1, arg2, ...) to IA2_CALL(opaque, arg1, arg2, ...), but could this work otherwise?

ayrtonm commented 1 year ago

Since we're starting to do things like protecting dependencies and allocator metadata, this seems like relatively higher-priority/low-hanging fruit that we should handle.

I think it'd be great to avoid this global ia2_fn_ptr but since we're not sure if removing it and generating reliable code is feasible we should just make this global thread/compartment specific, at least for now.

ayrtonm commented 1 year ago

I was initially thinking that we could define a static ia2_fn_ptr var in each IA2_CALL expansion but dealing with duplicates in each translation unit won't be super easy so we should just define one variable in each INIT_COMPARTMENT expansion and reference those.

ayrtonm commented 1 year ago

What stops us from having the pointer be an argument to the indirect callgate? We would probably have to change uses from IA2_CALL(opaque)(arg1, arg2, ...) to IA2_CALL(opaque, arg1, arg2, ...), but could this work otherwise?

@rinon I just realized that I missed this suggestion, but I think this could work. As far as generating the callgates goes we just have to make sure we change the ABI when we generate the __ia2_indirect_callgate_ID_PKEY wrappers, but the callgate generation code itself shouldn't have to change. I also want to point out that this callgate transitions to compartment 0, so we need to have the function pointer argument be the first to ensure it stays in a register if args get spilled onto the stack.