From afb7305b258cb655341583bbf06faf7083998021 Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Sun, 24 Jun 2018 09:02:38 +0900 Subject: [PATCH] Bug 1470701 - Use run-time page size when changing mapping permissions in elfhack injected code. r?froydnj When a binary has a PT_GNU_RELRO segment, the elfhack injected code uses mprotect to add the writable flag to relocated pages before applying relocations, removing it afterwards. To do so, the elfhack program uses the location and size of the PT_GNU_RELRO segment, and adjusts it to be aligned according to the PT_LOAD alignment. The problem here is that the PT_LOAD alignment doesn't necessarily match the actual page alignment, and the resulting mprotect may end up not covering the full extent of what the dynamic linker has protected read-only according to the PT_GNU_RELRO segment. In turn, this can lead to a crash on startup when trying to apply relocations to the still read-only locations. Practically speaking, this doesn't end up being a problem on x86, where the PT_LOAD alignment is usually 4096, which happens to be the page size, but on Debian armhf, it is 64k, while the run time page size can be 4k. Gbp-Pq: Topic fixes Gbp-Pq: Name Bug-1470701-Use-run-time-page-size-when-changing-map.patch --- build/unix/elfhack/elfhack.cpp | 98 +++++++++++++++++++--------------- build/unix/elfhack/inject.c | 31 ++++++----- build/unix/elfhack/test.c | 4 ++ 3 files changed, 78 insertions(+), 55 deletions(-) diff --git a/build/unix/elfhack/elfhack.cpp b/build/unix/elfhack/elfhack.cpp index 4d270af0e60..e7e2fa5b5ca 100644 --- a/build/unix/elfhack/elfhack.cpp +++ b/build/unix/elfhack/elfhack.cpp @@ -87,12 +87,13 @@ class ElfRelHackCode_Section : public ElfSection { public: ElfRelHackCode_Section(Elf_Shdr &s, Elf &e, ElfRelHack_Section &relhack_section, unsigned int init, - unsigned int mprotect_cb) + unsigned int mprotect_cb, unsigned int sysconf_cb) : ElfSection(s, nullptr, nullptr), parent(e), relhack_section(relhack_section), init(init), - mprotect_cb(mprotect_cb) { + mprotect_cb(mprotect_cb), + sysconf_cb(sysconf_cb) { std::string file(rundir); file += "/inject/"; switch (parent.getMachine()) { @@ -130,7 +131,6 @@ class ElfRelHackCode_Section : public ElfSection { "Couldn't find a symbol table for the injected code"); relro = parent.getSegmentByType(PT_GNU_RELRO); - align = parent.getSegmentByType(PT_LOAD)->getAlign(); // Find the init symbol entry_point = -1; @@ -357,12 +357,12 @@ class ElfRelHackCode_Section : public ElfSection { addr = init; } else if (relro && strcmp(name, "mprotect_cb") == 0) { addr = mprotect_cb; + } else if (relro && strcmp(name, "sysconf_cb") == 0) { + addr = sysconf_cb; } else if (relro && strcmp(name, "relro_start") == 0) { - // Align relro segment start to the start of the page it starts in. - addr = relro->getAddr() & ~(align - 1); - // Align relro segment end to the start of the page it ends into. + addr = relro->getAddr(); } else if (relro && strcmp(name, "relro_end") == 0) { - addr = (relro->getAddr() + relro->getMemSize()) & ~(align - 1); + addr = (relro->getAddr() + relro->getMemSize()); } else if (strcmp(name, "_GLOBAL_OFFSET_TABLE_") == 0) { // We actually don't need a GOT, but need it as a reference for // GOTOFF relocations. We'll just use the start of the ELF file @@ -418,9 +418,9 @@ class ElfRelHackCode_Section : public ElfSection { std::vector code; unsigned int init; unsigned int mprotect_cb; + unsigned int sysconf_cb; int entry_point; ElfSegment *relro; - unsigned int align; }; unsigned int get_addend(Elf_Rel *rel, Elf *elf) { @@ -727,6 +727,7 @@ int do_relocation_section(Elf *elf, unsigned int rel_type, } unsigned int mprotect_cb = 0; + unsigned int sysconf_cb = 0; // If there is a relro segment, our injected code will run after the linker // sets the corresponding pages read-only. We need to make our code change // that to read-write before applying relocations, which means it needs to @@ -740,37 +741,48 @@ int do_relocation_section(Elf *elf, unsigned int rel_type, // section temporarily (it will be restored to a null value before any code // can actually use it) if (elf->getSegmentByType(PT_GNU_RELRO)) { - Elf_SymValue *mprotect = symtab->lookup("mprotect", STT(FUNC)); - if (!mprotect) { - symtab->syms.emplace_back(); - mprotect = &symtab->syms.back(); - symtab->grow(symtab->syms.size() * symtab->getEntSize()); - mprotect->name = - ((ElfStrtab_Section *)symtab->getLink())->getStr("mprotect"); - mprotect->info = ELF32_ST_INFO(STB_GLOBAL, STT_FUNC); - mprotect->other = STV_DEFAULT; - new (&mprotect->value) ElfLocation(nullptr, 0, ElfLocation::ABSOLUTE); - mprotect->size = 0; - mprotect->defined = false; - - // The DT_VERSYM data (in the .gnu.version section) has the same number of - // entries as the symbols table. Since we added one entry there, we need - // to add one entry here. Zeroes in the extra data means no version for - // that symbol, which is the simplest thing to do. - ElfSection *gnu_versym = dyn->getSectionForType(DT_VERSYM); - if (gnu_versym) { - gnu_versym->grow(gnu_versym->getSize() + gnu_versym->getEntSize()); + ElfSection *gnu_versym = dyn->getSectionForType(DT_VERSYM); + auto lookup = [&symtab, &gnu_versym](const char *symbol) { + Elf_SymValue *sym_value = symtab->lookup(symbol, STT(FUNC)); + if (!sym_value) { + symtab->syms.emplace_back(); + sym_value = &symtab->syms.back(); + symtab->grow(symtab->syms.size() * symtab->getEntSize()); + sym_value->name = + ((ElfStrtab_Section *)symtab->getLink())->getStr(symbol); + sym_value->info = ELF32_ST_INFO(STB_GLOBAL, STT_FUNC); + sym_value->other = STV_DEFAULT; + new (&sym_value->value) ElfLocation(nullptr, 0, ElfLocation::ABSOLUTE); + sym_value->size = 0; + sym_value->defined = false; + + // The DT_VERSYM data (in the .gnu.version section) has the same number + // of entries as the symbols table. Since we added one entry there, we + // need to add one entry here. Zeroes in the extra data means no version + // for that symbol, which is the simplest thing to do. + if (gnu_versym) { + gnu_versym->grow(gnu_versym->getSize() + gnu_versym->getEntSize()); + } } - } + return sym_value; + }; - // Add a relocation for the mprotect symbol. - new_rels.emplace_back(); - Rel_Type &rel = new_rels.back(); - memset(&rel, 0, sizeof(rel)); - rel.r_info = ELF32_R_INFO( - std::distance(symtab->syms.begin(), - std::vector::iterator(mprotect)), - rel_type2); + Elf_SymValue *mprotect = lookup("mprotect"); + Elf_SymValue *sysconf = lookup("sysconf"); + + // Add relocations for the mprotect and sysconf symbols. + auto add_relocation_to = [&new_rels, &symtab, rel_type2]( + Elf_SymValue *symbol, unsigned int location) { + new_rels.emplace_back(); + Rel_Type &rel = new_rels.back(); + memset(&rel, 0, sizeof(rel)); + rel.r_info = ELF32_R_INFO( + std::distance(symtab->syms.begin(), + std::vector::iterator(symbol)), + rel_type2); + rel.r_offset = location; + return location; + }; // Find the beginning of the bss section, and use an aligned location in // there for the relocation. @@ -782,13 +794,14 @@ int do_relocation_section(Elf *elf, unsigned int rel_type, size_t ptr_size = Elf_Addr::size(elf->getClass()); size_t usable_start = (s->getAddr() + ptr_size - 1) & ~(ptr_size - 1); size_t usable_end = (s->getAddr() + s->getSize()) & ~(ptr_size - 1); - if (usable_end - usable_start >= ptr_size) { - mprotect_cb = rel.r_offset = usable_start; + if (usable_end - usable_start >= 2 * ptr_size) { + mprotect_cb = add_relocation_to(mprotect, usable_start); + sysconf_cb = add_relocation_to(sysconf, usable_start + ptr_size); break; } } - if (mprotect_cb == 0) { + if (mprotect_cb == 0 || sysconf_cb == 0) { fprintf(stderr, "Couldn't find .bss. Skipping\n"); return -1; } @@ -797,8 +810,9 @@ int do_relocation_section(Elf *elf, unsigned int rel_type, section->rels.assign(new_rels.begin(), new_rels.end()); section->shrink(new_rels.size() * section->getEntSize()); - ElfRelHackCode_Section *relhackcode = new ElfRelHackCode_Section( - relhackcode_section, *elf, *relhack, original_init, mprotect_cb); + ElfRelHackCode_Section *relhackcode = + new ElfRelHackCode_Section(relhackcode_section, *elf, *relhack, + original_init, mprotect_cb, sysconf_cb); // Find the first executable section, and insert the relhack code before // that. The relhack data is inserted between .rel.dyn and .rel.plt. ElfSection *first_executable = nullptr; diff --git a/build/unix/elfhack/inject.c b/build/unix/elfhack/inject.c index 862e9d9d975..7a1f65fd413 100644 --- a/build/unix/elfhack/inject.c +++ b/build/unix/elfhack/inject.c @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -29,6 +30,7 @@ extern __attribute__((visibility("hidden"))) Elf_Ehdr elf_header; extern __attribute__((visibility("hidden"))) int (*mprotect_cb)(void *addr, size_t len, int prot); +extern __attribute__((visibility("hidden"))) long (*sysconf_cb)(int name); extern __attribute__((visibility("hidden"))) char relro_start[]; extern __attribute__((visibility("hidden"))) char relro_end[]; @@ -58,34 +60,37 @@ __attribute__((section(".text._init"))) int init(int argc, char **argv, return 0; } -static inline __attribute__((always_inline)) void relro_pre() { +static inline __attribute__((always_inline)) void do_relocations_with_relro( + void) { + long page_size = sysconf_cb(_SC_PAGESIZE); + uintptr_t aligned_relro_start = ((uintptr_t)relro_start) & ~(page_size - 1); + uintptr_t aligned_relro_end = ((uintptr_t)relro_end) & ~(page_size - 1); // By the time the injected code runs, the relro segment is read-only. But // we want to apply relocations in it, so we set it r/w first. We'll restore // it to read-only in relro_post. - mprotect_cb(relro_start, relro_end - relro_start, PROT_READ | PROT_WRITE); -} + mprotect_cb((void *)aligned_relro_start, + aligned_relro_end - aligned_relro_start, PROT_READ | PROT_WRITE); + + do_relocations(); -static inline __attribute__((always_inline)) void relro_post() { - mprotect_cb(relro_start, relro_end - relro_start, PROT_READ); - // mprotect_cb is a pointer allocated in .bss, so we need to restore it to - // a NULL value. + mprotect_cb((void *)aligned_relro_start, + aligned_relro_end - aligned_relro_start, PROT_READ); + // mprotect_cb and sysconf_cb are allocated in .bss, so we need to restore + // them to a NULL value. mprotect_cb = NULL; + sysconf_cb = NULL; } __attribute__((section(".text._init_noinit_relro"))) int init_noinit_relro( int argc, char **argv, char **env) { - relro_pre(); - do_relocations(); - relro_post(); + do_relocations_with_relro(); return 0; } __attribute__((section(".text._init_relro"))) int init_relro(int argc, char **argv, char **env) { - relro_pre(); - do_relocations(); - relro_post(); + do_relocations_with_relro(); original_init(argc, argv, env); return 0; } diff --git a/build/unix/elfhack/test.c b/build/unix/elfhack/test.c index 3ca40b18e22..ab994a45d2d 100644 --- a/build/unix/elfhack/test.c +++ b/build/unix/elfhack/test.c @@ -127,6 +127,10 @@ int print_status() { __thread int foo; __thread long long int bar[512]; +/* We need a .bss that can hold at least 2 pointers. The static in + * end_test() plus this variable should do. */ +size_t dummy; + void end_test() { static size_t count = 0; /* Only exit when both constructors have been called */ -- 2.30.2