This is an initial version of the runtime cross-DSO CFI support library.
It contains a number of FIXMEs, ex. it does not support the diagnostic mode nor dlopen/dlclose, but it works and can be tested.
Diagnostic mode, in particular, would require some refactoring (we'd like to gather all CFI hooks in the UBSan library into one function so that we could easier pass the diagnostic information down to CFI_Check). It will be implemented later.
Once the diagnostic mode is in, I plan to create a second test configuration to run all existing tests in both modes. For now, this patch includes only a couple of new, cross-DSO tests.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/cfi/cfi.cc | ||
---|---|---|
39 | This can be a function rather than a macro. | |
60 | I think the code would be easier to read if this function were inlined into its caller, mainly because of the magic numbers you are using here. | |
79 | Yes, but there's also a reason why we might prefer to use this function all the time: an attacker may have corrupted the dynamic loader's internal data structures so as to make dlsym return some value of its choosing. | |
111 | This is reasonable if all major linkers place strtab immediately after symtab. Have you verified that this is the case? | |
113 | We should probably not use this as the function name, as it intrudes on the user's namespace. Instead, please use a C/C++ implementation reserved identifier (e.g. __CFI_Check). | |
133 | I believe that this is controlled by the RELRO program header. | |
150 | Implementation reserved identifier please. | |
159 | It's a little confusing that this function can return both function addresses and magic numbers. I would inline find_cfi_check_for_address into here. | |
176 | Can we link against ubsan dependent on whether diagnostics are enabled? That would save us from having to link ubsan into production builds. | |
test/cfi/cross-dso/icall/icall-from-dso.cpp | ||
12 | Was this intended to be a positive test? This isn't a positive test because it is a direct call. | |
test/cfi/cross-dso/icall/icall.cpp | ||
15 | Same here. | |
test/cfi/cross-dso/lit.local.cfg | ||
1 ↗ | (On Diff #42238) | It would be less confusing to add a new substitution instead of rewriting the existing one. |
test/cfi/cross-dso/simple-fail.cpp | ||
73 | We should make the other test cases do this as well. It doesn't have to be done now though. |
lib/cfi/cfi.cc | ||
---|---|---|
60 | Does it look better now? | |
111 | Yes, looks that way. | |
113 | Renamed to __cfi_check. | |
133 | keep as FIXME? | |
159 | PTAL | |
176 | So, we build libclang_rt.cfi and libclangrt.cfi_diag (the latter includes ubsan)? |
lib/cfi/cfi.cc | ||
---|---|---|
42 | Looks like most callers of this function would be better off with a uint16_t * so maybe should return that? | |
58 | assert(!is_invalid() && !is_unchecked()); ? | |
61 | Yes, this looks nicer. | |
72–73 | Is this necessary? Looks like you could just pass these directly to mem_to_shadow. | |
76 | This would become mem_to_shadow(end - 1) + 1. | |
84–85 | Same as above. | |
134 | Yes. | |
160 | Looks good. | |
177 | Maybe. Why don't we build a libclang_rt.sanitizer_common library and have the sanitizers use that instead of each having its own copy of sanitizer_common? Then we could just have a cfi library and link the regular ubsan library conditionally. | |
200 | Remove commented line. | |
211 | You don't need this cast. |
lib/cfi/cfi.cc | ||
---|---|---|
177 | The way it's done now, the "main" sanitizer calls Ubsan::InitAsPlugin and it also handles flags for ubsan. Replacing that with a runtime check could be brittle and would need to handle static vs shared linking, different initialization order (between the two runtime libraries), etc. Anyway, this is a bigger refactoring that would also affect asan+ubsan mode, I don't want to do it now. |
LGTM with nit
lib/cfi/cfi.cc | ||
---|---|---|
177 | Okay, let's not make things too complicated to start with. I'm happy with how things stand at the moment, but I think we should eventually resolve this by doing the refactoring. | |
248 | Won't this request one fewer byte than needed? This might not actually matter, as MmapNoReserveOrDie rounds up to the page size, but I think we should at least leave a comment here. |
lib/cfi/cfi.cc | ||
---|---|---|
250 | fixed |
This can be a function rather than a macro.