Page MenuHomePhabricator

[cfi] Support for dlopen and dlclose
ClosedPublic

Authored by eugenis on Dec 22 2015, 2:03 PM.

Details

Reviewers
kcc
pcc
Summary

Add dlopen/dlclose interceptors to update CFI shadow for loaded/unloaded libraries.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 43483.Dec 22 2015, 2:03 PM
eugenis updated this revision to Diff 43484.
eugenis retitled this revision from to [cfi] Support for dlopen and dlclose.
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.Jan 12 2016, 6:26 PM
lib/cfi/cfi.cc
297

As you mentioned offline, this won't correctly handle dependencies.

Perhaps the code should be building the shadow from scratch using dl_iterate_phdr every time a rebuild is needed? That way, you save needing to worry about dependencies. This may require a redesign of D16098 to avoid the copy.

eugenis updated this revision to Diff 44918.Jan 14 2016, 1:13 PM

A new approach.
dlclose may unmap (and dlopen may map) multiple libraries, so we actually need to do the entire dl_iterate_phdr run and update all libraries. For dlclose, we also need to detect libraries that have disappeared from the address space, so we do dl_iterate_phdr before and after dlclose and compare the results.

pcc added inline comments.Jan 14 2016, 6:44 PM
lib/cfi/cfi.cc
41

Looks like we already have a kMaxNumberOfModules constant defined.

228

I changed this interface in r257858.

279–305

Can you avoid needing to take the set difference by rebuilding from scratch?

lib/sanitizer_common/sanitizer_common.cc
341 ↗(On Diff #44918)

Should there be a single init() function that combines set and set_phdr?

lib/sanitizer_common/sanitizer_common.h
649

Is this function needed? Looks like you're passing a comparator to your sort function above.

eugenis updated this revision to Diff 45035.Jan 15 2016, 2:05 PM
eugenis marked 3 inline comments as done.
eugenis added inline comments.
lib/cfi/cfi.cc
279–305

Do you mean unmap or clean the entire shadow and then add the existing modules? That would be racy, we can not ensure that there are no concurrent virtual/indirect calls.

lib/sanitizer_common/sanitizer_common.h
649

removed

pcc added inline comments.Jan 15 2016, 2:09 PM
lib/cfi/cfi.cc
279–305

Yes, that's why I was thinking this would need to be done after D16098. If it's too painful to re-order the patches I'd be fine with combining them.

eugenis updated this revision to Diff 45047.Jan 15 2016, 3:56 PM

Yet another new approach, way simpler than before.
Deprecates D16098.

eugenis updated this revision to Diff 45288.Jan 19 2016, 11:44 AM

The previous approach contained a data race on the shadow pointer, which can not be updated without a lock in __cfi_slowpath. That would not be acceptable performance-wise. Probably.

This new version is linux-only as it requires mremap().

how does it look?

pcc added inline comments.Jan 25 2016, 4:13 PM
lib/cfi/cfi.cc
29–35

Maybe assert somewhere that this is equal to GetPageSize()?

33

This name is unclear. It should reflect that the shadow pointer is stored here, not the shadow itself.

48

Please inline into ShadowBuilder::Install, as the pre-conditions of this function aren't clear without context.

126

Do you need to set the memory protection for shadow_ first?

299

You can use it now I guess.

lib/sanitizer_common/sanitizer_posix.cc
178

This function is unused.

eugenis updated this revision to Diff 45928.Jan 25 2016, 4:24 PM
eugenis marked 5 inline comments as done.
eugenis added inline comments.
lib/cfi/cfi.cc
126

Of course.
Good catch.

pcc added inline comments.Jan 25 2016, 4:35 PM
lib/cfi/cfi.cc
13–14

This is done now, right?

126

Thanks. Is it possible to construct a test case that makes sure we get the memory protection right? I was thinking that the test case would try to write to the shadow at various points (e.g. start of main, after dlopen, after dlclose), and use %expect_crash to test that the write fails.

eugenis added inline comments.Jan 25 2016, 4:36 PM
lib/cfi/cfi.cc
126

But we don't really want there to be an easy way to find the cfi shadow.

eugenis updated this revision to Diff 46020.Jan 26 2016, 11:41 AM
eugenis marked 3 inline comments as done.

I'll move most of the implementation under a namespace in a separate commit.

eugenis updated this revision to Diff 46021.Jan 26 2016, 11:43 AM
pcc accepted this revision.Jan 26 2016, 11:59 AM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 26 2016, 11:59 AM