Skip to content

Commit

Permalink
Use PROT_NONE on the unused parts of CFI shadow.
Browse files Browse the repository at this point in the history
This replaces a single 2Gb readable memory region with a bunch of tiny
regions, and leaves the bulk of 2Gb mapped but unaccessible. This makes
it harder to defeat ASLR by probing for the CFI shadow region.

Sample CFI shadow mapping with this change:
7165151000-716541f000 ---p 00000000 00:00 0                              [anon:cfi shadow]
716541f000-7165420000 r--p 00000000 00:00 0                              [anon:cfi shadow]
7165420000-71654db000 ---p 00000000 00:00 0                              [anon:cfi shadow]
71654db000-71654dc000 r--p 00000000 00:00 0                              [anon:cfi shadow]
71654dc000-71654dd000 r--p 00000000 00:00 0                              [anon:cfi shadow]
71654dd000-71654f0000 ---p 00000000 00:00 0                              [anon:cfi shadow]
71654f0000-71654f1000 r--p 00000000 00:00 0                              [anon:cfi shadow]
71654f1000-71e5151000 ---p 00000000 00:00 0                              [anon:cfi shadow]

This change degrades CFI diagnostics for wild jumps and casts (i.e. when
the target of a CFI check is outside of any known library bounds). This
is acceptable, because CFI does not have much to tell about those cases
anyway. Such bugs will show up as SEGV_ACCERR crashes inside
__cfi_slowpath in libdl.so from now on.

Bug: 158113540
Test: bionic-unit-tests/cfi_test.*
Test: adb shell cat /proc/$PID/maps | grep cfi

Change-Id: I57cbd0d3f87eb1610ad99b48d98ffd497ba214b4
  • Loading branch information
eugenis committed Jun 6, 2020
1 parent 9174068 commit c3b3e86
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 7 deletions.
3 changes: 2 additions & 1 deletion linker/linker_cfi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class ShadowWrite {
reinterpret_cast<char*>(mmap(nullptr, aligned_end - aligned_start, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0));
CHECK(tmp_start != MAP_FAILED);
mprotect(aligned_start, aligned_end - aligned_start, PROT_READ);
memcpy(tmp_start, aligned_start, shadow_start - aligned_start);
memcpy(tmp_start + (shadow_end - aligned_start), shadow_end, aligned_end - shadow_end);
}
Expand Down Expand Up @@ -154,7 +155,7 @@ uintptr_t soinfo_find_cfi_check(soinfo* si) {

uintptr_t CFIShadowWriter::MapShadow() {
void* p =
mmap(nullptr, kShadowSize, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
mmap(nullptr, kShadowSize, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
CHECK(p != MAP_FAILED);
return reinterpret_cast<uintptr_t>(p);
}
Expand Down
9 changes: 3 additions & 6 deletions tests/libs/cfi_test_lib.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,9 @@ struct A {
void check_cfi_self() {
g_last_type_id = 0;
assert(&__cfi_slowpath);
// CFI check for an invalid address. Normally, this would kill the process by routing the call
// back to the calling module's __cfi_check, which does the right thing based on
// -fsanitize-recover / -fsanitize-trap. But this module has custom __cfi_check that does not do
// any of that, so the result looks like a passing check.
int zz;
__cfi_slowpath(13, static_cast<void*>(&zz));
// CFI check for an address inside this DSO. This goes to the current module's __cfi_check,
// which updates g_last_type_id.
__cfi_slowpath(13, static_cast<void*>(&g_last_type_id));
assert(g_last_type_id == 13);
// CFI check for a libc function. This never goes into this module's __cfi_check, and must pass.
__cfi_slowpath(14, reinterpret_cast<void*>(&exit));
Expand Down

0 comments on commit c3b3e86

Please sign in to comment.