linker: don't perform unnecessary mprotects
The linker only needs to mark the text segment as
writable iff the file has text relocations. Unnecessarily
calling mprotect when it isn't necessary is slow, and some
security enhanced kernels don't like it. Pages which are
simultaneously writable and executable are considered a no-no.
The vast majority of executables / shared libraries on Android
do NOT have text relocations.
Change-Id: Ic38ce30a99b7e33ecf21efd9c108547a58eafa35
diff --git a/linker/linker.cpp b/linker/linker.cpp
index 0e2d9dd..a19fd45 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -81,7 +81,7 @@
*/
-static int soinfo_link_image(soinfo *si, unsigned wr_offset);
+static int soinfo_link_image(soinfo *si);
static int socount = 0;
static soinfo sopool[SO_MAX];
@@ -817,20 +817,6 @@
return NULL;
}
- /* Unprotect the segments, i.e. make them writable, to allow
- * relocations to work properly. We will later call
- * phdr_table_protect_segments() after all of them are applied
- * and all constructors are run.
- */
- ret = phdr_table_unprotect_segments(phdr_table,
- phdr_count,
- load_bias);
- if (ret < 0) {
- DL_ERR("can't unprotect loadable segments for \"%s\": %s",
- name, strerror(errno));
- return NULL;
- }
-
soinfo_ptr si(name);
if (si.ptr == NULL) {
return NULL;
@@ -855,14 +841,12 @@
static soinfo *
init_library(soinfo *si)
{
- unsigned wr_offset = 0xffffffff;
-
/* At this point we know that whatever is loaded @ base is a valid ELF
* shared library whose segments are properly mapped in. */
TRACE("[ %5d init_library base=0x%08x sz=0x%08x name='%s') ]\n",
pid, si->base, si->size, si->name);
- if(soinfo_link_image(si, wr_offset)) {
+ if(soinfo_link_image(si)) {
/* We failed to link. However, we can only restore libbase
** if no additional libraries have moved it since we updated it.
*/
@@ -1432,7 +1416,7 @@
return return_value;
}
-static int soinfo_link_image(soinfo *si, unsigned wr_offset)
+static int soinfo_link_image(soinfo *si)
{
unsigned *d;
/* "base" might wrap around UINT32_MAX. */
@@ -1466,20 +1450,6 @@
&si->ARM_exidx, &si->ARM_exidx_count);
#endif
- if (si->flags & (FLAG_EXE | FLAG_LINKER)) {
- if (phdr_table_unprotect_segments(si->phdr,
- si->phnum,
- si->load_bias) < 0) {
- /* We can't call DL_ERR if the linker's relocations haven't
- * been performed yet */
- if (!relocating_linker) {
- DL_ERR("can't unprotect segments for \"%s\": %s",
- si->name, strerror(errno));
- }
- goto fail;
- }
- }
-
/* extract useful information from dynamic section */
for(d = si->dynamic; *d; d++){
DEBUG("%5d d = %p, d[0] = 0x%08x d[1] = 0x%08x\n", pid, d, d[0], d[1]);
@@ -1562,13 +1532,7 @@
si->preinit_array_count = ((unsigned)*d) / sizeof(Elf32_Addr);
break;
case DT_TEXTREL:
- /* TODO: make use of this. */
- /* this means that we might have to write into where the text
- * segment was loaded during relocation... Do something with
- * it.
- */
- DEBUG("%5d Text segment should be writable during relocation.\n",
- pid);
+ si->has_text_relocations = true;
break;
#if defined(ANDROID_MIPS_LINKER)
case DT_NEEDED:
@@ -1664,6 +1628,19 @@
}
}
+ if (si->has_text_relocations) {
+ /* Unprotect the segments, i.e. make them writable, to allow
+ * text relocations to work properly. We will later call
+ * phdr_table_protect_segments() after all of them are applied
+ * and all constructors are run.
+ */
+ if (phdr_table_unprotect_segments(si->phdr, si->phnum, si->load_bias) < 0) {
+ DL_ERR("can't unprotect loadable segments for \"%s\": %s",
+ si->name, strerror(errno));
+ goto fail;
+ }
+ }
+
if(si->plt_rel) {
DEBUG("[ %5d relocating %s plt ]\n", pid, si->name );
if(soinfo_relocate(si, si->plt_rel, si->plt_rel_count))
@@ -1684,12 +1661,14 @@
si->flags |= FLAG_LINKED;
DEBUG("[ %5d finished linking %s ]\n", pid, si->name);
- /* All relocations are done, we can protect our segments back to
- * read-only. */
- if (phdr_table_protect_segments(si->phdr, si->phnum, si->load_bias) < 0) {
- DL_ERR("can't protect segments for \"%s\": %s",
- si->name, strerror(errno));
- goto fail;
+ if (si->has_text_relocations) {
+ /* All relocations are done, we can protect our segments back to
+ * read-only. */
+ if (phdr_table_protect_segments(si->phdr, si->phnum, si->load_bias) < 0) {
+ DL_ERR("can't protect segments for \"%s\": %s",
+ si->name, strerror(errno));
+ goto fail;
+ }
}
/* We can also turn on GNU RELRO protection */
@@ -1911,7 +1890,7 @@
parse_LD_LIBRARY_PATH(ldpath_env);
parse_LD_PRELOAD(ldpreload_env);
- if(soinfo_link_image(si, 0)) {
+ if(soinfo_link_image(si)) {
char errmsg[] = "CANNOT LINK EXECUTABLE\n";
write(2, __linker_dl_err_buf, strlen(__linker_dl_err_buf));
write(2, errmsg, sizeof(errmsg));
@@ -2050,7 +2029,7 @@
linker_so.phnum = elf_hdr->e_phnum;
linker_so.flags |= FLAG_LINKER;
- if (soinfo_link_image(&linker_so, 0)) {
+ if (soinfo_link_image(&linker_so)) {
// It would be nice to print an error message, but if the linker
// can't link itself, there's no guarantee that we'll be able to
// call write() (because it involves a GOT reference).
diff --git a/linker/linker.h b/linker/linker.h
index ce811df..c416f81 100644
--- a/linker/linker.h
+++ b/linker/linker.h
@@ -176,6 +176,7 @@
/* When you read a virtual address from the ELF file, add this
* value to get the corresponding address in the process' address space */
Elf32_Addr load_bias;
+ int has_text_relocations;
};
