This is an archive of the discontinued LLVM Phabricator instance.

CFI runtime library (cross-DSO support)
ClosedPublic

Authored by eugenis on Dec 8 2015, 3:22 PM.

Details

Reviewers
kcc
pcc
Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 42238.Dec 8 2015, 3:22 PM
eugenis retitled this revision from to CFI runtime library (cross-DSO support).
eugenis updated this object.
eugenis added reviewers: pcc, kcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc added inline comments.Dec 8 2015, 4:28 PM
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.

eugenis updated this revision to Diff 42478.Dec 10 2015, 4:16 PM
eugenis marked 5 inline comments as done.
eugenis added inline comments.
lib/cfi/cfi.cc
60

Does it look better now?
I've split fill_shadow in 2: for function pointers, and for (0) and (-1) constants.
I've also refactored find_cfi_check_for_address to a class with, hopefully, cleaner interface.

111

Yes, looks that way.
Changed the code to bail out if symtab and strtab point to different ELF segments. Now, if they are inside one segment, nothing really bad can happen for over-scanning. The probability of misinterpreting a random sequence of bytes as __cfi_check symbol is super low.

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)?
And teach the driver to include the latter when needed.
Sounds doable.

pcc added inline comments.Dec 11 2015, 1:22 PM
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.

eugenis updated this revision to Diff 42572.Dec 11 2015, 1:58 PM
eugenis marked 9 inline comments as done.
eugenis added inline comments.
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.

pcc accepted this revision.Dec 11 2015, 2:48 PM
pcc edited edge metadata.

LGTM with nit

lib/cfi/cfi.cc
178

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.

249

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.

This revision is now accepted and ready to land.Dec 11 2015, 2:48 PM
eugenis updated this revision to Diff 42609.Dec 11 2015, 4:49 PM
eugenis edited edge metadata.
eugenis added inline comments.
lib/cfi/cfi.cc
249

fixed

eugenis updated this revision to Diff 42898.Dec 15 2015, 1:10 PM
eugenis closed this revision.Dec 15 2015, 3:04 PM

r255695, thanks for the review!