From a9940c2a53b3608cd6cfdab4e64cfa4b6a8081b0 Mon Sep 17 00:00:00 2001 From: Nick Desaulniers Date: Wed, 11 Dec 2019 15:33:39 -0800 Subject: [PATCH] zygote: fix mprotect range for non-page-aligned segments As of LLVM r369344 commit f66b767abe5e ("[ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges") it's no longer guaranteed that binaries linked with LLD have segments that are loaded at virtual addresses that are multiples of the page size. This saves significant space in the binary. This implies the subexpression from `man 3 dl_iterate_phdr`: addr == info->dlpi_addr + info->dlpi_phdr[x].p_vaddr; is not precise (as noted in bionic/linker/linker.cpp as well). This change results in failures to change page protections via calls to mprotect from execute only pages back to read+execute for apps targeting an Android SDK version < 28 (Q), since mprotect requires a page aligned address. When Zygote forks the app, the child may segfault (SIGSEGV) due to invalid permissions (SEGV_ACCERR). Previously, `info->dlpi_phdr[x].p_vaddr` was page aligned. Now that it's not, we must round it down to the closet multiple of the page size, and extend the range to account for this difference. Previously: +----+ (page) +---+| (segment) | || +---|+ +---+ ^ page boundary, segment p_vaddr (aligned) Now: +----+ (page) | +---+ (segment) | | || +-|--+| +---* ^ segment p_vaddr (unaligned) ^ page boundary Account for the alignment of the segment's virtual address not necessarily being a multiple of the page size by rounding down to the nearest multiple of the page size for the address passed to mprotect, then add the remainder back so the correct number of pages get remapped properly. (It would not be correct to subtract the remainder, otherwise a segment could span two pages, and we might not remap both). Example: p_vaddr == PAGE_START(p_vaddr) + PAGE_OFFSET(p_vaddr) 0x378c == 0x3000 + 0x78c | | | ^ Can't be passed to mprotect, not page aligned. ^ Added to dlpi_addr (which is already page aligned), then passed to mprotect. | ^ Added to size. Finally, check the return code of mprotect, and fail early in zygote, rather than segfault down the line in the child. Test: atest \ CtsSelinuxTargetSdk27TestCases:android.security.SELinuxTargetSdkTest#testNoExecuteOnly Test: launch Facebook or Instagram Bug: 145825270 Change-Id: I6c609e73b59b86c2fd493a8ccf91ccf6c4dc75bf Signed-off-by: Nick Desaulniers --- core/jni/com_android_internal_os_Zygote.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index f28c4221c6370..c4ac89acd0db6 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -74,6 +74,7 @@ #include #include #include +#include #include #include #include @@ -1389,9 +1390,14 @@ static int DisableExecuteOnly(struct dl_phdr_info* info, void* data [[maybe_unused]]) { // Search for any execute-only segments and mark them read+execute. for (int i = 0; i < info->dlpi_phnum; i++) { - if ((info->dlpi_phdr[i].p_type == PT_LOAD) && (info->dlpi_phdr[i].p_flags == PF_X)) { - mprotect(reinterpret_cast(info->dlpi_addr + info->dlpi_phdr[i].p_vaddr), - info->dlpi_phdr[i].p_memsz, PROT_READ | PROT_EXEC); + const auto& phdr = info->dlpi_phdr[i]; + if ((phdr.p_type == PT_LOAD) && (phdr.p_flags == PF_X)) { + auto addr = reinterpret_cast(info->dlpi_addr + PAGE_START(phdr.p_vaddr)); + size_t len = PAGE_OFFSET(phdr.p_vaddr) + phdr.p_memsz; + if (mprotect(addr, len, PROT_READ | PROT_EXEC) == -1) { + ALOGE("mprotect(%p, %zu, PROT_READ | PROT_EXEC) failed: %m", addr, len); + return -1; + } } } // Return non-zero to exit dl_iterate_phdr.